Fixes #19053 - properly respond to reload action

[lzap]

So the problem here is the standard practice is to send SIGHUP to daemons to release log files after rotating, base RHEL SELinux policy includes rules to allow logrotate to send this via systemctl reload command. Our proxy only supports SIGUSR1 which is not standard and therefore the only way supporting this is by doing changes in our SELinux policy. This includes creating two types, domain, macro, file contexts - this is too complicated. It is much better to follow what is expected and change our service to respond to reload properly, then we can change our logrotate script as well.

Therefore I am opening this and making the change in proxy to properly reopen logs when "reload" init command is sent.

[ekohl]

Does this also reload the config? As a user I'd expect that and be surprised if it didn't. If it does 👍

[dmitri-d]

@ekohl: no, this would only rotate logs. @lzap: is the plan to eventually support config reloads? This would require some work (mostly around cleaning up of various threads spawned by modules), although I don't think a huge amount.

I'm ok with this change.

[ekohl]

If it doesn't reload the configs then 👎 from me. As a user I would be very surprised if a reload only rotated the logs.

[dmitri-d]

As I mentioned, adding full config reload functionality will require some work. Config reload will amount to restarting the proxy -- is there actually need for something like this? Would it be acceptable to do a restart when reload is called (that would result in a different pid)?

[ekohl]

man systemctl states systemctl reload will Asks all units listed on the command line to reload their configuration. Currently the patch breaks the interface/contract with users and scripts.

[lzap]

@ekohl this contract was written by systemd, if you look into logrotate scripts, most if not all services do send SIGHUP. Systemd does not care of course because they invented "the journal" and considers logging and rotating files as bad idea.

I have no intentions to write log reloading, no. @ekohl would a warning in proxy logs explaining that config reloading is not yet implemented be sufficient?

[lzap]

@ekohl one more thing, this is what happens today:

systemctl reload foreman-proxy

Nothing happens, signal is ignored. My proposal:

systemctl reload foreman-proxy

And logs are reopened. I do not think this is a bad change in general. It is confusing today already.

For the record the very same problem is in dynflow process: http://projects.theforeman.org/issues/21032

[yoctozepto]

@lzap You sure? Before the change it said:

Job type reload is not applicable for unit foreman-proxy.service.

No signal is sent and there is a clear rejection.

But, I am against logging spam messages about routine tasks... Not all services reload their configs on reload. Sometimes only logs and some spool/journal files are written and config is unchanged. For the time being it would be best to document it somewhere. Even in the service file itself.

PS:
I have one issue with this log rotation with signal (but this does not directly concern this systemd stuff): since it is asynchronous, there is still a slight chance that foreman-proxy logs something to the old file which was already compressed by logrotate and removed. I guess it would be better if we had some synchronous method to deal with it.

[ekohl]

No signal is sent and there is a clear rejection.

@yoctozepto that is exactly what I meant. This is clear behavior as a user would expect it.

@ekohl this contract was written by systemd, if you look into logrotate scripts, most if not all services do send SIGHUP. Systemd does not care of course because they invented "the journal" and considers logging and rotating files as bad idea.

That is irrelevant. As a user I expect that if I can use reload then it'll reload the config. Users or config management tools might have scripts that use systemctl reload-or-restart.

That SELinux only allows a reload for logrotation really surprises me because I might be editing a config just as the logrotation takes place which could break my system but I realize that this pattern is already quite common.

I have one issue with this log rotation with signal (but this does not directly concern this systemd stuff): since it is asynchronous, there is still a slight chance that foreman-proxy logs something to the old file which was already compressed by logrotate and removed. I guess it would be better if we had some synchronous method to deal with it.

I've seen many logrotations that only gzip the next day. So today it moves file.log to file.log.1 and tomorrow file.log.2.gz (though the file.log-YYYYMMDD is a much better pattern but allows for something similar). That would mean that you don't lose data. Another is to move, signal and gzip if you see the process no longer has a file handle open but that's much more complex.

[yoctozepto]

@yoctozepto that is exactly what I meant. This is clear behavior as a user would expect it.

I know, I agree with you on that.

That SELinux only allows a reload for logrotation really surprises me because I might be editing a config just as the logrotation takes place which could break my system but I realize that this pattern is already quite common.

Still, some software uses kill directly to send their signals avoiding all the problems with reload.

I've seen many logrotations that only gzip the next day. So today it moves file.log to file.log.1 and tomorrow file.log.2.gz (though the file.log-YYYYMMDD is a much better pattern but allows for something similar). That would mean that you don't lose data. Another is to move, signal and gzip if you see the process no longer has a file handle open but that's much more complex.

That's what I had in mind as an alternative solution but the problem is foreman-proxy does not use delaycompress in its logrotate script yet but still uses compress.

[lzap]

When you rename a file with processed appending to it, it just works. Do not understand your async design there. Anyway, I get your points, I just think we are trying to solve generic problem with reload here (it should be done somewhere else like in systemd tree).

@witlessbird just to doublecheck, the new initializing design does not allow reloading of configs, does it? Before I will start doing SELinux changes, I want to make sure there is not an option to easily add reload into proxy (together with log reopening).

[yoctozepto]

When you rename a file with processed appending to it, it just works. Do not understand your async design there.

It does. But here we also compress it by default by calling gzip which compresses the file in some state and then removes it. Some entries can be lost this way. The solution is to also add delaycompress.

[dmitri-d]

the new initializing design does not allow reloading of configs, does it?

Config reloads have not been implemented. They would pretty much complete restarts anyway.

[lzap]

Thanks.