[systemd-devel] [PATCH] systemd restart patch
Lennart Poettering
lennart at poettering.net
Mon Jul 4 10:35:19 PDT 2011
On Mon, 04.07.11 18:18, Cristian Patrascu (Cristian.Patrascu at windriver.com) wrote:
>
> Thank you for your observations and interest!
>
> So I made some modifications:
> - changed restart variables to unsigned
> - changed to 0 as default (not -1, also dropped the define)
> - moved counter reset from unit.c to service.c (after unit_notify call)
> - because the "OnFailure" unit is triggered in unit.c, I still left
> the checking before that.
> - updated coding style
> - added restart counter (de)serialization
heya,
Can you please post this patch properly formatted with git's
format-patch command and not with line breaking added by your mailer?
Otherwise I have trouble reviewing and applying your patch!
Thanks,
Lennart
>
> The patch is for the same branch
> (3661ac04b4f2840d3345605aa35963bbde3c469d) as before:
>
> Index: systemd-29/src/dbus-service.c
> ===================================================================
> --- systemd-29.orig/src/dbus-service.c 2011-07-04
> 16:19:37.000000000 +0300
> +++ systemd-29/src/dbus-service.c 2011-07-04 17:07:09.971651549 +0300
> @@ -42,6 +42,8 @@
> " <property name=\"PIDFile\" type=\"s\" access=\"read\"/>\n" \
> " <property name=\"NotifyAccess\" type=\"s\"
> access=\"read\"/>\n" \
> " <property name=\"RestartUSec\" type=\"t\" access=\"read\"/>\n" \
> + " <property name=\"MaxRestartRetries\" type=\"u\"
> access=\"read\"/>\n" \
> + " <property name=\"RestartRetry\" type=\"u\" access=\"read\"/>\n" \
> " <property name=\"TimeoutUSec\" type=\"t\" access=\"read\"/>\n" \
> BUS_EXEC_COMMAND_INTERFACE("ExecStartPre") \
> BUS_EXEC_COMMAND_INTERFACE("ExecStart") \
> @@ -103,6 +105,8 @@
> { "org.freedesktop.systemd1.Service", "PIDFile",
> bus_property_append_string, "s", u->service.pid_file
> },
> { "org.freedesktop.systemd1.Service",
> "NotifyAccess", bus_service_append_notify_access, "s",
> &u->service.notify_access },
> { "org.freedesktop.systemd1.Service",
> "RestartUSec", bus_property_append_usec, "t",
> &u->service.restart_usec },
> + { "org.freedesktop.systemd1.Service",
> "MaxRestartRetries", bus_property_append_unsigned, "u",
> &u->service.restart_retries },
> + { "org.freedesktop.systemd1.Service",
> "RestartRetry", bus_property_append_unsigned, "u",
> &u->service.restart_current_retry },
> { "org.freedesktop.systemd1.Service",
> "TimeoutUSec", bus_property_append_usec, "t",
> &u->service.timeout_usec },
> BUS_EXEC_COMMAND_PROPERTY("org.freedesktop.systemd1.Service",
> u->service.exec_command[SERVICE_EXEC_START_PRE], "ExecStartPre"),
> BUS_EXEC_COMMAND_PROPERTY("org.freedesktop.systemd1.Service",
> u->service.exec_command[SERVICE_EXEC_START], "ExecStart"),
> Index: systemd-29/src/load-fragment.c
> ===================================================================
> --- systemd-29.orig/src/load-fragment.c 2011-07-04
> 16:19:37.000000000 +0300
> +++ systemd-29/src/load-fragment.c 2011-07-04 17:07:09.975651196 +0300
> @@ -1951,6 +1951,7 @@
> { "TimeoutSec", config_parse_usec,
> 0, &u->service.timeout_usec, "Service" },
> { "Type",
> config_parse_service_type, 0, &u->service.type,
> "Service" },
> { "Restart",
> config_parse_service_restart, 0, &u->service.restart,
> "Service" },
> + { "MaxRestartRetries", config_parse_unsigned,
> 0, &u->service.restart_retries, "Service" },
> { "PermissionsStartOnly", config_parse_bool,
> 0, &u->service.permissions_start_only, "Service" },
> { "RootDirectoryStartOnly", config_parse_bool,
> 0, &u->service.root_directory_start_only, "Service" },
> { "RemainAfterExit", config_parse_bool,
> 0, &u->service.remain_after_exit, "Service" },
> Index: systemd-29/src/service.c
> ===================================================================
> --- systemd-29.orig/src/service.c 2011-07-04 16:19:37.000000000 +0300
> +++ systemd-29/src/service.c 2011-07-04 17:14:32.823465057 +0300
> @@ -1151,6 +1166,20 @@
> if (s->meta.default_dependencies)
> if ((r = service_add_default_dependencies(s)) < 0)
> return r;
> +
> + /* If the option "RestartRetries=" is set then the
> service will be "revived" */
> + if (s->restart_retries > 0) {
> + /* If the option "Restart=" is not set,
> then for the service to be "revived"
> + we must set "Restart=" to "on-failure" */
> + if (s->restart == SERVICE_RESTART_NO)
> + s->restart = SERVICE_RESTART_ON_FAILURE;
> +
> + /* If the option "Restart=" is already set,
> then for the service to be "revived"
> + and be restarted on failure we must set
> "Restart=" to "always"
> + only if it's "on-success" */
> + else if (s->restart == SERVICE_RESTART_ON_SUCCESS)
> + s->restart = SERVICE_RESTART_ALWAYS;
> + }
> }
>
> return service_verify(s);
> @@ -1488,6 +1517,16 @@
> log_debug("%s changed %s -> %s", s->meta.id,
> service_state_to_string(old_state), service_state_to_string(state));
>
> unit_notify(UNIT(s), state_translation_table[old_state],
> state_translation_table[state], !s->reload_failure);
> +
> + /* If service finished, reset restart-retry counter, if
> applicable */
> + if (old_state != state &&
> + (state == SERVICE_DEAD || state == SERVICE_FAILED) &&
> + s->restart_current_retry > s->restart_retries) {
> + log_debug("Service %s entered dead or failed,
> restart retries at max, reset counter (%u -> 0)!",
> + s->meta.id, s->restart_current_retry);
> + s->restart_current_retry = 0;
> + }
> +
> s->reload_failure = false;
> }
>
> @@ -2291,6 +2330,7 @@
>
> /* Make sure we don't enter a busy loop of some kind. */
> if (!ratelimit_test(&s->ratelimit)) {
> + s->restart_current_retry = 0;
> log_warning("%s start request repeated too quickly,
> refusing to start.", u->meta.id);
> return -ECANCELED;
> }
> @@ -2311,6 +2351,7 @@
>
> /* This is a user request, so don't do restarts on this
> * shutdown. */
> + s->restart_current_retry = 0;
> s->forbid_restart = true;
>
> /* Already on it */
> @@ -2411,6 +2452,8 @@
> }
> }
>
> + unit_serialize_item_format(u, f, "restart-current-retry",
> "%u", s->restart_current_retry);
> +
> return 0;
> }
>
> @@ -2510,6 +2553,13 @@
> dual_timestamp_deserialize(value,
> &s->main_exec_status.start_timestamp);
> else if (streq(key, "main-exec-status-exit"))
> dual_timestamp_deserialize(value,
> &s->main_exec_status.exit_timestamp);
> + else if (streq(key, "restart-current-retry")) {
> + unsigned i;
> + if (safe_atou(value, &i) < 0)
> + log_debug("Failed to parse
> restart-current-retry %s", value);
> + else
> + s->restart_current_retry = i;
> + }
> else
> log_debug("Unknown serialization key '%s'", key);
>
> @@ -2604,6 +2654,11 @@
> * gone. */
> s->main_command = NULL;
>
> + if (success && s->restart_current_retry) {
> + log_debug("%s changed
> restart_crt_retry from %u to 0", s->meta.id,
> s->restart_current_retry);
> + s->restart_current_retry = 0;
> + }
> +
> switch (s->state) {
>
> case SERVICE_START_POST:
> @@ -2832,7 +2887,15 @@
> break;
>
> case SERVICE_AUTO_RESTART:
> - log_info("%s holdoff time over, scheduling
> restart.", u->meta.id);
> + if (s->restart_retries > 0) {
> + s->restart_current_retry ++;
> + if (s->restart_current_retry >
> s->restart_retries) {
> + log_warning("%s maximum number of
> restarting retries reached. Stopping.", s->meta.id);
> + service_enter_dead(s, true, false);
> + break;
> + }
> + }
> + log_info("%s holdoff time over, scheduling restart
> (retry %u of %u).", u->meta.id, s->restart_current_retry,
> s->restart_retries);
> service_enter_restart(s);
> break;
>
> Index: systemd-29/src/service.h
> ===================================================================
> --- systemd-29.orig/src/service.h 2011-07-04 16:19:37.000000000 +0300
> +++ systemd-29/src/service.h 2011-07-04 17:07:09.975651196 +0300
> @@ -92,6 +92,9 @@
> ServiceType type;
> ServiceRestart restart;
>
> + unsigned restart_retries;
> + unsigned restart_current_retry;
> +
> /* If set we'll read the main daemon PID from this file */
> char *pid_file;
>
> Index: systemd-29/src/unit.c
> ===================================================================
> --- systemd-29.orig/src/unit.c 2011-07-04 16:19:37.000000000 +0300
> +++ systemd-29/src/unit.c 2011-07-04 17:07:09.979652060 +0300
> @@ -1114,6 +1114,10 @@
> if (set_size(u->meta.dependencies[UNIT_ON_FAILURE]) <= 0)
> return;
>
> + if (u->meta.type == UNIT_SERVICE &&
> + u->service.restart_current_retry <=
> u->service.restart_retries)
> + return;
> +
> log_info("Triggering OnFailure= dependencies of %s.", u->meta.id);
>
> SET_FOREACH(other, u->meta.dependencies[UNIT_ON_FAILURE], i) {
>
>
> On 07/02/2011 03:21 AM, Lennart Poettering wrote:
> >On Fri, 01.07.11 15:35, Cristian Patrascu (Cristian.Patrascu at windriver.com) wrote:
> >
> >Heya!
> >
> >(BTW, got your first post of this patch too, just didn't find the time
> >to review the patch, sorry. It was still in my mail queue however.)
> >
> >> After "x" restarts have happened, the "OnFailure" unit will start
> >>(if specified) but only after all unsuccessfull restarts (after "x"
> >>restarts reached).
> >Patch looks pretty good. A few comments though.
> >>This patch is made for systemd-29, on master branch with SHA1 ID:
> >>3661ac04b4f2840d3345605aa35963bbde3c469d
> >>
> >>________systemd-restart-on-fail.patch : _______________________________
> >>
> >>Index: systemd-29/src/dbus-service.c
> >>===================================================================
> >>--- systemd-29.orig/src/dbus-service.c 2011-06-22 10:47:14.000000000 +0300
> >>+++ systemd-29/src/dbus-service.c 2011-06-22 13:48:51.292321742 +0300
> >>@@ -42,6 +42,8 @@
> >> "<property name=\"PIDFile\" type=\"s\" access=\"read\"/>\n" \
> >> "<property name=\"NotifyAccess\" type=\"s\" access=\"read\"/>\n" \
> >> "<property name=\"RestartUSec\" type=\"t\" access=\"read\"/>\n" \
> >>+ "<property name=\"MaxRestartRetries\" type=\"i\" access=\"read\"/>\n" \
> >>+ "<property name=\"RestartRetry\" type=\"i\"
> >>access=\"read\"/>\n" \
> >I Think it would make sense to make both of these unsigned, in order to
> >avoid confusion whether these actually ever can be negative.
> >
> >If I read your patch correctly right now MaxRestartRetries=-1 means that
> >there is no limit on the number of retries. I'd use 0 for that instead
> >(0 is not needed to indicate disabling, since we can indicate that with
> >Restart=no already.)
> >
> >>+ s->restart_retries = DEFAULT_RESTART_RETRIES;
> >>+ s->restart_crt_retry = 0;
> >We generally try not to abbrievate variables unnecessarily, so I'd suggest
> >to call this variable "restart_current_retry" or so.
> >
> >> #ifdef HAVE_SYSV_COMPAT
> >> s->sysv_start_priority = -1;
> >> s->sysv_start_priority_from_rcnd = -1;
> >>@@ -1151,6 +1153,22 @@
> >> if (s->meta.default_dependencies)
> >> if ((r = service_add_default_dependencies(s))< 0)
> >> return r;
> >>+
> >>+ /* If the option "RestartRetries=" is set then the service will be "revived" */
> >>+ if (s->restart_retries != DEFAULT_RESTART_RETRIES){
> >>+ /* If the option "Restart=" is not set, then for the service to be "revived"
> >>+ we must set "Restart=" to "on-failure" */
> >>+ if (s->restart == SERVICE_RESTART_NO){
> >>+ s->restart = SERVICE_RESTART_ON_FAILURE;
> >>+ }
> >Please follow coding style. We generally place no {} brackets around
> >single line blocks.
> >
> >> case SERVICE_AUTO_RESTART:
> >>- log_info("%s holdoff time over, scheduling restart.", u->meta.id);
> >>+ if (0<= s->restart_retries){
> >Please follow coding style, compare variables with constants not
> >constants with variables. Also, please place spaces before the<= and
> >the {.
> >
> >>+ if (u->meta.type == UNIT_SERVICE&&
> >>+ u->service.restart_crt_retry<= u->service.restart_retries)
> >>+ return;
> >>+
> >Huhm, I wonder if we could fine a better place for this, not sure where though.
> >
> >> log_info("Triggering OnFailure= dependencies of %s.", u->meta.id);
> >>
> >> SET_FOREACH(other, u->meta.dependencies[UNIT_ON_FAILURE], i) {
> >>@@ -1245,6 +1249,17 @@
> >> log_notice("Unit %s entered failed state.", u->meta.id);
> >> unit_trigger_on_failure(u);
> >> }
> >>+
> >>+ /* When a unit is of type service and has finished,
> >>+ reset restart-retry counter, if applicable */
> >>+ if (ns != os&&
> >>+ u->meta.type == UNIT_SERVICE&&
> >>+ UNIT_IS_INACTIVE_OR_FAILED(ns)&&
> >>+ u->service.restart_crt_retry> u->service.restart_retries ){
> >>+ log_debug("Unit %s entered inactive or failed, restart retries at max, reset counter (%d -> 0)!",
> >>+ u->meta.id, u->service.restart_crt_retry, u->service.restart_retries);
> >>+ u->service.restart_crt_retry = 0;
> >Hmm, I think it would be nicer to reset this counter in service.c only, not
> >in the generic code. I'd like to avoid that we access too many
> >service-private fields from generic unit code.
> >
> >> #include "execute.h"
> >> #include "condition.h"
> >>
> >>+/* default = infinite retries (= systemd behaviour not patched) */
> >>+#define DEFAULT_RESTART_RETRIES -1
> >>+
> >Hee, if I commit this, then it won't be patched anymore, so the comment
> >should not refer to it being unpatched ;-)
> >
> >Patch looks quite OK in general.
> >
> >Thanks for the work,
> >
> >Lennart
Lennart
--
Lennart Poettering - Red Hat, Inc.
More information about the systemd-devel
mailing list