From: Michal Koutný Date: Tue, 5 Dec 2017 15:51:19 +0000 (+0100) Subject: service: Don't stop unneeded units needed by restarted service (#7526) X-Git-Tag: v236~66 X-Git-Url: http://git.ipfire.org/?p=thirdparty%2Fsystemd.git;a=commitdiff_plain;h=deb4e7080db9dcd2a1d51ccf7c357f88ea863e54 service: Don't stop unneeded units needed by restarted service (#7526) An auto-restarted unit B may depend on unit A with StopWhenUnneeded=yes. If A stops before B's restart timeout expires, it'll be started again as part of B's dependent jobs. However, if stopping takes longer than the timeout, B's running stop job collides start job which also cancels B's start job. Result is that neither A or B are active. Currently, when a service with automatic restarting fails, it transitions through following states: 1) SERVICE_FAILED or SERVICE_DEAD to indicate the failure, 2) SERVICE_AUTO_RESTART while restart timer is running. The StopWhenUnneeded= check takes place in service_enter_dead between the two state mentioned above. We temporarily store the auto restart flag to query it during the check. Because we don't return control to the main event loop, this new service unit flag needn't be serialized. This patch prevents the pathologic situation when the service with Restart= won't restart automatically. As a side effect it also avoid restarting the dependency unit with StopWhenUnneeded=yes. Fixes: #7377 --- diff --git a/src/core/service.c b/src/core/service.c index e7a829a564f..97f129b8f02 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1493,9 +1493,13 @@ static bool service_shall_restart(Service *s) { } } -static bool service_will_restart(Service *s) { +static bool service_will_restart(Unit *u) { + Service *s = SERVICE(u); + assert(s); + if (s->will_auto_restart) + return true; if (s->state == SERVICE_AUTO_RESTART) return true; if (!UNIT(s)->job) @@ -1521,13 +1525,17 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) if (s->result != SERVICE_SUCCESS) log_unit_warning(UNIT(s), "Failed with result '%s'.", service_result_to_string(s->result)); + if (allow_restart && service_shall_restart(s)) + s->will_auto_restart = true; + /* Make sure service_release_resources() doesn't destroy our FD store, while we are changing through * SERVICE_FAILED/SERVICE_DEAD before entering into SERVICE_AUTO_RESTART. */ s->n_keep_fd_store ++; service_set_state(s, s->result != SERVICE_SUCCESS ? SERVICE_FAILED : SERVICE_DEAD); - if (allow_restart && service_shall_restart(s)) { + if (s->will_auto_restart) { + s->will_auto_restart = false; r = service_arm_timer(s, usec_add(now(CLOCK_MONOTONIC), s->restart_usec)); if (r < 0) { @@ -1554,7 +1562,7 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) s->exec_runtime = exec_runtime_unref(s->exec_runtime); if (s->exec_context.runtime_directory_preserve_mode == EXEC_PRESERVE_NO || - (s->exec_context.runtime_directory_preserve_mode == EXEC_PRESERVE_RESTART && !service_will_restart(s))) + (s->exec_context.runtime_directory_preserve_mode == EXEC_PRESERVE_RESTART && !service_will_restart(UNIT(s)))) /* Also, remove the runtime directory */ exec_context_destroy_runtime_directory(&s->exec_context, UNIT(s)->manager->prefix[EXEC_DIRECTORY_RUNTIME]); @@ -3772,6 +3780,8 @@ const UnitVTable service_vtable = { .active_state = service_active_state, .sub_state_to_string = service_sub_state_to_string, + .will_restart = service_will_restart, + .check_gc = service_check_gc, .sigchld_event = service_sigchld_event, diff --git a/src/core/service.h b/src/core/service.h index dfe27e894fa..5c5b24e3efd 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -164,6 +164,8 @@ struct Service { bool main_pid_alien:1; bool bus_name_good:1; bool forbid_restart:1; + /* Keep restart intention between UNIT_FAILED and UNIT_ACTIVATING */ + bool will_auto_restart:1; bool start_timeout_defined:1; char *bus_name; diff --git a/src/core/unit.c b/src/core/unit.c index 1bbe5fdded1..ed0cf026785 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2003,7 +2003,7 @@ static void unit_check_unneeded(Unit *u) { void *v; HASHMAP_FOREACH_KEY(v, other, u->dependencies[needed_dependencies[j]], i) - if (unit_active_or_pending(other)) + if (unit_active_or_pending(other) || unit_will_restart(other)) return; } @@ -3794,6 +3794,15 @@ bool unit_active_or_pending(Unit *u) { return false; } +bool unit_will_restart(Unit *u) { + assert(u); + + if (!UNIT_VTABLE(u)->will_restart) + return false; + + return UNIT_VTABLE(u)->will_restart(u); +} + int unit_kill(Unit *u, KillWho w, int signo, sd_bus_error *error) { assert(u); assert(w >= 0 && w < _KILL_WHO_MAX); diff --git a/src/core/unit.h b/src/core/unit.h index 92bb4779a97..fdd82315bac 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -484,6 +484,9 @@ struct UnitVTable { * unit is in. */ const char* (*sub_state_to_string)(Unit *u); + /* Additionally to UnitActiveState determine whether unit is to be restarted. */ + bool (*will_restart)(Unit *u); + /* Return true when there is reason to keep this entry around * even nothing references it and it isn't active in any * way */ @@ -706,6 +709,7 @@ const char *unit_slice_name(Unit *u); bool unit_stop_pending(Unit *u) _pure_; bool unit_inactive_or_pending(Unit *u) _pure_; bool unit_active_or_pending(Unit *u); +bool unit_will_restart(Unit *u); int unit_add_default_target_dependency(Unit *u, Unit *target);