tests: when running a manager object in a test, migrate to private cgroup subroot...

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 4f079876c436dc603d0fc7650196f2a479687c92..57f76559a7f97136495a0c871512cd44c4bdf002 100644 (file)
--- 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 b42088c680a68515566cc38dd02d0a2401a7ce5c..4a13d710dc295a0a97e11b985a3f441e8bb84672 100644 (file)
--- 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 8133343fb379a78804b5036445f52d135ced9a2b..b5d3da0d389bd5f8482b4a4f296742c1cf9601de 100644 (file)
--- 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 29c8fd613f070866c83a00590dde819628b1fa11..ac2cdd50ed9ca49c0ee67c670a661b1c68324b01 100644 (file)
--- 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 (file)
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 <http://www.gnu.org/licenses/>.
+***/
+
+#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 ddb10f88fd1956010a6cc411b89f9073d6d3bf77..8af32c8744f0bc73680195d5c6c3c05e8f8fe99e 100644 (file)
--- 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 70ac6b3df355e12d7565fe5eff3b2450db053299..6fc3f79d80d55c824339a6c1ce0dac377ada7d94 100644 (file)
--- 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 81d9abc2d51d7d26d9b6aee4c005af04e7433f88..062a02baf5f5b6d435698623647724c641ab2f10 100644 (file)
--- 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 fd797b587ec2cca38ba436b3a9bea1c7dc944d51..d0c844dddb3997900b10bf7a24b5a9e96bfdfefc 100644 (file)
--- 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 818ac5e372e2ef8d4cf86ed583f1ddf0365970c2..b8979c0e95638cc3e32f2abab5593eb92d166ca5 100644 (file)
--- 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();