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

There are some cases where the unit file enablement state might be surprising #9569

Open
fbuihuu opened this issue Jul 11, 2018 · 18 comments
Open

Comments

@fbuihuu
Copy link
Contributor

fbuihuu commented Jul 11, 2018

I must admit that the code is pretty hard to follow so I may miss something but some testing (see below) seems to confirm that there are some shortcomings at least ;)

For example it tries to search for any symlink pointing to a given unit in the loading paths and if (and only if) such a symlink is found in /etc/systemd/system then the unit is considered as enabled.

Such logic can lead to false-positives:

  • If the distro decides to install a symlink in /usr/lib/ to enable (by default) a unit then the unit will be reported as disabled:
# find /etc/systemd -name test\*.service
# find /usr/lib/systemd -name test\*.service
/usr/lib/systemd/system/default.target.wants/test.service
/usr/lib/systemd/system/test.service
# systemctl show -pUnitFileState  test.service
UnitFileState=disabled
  • Installing an alias can fool the logic:
# find /etc/systemd -name test\*.service
/etc/systemd/system/test-alias.service
# ll /etc/systemd/system/test-alias.service
lrwxrwxrwx 1 root root 36 Jul 11 15:30 /etc/systemd/system/test-alias.service -> /usr/lib/systemd/system/test.service
# find /usr/lib/systemd -name test\*.service
/usr/lib/systemd/system/test.service
# systemctl show -pUnitFileState  test.service
UnitFileState=enabled

It would be nice if the code could be improved in this area.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 11, 2018

I'm wondering why the code doesn't simply search for symlinks in paths ending with .wants.d/.requires.d to figure out if a unit is enabled or not...

@sourcejedi
Copy link
Contributor

sourcejedi commented Jul 11, 2018

systemctl disable removes aliases installed in /etc/systemd/system/. In principle a unit might only have aliases installed, and not be installed as a dependency of any unit. Indeed...

$ systemctl cat reboot.target
...
[Install]
Alias=ctrl-alt-del.target

so we'd need some solid reasoning if we were going to start some transition there.

If the distro decides to install a symlink in /usr/lib/ to enable (by default) a unit then the unit will be reported as disabled.

Here is a counter-example:

$ systemctl show -pUnitFileState dev-hugepages.mount
UnitFileState=static
$ systemctl status dev-hugepages.mount
● dev-hugepages.mount - Huge Pages File System
   Loaded: loaded (/usr/lib/systemd/system/dev-hugepages.mount; static; vendor preset: disabled)

@sourcejedi
Copy link
Contributor

That is, if you want to enable by installing a symlink in /usr/lib/systemd/system, your unit should not have any WantedBy= in the [Install] section. Then we will show the unit as static, and not disabled.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 12, 2018

That is, if you want to enable by installing a symlink in /usr/lib/systemd/system, your unit should not have any WantedBy= in the [Install] section.

I don't really see why and we agreed that distro defaults (defined by presets) should live in /usr/lib...

@sourcejedi
Copy link
Contributor

sourcejedi commented Jul 12, 2018

systemd "presets", live in /usr/lib/systemd/system-preset/. But when you run systemctl preset my-service, it does a normal enable, which creates links under /etc/systemd/system. (Or a normal disable, depending on what your presets say to do).

There is a very solid reason for this. If you declare your unit with [Install], you are saying that it can be enabled and disabled. This is considered changing the configuration. Configuration lives in /etc.

We never want systemctl disable to remove /usr/lib/systemd/system/default.target.wants/test.service. If an OS package installs files in /usr, this signifies that the package has full ownership of the files, it is not intended as an interface for the administrator to edit.

@poettering
Copy link
Member

If the distro decides to install a symlink in /usr/lib/ to enable (by default) a unit then the unit will be reported as disabled:

Well, a unit should be one of two things: "statically" enabled, in which case it should have one or more symlinks that enabled it in /usr, and not carry an [Install] section, or be "dynamically" enabled, in which case it should carry an [Install] section, and no symlinks in /usr. In your case you are creating something that both declares that it is something the user should enable, but then you enable it statically anyway. So yes, this will confuse systemd, because you did contradicting stuff with it. Decide whether the user shall enable it or if it is enabled statically. In the former case make it carry an [Install] section, and in the latter, remove any such section.

@poettering
Copy link
Member

Installing an alias can fool the logic:

Well, aliases are considerd "enabling", this goes through the entire design, i.e. that's why there is an "Alias=" stanza in [Install] after all.

We generally are quite permissive when checking how a unit is enabled: we do not verify whether the configuration in effect matches the one in [Install] in order to allow user modification, and allow sane upgrades: unit files may change their [Install] sections if they like, which is considered if the unit is enabled some time later, but this shouldn't change the fact that the unit was previously enabled. Also, this logic also matches the logic in "systemctl disable" which removes all symlinks in /etc, regardless of their name.

Quite frankly, yes, there's some niche behaviour that might be surprising, but I think it is quite systematic the way it is.

@poettering
Copy link
Member

So, I am not sure what we should do with this issue. i.e. please be more precise in what you are requesting.

@poettering poettering changed the title The logic to get the state of a unit file is borked There are some cases where the unit file enablement state might be surprising Jul 13, 2018
@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 16, 2018

it should have one or more symlinks that enabled it in /usr, and not carry an [Install] section, or be "dynamically" enabled, in which case it should carry an [Install] section,

@poettering that's not what we agreed on in issue #4830: in my understanding we decided to separate the choices of the admins from the distro's ones. If sysadmin chooses to disable a service for example, this should be made persistent no matter how the preferences of the distro (presets) will evolve. And this looks cleaner IMHO.

Regarding the current implementation, dependency symlinks are always in .require/.wants directories so the implementation should look into this directory only in order to figure out if a service is enabled/disabled. This seems more robust than the current code which simply searches for the symlinks in /etc/systemd/system as it can be easily fooled by some cases.

@poettering
Copy link
Member

@poettering that's not what we agreed on in issue #4830: in my understanding we decided to separate the choices of the admins from the distro's ones. If sysadmin chooses to disable a service for example, this should be made persistent no matter how the preferences of the distro (presets) will evolve. And this looks cleaner IMHO.

Sure, but so far nobody implemented #4830. We can certainly implement that, but until then, with the change of semantics that will introduce the current logic applies, and in that logic doing [Install] and doing symlinks in /usr is somewhat contradictory

@poettering
Copy link
Member

Regarding the current implementation, dependency symlinks are always in .require/.wants directories so the implementation should look into this directory only in order to figure out if a service is enabled/disabled. This seems more robust than the current code which simply searches for the symlinks in /etc/systemd/system as it can be easily fooled by some cases.

again, there's Alias= in [Install] and some units only have that as single stanza in there. we hence should also look for alias symlinks to determine whether a unit is enabled or not.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 16, 2018

Sure, but so far nobody implemented #4830.

Actually I was looking at it and found out that the current logic is pretty fragile even though it works with the usual cases by now.

We can certainly implement that, but until then, with the change of semantics that will introduce the current logic applies

OK then if we extend the logic to handle dependency symlinks in /usr/lib, wouldn't it be appropriate to rewrite the logic so

  • enable status can be found in all persistent search paths (not only in /etc/) and the symlinks must be in a directory which ends with .requires/.wants ?

  • we should follow these symlinks until we reach unit files so aliases are properly handled

Thanks.

@poettering
Copy link
Member

i

OK then if we extend the logic to handle dependency symlinks in /usr/lib, wouldn't it be appropriate to rewrite the logic so

As I wrote twice above, we should really consider alias symlinks as ways to enable a unit, too, as Alias= is a commonly used part of [Install] and often the only one, and hence should have an effect on the enablement state

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 16, 2018

OK got your point now.

I thought that the enablement state was only something related to the unit being able to be pulled in (and therefore activated) somehow but it seems that it's more related to "systemctl enable" being called.

But if we still follow this logic and once distros will be able to install symlinks in /usr/lib (according to the preset states), a unit could be reported as disabled whereas it will be pulled in via a symlink installed in /usr/lib/. Is that what you have in mind ?

That might be confusing but maybe it's just me...

@poettering
Copy link
Member

But if we still follow this logic and once distros will be able to install symlinks in /usr/lib (according to the preset states), a unit could be reported as disabled whereas it will be pulled in via a symlink installed in /usr/lib/. Is that what you have in mind ?

Well, when we add that i figure we need to revisit this logic, and probably consider all symlinks in all dirs (i.e. both /etc and /usr), and then remove all symlinks that are masked from that list, and then report if the result set is empty as "disabled" and otherwise as "enabled", or so. but dunno, there might be some corner cases I am not seeing...

@sourcejedi
Copy link
Contributor

sourcejedi commented Jul 16, 2018

Ah. I did not know there was a specific agreement being referred to. So far I agree with poettering.

I think currently systemctl {is-enabled,enable,disable} works just as well on Alias= installs as it does for WantedBy= installs. I think poettering's suggestion would let this keep working with Alias=. It means systemctl {is-enabled,enable,disable} keeps working as it did before, with existing units like reboot.target.

IIUC, we're currently in a confusing transition period where we've got an added feature in #5231, but we don't yet have the support particularly in systemctl disable to really make it useful. Aka "so far nobody implemented #4830".

What I might be missing is that I haven't quite gotten my head round the original motivation. I'm imagining it works nicely in a distribution which does not use systemd.preset. But I don't have a full understanding of what you're deciding between, when you decide if a hypothetical new distribution uses systemd.preset, or exploits the new mechanism and has packages drop symlinks into /usr.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 17, 2018

It means systemctl {is-enabled,enable,disable} keeps working as it did before, with existing units like reboot.target

I've always found this case pretty moot actually: "systemctl is-enabled ctrl-alt-del.target" reports that the target is disabled but if I press Ctrl+Alt+Del then the system will still reboot since the alias actually exists (in /usr).

What I might be missing is that I haven't quite gotten my head round the original motivation.
[...]

In my understanding packages will drop symlinks into /usr still based on the preset states so services are still enabled/disabled based on the distro policies by default. This is mainly to avoid cluttering /etc with symlinks which are distro defaults.

@fbuihuu
Copy link
Contributor Author

fbuihuu commented Jul 17, 2018

Well, when we add that i figure we need to revisit this logic, [...]

OK though we could revisit the logic right now as it wouldn't hurt I think.

In fact it might even improve the previous example given by @sourcejedi with ctrl-alt-del.target as systemctl is-enabled would report the target as enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants