Safe-Reload: Support command line argv

[yhabteab]

resolves #8911 refs #8971

[yhabteab]

Actually it does have a way and that is the following ${@:1}, however, as we are using sh shebang for whatever reason, this syntax is invalid.

[Al2Klimov]

Colleagues, is it reasonable to require bash?

[julianbrost]

What's the problem with just using shift? Does the job and works in a POSIX shell.

Also, would the proper way to set -D parameters be having two update both commands in the unit file where the change to safe-reload is non-obvious and will probably be forgotten? Or do we rather want to support it properly by adding something like our Debian init.d script already does with $DAEMON_ARGS?

[Al2Klimov]

Also, would the proper way to set -D parameters be having two update both commands in the unit file where the change to safe-reload is non-obvious and will probably be forgotten?

Good point! #7349 would do that better (but lacks feedback, yet).

[julianbrost]

I think this could be a valid solution as well, given that the daemon itself also checks the config before performing the actual reload. So we could possibly just get rid of the safe-reload script at all if we go that way.

[Al2Klimov]

And we'd get 2x speed!

[Al2Klimov]

... btw. on every reload via systemctl:

https://github.com/Icinga/icinga2/blob/1265eb76cc658e31743977305ab6d4ee6a704ffb/etc/initsystem/icinga2.service.cmake#L14

@lippserd Consider #7349 for 2.14.

[Al2Klimov]

That would require switching from sh to bash for this file.

I could live with that unless...

Shouldn't shift be enough to get rid of the entire loop?

[yhabteab]

That would require switching from sh to bash for this file.

I could live with that unless...

Shouldn't shift be enough to get rid of the entire loop?

https://github.com/Icinga/icinga2/pull/8971

[julianbrost]

Under the premise that the signature of the safe-reload should be changed so that extra icinga2 daemon arguments can be added at the end, #8971 is pretty much the exact change I would do.

The fundamental question is how to add extra command line arguments to the icinga2 daemon call. With the current proposal, you'd have to change this:

ExecStart=/usr/sbin/icinga2 daemon --close-stdio -e ${ICINGA2_ERROR_LOG}
ExecReload=/usr/lib/icinga2/safe-reload /etc/default/icinga2

Into something like this:

ExecStart=/usr/sbin/icinga2 daemon --close-stdio -e ${ICINGA2_ERROR_LOG} -DFoo=Bar
ExecReload=/usr/lib/icinga2/safe-reload /etc/default/icinga2 -DFoo=Bar

Where the change to the second line is totally non-obvious and will likely be missed resulting in surprising behavior. If that behavior doesn't immediately result in an error, it might just go unnoticed (but also things won't work as expected), otherwise it results in some annoying debugging.

Also looks like we're going in circles here: my comment https://github.com/Icinga/icinga2/pull/8973#issuecomment-903652827 still fully applies. So with the other idea from that comment, the icinga2.service file would look something like this by default:

ExecStart=/usr/sbin/icinga2 daemon --close-stdio -e ${ICINGA2_ERROR_LOG} $DAEMON_ARGS
ExecReload=/usr/lib/icinga2/safe-reload /etc/default/icinga2

This would immediately hint that you can add custom arguments in that variable and if you set it (either in /etc/default/icinga2 (or the corresponding path on other distros) or an Enviornment=... line in the unit file), the safe-reload script could automatically pick it up and it would just work. Only downside I can see with that approach is that you can't specify parameters that would need additional quoting, but that should be fine for most cases (as long as you don't start using spaces in paths and want to set them with -D).

One alternative could be – if systemd supports it ­– something like ExecReload=/usr/lib/icinga2/safe-reload /etc/default/icinga2 $ExecStart (i.e. automatically append the command from the ExecStart directive). I haven't checked if systemd allows something like this but I doubt it.

Another alternative would indeed be something like #7349 as this would remove the need for safe-reload and thereby the need to pass arguments to it altogether, but that won't be a quick fix compared to the other options.

[Al2Klimov]

Quick? Indeed not. But an additional benefit you din't mention is 2x shorter reload time due to only one validation and no race condition inbetween the two. Also the caller would get feedback about reload success. Do we agree that #7349 just rocks 🪨🪨 (conceptionally)?

[julianbrost]

But an additional benefit you din't mention is 2x shorter reload time due to only one validation and no race condition inbetween the two.

Only for reloads triggered from systemd or init.d. Config deployments using the config package API not affected by this.

Also the caller would get feedback about reload success.

How would that differ? Isn't that pretty much the purpose of the safe-reload script (apart from the race condition that it might not check exactly the same config that would be loaded later).

Do we agree that #7349 just rocks :rock::rock: (conceptionally)?

Getting rid of an extra config validation that's just done to obtain the output from it when the same can be obtained from the actual config load during the reload is ultimately a good idea. But that PR isn't close to being ready to be merged.

Just using $DAEMON_ARGS in the service file and safe-reload would provide a quick (just a few lines changed) fix for anyone who wants to pass custom arguments. Also, compared to this PR, it would not involve changing the ExecReload directive in the systemd unit file, so that an Icinga 2 update can just change the ExecReload command to something better without breaking because the command was modified in an override file (as it would be required with this PR). Also, that updated version can just continue to pick up the arguments from $DAEMON_ARGS, so it would just work.