Project

General

Profile

Actions

Refactor #32108

closed

Refactor service management of foreman-maintain

Added by Amit Upadhye over 3 years ago. Updated 4 months ago.

Status:
Rejected
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Difficulty:
Triaged:
No
Fixed in Releases:
Found in Releases:

Description

The improvement thoughts from Ewoud:

It's interesting that the ForemanMaintain::Utils::Service::Abstract does not define this. Effectively it means that on non-systemd it will fail. Luckily we don't have that anymore, but it's a good thing to be aware of.

If we read the systemctl man page, we get a nice table of statuses. What mostly jumps out to me is enabled-runtime as a possible return status when it's enabled via /run/systemd/system/ instead of /etc/systemd/system/. I don't think we use that today, but this can be a potential failure mode.

On a related note, the exist? definition doesn't deal with this either and can also fail on masked (and masked-runtime) where a service does exist but is prevented from starting.

Possible use cases for -runtime would be maintenance mode. Services could be masked in runtime only so that they are started again after a reboot but disallow starting it during the maintenance window. This could be used together with systemctl isolate to let systemd manage all services. It is possible that systemctl rescue is sufficient. This would also inform systemd it's in maintenance mode. These are just thoughts about embracing system-provided utilities for this and possible alignment. At this point it looks like the code would be compatible with it.

My next question is what it affects. There are many layers in foreman-maintain and it's a bit hard to follow. It would be nice if there were some tests to show some scenarios.

Something else I noticed is that it uses systemctl status but according to the same systemctl man page that's really intended for humans. Computers should use systemctl show, possibly together with --property. This also gives you the property UnitFileState which appears to be the same as systemctl is-enabled. It means you can bulk query all services instead of invoking systemctl for every service. This may lead to a lot less process overhead resulting in faster execution.

I also realize that that's probably a larger rewrite. It is roughly the approach I took in foreman-installer but there I use list-units since I was interested in what's available and what's running.

Actions #1

Updated by Amit Upadhye over 3 years ago

  • Tracker changed from Bug to Refactor
Actions #2

Updated by Eric Helms 4 months ago

  • Status changed from New to Rejected
Actions

Also available in: Atom PDF