Added plugin 'service' #293

yarda · 2020-08-21T11:13:54Z

The service plugin can handle sysvinit, sysv-rc, openrc, systemd.
Syntax is the following:

[service]
service.sendmail = start,enable,file:${i:PROFILE_DIR}/tuned-sendmail.conf

'start' means to start the service, 'stop' to stop, 'enable' to enable,
'disable' to disable, file:FILE is the overlay configuration file
that will be installed for the service. Multiple directives can be
separated by comma ',' or semicolon ';'. If the directives are
conflicting, the last one will be used. The internal variable
${i:PROFILE_DIR} points to the directory from which the profile is
loaded.

The service plugin supports configuration overlays only for the systemd,
for other init systems this directive is ignored. The configuration
overlay files are copied to the /etc/systemd/system/SERVICE_NAME.service.d/
directories. Upon unloading if the directory is empty, it's removed.

With systemd, the 'start' directive is implemented by 'restart' in order
to allow loading of the service configuration file overlay.

With init systems other than systemd, it works only with the current runlevel.

Resolves: rhbz#1869991

Signed-off-by: Jaroslav Škarvada jskarvad@redhat.com

packit-as-a-service · 2020-08-21T11:18:35Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/redhat-performance-tuned-293
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

jmencak · 2020-08-21T15:41:16Z

First of all, thank you! This is great! I didn't go through the code much, but I did test the systemd init system part of this new service plugin and it does what's "on the tin". The only suggestion I have at this point is how should the "unapply" of settings be handled. Should a service be restarted in case it was running to go back to the old settings once the overlay file is gone? If so, should the restart be optional or automatic? I'll play with this PR more later on and see if I have any more suggestions. Many thanks!

jmencak · 2020-08-21T17:03:05Z

Just tried to run this inside a container. One problem I'm seeing is the runlevel detection. Inside a container, I'm getting runlevel "unknown". I'll try to dig more into this and see if I can make the runlevel detection possible. Failing that, is the runlevel detection really needed?

Edit: I was able to work around this by providing a custom runlevel script inside the container.

yarda · 2020-08-24T06:43:09Z

First of all, thank you! This is great! I didn't go through the code much, but I did test the systemd init system part of this new service plugin and it does what's "on the tin". The only suggestion I have at this point is how should the "unapply" of settings be handled. Should a service be restarted in case it was running to go back to the old settings once the overlay file is gone? If so, should the restart be optional or automatic? I'll play with this PR more later on and see if I have any more suggestions. Many thanks!

I think with the systemd the service is restarted even on unapply (in case it should be running). With other init systems it's only started, but it shouldn't be problem, because configuration overlay is supported only with the systemd.

yarda · 2020-08-24T06:44:15Z

Just tried to run this inside a container. One problem I'm seeing is the runlevel detection. Inside a container, I'm getting runlevel "unknown". I'll try to dig more into this and see if I can make the runlevel detection possible. Failing that, is the runlevel detection really needed?

Edit: I was able to work around this by providing a custom runlevel script inside the container.

In fact the runlevel is not used with the systemd handler, so I bypassed the check when systemd is in use.

jmencak · 2020-08-24T12:04:47Z

First of all, thank you! This is great! I didn't go through the code much, but I did test the systemd init system part of this new service plugin and it does what's "on the tin". The only suggestion I have at this point is how should the "unapply" of settings be handled. Should a service be restarted in case it was running to go back to the old settings once the overlay file is gone? If so, should the restart be optional or automatic? I'll play with this PR more later on and see if I have any more suggestions. Many thanks!

I think with the systemd the service is restarted even on unapply (in case it should be running). With other init systems it's only started, but it shouldn't be problem, because configuration overlay is supported only with the systemd.

Actually, I don't think the unapply/rollback is happening at the moment. So the question is whether it should -- and I believe so.

Consider scenario 1. The service is stopped/disabled. Then the Tuned starts/enables it. What should be the rollback? Can/Should we stop/disable the service in that case?

Scenario 2. The service is running. Tuned starts/enables the service but with different overlay file. Should Tuned restart the service (without the overlay) on the unapply/rollback?

There's probably quite a few scenarios. However, I believe that the Tuned daemon should probably try to unapply/rollback to the previous state as much as possible. To that end, it probably needs to keep track of what was the state of the service prior to running the [service] plugin. Can this be done?

yarda · 2020-08-24T13:11:05Z

Actually, I don't think the unapply/rollback is happening at the moment. So the question is whether it should -- and I believe so.

It should rollback in the _instance_unapply_static.

Consider scenario 1. The service is stopped/disabled. Then the Tuned starts/enables it. What should be the rollback? Can/Should we stop/disable the service in that case?

The service was stopped/disabled, so I think it should stop/disable the service. It should work this way now.

Scenario 2. The service is running. Tuned starts/enables the service but with different overlay file. Should Tuned restart the service (without the overlay) on the unapply/rollback?

Yes, exactly. It should also do it, but it seems I introduced bug, because I restart the service with the overlay, which is wrong. I will fix it.

MarSik · 2020-08-27T15:38:46Z

tuned/plugins/plugin_service.py

+		if not os.path.exists(path):
+			log.error("Service '%s' configuration not installed in '%s'" % (name, path))
+			return False
+		md5sum1 = cmd.md5sum(cfg_file)


I am a bit unsure about this. MD5?

Yep, it's very simple check that the file wasn't modified, it's unlikely that somebody will play with the md5 collisions on the service configuration overlay file just for the profile verification to fail :D. But no problem to change to e.g. sha-256 or whatever is supported by hashlib.

MarSik · 2020-08-27T15:39:29Z

In general it looks good and we saw it working, what can we do to get this merged? I see some builds failed.

yarda · 2020-08-27T19:48:38Z

In general it looks good and we saw it working, what can we do to get this merged? I see some builds failed.

I would like to see approval from Tuned developer, @olysonek could you please take a look?

olysonek · 2020-08-28T07:00:38Z

Hi, I'll get to it on Monday.

jmencak · 2020-08-28T16:28:36Z

I wonder if it would make sense to add some info messages like the bootloader plugin does for example with:

2020-08-28 16:01:32,583 INFO     tuned.plugins.plugin_bootloader: installing additional boot command line parameters to grub2

This might help troubleshooting this new functionality a bit even though these messages are probably much closer to debugging.

olysonek

Looks good to me overall. With the is_enabled issue resolved, consider this approved.

olysonek · 2020-08-31T08:27:07Z

tuned/plugins/plugin_service.py

+
+	def is_enabled(self, name, runlevel):
+		(retcode, out) = cmd.execute(["systemctl", "is-enabled", name], no_errors = [0])
+		return retcode == 0


If the enable status is "static", the exit code is 0 too. "static" means the unit is not enabled. See systemctl(1).

Looking at it again, it turns out "static" means the unit cannot be enabled. Not sure about the other enable statuses that result in a zero exit code. Still, I think it's worth looking into if you haven't done so.

With a "static" service, the following can happen. Let's say we have a profile as follows

[service] service.a=start

where a.service is a static service. On rollback of this profile, Tuned will unnecessarily/wrongly attempt to enable a.service.

2020-09-01 04:12:45,395 DEBUG tuned.utils.commands: Executing ['systemctl', 'enable', 'a'].

This is not exactly serious and it doesn't cause errors, but still worth fixing in my opinion.

olysonek · 2020-08-31T08:53:44Z

tuned/plugins/plugin_service.py

+		if cfg_file is None:
+			return None
+		path = "%s/%s" % (consts.SERVICE_SYSTEMD_CFG_PATH % name, os.path.basename(cfg_file))
+		if not os.path.exists(cfg_file):


Unrelated: this made me realize that there is a distinction between verifying the profile as it was applied and the profile as it currently exists on disk. Currently we do a mixture of the two. Oh, well..

There is a related issue regarding the script plugin and rollback. Let's say we have a script with code that applies and code that rolls back some tuning. If a new tuned version drops that code from the script (as we have recently done, IIRC), the tuning that has been done by the script will never be rolled back.

Just something to be aware of. In the future it might be nice to consolidate things and do backups of the profiles that are applied.

jmencak · 2020-08-31T13:34:24Z

One thing that will definitely need to be fixed is when the overlay configuration file is changed and then the Tuned daemon reloaded, you'll get something like this:

Warning: The unit file, source configuration file or drop-ins of XYZ.service changed on disk. Run 'systemctl daemon-reload' to reload units.

New env variable settings will not be reflected when the Tuned daemon is reloaded (e.g. by sending it a HUP signal). Hitting this issue right now.

jmencak · 2020-09-01T07:56:50Z

One thing that will definitely need to be fixed is when the overlay configuration file is changed and then the Tuned daemon reloaded, you'll get something like this:
Warning: The unit file, source configuration file or drop-ins of XYZ.service changed on disk. Run 'systemctl daemon-reload' to reload units.
New env variable settings will not be reflected when the Tuned daemon is reloaded (e.g. by sending it a HUP signal). Hitting this issue right now.

I think this can be easily fixed by adding something like this:

        def daemon_reload(self):
                cmd.execute(["systemctl", "daemon-reload"])

to SystemdHandler and then calling it from cfg_install() and cfg_uninstall(). Can we have this fixed, Jardo? Thanks!
/cc @MarSik

See: redhat-performance/tuned#293 (comment)

The service plugin can handle sysvinit, sysv-rc, openrc, systemd. Syntax is the following: [service] service.sendmail = start,enable,file:${i:PROFILE_DIR}/tuned-sendmail.conf 'start' means to start the service, 'stop' to stop, 'enable' to enable, 'disable' to disable, file:FILE is the overlay configuration file that will be installed for the service. Multiple directives can be separated by comma ',' or semicolon ';'. If the directives are conflicting, the last one will be used. The internal variable ${i:PROFILE_DIR} points to the directory from which the profile is loaded. The service plugin supports configuration overlays only for the systemd, for other init systems this directive is ignored. The configuration overlay files are copied to the /etc/systemd/system/SERVICE_NAME.service.d/ directories. Upon unloading if the directory is empty, it's removed. With systemd, the 'start' directive is implemented by 'restart' in order to allow loading of the service configuration file overlay. With init systems other than systemd, it works only with the current runlevel. Resolves: rhbz#1869991 Signed-off-by: Jaroslav Škarvada <jskarvad@redhat.com>

yarda · 2020-09-02T14:51:11Z

added log.info when installing/uninstalling the service configuration overlay file
is_enabled returns True if service is "enabled", False if service is "disabled", None otherwise
I haven't used 'systemctl daemon-reload' workaround, but more portable 'telinit q'

jmencak · 2020-09-02T16:15:35Z

* added log.info when installing/uninstalling the service configuration overlay file

* is_enabled returns True if service is "enabled", False if service is "disabled", None otherwise

* I haven't used 'systemctl daemon-reload' workaround, but more portable 'telinit q'

Thanks, I confirm the telinit q also works.

olysonek

Note that I'm unable to verify some of the non-systemd assumptions, but it looks good to me.

yarda · 2020-09-03T10:23:42Z

Thanks, merging.

yarda force-pushed the service-plugin branch from 041489b to 54cf8bd Compare August 24, 2020 06:34

yarda force-pushed the service-plugin branch from 54cf8bd to 6c834b8 Compare August 24, 2020 13:14

MarSik reviewed Aug 27, 2020

View reviewed changes

olysonek reviewed Aug 31, 2020

View reviewed changes

jmencak mentioned this pull request Aug 31, 2020

Bug 1878602: Support for stall daemon. openshift/cluster-node-tuning-operator#152

Merged

jmencak added a commit to jmencak/cluster-node-tuning-operator that referenced this pull request Sep 1, 2020

Adding an unofficial fix to the [service] plugin.

f132b64

See: redhat-performance/tuned#293 (comment)

yarda force-pushed the service-plugin branch from 6c834b8 to c85f3ab Compare September 2, 2020 14:49

olysonek approved these changes Sep 3, 2020

View reviewed changes

yarda merged commit 5444669 into redhat-performance:master Sep 3, 2020

yanirq mentioned this pull request Sep 8, 2020

Enable support for stall daemon in base profile openshift-kni/performance-addon-operators#340

Merged

Added plugin 'service' #293

Added plugin 'service' #293

yarda commented Aug 21, 2020

packit-as-a-service bot commented Aug 21, 2020

jmencak commented Aug 21, 2020

jmencak commented Aug 21, 2020 •

edited

yarda commented Aug 24, 2020

yarda commented Aug 24, 2020

jmencak commented Aug 24, 2020

yarda commented Aug 24, 2020 •

edited

MarSik Aug 27, 2020

yarda Aug 27, 2020

MarSik commented Aug 27, 2020

yarda commented Aug 27, 2020

olysonek commented Aug 28, 2020

jmencak commented Aug 28, 2020

olysonek left a comment

olysonek Aug 31, 2020

olysonek Sep 1, 2020

olysonek Aug 31, 2020

jmencak commented Aug 31, 2020 •

edited

jmencak commented Sep 1, 2020

yarda commented Sep 2, 2020

jmencak commented Sep 2, 2020

olysonek left a comment

yarda commented Sep 3, 2020

Added plugin 'service' #293

Added plugin 'service' #293

Conversation

yarda commented Aug 21, 2020

packit-as-a-service bot commented Aug 21, 2020

jmencak commented Aug 21, 2020

jmencak commented Aug 21, 2020 • edited

yarda commented Aug 24, 2020

yarda commented Aug 24, 2020

jmencak commented Aug 24, 2020

yarda commented Aug 24, 2020 • edited

MarSik Aug 27, 2020

Choose a reason for hiding this comment

yarda Aug 27, 2020

Choose a reason for hiding this comment

MarSik commented Aug 27, 2020

yarda commented Aug 27, 2020

olysonek commented Aug 28, 2020

jmencak commented Aug 28, 2020

olysonek left a comment

Choose a reason for hiding this comment

olysonek Aug 31, 2020

Choose a reason for hiding this comment

olysonek Sep 1, 2020

Choose a reason for hiding this comment

olysonek Aug 31, 2020

Choose a reason for hiding this comment

jmencak commented Aug 31, 2020 • edited

jmencak commented Sep 1, 2020

yarda commented Sep 2, 2020

jmencak commented Sep 2, 2020

olysonek left a comment

Choose a reason for hiding this comment

yarda commented Sep 3, 2020

jmencak commented Aug 21, 2020 •

edited

yarda commented Aug 24, 2020 •

edited

jmencak commented Aug 31, 2020 •

edited