Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use systemctl's helper command to determine enabled & active status #863

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

stevendanna
Copy link
Contributor

@stevendanna stevendanna commented Aug 3, 2016

The output of systemctl show SERVICENAME can be misleading in the
case of non-native services (i.e. services configured via an init script
and integrated with systemd via a shim) or for more sophisticated unit
types.

For example, the UnitFileState of ntp is "bad":

> systemctl show ntp | grep UnitFileState
UnitFileState=bad

despite systemd reporting it as enabled:

> systemctl is-enabled ntp
ntp.service is not a native service, redirecting to
systemd-sysv-install
Executing /lib/systemd/systemd-sysv-install is-enabled ntp
enabled

Further, the old parsing code would have missed unit files in the
following states that are technically enabled:

enabled-runtime, indirect, generated, and transient

Using the is-enabled commands ensures that we report the same enabled
status that systemd reports, without having to update our own parsing in
the event that new unit states are added. Additionally, as shown above,
it handles the sysv compatibility helper.

Similarly, the is-active helper command ensures that we always report
the same active/not-active status as systemd would natively. For
instance, a quick reading of src/systemctl/systemctl.c in the systemd
source shows that systemctl reports units as active if they are in the
state UNIT_ACTIVE or UNIT_RELOADING.

Fixes #749

Signed-off-by: Steven Danna steve@chef.io

@stevendanna
Copy link
Contributor Author

Doing some manual testing of this until I can fix up some of the test environments.

@stevendanna
Copy link
Contributor Author

sdanna@thrace ~/oc/code/opscode/inspec (master) > be bin/inspec shell -t ssh://127.0.0.1:2200
Welcome to the interactive InSpec Shell
To find out how to use it, type: help

inspec> service('ntp').enabled?
=> false
inspec>

sdanna@thrace ~/oc/code/opscode/inspec (master) > git co ssd/issue-749
Switched to branch 'ssd/issue-749'
sdanna@thrace ~/oc/code/opscode/inspec (ssd/issue-749) > be bin/inspec shell -t ssh://127.0.0.1:2200 
Welcome to the interactive InSpec Shell
To find out how to use it, type: help

inspec> service('ntp').enabled?
=> true

@stevendanna stevendanna changed the title WIP: Use systemctl's helper command to determine enabled & active status Use systemctl's helper command to determine enabled & active status Aug 3, 2016
@chris-rock
Copy link
Contributor

@stevendanna Can you explain, why the information we retrieve from systemd is not enough? This PR would introduce more commands.

The output of `systemctl show SERVICENAME` can be misleading in the
case of non-native services (i.e. services configured via an init script
and integrated with systemd via a shim) or for more sophisticated unit
types.

For example, the UnitFileState of ntp is "bad":

    > systemctl show ntp | grep UnitFileState
    UnitFileState=bad

despite systemd reporting it as enabled:

   > systemctl is-enabled ntp
   ntp.service is not a native service, redirecting to
   systemd-sysv-install
   Executing /lib/systemd/systemd-sysv-install is-enabled ntp
   enabled

Further, the old parsing code would have missed unit files in the
following states that are technically enabled:

   enabled-runtime, indirect, generated, and transient

Using the `is-enabled` commands ensures that we report the same enabled
status that systemd reports, without having to update our own parsing in
the event that new unit states are added. Additionally, as shown above,
it handles the sysv compatibility helper.

Similarly, the is-active helper command ensures that we always report
the same active/not-active status as systemd would natively. For
instance, a quick reading of `src/systemctl/systemctl.c` in the systemd
source shows that systemctl reports units as active if they are in the
state `UNIT_ACTIVE` or `UNIT_RELOADING`.

Fixes #749

Signed-off-by: Steven Danna <steve@chef.io>
@stevendanna
Copy link
Contributor Author

@chris-rock Added a comment with some justification of moving to the helper commands. Let me know your thoughts. I agree having to run 3 commands to get the status of a service is a big downside. The alternative is to add a bit more sophisticated handling that replicates what systemctl does. In the past systemd was moving more quickly, but given that new unit states haven't been added in some time, perhaps it wouldn't be that bad.

@chris-rock
Copy link
Contributor

We discussed that it is more stable to call systemd for enabled and running, to ensure we are not missing any edge-case. We take into account that this adds some additional calls, but since they are fast we decided not to do the effort to replicate systemd behavior.

Thanks @stevendanna for the great discussion!

@chris-rock chris-rock merged commit e6f73ff into master Aug 3, 2016
@chris-rock chris-rock deleted the ssd/issue-749 branch August 3, 2016 13:22
@stevendanna
Copy link
Contributor Author

Comments from an in-person discussion:

  • In the long run we may want to alter how service information gathering works to get information for /all/ services and cache it. Having to make per-service calls like this will interfere with that. However, this class will have to change substantially anyway to support that and we can always back this change out during that work.
  • The alternative to this would be to replicate some of the logic in systemctl itself including:
    • Falling back to the shim when no unit file exists for that file
    • Handling all of the currently missing states for enable and active.
    • Handle /usr/lib vs /lib differences on various platforms with respect to the location of systemd-sysv-install and unit files.
    • Ongoing maintenance as systemd changes

None of these tasks are hard individually, but together it would be nice to have some of the testing improvements (#861 and #862) in place before we do it.

@chris-rock chris-rock modified the milestone: 0.29.0 Aug 4, 2016
@chris-rock chris-rock added the Type: Bug Feature not working as expected label Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants