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

Make unit DefaultDependencies apply to their default slices #2235

Closed
wants to merge 1 commit into from

Conversation

martinpitt
Copy link
Contributor

Since commit 8c8da0e instantiated units have a Requires= dependency to their slice. This causes units to get stopped on shutdown even if they don't conflict to shutdown.target themselves, as their parent slice does have DefaultDependencies=yes.

To avoid the unintended stopping of instantiated units on shutdown, stop adding default dependencies to their default slice.

Fixes #2189

This is a conservative approach which keeps the DefaultDependencies=yes for e. g. the user-XXXX.slice. I'm not sure about whether those should also not be stopped explicitly on shutdown. If we never need to stop slices, we could instead drop slice_add_default_dependencies in slice.c completely.

Since commit 8c8da0e instantiated units have a Requires= dependency to their
slice. This causes units to get stopped on shutdown even if they don't conflict
to shutdown.target themselves, as their parent slice does have
DefaultDependencies=yes.

To avoid the unintended stopping of instantiated units on shutdown, copy the
unit's DefaultDependencies flag to their default slice so that they agree on
the shutdown behaviour.

Fixes systemd#2189
@martinpitt
Copy link
Contributor Author

I force-pushed a refined patch which, instead of always setting the slice's DefaultDependencies=false, copies the value from the unit itself. That way the unit and their slice agree on their behaviour on shutdown and have the same value. IMHO this is more intuitive and consistent.

@martinpitt martinpitt changed the title Don't add default dependencies to unit default slices Make unit DefaultDependencies apply to their default slices Dec 29, 2015
@poettering
Copy link
Member

I thought about this for a while, and I am now pretty sure this should be solved differently: I don't like patching from the code of one unit the settings of another, possibly overriding explicit user configuration. Also, it's really incomplete, as each slice might implicitly load+start their parent slices implicitly, which would also have to inherit the flag?

Instead I'd like to propose that the slice units in question are explicitly configured to use DefaultDependencies=no. Specifically this would mean to add slice unit files adjacent to all template units that have DefaultDependencies=no. These slice units should then explicitly set DefaultDependencies=no (and of course, setting a nice Description= would be cool too).

Yes, I acknowledge that this is a bit of a compat breakage, but it's really only one for very low-level, specialist services, so I'd argue that this is ok. But it deserves documentation I figure...

I tried to come up with other ideas. For example we could just leave all .slice units up all the time, for-ever, but then again, cgroups aren't free, we probably should clean them up eventually... Hence I figure not trying to be smart here is probably the best option, and just ask people to configure their own slices....

(An alternative is to override the default Slice= setting in the service unit file btw, and move the service's slice to system.slice explicitly)

Anyway, I'd suggest ifupdown starts shipping a slice unit system-ifup.slice like this:

[Unit]
Description=Network Configuration Slice
DefaultDependencies=no

Does that make sense?

@poettering poettering added pid1 needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer labels Jan 12, 2016
@martinpitt
Copy link
Contributor Author

Adding Slice=system.slice or adding an explicit .slice unit indeed work, I didn't think of that. Thanks for the hint!

But it deserves documentation I figure...

An appropriate place would be in the documentation of Slice= itself in systemd.resource-control(5). Not totally easy to discover, but it's the place that documents the fact that instantiated units get their own slice. So after "Instance units are by default placed in a subslice of system.slice that is named after the template name." we could perhaps add something like

By default these instance unit slices get stopped on shutdown; if that is not desirable, create an explicit .slice unit with DefaultDependencies=no, or put the service into Slice=system.slice.

? (Can do a PR, but discussing the wording and place is easier here than force-pushing five wording changes ☺ )

@martinpitt
Copy link
Contributor Author

Let's close this PR, and discuss the documentation in #2189 instead.

@martinpitt martinpitt closed this Jan 12, 2016
@poettering
Copy link
Member

I think we should document this near Slice= in systemd.resource-control(5), as well as in the "Automatic Dependencies" section of systemd.service(5). The longer explanation should be in the latter I guess, and a terse statement with a reference to the latter should be in the former, I figure...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer pid1
Development

Successfully merging this pull request may close these issues.

None yet

2 participants