Autodetect systemd version in containers started by systemd-nspawn by keszybz · Pull Request #4310 · systemd/systemd

keszybz · 2016-10-08T06:45:41Z

No description provided.

poettering

Looks pretty good, the only real issue I see is the missing check for lib64/ in addition to lib/...

(Thinking about this, we probably should also check /usr/lib and /usr/lib64, since Gentoo has a really strange usrmerge approach, where they manually move everything into /usr/lib without making /lib a symlink...)

poettering · 2016-10-08T11:07:01Z

src/basic/path-util.c

+         * is non-standard. False positives should be relatively rare.
+         */
+
+        path = prefix_root(root, "lib/systemd/libsystemd-shared-*.so");


what about lib64?

I thought we always install systemd in /usr/lib, and libsystemd-shared also ends up in the non-arch-specific path. This is true for Fedora, Debian, Ubuntu. I don't see a Mageia systemd 231 package, but in 230, they are also using /usr/lib/systemd/. Is anyone using /usr/lib64/systemd/?

arch also has [/usr]/lib/systmed/libsystemd-shared-231.so on amd64.

Yeah, i think Gentoo again is doing things differently. Generally, lib64 is understood as the place to put libraries for 64bit if both 32bit and 64bit are to be supported. Except on Gentoo, where lib64 is understood as the place to place 64bit libraries, full stop.

Gentoo's scheme is borked, but if you want to cover that, you'd also have to check for lib64. Now I personally don't care about Gentoo I must say, so the patch is actually fine, but heh...

I'll let somebody who is an actual Gentoo user submit a patch if necessary. I wouldn't even know how to test this patch against Gentoo. To anyone reading this: the table is at https://github.com/systemd/systemd/blob/master/src/basic/path-util.c#L834.

Hm, actually #4173 (duplicate of #4008) and #4181

Used distribution: Gentoo (Host)

@eliasp , please have a look. Do you know somebody who can fix (and test) the version detection on Gentoo?

./test-path-util /path/to/container...

@floppym Could you provide assistance here?

Gentoo installs libsystemd-shared in /usr/lib/systemd.

Due to a poor decision several years ago, /usr/lib is often a symlink to /usr/lib64 on Gentoo, but that should not matter in this context.

poettering · 2016-10-08T11:07:45Z

src/basic/path-util.c

+        if (r < 0)
+                return r;
+
+        endswith(path, "*.so")[0] = '\0'; /* truncate the glob part */


i'd probably do that in two steps and add an assert here, just in case:

assert_se(c = endswith(path, "*.so")); *c = 0;

poettering · 2016-10-08T11:09:51Z

src/test/test-path-util.c

+        unsigned i;
+
+        for (i = 0; i < ELEMENTSOF(versions); i++) {
+                r = systemd_installation_has_version(path, versions[i]);


hmm, given that s_i_h_v() returns a bool, I'd probably not reuse "r" here, but instead write:

for (…) { bool b; b = s_i_h_v(…); … }

i.e.avoid the implicit bool → int case...

It returns an int (one of 0, 1, -ENOMEM).

evverx · 2016-10-08T12:26:02Z

Tested by running the host with mixed hierarchy (i.e. simply using a recent
kernel with systemd from git), and booting first a container with older systemd,
and then one with a newer systemd.

But what about patched systemd? I mean people can backport mixed hierarchy (or anything else) to older versions. Or people can revert mixed hierarchy on newer versions.

BTW: the version detection mechanism doesn't work. For example, the control group namespaces were introduced in 4.6. But Ubuntu (4.4) understands unshare -C. So, uname -r doesn't make sense.

keszybz · 2016-10-08T18:46:58Z

But what about patched systemd? I mean people can backport mixed hierarchy (or anything else) to older versions. Or people can revert mixed hierarchy on newer versions.

If you revert patches in released systemd, then you are on your own, and get to keep both pieces. If you backport the support, then you either have to use the env. var to override the check, or also patch the systemd on the host. I wouldn't worry about those cases too much, because the only problematic case would be a false positive, where we think systemd in the container has support, but for some reason it does not. But I think this could only happen if you do some rogue patching.

…version This is a bit crude and only works for new systemd versions which have libsystemd-shared.

If we are going to use the env var to override the detection result anyway, there is not point in doing the detection, especially that it can fail.

The new function has 416 lines by itself! "return log_error_errno" is used to nicely reduce the volume of error handling code. A few minor issues are fixed on the way: - positive value was used as error value (EIO), causing systemd-nspawn to return success, even though it shouldn't. - In two places random values were used as error status, when the actual value was in an unusual place (etc_password_lock, notify_socket). Those are the only functional changes. There is another potential issue, which is marked with a comment, and left unresolved: the container can also return 133 by itself, causing a spurious reboot.

systemd-soon-to-be-released-232 is able to deal with the mixed hierarchy. So make an educated guess, and use the mixed hierarchy in that case. Tested by running the host with mixed hierarchy (i.e. simply using a recent kernel with systemd from git), and booting first a container with older systemd, and then one with a newer systemd. Fixes systemd#4008.

evverx · 2016-10-08T18:57:49Z

If you revert patches in released systemd, then you are on your own, and get to keep both pieces.

Well, I can patch both pieces of the A-distro. But this doesn't help me if I run the A-distro on the non-patched B-distro.

I wouldn't worry about those cases too much

Well, this makes sense. But why not to mount the legacy hierarchy by default? This works always.

(Yes, this breaks

By default, nspawn will use the
unified hierarchy for the containers if the host uses the
unified hierarchy, and the legacy hierarchy otherwise.

But who cares? Well, I care. But I can live without it)

keszybz · 2016-10-08T19:03:46Z

Well, I can patch both pieces of the A-distro. But this doesn't help me if I run the A-distro on the non-patched B-distro.

If you have an old host system (B-distro if I understand correctly), then you get nothing. But that's expected. This patch is only about new B and old A.

keszybz · 2016-10-08T19:07:47Z

Pushed with the requested style fixes. I didn't add /lib64/ paths because I don't see any distro that would actually use them. But not it'd just be a question of adding lines to a nulstr, so it's a trivial patch that can be added on top if it turns out to be desired.

evverx · 2016-10-08T19:12:18Z

If you have an old host systemd

Sorry, I mean

host - distro A (Fedora, systemd-v232, systemd-nspawn-v232)
container - distro B (systemd-v232-with-reverted-mixed-hierarchy)

This combination doesn't work.

Anyway, why not to mount the legacy hierarchy by default?

fsateler · 2016-10-08T19:42:09Z

src/basic/path-util.c

+                *c = '\0'; /* truncate the glob part */
+
+                STRV_FOREACH(name, names) {
+                        /* This is most likely to run only once, hence let's not optimize anything. */


This loop means that if I have libsystemd-shared-232.so and libsystemd-shared-231.so, then this function will return true. I'd argue that this should be more conservative and either return true only when there is only one name, or all names satisfy the needed version.

Well, if you just run make install over an existing directory, you can easily end up with multiple versions of the lib, so we should support that. OTOH, if you install a newer systemd and than an older one over that, I don't think we can have to support that.

I agree with @keszybz. I think it's OK to assume that if people install multiple versions of the .so, they installed them in the same order as the versions indicate, and thus we can only look at the newest release.

I mean, upgrades are generally the norm, downgrades are the exception.

keszybz · 2016-10-08T19:55:56Z

container - distro B (systemd-v232-with-reverted-mixed-hierarchy)

Seriously, what would the point of that be? If you don't like the mixed hierarchy, just force legacy hierarchy on the host.

evverx · 2016-10-08T20:00:13Z

just force legacy hierarchy on the host

@keszybz , sure, I can force everything I want :-) We are talking about defaults. And I think the legacy hierarchy is the good default (it works always)

I'm not against this PR. I just don't understand why do we choose this approach.

evverx · 2016-10-08T20:42:51Z

Another problem: systemd < 230 is not able to deal with the "full" unified hierarchy. Why not to check this too?

keszybz · 2016-10-08T20:56:14Z

Full unified hierarchy requires an explicit command line switch, but the mixed one is started implicitly if the kernel supports it. I don't think we can do anything to make old systemd run in the full unified hierarchy, but at least it requires an explicit choice.

Anyway, why not to mount the legacy hierarchy by default?

Because v2 is better than mixed, and mixed is better than v1 (more efficient, and the notifications work better). So we want to use v2 as much as possible, or at least mixed as long as we cannot make v2 the default.

evverx · 2016-10-08T21:31:52Z

Full unified hierarchy requires an explicit command line switch

It doesn't require an explicit command line switch

By default, nspawn will use the
unified hierarchy for the containers if the host uses the
unified hierarchy

am I missing something?

I don't think we can do anything to make old systemd run in the full unified hierarchy

systemd-nspawn can setup the legacy hierarchy

So we want to use v2 as much as possible

OK. So, why do we mount the v1 hierarchy in the container? We should try:

mixed (>= 232)
full unified (>= 230 && < 232)
legacy (< 230)

evverx · 2016-10-08T22:12:23Z

systemd-nspawn can setup the legacy hierarchy

I mean

-bash-4.3# grep cgroup /proc/self/mountinfo
23 17 0:20 / /sys/fs/cgroup rw,nosuid,nodev,noexec,relatime shared:7 - cgroup2 cgroup rw

-bash-4.3# systemd-nspawn -D /var/lib/machines/v229/ -b
Spawning container v229 on /var/lib/machines/v229.
Press ^] three times within 1s to kill container.
Failed to mount /etc/resolv.conf in the container, ignoring: No such file or directory
Failed to mount cgroup at /sys/fs/cgroup/systemd: Operation not permitted
[!!!!!!] Failed to mount API filesystems, freezing.
Freezing execution.
^]^]

-bash-4.3# UNIFIED_CGROUP_HIERARCHY=no systemd-nspawn -D /var/lib/machines/v229/ -b
Press ^] three times within 1s to kill container.
systemd 229 running in system mode. (+PAM +AUDIT +SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ +LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN)
Detected virtualization systemd-nspawn.
Detected architecture x86-64.
...
works fine
...

And I don't understand why do we check the container's version on the mixed hierarchy and don't check version on the unified hierarchy.

Current systemd version detection routine cannot detect systemd 230, only systmed >= 231. This means that we'll still use the legacy hierarchy in some cases where we wouldn't have too. If somebody figures out a nice way to detect systemd 230 this can be later improved.

keszybz · 2016-10-08T23:42:43Z

Hm, it's more complicated than the list in #4310 (comment).

Indeed, systemd-229 breaks when booted with full unified, but systemd-208 does not! I have a Fedora20 container here, and it's perfectly happy to boot with both tmpfs and cgroup2 mount on /sys/fs/cgroup.

Nevertheless, I like your idea of mounting legacy hierarchy for systemd <= 229. I'll push a patch which purports to do that shortly.

evverx · 2016-10-09T00:15:46Z

Indeed, systemd-229 breaks when booted with full unified, but systemd-208 does not!

Oh, I didn't know. Thanks!

If somebody figures out a nice
way to detect systemd 230 this can be later improved

[[ $(strings path-to-systemd | perl -lne 'do { print $1; exit; } if /^systemd (\d+) running/') = 230 ]]

:-)

@keszybz , thanks!

keszybz · 2016-10-09T00:37:47Z

Yeah, I had a similar idea too, but @poettering wasn't impressed ;) Let's leave that discussion for another PR.

evverx · 2016-10-09T01:14:48Z

I'm not sure. Should we mark #4181 as "release-critical" too? What about #3388?

Anyway, I'll test this PR on Monday. Stay tuned :)

poettering · 2016-10-10T13:52:19Z

src/test/test-path-util.c


+static void test_systemd_installation_has_version(const char *path) {
+        int r;
+        const unsigned versions[] = {0, 231, atoi(PACKAGE_VERSION), 999};


atoi(PACKAGE_VERSION)? yuck!

poettering · 2016-10-10T13:56:26Z

lgtm. Of course, I'd always prefer if we could somehow check for features rather than for version numbers (so that we can support backported patches), but I am not sure this is possible in this case.

But maybe we can add some infrastructure for this for the future. Some scheme how the system in the container can declare the feature set it supports, or so....

poettering · 2016-10-10T17:11:54Z

so, should we merge this? @evverx anything else you wanted to test about this?

evverx · 2016-10-10T17:17:15Z

@poettering , sorry for the delay. I'm testing now. I didn't see any regressions. But I should check SYSTEMD_NSPAWN_USE_CGNS, -n and so on. I need ~~ 1 hour

evverx · 2016-10-10T17:22:22Z

@poettering
Please, note #4181
So, ~~this PR partially fixes #4008~~ doesn't fix nspawn -U

evverx · 2016-10-10T17:47:28Z

Works fine. @keszybz , thanks!

keszybz · 2016-10-10T19:17:21Z

Thanks for testing! I think we're moving in the right direction, albeit step by step.

keszybz added nspawn cgroups labels Oct 8, 2016

poettering requested changes Oct 8, 2016

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Oct 8, 2016

keszybz added 5 commits October 8, 2016 14:48

path-util: add a function to peek into a container and guess systemd …

5a46d55

…version This is a bit crude and only works for new systemd versions which have libsystemd-shared.

nspawn: check env var first, detect second

98afd6a

If we are going to use the env var to override the detection result anyway, there is not point in doing the detection, especially that it can fail.

nspawn: fix spurious reboot if container process returns 133

27e29a1

keszybz force-pushed the nspawn-autodetect branch from 6c8f800 to 0fd9563 Compare October 8, 2016 19:05

keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Oct 8, 2016

fsateler reviewed Oct 8, 2016

View reviewed changes

poettering reviewed Oct 10, 2016

View reviewed changes

poettering approved these changes Oct 10, 2016

View reviewed changes

evverx merged commit a0f72a2 into systemd:master Oct 10, 2016

keszybz deleted the nspawn-autodetect branch October 10, 2016 19:14

This was referenced Oct 14, 2016

Add a compile-time switch for cgroup unified hierarchy handling #4368

Closed

tests: add smoke test for systemd-nspawn #4378

Merged

Uh oh!

Conversation

keszybz commented Oct 8, 2016

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evverx commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 8, 2016

Uh oh!

evverx commented Oct 8, 2016

Uh oh!

keszybz commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 8, 2016

Uh oh!

evverx commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz commented Oct 8, 2016

Uh oh!

evverx commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evverx commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 8, 2016

Uh oh!

evverx commented Oct 8, 2016

Uh oh!

evverx commented Oct 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Oct 8, 2016

Uh oh!

evverx commented Oct 9, 2016

Uh oh!

keszybz commented Oct 9, 2016

Uh oh!

evverx commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Oct 10, 2016

Uh oh!

poettering commented Oct 10, 2016

evverx commented Oct 8, 2016 •

edited

Loading

keszybz commented Oct 8, 2016 •

edited

Loading

evverx commented Oct 8, 2016 •

edited

Loading

evverx commented Oct 8, 2016 •

edited

Loading

evverx commented Oct 8, 2016 •

edited

Loading

evverx commented Oct 8, 2016 •

edited

Loading

evverx commented Oct 10, 2016 •

edited

Loading