__service: Suggestions on improving the type #62

New Issue

ungleich-gitea · 2021-11-20T11:25:20Z

ungleich-gitea commented

2021-11-20 11:25:20 +00:00

After having invested some time into looking at different init systems (cf. !850),
I believe that having a "wrapper" __service type which calls different types to do the work isn't a good approach.
There are dozens of different init systems and supervisor deamons all having their quirks but essentially doing the same thing.
I can't think of more possible actions than "start", "stop" and "restart" which are all shared among all of the different systems.
Does anyone see other actions that would be specific to a certain init system/supervisor deamon?

Following this design, it will eventually lead to a dozen different "implementation types" only consisting of a few lines of code.
I'm proposing to merge the actual execution logic into __service like there is a single __hostname type for all OSes.
I would also rename the --action parameter to --state {started,stopped} [--restart-if-started] to fit the declarative style of cdist better.

(This has originally been a comment in !847)

1. After having invested some time into looking at different init systems (cf. !850), I believe that having a "wrapper" `__service` type which calls different types to do the work isn't a good approach. There are dozens of different init systems and supervisor deamons all having their quirks but essentially doing the same thing. I can't think of more possible actions than "start", "stop" and "restart" which are all shared among all of the different systems. Does anyone see other actions that would be specific to a certain init system/supervisor deamon? Following this design, it will eventually lead to a dozen different "implementation types" only consisting of a few lines of code. I'm proposing to merge the actual execution logic into `__service` like there is a single `__hostname` type for all OSes. 2. I would also rename the `--action` parameter to `--state {started,stopped} [--restart-if-started]` to fit the declarative style of cdist better. (This has originally been a comment in !847)

ungleich-gitea commented

2021-11-20 15:33:12 +00:00

mentioned in issue #843

ungleich-gitea commented

2021-11-20 15:33:13 +00:00

I've looked around why there's a special __config_file type, but the reason is to reload a daemon configurations if the content of the current file changes. This is done by passing a command, which usually would consist of simply service foo reload.

This is some other possibility to reload a service, which is already implemented. But it's not general, e. g. it can only used for the __config_file type and isn't flexible for different init systems. Also, if multiple configuration files are changed, each of them will reload the service.

At the point: Currently, cdist doesn't have a good and robust way of doing something on an on-event basis. Something "if that changes, do this". Now, it's the question if cdist should support such feature and how it works with the design of cdist. We've stated the use cases and gave some ideas to implement it.

Btw., to your point 3: I think the current message system can be used for this. This is done on a single assumption: sending a message means something changed. Also, each type have an explanation (or should have) for each message, but this isn't relevant for the __service type.

I've looked around why there's a special `__config_file` type, but the reason is to reload a daemon configurations if the content of the current file changes. This is done by passing a command, which usually would consist of simply `service foo reload`. This is some other possibility to reload a service, which is already implemented. But it's not general, e. g. it can only used for the `__config_file` type and isn't flexible for different init systems. Also, if multiple configuration files are changed, each of them will reload the service. **At the point**: Currently, cdist doesn't have a good and robust way of doing something on an on-event basis. Something "if that changes, do this". Now, it's the question **if cdist should support such feature** and **how it works with the design of cdist**. We've stated the use cases and gave some ideas to implement it. Btw., to your point 3: I think the current message system can be used for this. This is done on a single assumption: sending a message means something changed. Also, each type have an explanation (or should have) for each message, but this isn't relevant for the `__service` type.

ungleich-gitea commented

2021-11-20 15:33:15 +00:00

My suggestion is how I made the __systemd_service type (see !844). States are sure, because this is implemented by cdist's design.

The thing you referred from puppet looks like hooks which will triggered if something related to the service changes. If then, cdist needs to know if it belongs to the service and requires the implementation of hooks or events.

Here, we come to a problem to the design of cdist. I think it's something a configuration management should have, because it's the way how the configuration will take effect, but we shouldn't screw up the design of cdist.

I currently haven't any idea of the message system, as it is kept really simple and follows the unix style of things. May more meta-information could be stored in the instance directories to resolve such things as hierarchic structures or define a special wrapper type to help providing an other interface for the same type. This comes close to the common code you mentioned in your last point.

My suggestion is how I made the `__systemd_service` type (see !844). States are sure, because this is implemented by cdist's design. The thing you referred from puppet looks like hooks which will triggered if something related to the service changes. If then, cdist needs to know if it belongs to the service and requires the implementation of _hooks_ or _events_. Here, we come to a problem to the design of cdist. I think it's something a configuration management should have, because it's **the way how the configuration will take effect**, but we shouldn't screw up the design of cdist. I currently haven't any idea of the message system, as it is kept really simple and follows the unix style of things. May more meta-information could be stored in the instance directories to resolve such things as hierarchic structures or define a special wrapper type to help providing an other interface for the same type. This comes close to the _common code_ you mentioned in your last point.

ungleich-gitea commented

2021-11-20 15:33:16 +00:00

@matze You raise some valid points, but they all show shortcomings of cdist, don't they?

__service being a type is unfortunate for types which do their changes in code-remote and not through some other types, because there is currently no way to specify that a type should be applied after the current type.
Even if that was possible these types would need to determine if a restart is required in their manifest, which should be possible, but is annoying.
The messages system, as it is, is fragile because everything is strings and the values are poorly documented. Maybe this could be addressed with Python types?

@matze You raise some valid points, but they all show shortcomings of cdist, don't they? 1. `__service` being a type is unfortunate for types which do their changes in code-remote and not through some other types, because there is currently no way to specify that a type should be applied _after_ the current type. 2. Even if that was possible these types would need to determine if a restart is required in their manifest, which should be possible, but is annoying. 3. The messages system, as it is, is fragile because everything is strings and the values are poorly documented. Maybe this could be addressed with Python types?

ungleich-gitea commented

2021-11-20 15:33:17 +00:00

I require to restart/reload a service. For my case, I generate a dns configuration and reload the dns daemon, so the changes will be applied. Also referring to the manual, the chapter "Drive into real world cdist" (go to chapter) goes through the creation of a type. There, a gencode-remote will be created containing multiple service some-daemon restart lines to restart some daemons. A __service type could make this more easier and general. The bigger problem is where you want to reload/restart a daemon in the manifest, because you haven't any possibility to generate remote code.

Other use-case are, if you don't want to have an active service for whatever reason. This could be, if a daemon is not required (or why ever you want this), but I think it's a good thing to handle the services in cdist.

Now coming to the magic onchange or "reload/restart on changes", I tried to do this in the __systemd_service type. It's the --if-required option, who searches the messages for messages from dependencies. If there are no messages from dependencies, it skips the execution of an action (but only gets applied if an action will take effect). I uses it as follow:

require="$requires_service" __systemd_service nsd --if-required --action reload

The biggest problem of this are wrapper types like __config_file or __dot_file, who passes all to __file. They will not write anything to $__messages_out, only __file will do this. Because the sub-called type __file is no dependency for __systemd_service, the detection is unable to know anything has changed.

I require to restart/reload a service. For my case, I generate a dns configuration and reload the dns daemon, so the changes will be applied. Also referring to the manual, the chapter "Drive into real world cdist" ([go to chapter](https://www.cdi.st/manual/latest/cdist-real-world.html)) goes through the creation of a type. There, a `gencode-remote` will be created containing multiple `service some-daemon restart` lines to restart some daemons. A `__service` type could make this more easier and general. The bigger problem is where you want to reload/restart a daemon in the manifest, because you haven't any possibility to generate remote code. Other use-case are, if you don't want to have an active service for whatever reason. This could be, if a daemon is not required (or why ever you want this), but I think it's a good thing to handle the services in cdist. Now coming to the magic `onchange` or "reload/restart on changes", I tried to do this in the `__systemd_service` type. It's the `--if-required` option, who searches the messages for messages from dependencies. If there are no messages from dependencies, it skips the execution of an action (but only gets applied if an action will take effect). I uses it as follow: ```sh require="$requires_service" __systemd_service nsd --if-required --action reload ``` The biggest problem of this are wrapper types like `__config_file` or `__dot_file`, who passes all to `__file`. They will not write anything to `$__messages_out`, only `__file` will do this. Because the sub-called type `__file` is no dependency for `__systemd_service`, the detection is unable to know anything has changed.

ungleich-gitea commented

2021-11-20 15:33:19 +00:00

I can see three real-world use cases:

you install a package and want to make sure that the service is running after installation,
you need to stop a service to uninstall a package,
you changed a config file and need to restart/reload a service.

The last of the three you brought up.
IMO the magic onchange variable should be a function changed(), e.g.:

if changed __some_type/foo
then
    notify __service/bar restart
fi

This could also allow more complex conditionals:

if changed __some_type/foo || changed __another_type/foo
then
  require='__some/thing __other/thing' __service bar --restart
fi

if (changed __some_type/foo && changed __some_type/bar) || changed __another_type/foo
then
  ...
fi

__line /etc/ssh/sshd_config:PermitRootLogin ...
__line /etc/ssh/sshd_config:PasswordAuthentication ...
__line /etc/ssh/sshd_config:X11Forwarding ...
if changed __line/etc/ssh/sshd_config:*
then
  require='__line/etc/ssh/sshd_config:*' notify __service/ssh restart
fi

I can see three real-world use cases: * you install a package and want to make sure that the service is running after installation, * you need to stop a service to uninstall a package, * you changed a config file and need to restart/reload a service. The last of the three you brought up. IMO the magic `onchange` variable should be a function `changed()`, e.g.: ```sh if changed __some_type/foo then notify __service/bar restart fi ``` This could also allow more complex conditionals: ```sh if changed __some_type/foo || changed __another_type/foo then require='__some/thing __other/thing' __service bar --restart fi ``` ```sh if (changed __some_type/foo && changed __some_type/bar) || changed __another_type/foo then ... fi ``` ```sh __line /etc/ssh/sshd_config:PermitRootLogin ... __line /etc/ssh/sshd_config:PasswordAuthentication ... __line /etc/ssh/sshd_config:X11Forwarding ... if changed __line/etc/ssh/sshd_config:* then require='__line/etc/ssh/sshd_config:*' notify __service/ssh restart fi ```

ungleich-gitea commented

2021-11-20 15:33:20 +00:00

Difference between the different __package_* and __service_* types I can see are:
The different package implementations

have quite a bit of code,
take different arguments.

While for service I can't think of any argument that would be specific to a service manager.

And yes to your second point: The types should be "state-defined", not defined by an action (but this is ok for a restart/reload).

So what is your suggestion?
Usually you tell cdist to "make sure that X applies". This works for --state running and --state stopped.

But restart/reload are no states but actions and as such they do not fit into cdist.
In Puppet there is notify and I think that there should be something similar in cdist.
Maybe messages could be extended to not only allow a type to tell others what it has done but to allow other types to notify a type.

Btw, the "sub-calling" of more specific types from one big wrapper results to a problem when messages are required. Because only the sub-type sends this message, but the caller assumes the message from the wrapper type and can't know the sub-type, it is more difficult to depend on this.

Agreed. Maybe the super-type should relay messages?
If you're asking me, not having hierarchic types is one of the shortcomings of cdist.

I brought this point up already, but it hasn't really been considered: #726

Difference between the different `__package_*` and `__service_*` types I can see are: The different package implementations * have quite a bit of code, * take different arguments. While for service I can't think of any argument that would be specific to a service manager. > And yes to your second point: The types should be "state-defined", not defined by an action (but this is ok for a *restart*/*reload*). So what is your suggestion? Usually you tell cdist to "make sure that X applies". This works for `--state running` and `--state stopped`. But *restart*/*reload* are no states but actions and as such they do not fit into cdist. In Puppet there is [notify](https://puppet.com/docs/puppet/latest/lang_relationships.html) and I think that there should be something similar in cdist. Maybe messages could be extended to not only allow a type to tell others what it has done but to allow other types to notify a type. > Btw, the "sub-calling" of more specific types from one big wrapper results to a problem when messages are required. Because only the sub-type sends this message, but the caller assumes the message from the wrapper type and can't know the sub-type, it is more difficult to depend on this. Agreed. Maybe the super-type should relay messages? If you're asking me, not having hierarchic types is one of the shortcomings of cdist. I brought this point up already, but it hasn't really been considered: https://code.ungleich.ch/ungleich-public/cdist/issues/726

ungleich-gitea commented

2021-11-20 15:33:21 +00:00

any real world example why one should need or use __service?

I can only think one usecase, but this isn't supported by cdist.

__some_type foo --doing something

onchange='__some_type/foo' __service bar --restart

and it would work like this: if __some_type foo generates code, then run __service bar after it.

and of course we can stack:

onchange='__some_type/foo __another_type/foo' \
require='__some/thing __other/thing' \
   __service bar --restart

require is AND, onchange should be OR? ahh yes, now we are getting into something really deep. onchange_and and onchange_or? allow only one? 😫

do we want to get into that discussion? again. 🐶

any real world example why one should need or use `__service`? I can only think one usecase, but this isn't supported by cdist. ``` __some_type foo --doing something onchange='__some_type/foo' __service bar --restart ``` and it would work like this: if `__some_type foo` generates code, then run `__service bar` after it. and of course we can stack: ``` onchange='__some_type/foo __another_type/foo' \ require='__some/thing __other/thing' \ __service bar --restart ``` require is AND, onchange should be OR? ahh yes, now we are getting into something really deep. onchange_and and onchange_or? allow only one? :tired_face: do we want to get into that discussion? again. :dog:

ungleich-gitea commented

2021-11-20 15:33:23 +00:00

You're be right with "a single type for all implementations" which makes the usage more intuitive. But there also exists the opposite: __package_* types as example. It's a bigger mess to have many types for these little features, except one type breaks out of the general features and support something others can't provide.

And yes to your second point: The types should be "state-defined", not defined by an action (but this is ok for a restart/reload).

Btw, the "sub-calling" of more specific types from one big wrapper results to a problem when messages are required. Because only the sub-type sends this message, but the caller assumes the message from the wrapper type and can't know the sub-type, it is more difficult to depend on this.

P. S.: with your __service type, you calling the __systemd_service type and passes all actions straight through. This will not work if you want to define the type state (e.g. --action stop), because states aren't considered as action like reload or restart.

You're be right with "a single type for all implementations" which makes the usage more intuitive. But there also exists the opposite: `__package_*` types as example. It's a bigger mess to have many types for these little features, except one type breaks out of the general features and support something others can't provide. And yes to your second point: The types should be "state-defined", not defined by an action (but this is ok for a *restart*/*reload*). Btw, the "sub-calling" of more specific types from one big wrapper results to a problem when messages are required. Because only the sub-type sends this message, but the caller assumes the message from the wrapper type and can't know the sub-type, it is more difficult to depend on this. P. S.: with your `__service` type, you calling the `__systemd_service` type and passes all actions straight through. This will not work if you want to define the type state (e.g. `--action stop`), because states aren't considered as action like *reload* or *restart*.

ungleich-gitea commented

2021-11-20 15:33:24 +00:00

mentioned in merge request !847

Sign in to join this conversation.