From: Lennart Poettering Date: Wed, 9 Aug 2017 13:42:49 +0000 (+0200) Subject: tests: when running a manager object in a test, migrate to private cgroup subroot... X-Git-Tag: v235~249 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8c759b33a435c6a9390a18ebe9259402e2fc595e;p=platform%2Fupstream%2Fsystemd.git tests: when running a manager object in a test, migrate to private cgroup subroot first (#6576) Without this "meson test" will end up running all tests in the same cgroup root, and they all will try to manage it. Which usually isn't too bad, except when they end up clearing up each other's cgroups. This race is hard to trigger but has caused various CI runs to fail spuriously. With this change we simply move every test that runs a manager object into their own private cgroup. Note that we don't clean up the cgroup at the end, we leave that to the cgroup manager around it. This fixes races that become visible by test runs throwing out errors like this: ``` exec-systemcallfilter-failing.service: Passing 0 fds to service exec-systemcallfilter-failing.service: About to execute: /bin/echo 'This should not be seen' exec-systemcallfilter-failing.service: Forked /bin/echo as 5693 exec-systemcallfilter-failing.service: Changed dead -> start exec-systemcallfilter-failing.service: Failed to attach to cgroup /exec-systemcallfilter-failing.service: No such file or directory Received SIGCHLD from PID 5693 ((echo)). Child 5693 ((echo)) died (code=exited, status=219/CGROUP) exec-systemcallfilter-failing.service: Child 5693 belongs to exec-systemcallfilter-failing.service exec-systemcallfilter-failing.service: Main process exited, code=exited, status=219/CGROUP exec-systemcallfilter-failing.service: Changed start -> failed exec-systemcallfilter-failing.service: Unit entered failed state. exec-systemcallfilter-failing.service: Failed with result 'exit-code'. exec-systemcallfilter-failing.service: cgroup is empty Assertion 'service->main_exec_status.status == status_expected' failed at ../src/src/test/test-execute.c:71, function check(). Aborting. ``` BTW, I tracked this race down by using perf: ``` # perf record -e cgroup:cgroup_mkdir,cgroup_rmdir … # perf script ``` Thanks a lot @iaguis, @alban for helping me how to use perf for this. Fixes #5895. --- diff --git a/src/test/meson.build b/src/test/meson.build index 4f07987..57f7655 100644 --- a/src/test/meson.build +++ b/src/test/meson.build @@ -42,7 +42,8 @@ tests += [ [], []], - [['src/test/test-engine.c'], + [['src/test/test-engine.c', + 'src/test/test-helper.c'], [libcore, libudev, libsystemd_internal], @@ -105,7 +106,8 @@ tests += [ [], 'ENABLE_EFI'], - [['src/test/test-unit-name.c'], + [['src/test/test-unit-name.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, @@ -115,7 +117,8 @@ tests += [ libmount, libblkid]], - [['src/test/test-unit-file.c'], + [['src/test/test-unit-file.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, @@ -456,7 +459,8 @@ tests += [ '', 'manual'], - [['src/test/test-cgroup-mask.c'], + [['src/test/test-cgroup-mask.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, @@ -486,7 +490,8 @@ tests += [ [], []], - [['src/test/test-path.c'], + [['src/test/test-path.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, @@ -496,7 +501,8 @@ tests += [ libmount, libblkid]], - [['src/test/test-execute.c'], + [['src/test/test-execute.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, @@ -524,7 +530,8 @@ tests += [ [], []], - [['src/test/test-sched-prio.c'], + [['src/test/test-sched-prio.c', + 'src/test/test-helper.c'], [libcore, libshared], [threads, diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index b42088c..4a13d71 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -34,6 +34,8 @@ static int test_cgroup_mask(void) { FDSet *fdset = NULL; int r; + enter_cgroup_subroot(); + /* Prepare the manager. */ assert_se(set_unit_path(get_testdata_dir("")) >= 0); assert_se(runtime_dir = setup_fake_runtime_dir()); diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 8133343..b5d3da0 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -37,6 +37,8 @@ int main(int argc, char *argv[]) { Job *j; int r; + enter_cgroup_subroot(); + /* prepare the test */ assert_se(set_unit_path(get_testdata_dir("")) >= 0); assert_se(runtime_dir = setup_fake_runtime_dir()); diff --git a/src/test/test-execute.c b/src/test/test-execute.c index 29c8fd6..ac2cdd5 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -526,6 +526,8 @@ int main(int argc, char *argv[]) { return EXIT_TEST_SKIP; } + enter_cgroup_subroot(); + assert_se(setenv("XDG_RUNTIME_DIR", "/tmp/", 1) == 0); assert_se(set_unit_path(get_testdata_dir("/test-execute")) >= 0); diff --git a/src/test/test-helper.c b/src/test/test-helper.c new file mode 100644 index 0000000..5b707c3 --- /dev/null +++ b/src/test/test-helper.c @@ -0,0 +1,41 @@ +/*** + This file is part of systemd. + + Copyright 2017 Lennart Poettering + + 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 . +***/ + +#include "test-helper.h" +#include "random-util.h" +#include "alloc-util.h" +#include "cgroup-util.h" + +void enter_cgroup_subroot(void) { + _cleanup_free_ char *cgroup_root = NULL, *cgroup_subroot = NULL; + CGroupMask supported; + + assert_se(cg_pid_get_path(NULL, 0, &cgroup_root) >= 0); + assert_se(asprintf(&cgroup_subroot, "%s/%" PRIx64, cgroup_root, random_u64()) >= 0); + assert_se(cg_mask_supported(&supported) >= 0); + + /* If this fails, then we don't mind as the later cgroup operations will fail too, and it's fine if we handle + * any errors at that point. */ + + if (cg_create_everywhere(supported, _CGROUP_MASK_ALL, cgroup_subroot) < 0) + return; + + if (cg_attach_everywhere(supported, cgroup_subroot, 0, NULL, NULL) < 0) + return; +} diff --git a/src/test/test-helper.h b/src/test/test-helper.h index ddb10f8..8af32c8 100644 --- a/src/test/test-helper.h +++ b/src/test/test-helper.h @@ -39,3 +39,5 @@ -ENOENT, \ -ENOMEDIUM /* cannot determine cgroup */ \ ) + +void enter_cgroup_subroot(void); diff --git a/src/test/test-path.c b/src/test/test-path.c index 70ac6b3..6fc3f79 100644 --- a/src/test/test-path.c +++ b/src/test/test-path.c @@ -45,6 +45,8 @@ static int setup_test(Manager **m) { assert_se(m); + enter_cgroup_subroot(); + r = manager_new(UNIT_FILE_USER, true, &tmp); if (MANAGER_SKIP_TEST(r)) { log_notice_errno(r, "Skipping test: manager_new: %m"); diff --git a/src/test/test-sched-prio.c b/src/test/test-sched-prio.c index 81d9abc..062a02b 100644 --- a/src/test/test-sched-prio.c +++ b/src/test/test-sched-prio.c @@ -34,6 +34,8 @@ int main(int argc, char *argv[]) { FDSet *fdset = NULL; int r; + enter_cgroup_subroot(); + /* prepare the test */ assert_se(set_unit_path(get_testdata_dir("")) >= 0); assert_se(runtime_dir = setup_fake_runtime_dir()); diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index fd797b5..d0c844d 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -848,6 +848,8 @@ int main(int argc, char *argv[]) { log_parse_environment(); log_open(); + enter_cgroup_subroot(); + assert_se(runtime_dir = setup_fake_runtime_dir()); r = test_unit_file_get_set(); diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c index 818ac5e..b8979c0 100644 --- a/src/test/test-unit-name.c +++ b/src/test/test-unit-name.c @@ -469,6 +469,8 @@ int main(int argc, char* argv[]) { log_parse_environment(); log_open(); + enter_cgroup_subroot(); + assert_se(runtime_dir = setup_fake_runtime_dir()); test_unit_name_is_valid();