[systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications

[Lennart Poettering]

On Thu, 01.01.15 19:15, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:

> On Tue, Dec 30, 2014 at 08:22:27PM +0100, Jouke Witteveen wrote:
> > ---
> > 
> > This fixes #87251
> 
> This is actually important information that should be included in the
> commit message (i.e. above not below "---"). We usually include the
> full url, since we also use distribution bug trackers and having the full
> url makes things easier to click. In this case:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=87251
> 
> >  static void service_enter_reload_by_notify(Service *s) {
> > +        _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
> > +        int r;
> > +
> >          assert(s);
> >  
> >          if (s->timeout_start_usec > 0)
> >                  service_arm_timer(s, s->timeout_start_usec);
> >  
> > +        r = manager_propagate_reload(UNIT(s)->manager, UNIT(s), JOB_REPLACE, false, &error);
> > +        if(r < 0)
> > +                log_unit_warning(UNIT(s)->id, "%s failed to schedule propagation of reload: %s", UNIT(s)->id, bus_error_message(&error, -r));
> > +
> 
> Let's say that a.service has PropagateReloadsTo=b.service, and a.service provides
> the RELOADING=1 notification during a reload.
> What happens if a reload is requested with 'systemctl reload a', and systemd
> schedules a reload of a and b. Is it possible for b to be reloaded a second time
> as a result of notification of a? This should not happen, have you verified that
> this will not happen?

THis is a real issue I fear.

If service units use an asynchronous command in ExecReload= (such as
one that just sends a SIGHUP signal to the daemon and terminates),
then the daemon might send its RELOADING=1 later than the ExecReload=
finishes. This would then trigger a second reload in b.service...

This problem goes away if the people use synchronous commands in
ExecReload= however, that wait for the reload to complete. In that
case the RELOADING=1 from the service would be queued in s->notify_state,
and be unset again when the daemon reports READY=1 again. And only
after that the ExecReload= process would exit, and hence no second
reload be propagated. (this is race-free because we always process
notification messages before SIGCHLD).

Now I wonder what to do with this. I think the change would be
welcome, but I wonder if we can simply require people who use
PropagateReloadsTo= to define synchronous ExecReload= processes or
accept double reloads to be forwarded...

I leaning towards merging the patch, but this would require an
explanation in the man page of sd_notify() and in systemd.unit near
the description of PropagateReloadsTo=.

Jouke, could you resend this patch with such an addition?

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat