From e266c068b5597e18b2299f9c9d3ee6cf04198c41 Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Mon, 23 Jan 2017 17:12:35 +0100 Subject: [PATCH 1/2] service: serialize information about currently executing command Stored information will help us to resume execution after the daemon-reload. This commit implements following scheme, * On serialization: - we count rank of the currently executing command - we store command type, its rank and command line arguments * On deserialization: - configuration is parsed and loaded - we deserialize stored data, command type, rank and arguments - we look at the given rank in the list and if command there has same arguments then we restore execution at that point - otherwise we search respective command list and we look for command that has the same arguments - if both methods fail we do not do not resume execution at all To better illustrate how does above scheme works, please consider following cases (<<< denotes position where we resume execution after reload) ; Original unit file [Service] ExecStart=/bin/true <<< ExecStart=/bin/false ; Swapped commands ; Second command is not going to be executed [Service] ExecStart=/bin/false ExecStart=/bin/true <<< ; Commands added before ; Same commands are problematic and execution could be restarted at wrong place [Service] ExecStart=/bin/foo ExecStart=/bin/bar ExecStart=/bin/true <<< ExecStart=/bin/false ; Commands added after ; Same commands are not an issue in this case [Service] ExecStart=/bin/true <<< ExecStart=/bin/false ExecStart=/bin/foo ExecStart=/bin/bar ; New commands interleaved with old commands ; Some new commands will be executed while others won't ExecStart=/bin/foo ExecStart=/bin/true <<< ExecStart=/bin/bar ExecStart=/bin/false As you can see, above scheme has some drawbacks. However, in most cases (we assume that in most common case unit file command list is not changed while some other command is running for the same unit) it should cause that systemd does the right thing, which is restoring execution exactly at the point we were before daemon-reload. Fixes #518 --- src/core/service.c | 196 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 181 insertions(+), 15 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 74054887b9..5e681fb715 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -45,6 +45,7 @@ #include "service.h" #include "signal-util.h" #include "special.h" +#include "stdio-util.h" #include "string-table.h" #include "string-util.h" #include "strv.h" @@ -2140,6 +2141,80 @@ _pure_ static bool service_can_reload(Unit *u) { return !!s->exec_command[SERVICE_EXEC_RELOAD]; } +static unsigned service_exec_command_index(Unit *u, ServiceExecCommand id, ExecCommand *current) { + Service *s = SERVICE(u); + unsigned idx = 0; + ExecCommand *first, *c; + + assert(s); + + first = s->exec_command[id]; + + /* Figure out where we are in the list by walking back to the beginning */ + for (c = current; c != first; c = c->command_prev) + idx++; + + return idx; +} + +static int service_serialize_exec_command(Unit *u, FILE *f, ExecCommand *command) { + Service *s = SERVICE(u); + ServiceExecCommand id; + unsigned idx; + const char *type; + char **arg; + _cleanup_strv_free_ char **escaped_args = NULL; + _cleanup_free_ char *args = NULL, *p = NULL; + size_t allocated = 0, length = 0; + + assert(s); + assert(f); + + if (!command) + return 0; + + if (command == s->control_command) { + type = "control"; + id = s->control_command_id; + } else { + type = "main"; + id = SERVICE_EXEC_START; + } + + idx = service_exec_command_index(u, id, command); + + STRV_FOREACH(arg, command->argv) { + size_t n; + _cleanup_free_ char *e = NULL; + + e = xescape(*arg, WHITESPACE); + if (!e) + return -ENOMEM; + + n = strlen(e); + if (!GREEDY_REALLOC(args, allocated, length + 1 + n + 1)) + return -ENOMEM; + + if (length > 0) + args[length++] = ' '; + + memcpy(args + length, e, n); + length += n; + } + + if (!GREEDY_REALLOC(args, allocated, length + 1)) + return -ENOMEM; + args[length++] = 0; + + p = xescape(command->path, WHITESPACE); + if (!p) + return -ENOMEM; + + fprintf(f, "%s-command=%s %u %s %s\n", type, service_exec_command_to_string(id), idx, p, args); + + return 0; +} + static int service_serialize(Unit *u, FILE *f, FDSet *fds) { Service *s = SERVICE(u); ServiceFDStore *fs; @@ -2167,11 +2242,8 @@ static int service_serialize(Unit *u, FILE *f, FDSet *fds) { if (r < 0) return r; - /* FIXME: There's a minor uncleanliness here: if there are - * multiple commands attached here, we will start from the - * first one again */ - if (s->control_command_id >= 0) - unit_serialize_item(u, f, "control-command", service_exec_command_to_string(s->control_command_id)); + service_serialize_exec_command(u, f, s->control_command); + service_serialize_exec_command(u, f, s->main_command); r = unit_serialize_item_fd(u, f, fds, "stdin-fd", s->stdin_fd); if (r < 0) @@ -2227,6 +2299,106 @@ static int service_serialize(Unit *u, FILE *f, FDSet *fds) { return 0; } +static int service_deserialize_exec_command(Unit *u, const char *key, const char *value) { + Service *s = SERVICE(u); + int r; + unsigned idx = 0, i; + bool control, found = false; + ServiceExecCommand id = _SERVICE_EXEC_COMMAND_INVALID; + ExecCommand *command = NULL; + _cleanup_free_ char *args = NULL, *path = NULL; + _cleanup_strv_free_ char **argv = NULL; + + enum ExecCommandState { + STATE_EXEC_COMMAND_TYPE, + STATE_EXEC_COMMAND_INDEX, + STATE_EXEC_COMMAND_PATH, + STATE_EXEC_COMMAND_ARGS, + _STATE_EXEC_COMMAND_MAX, + _STATE_EXEC_COMMAND_INVALID = -1, + } state; + + assert(s); + assert(key); + assert(value); + + control = streq(key, "control-command"); + + state = STATE_EXEC_COMMAND_TYPE; + + for (;;) { + _cleanup_free_ char *arg = NULL; + + r = extract_first_word(&value, &arg, NULL, EXTRACT_CUNESCAPE); + if (r == 0) + break; + else if (r < 0) + return r; + + switch (state) { + case STATE_EXEC_COMMAND_TYPE: + id = service_exec_command_from_string(arg); + if (id < 0) + return -EINVAL; + + state = STATE_EXEC_COMMAND_INDEX; + break; + case STATE_EXEC_COMMAND_INDEX: + r = safe_atou(arg, &idx); + if (r < 0) + return -EINVAL; + + state = STATE_EXEC_COMMAND_PATH; + break; + case STATE_EXEC_COMMAND_PATH: + path = arg; + arg = NULL; + state = STATE_EXEC_COMMAND_ARGS; + + if (!path_is_absolute(path)) + return -EINVAL; + break; + case STATE_EXEC_COMMAND_ARGS: + r = strv_extend(&argv, arg); + if (r < 0) + return -ENOMEM; + break; + default: + assert_not_reached("Unknown error at deserialization of exec command"); + break; + } + } + + if (state != STATE_EXEC_COMMAND_ARGS) + return -EINVAL; + + /* Let's check whether exec command on given offset matches data that we just deserialized */ + for (command = s->exec_command[id], i = 0; command; command = command->command_next, i++) { + if (i != idx) + continue; + + found = strv_equal(argv, command->argv) && streq(command->path, path); + break; + } + + if (!found) { + /* Command at the index we serialized is different, let's look for command that exactly + * matches but is on different index. If there is no such command we will not resume execution. */ + for (command = s->exec_command[id]; command; command = command->command_next) + if (strv_equal(command->argv, argv) && streq(command->path, path)) + break; + } + + if (command && control) + s->control_command = command; + else if (command) + s->main_command = command; + else + log_unit_warning(u, "Current command vanished from the unit file, execution of the command list won't be resumed."); + + return 0; +} + static int service_deserialize_item(Unit *u, const char *key, const char *value, FDSet *fds) { Service *s = SERVICE(u); int r; @@ -2309,16 +2481,6 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, s->status_text = t; } - } else if (streq(key, "control-command")) { - ServiceExecCommand id; - - id = service_exec_command_from_string(value); - if (id < 0) - log_unit_debug(u, "Failed to parse exec-command value: %s", value); - else { - s->control_command_id = id; - s->control_command = s->exec_command[id]; - } } else if (streq(key, "accept-socket")) { Unit *socket; @@ -2437,6 +2599,10 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value, s->watchdog_override_enable = true; s->watchdog_override_usec = watchdog_override_usec; } + } else if (STR_IN_SET(key, "main-command", "control-command")) { + r = service_deserialize_exec_command(u, key, value); + if (r < 0) + log_unit_debug_errno(u, r, "Failed to parse serialized command \"%s\": %m", value); } else log_unit_debug(u, "Unknown serialization key: %s", key); From 123d672e85d0c52ff7cf81997d4910990da409c1 Mon Sep 17 00:00:00 2001 From: Michal Sekletar Date: Wed, 15 Feb 2017 12:40:52 +0100 Subject: [PATCH 2/2] tests: add new test for issue #518 --- Makefile.am | 3 +- test/test-exec-deserialization.py | 192 ++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-) create mode 100755 test/test-exec-deserialization.py diff --git a/Makefile.am b/Makefile.am index ec711bd184..06ff38ee24 100644 --- a/Makefile.am +++ b/Makefile.am @@ -5954,7 +5954,8 @@ EXTRA_DIST += \ src/network/systemd-networkd.pkla \ units/systemd-networkd.service.m4.in \ units/systemd-networkd-wait-online.service.in \ - test/networkd-test.py + test/networkd-test.py \ + test/test-exec-deserialization.py # ------------------------------------------------------------------------------ if ENABLE_LOGIND diff --git a/test/test-exec-deserialization.py b/test/test-exec-deserialization.py new file mode 100755 index 0000000000..b974b1c133 --- /dev/null +++ b/test/test-exec-deserialization.py @@ -0,0 +1,192 @@ +#!/usr/bin/python3 + +# +# Copyright 2017 Michal Sekletar +# +# systemd is free software; you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation; either version 2.1 of the License, or +# (at your option) any later version. +# +# systemd is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with systemd; If not, see . + +# ATTENTION: This uses the *installed* systemd, not the one from the built +# source tree. + +import unittest +import time +import os +import tempfile +import subprocess + +from enum import Enum + +class UnitFileChange(Enum): + NO_CHANGE = 0 + LINES_SWAPPED = 1 + COMMAND_ADDED_BEFORE = 2 + COMMAND_ADDED_AFTER = 3 + COMMAND_INTERLEAVED = 4 + REMOVAL = 5 + +class ExecutionResumeTest(unittest.TestCase): + def setUp(self): + self.unit = 'test-issue-518.service' + self.unitfile_path = '/run/systemd/system/{0}'.format(self.unit) + self.output_file = tempfile.mktemp() + self.unit_files = {} + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/sleep 2 + ExecStart=/bin/bash -c "echo foo >> {0}" + '''.format(self.output_file) + self.unit_files[UnitFileChange.NO_CHANGE] = unit_file_content + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/bash -c "echo foo >> {0}" + ExecStart=/bin/sleep 2 + '''.format(self.output_file) + self.unit_files[UnitFileChange.LINES_SWAPPED] = unit_file_content + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/bash -c "echo bar >> {0}" + ExecStart=/bin/sleep 2 + ExecStart=/bin/bash -c "echo foo >> {0}" + '''.format(self.output_file) + self.unit_files[UnitFileChange.COMMAND_ADDED_BEFORE] = unit_file_content + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/sleep 2 + ExecStart=/bin/bash -c "echo foo >> {0}" + ExecStart=/bin/bash -c "echo bar >> {0}" + '''.format(self.output_file) + self.unit_files[UnitFileChange.COMMAND_ADDED_AFTER] = unit_file_content + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/bash -c "echo baz >> {0}" + ExecStart=/bin/sleep 2 + ExecStart=/bin/bash -c "echo foo >> {0}" + ExecStart=/bin/bash -c "echo bar >> {0}" + '''.format(self.output_file) + self.unit_files[UnitFileChange.COMMAND_INTERLEAVED] = unit_file_content + + unit_file_content = ''' + [Service] + Type=oneshot + ExecStart=/bin/bash -c "echo bar >> {0}" + ExecStart=/bin/bash -c "echo baz >> {0}" + '''.format(self.output_file) + self.unit_files[UnitFileChange.REMOVAL] = unit_file_content + + def reload(self): + subprocess.check_call(['systemctl', 'daemon-reload']) + + def write_unit_file(self, unit_file_change): + if not isinstance(unit_file_change, UnitFileChange): + raise ValueError('Unknown unit file change') + + content = self.unit_files[unit_file_change] + + with open(self.unitfile_path, 'w') as f: + f.write(content) + + self.reload() + + def check_output(self, expected_output): + try: + with open(self.output_file, 'r') as log: + output = log.read() + except IOError: + self.fail() + + self.assertEqual(output, expected_output) + + def setup_unit(self): + self.write_unit_file(UnitFileChange.NO_CHANGE) + subprocess.check_call(['systemctl', '--job-mode=replace', '--no-block', 'start', self.unit]) + + def test_no_change(self): + expected_output = 'foo\n' + + self.setup_unit() + self.reload() + time.sleep(4) + + self.check_output(expected_output) + + def test_swapped(self): + expected_output = '' + + self.setup_unit() + self.write_unit_file(UnitFileChange.LINES_SWAPPED) + self.reload() + time.sleep(4) + + self.assertTrue(not os.path.exists(self.output_file)) + + def test_added_before(self): + expected_output = 'foo\n' + + self.setup_unit() + self.write_unit_file(UnitFileChange.COMMAND_ADDED_BEFORE) + self.reload() + time.sleep(4) + + self.check_output(expected_output) + + def test_added_after(self): + expected_output = 'foo\nbar\n' + + self.setup_unit() + self.write_unit_file(UnitFileChange.COMMAND_ADDED_AFTER) + self.reload() + time.sleep(4) + + self.check_output(expected_output) + + def test_interleaved(self): + expected_output = 'foo\nbar\n' + + self.setup_unit() + self.write_unit_file(UnitFileChange.COMMAND_INTERLEAVED) + self.reload() + time.sleep(4) + + self.check_output(expected_output) + + def test_removal(self): + self.setup_unit() + self.write_unit_file(UnitFileChange.REMOVAL) + self.reload() + time.sleep(4) + + self.assertTrue(not os.path.exists(self.output_file)) + + def tearDown(self): + for f in [self.output_file, self.unitfile_path]: + try: + os.remove(f) + except OSError: + # ignore error if log file doesn't exist + pass + + self.reload() + +if __name__ == '__main__': + unittest.main()