Mask individual .wants/.requires symlinks #5231
Conversation
|
Rebased. |
|
I think your first commit is a mix of 2 unrelated things: the rewrite of the loading of .wants/.requires symlinks by using Regarding the implementation of the dropin mask: your version is much simpler because you chose to not handle dependencies specified via aliases.
yeah and that's where all the troubles are coming from. I think this should be clarified/discussed further as in my understanding some people expect that a mask using an alias name should mask the 'target' unit and all its aliases. And similarly a mask using an exact unit name should also mask all symlink dependencies specified through aliases. After all, an alias is nothing more than an additional name given to a unit and I don't see why this mechanism should work except for the dropin symlink mask.
I don't follow here. I don't see why a symlink dropin mask should also mask all Wants= specified in .d/*.conf dropins (if that's what you meant). |
src/core/load-dropin.c
Outdated
| continue; | ||
| } | ||
| if (r == 0) { | ||
| log_unit_warning(u, "%s dependency dropin %s is not a symlink, ignoring", |
There was a problem hiding this comment.
sentence should end with a full stop...
There was a problem hiding this comment.
I added dots to "all" full sentences. (Not really all, because "XXX: %m" is also a full sentence, but we don't use a dot there.) I think those dots only make messages longer, but OK.
|
I am fine with not covering the alias-case btw. I think masking a unit should really have the effect that the unit is masked by all names, even if it has many (which is something we don't implement correctly right now — see #4122). However, for dependencies I think that only the precise specified dependency should be masked, not necessary under all names. |
…conf dropins Essentially, instead of sequentially adding deps based on all symlinks encountered in .wants and .requires dirs for each name and each unit file load path, iteratate over the load paths and unit names gathering symlinks, then order them based on priority, and then iterate over the final list, adding dependencies. This patch doesn't change the logic too much, except that the order in which dependencies are applied might be different. It wasn't defined before, so that not really a change. Adding filtering on the symlinks is left for later patches.
Fixes systemd#1169. Fixes systemd#4830. Example log errors: Feb 04 22:13:28 systemd[1462]: foo.service: Wants dependency on empty_file.service is masked by /home/zbyszek/.config/systemd/user/foo.service.wants/empty_file.service, ignoring Feb 04 22:13:28 systemd[1462]: foo.service: Wants dependency on masked.service is masked by /home/zbyszek/.config/systemd/user/foo.service.wants/masked.service, ignoring
Feb 04 22:35:42 systemd[1462]: foo.service: Wants dependency dropin /home/zbyszek/.config/systemd/user/foo.service.wants/diffname.service target ../barbar.service has different name Feb 04 22:35:42 systemd[1462]: foo.service: Wants dependency dropin /home/zbyszek/.config/systemd/user/foo.service.wants/wrongname is not a valid unit name, ignoring
[zj: tests assertions adjusted to the different logic in which masking
of a dependency through one name, does not forbid the dependency
being added through another name.]
OK. I don't think it makes that much of a difference, but I did a split into three commits:
I don't see it this way. In this patchset, the mask operates only a specific symlink. Doing anything else would be inconsistent. In particular consider a unit which has Wants=some.target in the unit file and also a .wants/some.target. I'm stating that I think that masking the symlink should not interfere with the unit file, i.e. the dependency should still be present. Also, let's consider the case where a unit has .wants/multi-user.target and .wants/default.target. And default.target is symlinked to multi-user.target. If I now mask the dep on default.target, it doesn't necessarily mean that I want to lose both deps. In fact, I might expect the opposite. Anyway, I think such doubled deps should be a rare case. If this turns out to be a problem and causes user confusion, we can always reconsider. |
|
On 02/08/2017 03:56 AM, Zbigniew Jędrzejewski-Szmek wrote:
I don't see it this way. In this patchset, the mask operates only a
specific symlink. Doing anything else would be inconsistent. In
particular consider a unit which has Wants=some.target in the unit file
and also a .wants/some.target. I'm stating that I think that masking the
symlink should not interfere with the unit file, i.e. the dependency
should still be present.
Again I don't think I claimed the opposite and my patch definitively
doesn't implement that.
And it's probably due to my wording. When I wrote: "a mask using an
alias name should mask the 'target' unit and all its aliases."
I meant that the (symlink) mask should mask all symlinks pointing to the
"target/final" unit and all symlinks pointing to its aliases.
Also, let's consider the case where a unit has .wants/multi-user.target
and .wants/default.target. And default.target is symlinked to
multi-user.target. If I now mask the dep on default.target, it doesn't
necessarily mean that I want to lose both deps. In fact, I might expect
the opposite.
Well this point is debatable. I don't necessarily say that your point of
view is wrong but we shouldn't choose it only because its implementation
is much simpler than the case where we support aliases properly.
For instance let's say "network.service" is an alias for
"NetworkManager.service". But it can also be an alias for
"systemd-networkd.service" depending on which network manager has been
configured by the user.
If the user does "systemctl disable network.service", I have the feeling
that the user can expect (correctly) that it will disable the underlying
network manager in use.
|
|
BTW I think you should document the symlink mask somewhere since it introduces a new concept and also the limitation regarding the alias names when used as masks. |
This is an alternative version of #5195, that implements the logic suggested in #4830 (comment).
It does away with the cache of deps and deferred loading, and unifies the logic of loading
.d/*.confwith the loading of.wants/*and.wants/*. I think this is a superior solution because it is much simpler (in fact the diffstat of the first two commits is negative), unifies two different ways of loading "stuff" from drop-in directories, and seems more in line with the way we handle other deps.In #5195, adding a mask on a .wants dependency not only kills that exact dependency, but also dependencies specified through aliases. But it does not kill aliases specified as Wants=. I dislike this special treatment of aliases and magic behaviour. In my solution, masking a dep in .wants is like removing the symlink that created that dep, it changes nothing else.
This should probably get some documentation, but I'll wait to see if people like this approach.