Merge ~ogayot/curtin:systemd-offline into curtin:master

[Olivier Gayot]

apt: ensure systemd knows it runs in a chroot, when executing postinst

Since we added the --mount-proc option to unshare, the postinst script
for openssh-server (and most likely other packages) started failing with
the following error when `systemctl daemon-reload` was invoked:

  > Failed to connect to bus: No data available

Before the option was added, it would simply do nothing because systemd
rightly understood it was running in a chroot.

To determine if we are running in a chroot, systemd checks if
/proc/1/root (corresponding to the init process) and / are the same
inode. If they are different, systemd assumes we are in a chroot.

However, we are running apt-get in a new PID namespace which means that
in the new namespace, apt-get gets assigned PID 1 and is therefore the
"init" process.

Now that /proc is properly mounted in the chroot, when systemd compares
/proc/1/root and /, it sees they are identical because the init process
(which is apt-get) is actually running inside the chroot.

Without the --mount-proc option, /proc/1 in the chroot would still refer
to the systemd init process (running outside the chroot), so it would
work properly.

With the SYSTEMD_OFFLINE variable, one can "force" systemd to assume
it is running in a chroot. Let's use it when running commands in a
chroot, and when the variable is not already defined.

LP: #2056570

Signed-off-by: Olivier Gayot <email address hidden>

Since we added the --mount-proc option to unshare, the postinst script for openssh-server (and most likely other packages) started failing with the following error when `systemctl daemon-reload` was invoked:

  > Failed to connect to bus: No data available

This would cause failed installations of Ubuntu. This happens because systemd fails to determine that we are running in a chroot when executed in the postinst script context.

To determine if we are running in a chroot, systemd checks if /proc/1/root (corresponding to the init process) and / are the same inode. If they are different, systemd assumes we are in a chroot.

However, we are running apt-get in a new PID namespace which means that in the new namespace, apt-get gets assigned PID 1 and is therefore the "init" process.

Now that /proc is properly mounted in the chroot, when systemd compares /proc/1/root and /, it sees they are identical because the init process (which is apt-get) is actually running inside the chroot.

Without the --mount-proc option, /proc/1 in the chroot would still refer to the systemd init process (running outside the chroot), so it would work properly.

With the SYSTEMD_OFFLINE variable, one can "force" systemd to assume it is running in a chroot. Let's use it.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:8147f7019071577fb3ba49de55739fcc49460773
https://jenkins.canonical.com/server-team/job/curtin-ci/251/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/251/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/251/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/251/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/251/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/251//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6c826814fd235df7cfb78c926d4d8179dcc51980
https://jenkins.canonical.com/server-team/job/curtin-ci/252/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/252/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/252/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/252/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/252/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/252//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

review: Needs Information

[Dan Bungert (dbungert)]

Also do we want this set more broadly than the apt-install code? I bet I can run into the same problem in late-commands.

[Chris Peterson (cpete)]

Nice work! Looks good to me. I was slightly confused to see SYSTEMD_OFFLINE='true' work since the docs says 0 or 1, but it looks like systemd will check for truthy values too.

review: Approve

[Olivier Gayot (ogayot)]

Thanks for the comments, Dan and Chris! I updated the MP accordingly.

SYSTEMD_OFFLINE will now take values '0' or '1' instead of 'true'. Originally I found out about this variable while reading the systemd code and I didn't look at the documentation. But, I agree that it is better to follow the doc.

I also liked the idea of `systemd_force_offline` defaulting to None so I implemented the change.

Lastly, I moved the systemd_force_offline variable to a more global location in `curtin.util.subp`. The logic is as follows now:

* if systemd_force_offline is True or False, then SYSTEMD_OFFLINE` is set accordingly in the environment (any previous value is overridden).
* if systemd_force_offline is None, then SYSTEMD_OFFLINE is set to '1' if and only if the two following conditions are met:
  * We are executing the command in a chroot (i.e., target is not /)
  * The SYSTEMD_OFFLINE variable is not present in the environment

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:568b903a1dbef12519069b7022f541a889d95187
https://jenkins.canonical.com/server-team/job/curtin-ci/256/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/256/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/256/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/256/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/256/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/256//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

LGTM

yes, 'true'/'false' should be fine and '1'/'0' has a slight edge due to being the documented interface. Thanks!

[Dan Bungert (dbungert)]

review: Approve

diff --git a/curtin/util.py b/curtin/util.py
index 58ee7e1..dcf9422 100644
--- a/curtin/util.py
+++ b/curtin/util.py
@@ -73,12 +73,36 @@ class NotExclusiveError(OSError):
 def _subp(args, data=None, rcs=None, env=None, capture=False,
           combine_capture=False, shell=False, logstring=False,
           decode="replace", target=None, cwd=None, log_captured=False,
-          unshare_pid=None):
+          unshare_pid=None,
+          *, systemd_force_offline: Optional[bool] = None):
     if rcs is None:
         rcs = [0]
     devnull_fp = None
 
     tpath = paths.target_path(target)
+
+    env = env.copy() if env is not None else os.environ.copy()
+    # To determine if we are running in a chroot, systemd checks if
+    # /proc/1/root (corresponding to the init process) and / are the same
+    # inode. If they are different, systemd assumes we are in a chroot.
+    # However, we are running apt-get in a new PID namespace (with /proc
+    # properly mounted). This means that in the new namespace, apt-get gets
+    # assigned PID 1 and is therefore the "init" process.
+    # When systemd compares /proc/1/root and /, it sees they are identical
+    # because the init process is actually running in the chroot.
+    #
+    # Before we started passing the --mount-proc option to unshare, it was
+    # working because /proc/1 in the chroot would still refer to the systemd
+    # init process (running outside the chroot).
+    #
+    # With the SYSTEMD_OFFLINE variable, one can "force" systemd to assume it
+    # is running in a chroot. Let's use it.
+    if systemd_force_offline is not None:
+        # Override the SYSTEMD_OFFLINE variable even if it already exists.
+        env['SYSTEMD_OFFLINE'] = str(int(systemd_force_offline))
+    elif 'SYSTEMD_OFFLINE' not in env and tpath != "/":
+        env['SYSTEMD_OFFLINE'] = '1'
+
     chroot_args = [] if tpath == "/" else ['chroot', target]
     sh_args = ['sh', '-c'] if shell else []
     if isinstance(args, string_types):
@@ -260,6 +284,10 @@ def subp(*args, **kwargs):
         unshare the pid namespace.
         default value (None) is to unshare pid namespace if possible
         and target != /
+    :param systemd_force_offline:
+        if not None, will set the SYSTEMD_OFFLINE env variable to '1' or '0'
+        if None, the variable will be set to '1' only if running in a chroot
+        (i.e., target is not /) and the variable is not already set.
 
     :return
         if not capturing, return is (None, None)
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index 2dd63a7..96f260f 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -350,6 +350,57 @@ class TestSubp(CiTestCase):
                     ['apt-get', 'install'])
         self.assertEqual(expected, args[0])
 
+    @mock.patch.dict(os.environ, clear=True)
+    @mock.patch('curtin.util.subprocess.Popen', side_effect=OSError)
+    def test_systemd_offline_default__no_chroot(self, m_popen):
+        with self.assertRaises(util.ProcessExecutionError):
+            util.subp(['ls', '-l'], target='/')
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'], {})
+
+    @mock.patch('curtin.util.subprocess.Popen', side_effect=OSError)
+    def test_systemd_offline_default__in_chroot(self, m_popen):
+        with mock.patch.dict(os.environ, clear=True):
+            with self.assertRaises(util.ProcessExecutionError):
+                util.subp(['ls', '-l'], target='/target')
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'],
+                         {'SYSTEMD_OFFLINE': '1'})
+
+    @mock.patch('curtin.util.subprocess.Popen', side_effect=OSError)
+    def test_systemd_offline_default__no_override(self, m_popen):
+        with mock.patch.dict(os.environ, {'SYSTEMD_OFFLINE': '1'}, clear=True):
+            with self.assertRaises(util.ProcessExecutionError):
+                util.subp(['ls', '-l'], target='/target')
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'],
+                         {'SYSTEMD_OFFLINE': '1'})
+
+        m_popen.reset_mock()
+        with mock.patch.dict(os.environ, {'SYSTEMD_OFFLINE': '0'}, clear=True):
+            with self.assertRaises(util.ProcessExecutionError):
+                util.subp(['ls', '-l'], target='/target')
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'],
+                         {'SYSTEMD_OFFLINE': '0'})
+
+    @mock.patch('curtin.util.subprocess.Popen', side_effect=OSError)
+    def test_systemd_offline_specified(self, m_popen):
+        with mock.patch.dict(os.environ, {'SYSTEMD_OFFLINE': '0'}, clear=True):
+            with self.assertRaises(util.ProcessExecutionError):
+                util.subp(['ls', '-l'], systemd_force_offline=True)
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'],
+                         {'SYSTEMD_OFFLINE': '1'})
+
+        m_popen.reset_mock()
+        with mock.patch.dict(os.environ, {'SYSTEMD_OFFLINE': '1'}, clear=True):
+            with self.assertRaises(util.ProcessExecutionError):
+                util.subp(['ls', '-l'], systemd_force_offline=False)
+        m_popen.assert_called_once()
+        self.assertEqual(m_popen.call_args.kwargs['env'],
+                         {'SYSTEMD_OFFLINE': '0'})
+
 
 class TestGetUnsharePidArgs(CiTestCase):
     """Test the internal implementation for when to unshare."""