[systemd-devel] Conflation of propagation in dependencies creates race windows

[Jonathon Kowalski]

On Sun, Jan 20, 2019 at 2:59 AM Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
>
> On Sun, 2019-01-20 at 00:34 +0000, Jonathon Kowalski wrote:
> > On Sat, Jan 19, 2019 at 5:05 PM Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
> > > I think you're wrong here. It makes perfect sense that if unit A has
> > > Requires= for another unit, stopping that required unit which A can't
> > > work without will stop A too. Removing that logic is not a good
> > > solution.
>
> > I am NOT advocating for changing how Requires= currently works, and
> > also, no many people by accident use Requires= for its semantics at
> > startup.
>
> So what exactly are you saying instead? That this case shouldn't use
> Requires= (would be a bad idea, not an appropriate solution to the
> actual problem)? That you made a mistake when you brought up this case,
> nothing you say is actually relevant to it, and none of your proposed
> changes would help solve this issue? Something else you haven't
> explained?
>
>
> > Also, this cannot work. Suppose I have Restart=on-failure in service,
> > and service exits on its own normally. How will systemd decide X
> > should not be stopped, in case an ExecStop* statement ends up failing,
> > and then it *should* restart our service? All of this is going to be
> > very racy and undeterministic.
>
> Systemd could consider X "unneeded" only when Y is not active, has
> nothing executing, and has nothing scheduled to execute. This does not
> seem racy or undeterministic.
>

I now see that logic was reworked to be a bit more robust, previously
it triggered on every state change which WAS racy, so yes, this
particular case has been dealt with (in v240) now.
https://github.com/systemd/systemd/commit/a3c1168ac293f16d9343d248795bb4c246aaff4a
(now it correctly waits for the other unit to reach a state reliably
with no jobs installed before acting on itself). I only tested with
v239, and this change was so recent, that I did not know...

> Anyway, the only realistic alternatives are to either restart X
> automatically when Y restarts, or leave Y stopped after all. Restarting
> Y while leaving X stopped is not a sane alternative (if the user wants
> to allow that, it should be Wants= instead of Requires=). So this still
> requires some solution, and nothing you have proposed would help at
> all.
>
>

My point still stands about the oneshot case I point out. I guess I
incorrectly pointed out the second case, but atleast in the first case
the race still exists. The fix above dealt with a particular situation
where SWU=true was involved, but it is still possible using deps to
create circular job forwarding situations.

I guess I was also incorrect in thinking that not using Requires=
there was the correct solution, now looking at it again I think I
agree with you that use of Requires= in both cases is probably the
correct thing, and there are problems elsewhere (and not in
Requires=). So I accept that I was incorrect there, my bad.

However, there is still a case where you just want the effect
Requires= has at start up, and not the PartOf= it currently combines
into itself.

In the oneshot scenario, where you have Requires=oneshot.service,
After=oneshot.service. It going to inactive is success, and you do
want your start job to fail if the oneshot fails. But, you then don't
want spurious stop jobs on the oneshot being enqueued to also take
down your unit, that too when the oneshot has no RemainAfterExit=true.
This is not desirable. Wants= is also not the solution here, because
there is no failing of dependent jobs for it. I think the correct fix
here hence is indeed introducing a new dep that only has the effect
Requires= has but at start up (and not the combined PartOf= with it).
You also want to pull the oneshot everytime you have a start job (or a
restart that is just stop which converts itself to start later).

> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/systemd-devel