make even more nspawn concepts configurable #9024

poettering · 2018-05-17T18:56:13Z

This is based on #8940 and includes it. Please review/merge that one first!

poettering · 2018-05-18T17:33:08Z

I have now force pushed a new, rebased version, that is hence much much shorter, given that #8940 has been merged in the meantime. PTAL!

keszybz · 2018-05-19T12:19:25Z

man/systemd-nspawn.xml

+        connectible its static <filename>resolv.conf</filename> file is used, and if not the host's
+        <filename>/etc/resolv.conf</filename> file is used. In the latter cases the file is copied if the image is
+        writable, and bind mounted otherwise. Note that if the file is copied the container payload can modify the file
+        during its runtime. If it is bind mounted that's not possible. Note that both if the file is bind mounted and


It may unmount the file, and modify it. So effectively in both cases it can be modified. So maybe it's better to skip those two sentences altogether.

well, it only can do that if the container has the perms to. Let me rephrase this though.

I really want to get the message across that if people want the container to be able to deviate from the host DNS defaults they should use "copy", and otherwise "bind", as in the letter case the thing is read-only mounted, and hence very likely most scripts trying to modify it will fails, except if they go out of their way and unmount it first.

keszybz · 2018-05-19T12:20:49Z

src/nspawn/nspawn-settings.c

+        [RESOLV_CONF_BIND_STATIC] = "bind-static",
+        [RESOLV_CONF_DELETE] = "delete",
+        [RESOLV_CONF_OFF] = "off",
+        [RESOLV_CONF_AUTO] = "auto",


Please put this in the definition order.

keszybz · 2018-05-19T12:21:39Z

src/nspawn/nspawn.c

@@ -286,6 +287,7 @@ static void help(void) {
               "     --link-journal=MODE    Link up guest journal, one of no, auto, guest, \n"
               "                            host, try-guest, try-host\n"
               "  -j                        Equivalent to --link-journal=try-guest\n"
+               "     --resolv-conf=MODE     Select mode of /etc/resolv.conf synchronization\n"


synchronization → initialization to underline the fact that there is no synchronization actually?

keszybz · 2018-05-19T12:22:18Z

src/nspawn/nspawn.c

+        assert(path);
+
+        if (access(path, F_OK) < 0) {
+


Newline not needed.

keszybz · 2018-05-19T12:22:43Z

src/nspawn/nspawn.c


        assert(dest);

-        if (arg_private_network)
+        if (arg_resolv_conf == RESOLV_CONF_AUTO) {
+


keszybz · 2018-05-19T12:23:55Z

src/nspawn/nspawn.c

-                log_warning_errno(found, "Failed to resolve /etc/resolv.conf path in container, ignoring: %m");
+
+        if (m == RESOLV_CONF_DELETE) {
+


keszybz · 2018-05-19T12:25:48Z

src/nspawn/nspawn.c

-        if (r <= 0)
-                return r;
+        if (r < 0)
+                return log_debug_errno(r, "Failed to check whether resolved bus name is taken: %m");


I think it'd be nicer to just say "Failed to check if org.freedesktop.resolve1 name is taken: %m". This requires less context to understand the log line.

keszybz · 2018-05-19T12:28:51Z

man/systemd-nspawn.xml

+        synchronization from host to container) shall be handled. Takes one of <literal>off</literal>,
+        <literal>copy</literal>, <literal>bind</literal>, <literal>symlink</literal>, <literal>delete</literal> or
+        <literal>auto</literal>. If set to <literal>off</literal> the <filename>/etc/localtime</filename> file in the
+        container is left as it is included in the image, and neither modified nor bind mounted over. If set to


"is left as it is included in the image, and neither modified nor bind mounted over" → "is left untouched". Shorter but easier to read, and then later on overwriting and bind-mounting is talked about anyway, so no need to repeat this here.

keszybz · 2018-05-19T12:50:39Z

src/nspawn/nspawn.c

                if (r >= 0)
                        return mount_verbose(LOG_ERR, NULL, resolved, NULL, MS_BIND|MS_REMOUNT|MS_RDONLY|MS_NOSUID|MS_NODEV, NULL);
        }

        /* If that didn't work, let's copy the file */
-        r = copy_file("/etc/resolv.conf", where, O_TRUNC|O_NOFOLLOW, 0644, 0, COPY_REFLINK);
+        r = copy_file(what, where, O_TRUNC|O_NOFOLLOW, 0644, 0, COPY_REFLINK);
        if (r < 0) {
                /* If the file already exists as symlink, let's suppress the warning, under the assumption that
                 * resolved or something similar runs inside and the symlink points there.


Maybe the log level should be changed to something higher then LOG_DEBUG here? It seems strange to request e.g. --resolv-conf=copy-static and get no message whatsoever if the operation fails because somebody did chattr +i /etc/resolv.conf on the container.

keszybz · 2018-05-19T13:05:14Z

I tried to test this, and I have the strangest effect. I'd assume that this is some fluke or strange tty bug, but it's entirely repeatable:

$ sudo SYSTEMD_LOG_LEVEL=debug build/systemd-nspawn --link-journal=try-guest -U --resolv-conf=auto -M rawhidex --bind /home -b
Found cgroup2 on /sys/fs/cgroup/unified, unified hierarchy for systemd controller
Setting RLIMIT_CPU to infinity.
...
Spawning container rawhidex on /var/lib/machines/rawhidex.
Press ^] three times within 1s to kill container.
(sd-namespace) succeeded.
Init process invoked as PID 32105
...
Failed to chown "/sys/fs/cgroup/systemd/machine.slice/machine-rawhidex.scope/payload/cgroup.threads", ignoring: No such file or directory
(hangs)
$  pgrep systemd-nspawn|xargs ps uw|cat
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root     32103  0.0  0.0  90636  6028 pts/1    S+   14:54   0:00 build/systemd-nspawn --link-journal=try-guest -U --resolv-conf=auto -M rawhidex --bind /home -b
2256404+ 32105  0.0  0.0  69004   884 pts/1    S+   14:54   0:00 build/systemd-nspawn --link-journal=try-guest -U --resolv-conf=auto -M rawhidex --bind /home -b
$  sudo pstack 32105
#0  0x00007f20b0e6f610 in cfsetospeed () from target:/lib64/libc.so.6
#1  0x00007f20b189d0a5 in write_to_console (level=31, error=0, file=0x7f20b19a68a1 "../src/basic/mount-util.c", line=837, func=0x7f20b19a7010 <__func__.10730> "mount_verbose", buffer=0x7ffc225f03a0 "Mounting cgroup2 on /sys/fs/cgroup/unified (MS_NOSUID|MS_NODEV|MS_NOEXEC \"\")...") at ../src/basic/log.c:369
#2  0x00007f20b189df80 in log_dispatch_internal (level=31, error=0, file=0x7f20b19a68a1 "../src/basic/mount-util.c", line=837, func=0x7f20b19a7010 <__func__.10730> "mount_verbose", object_field=0x0, object=0x0, extra_field=0x0, extra=0x0, buffer=0x7ffc225f03a0 "Mounting cgroup2 on /sys/fs/cgroup/unified (MS_NOSUID|MS_NODEV|MS_NOEXEC \"\")...") at ../src/basic/log.c:637
#3  0x00007f20b189e1a2 in log_internalv_realm (level=7, error=0, file=0x7f20b19a68a1 "../src/basic/mount-util.c", line=837, func=0x7f20b19a7010 <__func__.10730> "mount_verbose", format=0x7f20b19a6e50 "Mounting %s on %s (%s \"%s\")...", ap=0x7ffc225f0c20) at ../src/basic/log.c:694
#4  0x00007f20b189e2a2 in log_internal_realm (level=7, error=0, file=0x7f20b19a68a1 "../src/basic/mount-util.c", line=837, func=0x7f20b19a7010 <__func__.10730> "mount_verbose", format=0x7f20b19a6e50 "Mounting %s on %s (%s \"%s\")...") at ../src/basic/log.c:709
#5  0x00007f20b18a46c4 in mount_verbose (error_log_level=3, what=0x557b591850c9 "cgroup", where=0x7ffc225f0df0 "/sys/fs/cgroup/unified", type=0x557b591850c1 "cgroup2", flags=14, options=0x0) at ../src/basic/mount-util.c:836
#6  0x0000557b5915c489 in mount_legacy_cgroup_hierarchy (dest=0x557b59184be0 "", controller=0x557b5918506e "name=unified", hierarchy=0x557b59185214 "unified", read_only=false) at ../src/nspawn/nspawn-mount.c:952
#7  0x0000557b5915cb40 in mount_legacy_cgns_supported (dest=0x557b5918bd07 "", unified_requested=CGROUP_UNIFIED_SYSTEMD, userns=true, uid_shift=225640448, uid_range=65536, selinux_apifs_context=0x0) at ../src/nspawn/nspawn-mount.c:1058
#8  0x0000557b5915db6c in mount_cgroups (dest=0x557b5918bd07 "", unified_requested=CGROUP_UNIFIED_SYSTEMD, userns=true, uid_shift=225640448, uid_range=65536, selinux_apifs_context=0x0, use_cgns=true) at ../src/nspawn/nspawn-mount.c:1219
#9  0x0000557b59177be5 in inner_child (barrier=0x7ffc225f1650, directory=0x557b5b2f23d0 "/var/lib/machines/rawhidex", secondary=false, kmsg_socket=12, rtnl_socket=14, fds=0x0) at ../src/nspawn/nspawn.c:2583
#10 0x0000557b5917a537 in outer_child (barrier=0x7ffc225f1650, directory=0x557b5b2f23d0 "/var/lib/machines/rawhidex", console=0x557b5b2f2400 "/dev/pts/3", dissected_image=0x0, interactive=true, secondary=false, pid_socket=-1, uuid_socket=-1, notify_socket=-1, kmsg_socket=12, rtnl_socket=14, uid_shift_socket=-1, unified_cgroup_hierarchy_socket=-1, fds=0x0, netns_fd=-1) at ../src/nspawn/nspawn.c:3090
#11 0x0000557b5917dc03 in run (master=-1, console=0x557b5b2f2400 "/dev/pts/3", dissected_image=0x0, interactive=true, secondary=false, fds=0x0, veth_name=0x7ffc225f19e0 "", veth_created=0x7ffc225f178e, exposed=0x7ffc225f19c0, pid=0x7ffc225f179c, ret=0x7ffc225f1798) at ../src/nspawn/nspawn.c:3728
#12 0x0000557b59181706 in main (argc=9, argv=0x7ffc225f1b08) at ../src/nspawn/nspawn.c:4491
$ sudo pstack 32103
#0  0x00007f20b0e6da40 in __poll_nocancel () from /lib64/libc.so.6
#1  0x00007f20b185effc in barrier_read (b=0x7ffc225f1650, comp=0) at ../src/basic/barrier.c:230
#2  0x00007f20b185f4ac in barrier_sync (b=0x7ffc225f1650) at ../src/basic/barrier.c:401
#3  0x0000557b5916be64 in barrier_place_and_sync (b=0x7ffc225f1650) at ../src/basic/barrier.h:79
#4  0x0000557b5917f127 in run (master=5, console=0x557b5b2f2400 "/dev/pts/3", dissected_image=0x0, interactive=true, secondary=false, fds=0x0, veth_name=0x7ffc225f19e0 "", veth_created=0x7ffc225f178e, exposed=0x7ffc225f19c0, pid=0x7ffc225f179c, ret=0x7ffc225f1798) at ../src/nspawn/nspawn.c:4006
#5  0x0000557b59181706 in main (argc=9, argv=0x7ffc225f1b08) at ../src/nspawn/nspawn.c:4491

This seems to happen only with --resolv-conf=auto. bind-host, copy-host, off, bind-static, copy-static all work fine. So it seems the process of detection is actually the source of problems.

keszybz · 2018-05-19T13:10:32Z

As expected

--- src/nspawn/nspawn.c
+++ src/nspawn/nspawn.c
@@ -1598,6 +1598,8 @@ static int resolved_listening(void) {
 
         /* Check if resolved is listening */
 
+        return true;
+
         r = sd_bus_open_system(&bus);
         if (r < 0)
                 return log_debug_errno(r, "Failed to open system bus: %m");

makes the hang go away. So this seems to be some strange interaction with dbus.

poettering · 2018-05-22T13:35:26Z

So the hang is a deadlock caused by the debug logging. Basically, our child process logs to stderr, but stderr is already connected to a pty, which the parent process is supposed to process and propagate, but it's waiting for some client operation to finish before it will do so.

So this bug is not introduced by this PR, all that happens is that we now generate so much debug output that the pty buffer will be fully filled.

We should really fix that (for example, by keeping the original log fd open in the child all the way until we are past the the last synchronization point against the parent, so that we never log to the pty on our own).

I'll prep a separate PR for this, I think we should treat this PR independent of that, as this is merely the trigger for the deadlock, not the cause.

poettering · 2018-05-22T14:03:21Z

I force pushed a new version now. Changes are everything @keszybz pointed out (i reworded the thing about copy/mount, please have a look). i also added three minor commits on top.

Fixes: systemd#8014 systemd#1781

…dling more configurable Fixes: systemd#9009

Just some preparation in case we need a similar hack in another enum one day.

poettering · 2018-05-22T14:25:01Z

Force pushed a new rebased version now. I dropped the three extra commits again and put them in #9065 instead, I figure that should make it easier to review.

PTAL!

…tderr as long as possible If we log to the pty that is configured as stdin/stdout/stderr of the container too early we risk filling it up in full before we start processing the pty from the parent process, resulting in deadlocks. Let's hence keep a copy of the original tty we were started on before setting up stdin/stdout/stderr, so that we can log to it, and keep using it as long as we can. Since the kernel's pty internal buffer is pretty small this actually triggered deadlocks when we debug logged at lot from nspawn's child processes, see: systemd#9024 (comment) With this change we won't use the pty at all, only the actual payload we start will, and hence we won't deadlock on it, ever.

poettering · 2018-05-22T14:57:03Z

I have prepped a fix for the logging deadlock independently in PR #9068 now. PTAL!

keszybz · 2018-05-24T14:25:45Z

I can confirm that #9068 fixes the issue.

…tderr as long as possible If we log to the pty that is configured as stdin/stdout/stderr of the container too early we risk filling it up in full before we start processing the pty from the parent process, resulting in deadlocks. Let's hence keep a copy of the original tty we were started on before setting up stdin/stdout/stderr, so that we can log to it, and keep using it as long as we can. Since the kernel's pty internal buffer is pretty small this actually triggered deadlocks when we debug logged at lot from nspawn's child processes, see: systemd/systemd#9024 (comment) With this change we won't use the pty at all, only the actual payload we start will, and hence we won't deadlock on it, ever.

poettering added the nspawn label May 17, 2018

This was referenced May 17, 2018

systemd-nspawn permanently changes the timezone of the started container #9009

Closed

[nspawn] add an option to leave resolv.conf alone #8014

Closed

Document resolv.conf handling in systemd-nspawn #1781

Closed

poettering force-pushed the nspawn-attrs-more branch 2 times, most recently from 6ef6c81 to 7f95bd6 Compare May 18, 2018 17:32

keszybz reviewed May 19, 2018

View reviewed changes

poettering mentioned this pull request May 22, 2018

Fun with UINT64_C #9058

Merged

poettering force-pushed the nspawn-attrs-more branch from 7f95bd6 to 9cd6791 Compare May 22, 2018 14:02

poettering force-pushed the nspawn-attrs-more branch from 9cd6791 to bc788fa Compare May 22, 2018 14:13

poettering added 7 commits May 22, 2018 16:19

nspawn: add greater control over how /etc/resolv.conf is handled

09d423e

Fixes: systemd#8014 systemd#1781

nspawn: add a bit of debug logging to resolved_listening()

b8ea7a6

nspawn: make --link-journal= configurable through .nspawn files, too

4e1d6aa

nspawn: complain if people still use --share-system

63d1c29

nspawn: similar to the previous patches, also make /etc/localtime han…

1688841

…dling more configurable Fixes: systemd#9009

nspawn: let's rename _FORCE_ENUM_WIDTH → _SETTING_FORCE_ENUM_WIDTH

f728ab1

Just some preparation in case we need a similar hack in another enum one day.

update TODO

72d711e

poettering force-pushed the nspawn-attrs-more branch from bc788fa to 72d711e Compare May 22, 2018 14:23

keszybz merged commit 17c1b9a into systemd:master May 24, 2018

mathstuf mentioned this pull request Oct 14, 2018

man/systemd.nspawn: fix reference to --timezone argument #10403

Merged

joshuaspence mentioned this pull request May 23, 2020

dont use resolveconf on systemd solo-io/packer-plugin-arm-image#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make even more nspawn concepts configurable #9024

make even more nspawn concepts configurable #9024

poettering commented May 17, 2018

poettering commented May 18, 2018

keszybz May 19, 2018

poettering May 22, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz May 19, 2018

keszybz commented May 19, 2018

keszybz commented May 19, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

keszybz commented May 24, 2018

		log_warning_errno(found, "Failed to resolve /etc/resolv.conf path in container, ignoring: %m");

		if (m == RESOLV_CONF_DELETE) {

make even more nspawn concepts configurable #9024

make even more nspawn concepts configurable #9024

Conversation

poettering commented May 17, 2018

poettering commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented May 19, 2018

keszybz commented May 19, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

poettering commented May 22, 2018

keszybz commented May 24, 2018