Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Tue, 06.01.15 16:33, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: > > 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. I think you are reading the code right. Not sure though whether that should be considered a bug though... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Thu, 01.01.15 19:15, Zbigniew Jędrzejewski-Szmek (zbys...@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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Tue, 30.12.14 20:22, Jouke Witteveen (j.wittev...@gmail.com) wrote:
Hmm, what happened to this patch? Did you ever resend it with the
commit msg fix Zbigniew suggested? I don't see it in the mail
archives?
> ---
>
> This fixes #87251
>
> src/core/manager.c | 42 ++
> src/core/manager.h | 1 +
> src/core/service.c | 7 +++
> 3 files changed, 50 insertions(+)
>
> diff --git a/src/core/manager.c b/src/core/manager.c
> index 9705e64..11cca17 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -1226,6 +1226,48 @@ int manager_add_job_by_name(Manager *m, JobType type,
> const char *name, JobMode
> return manager_add_job(m, type, unit, mode, override, e, _ret);
> }
>
> +int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool
> override, sd_bus_error *e) {
> +int r;
> +Transaction *tr;
> +Iterator i;
> +Unit *dep;
> +
> +
Spurious newline...
Otherwise looks great!
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
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
> 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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Thu, Jan 1, 2015 at 7:15 PM, Zbigniew Jędrzejewski-Szmek
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 :-).
Regards,
- Jouke
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
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?
Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
---
This fixes #87251
src/core/manager.c | 42 ++
src/core/manager.h | 1 +
src/core/service.c | 7 +++
3 files changed, 50 insertions(+)
diff --git a/src/core/manager.c b/src/core/manager.c
index 9705e64..11cca17 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1226,6 +1226,48 @@ int manager_add_job_by_name(Manager *m, JobType type,
const char *name, JobMode
return manager_add_job(m, type, unit, mode, override, e, _ret);
}
+int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool
override, sd_bus_error *e) {
+int r;
+Transaction *tr;
+Iterator i;
+Unit *dep;
+
+
+assert(m);
+assert(unit);
+assert(mode < _JOB_MODE_MAX);
+
+tr = transaction_new(mode == JOB_REPLACE_IRREVERSIBLY);
+if (!tr)
+return -ENOMEM;
+
+SET_FOREACH(dep, unit->dependencies[UNIT_PROPAGATES_RELOAD_TO], i) {
+r = transaction_add_job_and_dependencies(tr, JOB_RELOAD, dep,
NULL, false, override, false, false, mode == JOB_IGNORE_DEPENDENCIES, e);
+if (r < 0) {
+log_unit_warning(dep->id,
+ "Cannot add dependency reload job for
unit %s, ignoring: %s",
+ dep->id, bus_error_message(e, r));
+
+if (e)
+sd_bus_error_free(e);
+
+goto tr_abort;
+}
+}
+
+r = transaction_activate(tr, m, mode, e);
+if (r < 0)
+goto tr_abort;
+
+transaction_free(tr);
+return 0;
+
+tr_abort:
+transaction_abort(tr);
+transaction_free(tr);
+return r;
+}
+
Job *manager_get_job(Manager *m, uint32_t id) {
assert(m);
diff --git a/src/core/manager.h b/src/core/manager.h
index ab75f90..bc11f87 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -316,6 +316,7 @@ int manager_load_unit_from_dbus_path(Manager *m, const char
*s, sd_bus_error *e,
int manager_add_job(Manager *m, JobType type, Unit *unit, JobMode mode, bool
force, sd_bus_error *e, Job **_ret);
int manager_add_job_by_name(Manager *m, JobType type, const char *name,
JobMode mode, bool force, sd_bus_error *e, Job **_ret);
+int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool force,
sd_bus_error *e);
void manager_dump_units(Manager *s, FILE *f, const char *prefix);
void manager_dump_jobs(Manager *s, FILE *f, const char *prefix);
diff --git a/src/core/service.c b/src/core/service.c
index bfbe959..71c7bf6 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1496,11 +1496,18 @@ fail:
}
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));
+
service_set_state(s, SERVICE_RELOAD);
}
--
2.2.1
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
