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

man: Requires= needs After= to deactivate "this unit" #6869

Merged
merged 1 commit into from Sep 22, 2017

Conversation

@johnlinp
Copy link
Contributor

commented Sep 19, 2017

Fixes: #6856

@keszybz
Copy link
Member

left a comment

Duh, I reviewed this, but forgot to submit. Sorry for the delay.

@@ -454,7 +454,8 @@
<term><varname>Requires=</varname></term>

<listitem><para>Configures requirement dependencies on other units. If this unit gets activated, the units
listed here will be activated as well. If one of the other units gets deactivated or its activation fails, this
listed here will be activated as well. If one of the other units fails to activate, and an ordering dependency
<varname>After=</varname> on the failing unit is set, this

This comment has been minimized.

Copy link
@keszybz

keszybz Sep 21, 2017

Member

Hmm, not your fault, because it was part of the existing text, but "this unit will be deactivated" seems wrong. Wouldn't "this unit will not be started" be better?

This comment has been minimized.

Copy link
@johnlinp

johnlinp Sep 21, 2017

Author Contributor

Yes, I think the latter is clearer and more precise. I'll fix it.

@johnlinp johnlinp force-pushed the johnlinp:master branch from 65cbfe6 to 48ae9e0 Sep 21, 2017

@boucman

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

if there is no ordering dependencies and the Required unit fails, my understanding is that this unit will be stopped. Am I right ? if no, what is the difference with Wants ( when there are no dependencies) ?

@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

if there is no ordering dependencies and the Required unit fails, my understanding is that this unit will be stopped. Am I right ?

No. That's what this whole PR is about.

if no, what is the difference with Wants ( when there are no dependencies) ?

It seems that there's no difference.

Hmm, @poettering, current documentation does not match behaviour. This patch modifies the docs to match the code. But maybe it should be the other way around?

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@keszybz Since Requires= doesn't imply activation ordering between this unit and the other unit, it's possible that this unit can be activated before the other unit. In such situation, if the other unit failed to start, are you suggesting that the code should deactivate this unit after successfully starting it?

@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

Well, that's the documented behaviour. I'm not sure if the implementation changed at some point, of if the docs were always wrong.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

I see. Let's wait for @poettering.

@boucman

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

alternatively Requires= could imply After=, but that still needs to be documented properly...

@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 21, 2017

That'd break too much stuff.

@poettering

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

so, the failure propagation effect of Requires= is defined in respect to jobs. This means, if you have Requires= from A to B, and there are two start jobs queued for both A and B, and the one for B fails, then the one for A will be canceled with a JOB_DEPENDENCY result code. Now, if there's also After= from A to B, then this is all well-defined, as the start job for A will not be processed until the start job for B is completed. If you however omit the After= then behaviour depends on what else A is waiting for. If there's something else it is waiting for, and that other thing hasn't become available yet, then the job is still pending at the moment B fails, and hence the job on A will fail too. However, if there's nothing else A is waiting for, then its job would be dispatched right-away, and would be completed already at the moment B fails, and then the Requires= would not have any effect anymore, as there is not job on A anymore that could fail.

Now, this is all pretty complex I figure, and I doubt it needs to be in all this detail in the man page. So I think we should really say that After= should be used, and not mention what happens if After= isn't used, as that case isn't useful and hard to explain...

@poettering

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Or in other words, I like the proposed patch! and I think the currently implemented logic in the code does make a lot of sense. (well, one could argue that implying After= in Requires= would have been a very good idea, but I fear it's too late for that now...)

@poettering poettering merged commit a195dd8 into systemd:master Sep 22, 2017

4 checks passed

semaphoreci The build passed on Semaphore.
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-s390x autopkgtest finished (success)
Details
@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

Thanks for the explanation. I think you might have explained this at some point, possibly more than once ;) Hopefully it'll stick this time.

(Agreed: with the new wording, the fact that a start job was scheduled and then aborted is not visible from the point of the user, so this doesn't have to be mentioned).

@boucman

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2017

to put it in once sentence

if A is not started when B fails, then A will never be started. However if A is already in the process of starting, systemd will not stop A

@poettering

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

correct

@arvidjaar

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2017

@keszybz

That'd [Requires= could imply After=] break too much stuff.

I'm yet to see useful example of Requires without After. So far every reported problem was answered by "you must use After if you want to use Requires". So could you give example of setup that would be broken by this?

@poettering

if there's nothing else A is waiting for, then its job would be dispatched right-away, and would be completed already at the moment B fails, and then the Requires= would not have any effect anymore, as there is not job on A anymore that could fail

I would expect STOP job for A to be queued when B fails.

@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 24, 2017

I would expect STOP job for A to be queued when B fails.

Use BindsTo= for this.

So could you give example of setup that would be broken by this?

Well, that'd implicitly add After= dependencies where before there were none. It could even cause dependency loops. We could add emit a warning, maybe in systemd-analyze verify, but I don't think changing the semantics is possible.

@arvidjaar

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2017

I would expect STOP job for A to be queued when B fails.

Use BindsTo= for this.

Sorry? BindsTo is meant to handle situation when "B suddenly disappears". And Requires manual page says that "if activation of other unit fails, this unit will be deactivated". This is exactly what is being discussed here, right? If this does not happen, either documentation is wrong or it is systemd implementation bug, because activation of B fails but A is left running.

@keszybz

This comment has been minimized.

Copy link
Member

commented Sep 24, 2017

That's what the documentation used to say. With this PR merged, it just says "will not be activated". So it seems that this PR implements exactly what you are asking for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.