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

[Zbigniew Jędrzejewski-Szmek]

On Thu, Jan 01, 2015 at 11:15:47PM +0100, Jouke Witteveen wrote:
> On Thu, Jan 1, 2015 at 7:15 PM, 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
> 
> Will do. For consistency it is also better to not abort propagation if
> one unit fails to reload, so I have a second version ready already.
> 
> >>  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?
> 
> Isn't that just bad behavior? Sending a RELOADING=1 notification after
> a reload is initiated? I guess if both service_enter_reload() and
> service_enter_reload_by_notify() are called it is justified to
> propagate two reloads. Before testing it might be nice to know what we
> want :-).

Hm. Looking at the code, sending periodic RELOADING=1 notifications
can be used to sidestep the timeouts. This does not look OK, but maybe
I'm misreading the code.

You are right that if a service sends a spurious RELOADING=1 message
it is buggy. What we want is idempotent behaviour, where a message
sent when the unit was already known by systemd to be reloading is
ignored.

Zbyszek