payload/rpm-ostree: Include program output in exception

The rpm-ostree container deployment path can fail for many reasons
from networking to details in mount setup.

What we really want is a proper API with progress out of bootc/ostree;
I will work on that at some point.

In the meantime though, just capture stderr on failure and include
it in the payload installation error so people don't have to
dig into `program.log` which is very obscure.

Signed-off-by: Colin Walters <[email protected]>

pyanaconda/core/util.py

@@ -286,6 +286,26 @@ def execWithCapture(command, argv, stdin=None, root='/', env_prune=None, env_add
     return _run_program(argv, stdin=stdin, root=root, log_output=log_output, env_prune=env_prune,
                         env_add=env_add, filter_stderr=filter_stderr, do_preexec=do_preexec)[1]
 
+def execProgram(command, argv, stdin=None, root='/', env_prune=None, env_add=None,
+                log_output=True, filter_stderr=False, do_preexec=True):
+    """ Run an external program and capture standard out and err as well as the return code.
+
+        :param command: The command to run
+        :param argv: The argument list
+        :param stdin: The file object to read stdin from.
+        :param root: The directory to chroot to before running command.
+        :param env_prune: environment variable to remove before execution
+        :param env_add: environment variables added for the execution
+        :param log_output: Whether to log the output of command
+        :param filter_stderr: Whether stderr should be excluded from the returned output
+        :param do_preexec: whether to use the preexec function
+        :return: Tuple of the return code and the output of the command
+    """
+    argv = [command] + argv
+
+    return _run_program(argv, stdin=stdin, root=root, log_output=log_output, env_prune=env_prune,
+                        env_add=env_add, filter_stderr=filter_stderr, do_preexec=do_preexec)
+
 
 def execWithCaptureAsLiveUser(command, argv, stdin=None, root='/', log_output=True,
                               filter_stderr=False, do_preexec=True):

pyanaconda/modules/payloads/payload/rpm_ostree/installation.py

@@ -26,7 +26,7 @@
 from pyanaconda.core.glib import GError, Variant, create_new_context, format_size_full
 from pyanaconda.core.i18n import _
 from pyanaconda.core.path import make_directories, set_system_root
-from pyanaconda.core.util import execWithRedirect
+from pyanaconda.core.util import execWithRedirect, execProgram
 from pyanaconda.modules.common.constants.objects import BOOTLOADER, DEVICE_TREE
 from pyanaconda.modules.common.constants.services import LOCALIZATION, STORAGE
 from pyanaconda.modules.common.errors.installation import (
@@ -45,16 +45,17 @@
 log = get_module_logger(__name__)
 
 
-def safe_exec_with_redirect(cmd, argv, successful_return_codes=(0,), **kwargs):
-    """Like util.execWithRedirect, but treat errors as fatal.
+def safe_exec_program(cmd, argv, successful_return_codes=(0,), **kwargs):
+    """Like util.execProgram, but treat errors as fatal.
 
     :raise: PayloadInstallationError if the call fails for any reason
     """
-    rc = execWithRedirect(cmd, argv, **kwargs)
+    rc, output = execProgram(cmd, argv, **kwargs)
 
     if rc not in successful_return_codes:
         raise PayloadInstallationError(
-            "The command '{}' exited with the code {}.".format(" ".join([cmd] + argv), rc)
+            "The command '{}' exited with the code {}:\n{}".format(" ".join([cmd] + argv), rc,
+            output)
         )
 
 
@@ -168,8 +169,8 @@ def _setup_internal_bindmount(self, src, dest=None,
         dest = os.path.realpath(dest)
 
         if bind_ro:
-            safe_exec_with_redirect("mount", ["--bind", src, src])
-            safe_exec_with_redirect("mount", ["--bind", "-o", "remount,ro", src, src])
+            safe_exec_program("mount", ["--bind", src, src])
+            safe_exec_program("mount", ["--bind", "-o", "remount,ro", src, src])
         else:
             # Create missing directories for user defined mount points
             if not os.path.exists(dest):
@@ -181,7 +182,7 @@ def _setup_internal_bindmount(self, src, dest=None,
                 bindopt = '--rbind'
             else:
                 bindopt = '--bind'
-            safe_exec_with_redirect("mount", [bindopt, src, dest])
+            safe_exec_program("mount", [bindopt, src, dest])
 
         self._internal_mounts.append(src if bind_ro else dest)
 
@@ -238,7 +239,7 @@ def _create_tmpfiles(self, path):
         # Therefore we ignore error 65, since this is coming from
         # the payload itself and the actual execution of it was fine
 
-        safe_exec_with_redirect(
+        safe_exec_program(
             "systemd-tmpfiles", [
                 "--create",
                 "--boot",
@@ -375,7 +376,7 @@ def _run(self):
             # doesn't, we're not on a UEFI system, so we don't want to copy the data.
             if not fname == 'efi' or (is_efi and os.path.isdir(os.path.join(physboot, fname))):
                 log.info("Copying bootloader data: %s", fname)
-                safe_exec_with_redirect('cp', ['-r', '-p', srcpath, physboot])
+                safe_exec_program('cp', ['-r', '-p', srcpath, physboot])
 
             # Unfortunate hack, see https://github.com/rhinstaller/anaconda/issues/1188
             efi_grubenv_link = physboot + '/grub2/grubenv'
@@ -403,7 +404,7 @@ def run(self):
 
         This will create the repository as well.
         """
-        safe_exec_with_redirect(
+        safe_exec_program(
             "ostree",
             ["admin",
              "--sysroot=" + self._physroot,
@@ -582,7 +583,7 @@ def _set_kargs(self):
 
         set_kargs_args.append("rw")
 
-        safe_exec_with_redirect("ostree", set_kargs_args, root=self._sysroot)
+        safe_exec_program("ostree", set_kargs_args, root=self._sysroot)
 
 
 class DeployOSTreeTask(Task):
@@ -608,7 +609,7 @@ def run(self):
 
         self.report_progress(_("Deployment starting: {}").format(ref))
 
-        safe_exec_with_redirect(
+        safe_exec_program(
             "ostree",
             ["admin",
              "--sysroot=" + self._physroot,
@@ -630,13 +631,13 @@ def run(self):
             if not self._data.signature_verification_enabled:
                 args.append("--no-signature-verification")
 
-            safe_exec_with_redirect(
+            safe_exec_program(
                 "ostree",
                 args
             )
         else:
             log.info("ostree admin deploy starting")
-            safe_exec_with_redirect(
+            safe_exec_program(
                 "ostree",
                 ["admin",
                  "--sysroot=" + self._physroot,
@@ -647,7 +648,7 @@ def run(self):
 
         log.info("ostree config set sysroot.readonly true")
 
-        safe_exec_with_redirect(
+        safe_exec_program(
             "ostree",
             ["config",
              "--repo=" + self._physroot + "/ostree/repo",

tests/unit_tests/pyanaconda_tests/modules/payloads/payload/test_rpm_ostree_tasks.py

@@ -74,7 +74,7 @@ class PrepareOSTreeMountTargetsTaskTestCase(unittest.TestCase):
 
     @patch("os.path.exists")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_setup_internal_bindmount(self, exec_mock, mkdir_mock, exists_mock):
         """Test OSTree mount target prepare task _setup_internal_bindmount"""
         exec_mock.return_value = 0
@@ -87,7 +87,7 @@ def test_setup_internal_bindmount(self, exec_mock, mkdir_mock, exists_mock):
 
     @patch("os.path.exists")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_container_setup_internal_bindmount(self, exec_mock, mkdir_mock, exists_mock):
         """Test OSTree mount target prepare task _setup_internal_bindmount with ostreecontainer"""
         exec_mock.return_value = 0
@@ -154,7 +154,7 @@ def _check_setup_internal_bindmount(self, task, exec_mock, mkdir_mock, exists_mo
         exec_mock.reset_mock()
         exists_mock.return_value = True
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("os.path.exists", returns=True)
@@ -165,7 +165,7 @@ def test_run_with_var(self, exist_mock, storage_mock, mkdir_mock, exec_mock):
         data = _make_config_data()
         self._check_run_with_var(data, exist_mock, storage_mock, mkdir_mock, exec_mock)
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("os.path.exists", returns=True)
@@ -222,7 +222,7 @@ def _check_run_with_var(self, data, exist_mock, storage_mock, mkdir_mock, exec_m
         assert len(exec_mock.mock_calls) == 19
         mkdir_mock.assert_called_once_with("/sysroot/var/lib")
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("os.path.exists", returns=True)
@@ -233,7 +233,7 @@ def test_run_without_var(self, exists_mock, storage_mock, mkdir_mock, exec_mock)
         data = _make_config_data()
         self._check_run_without_var(data, exists_mock, storage_mock, mkdir_mock, exec_mock)
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("os.path.exists", returns=True)
@@ -289,7 +289,7 @@ def _check_run_without_var(self, data, exists_mock, storage_mock, mkdir_mock, ex
         assert len(exec_mock.mock_calls) == 19
         mkdir_mock.assert_called_once_with("/sysroot/var/lib")
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     def test_run_failed(self, storage_mock, mkdir_mock, exec_mock):
@@ -299,7 +299,7 @@ def test_run_failed(self, storage_mock, mkdir_mock, exec_mock):
         data = _make_config_data()
         self._check_run_failed(data, storage_mock, mkdir_mock, exec_mock)
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.make_directories")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     def test_container_run_failed(self, storage_mock, mkdir_mock, exec_mock):
@@ -392,7 +392,7 @@ def test_run_failed(self, listdir_mock, isdir_mock, storage_mock):
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.isdir")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.listdir")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.islink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.unlink")
     def test_run_noefi_noefidir_nolink(
@@ -418,7 +418,7 @@ def test_run_noefi_noefidir_nolink(
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.isdir")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.listdir")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.islink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.unlink")
     def test_run_noefi_efidir_link(
@@ -444,7 +444,7 @@ def test_run_noefi_efidir_link(
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.isdir")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.listdir")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.islink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.unlink")
     def test_run_efi_nolink(
@@ -471,7 +471,7 @@ def test_run_efi_nolink(
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.STORAGE")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.isdir")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.listdir")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.path.islink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.unlink")
     def test_run_noefi_notadir(
@@ -496,7 +496,7 @@ def test_run_noefi_notadir(
 
 
 class InitOSTreeFsAndRepoTaskTestCase(unittest.TestCase):
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_run(self, exec_mock):
         """Test OSTree fs and repo init task"""
         exec_mock.return_value = 0
@@ -684,7 +684,7 @@ def test_container_options(self, sysroot_cls, gio_file_cls, conf_mock):
 
 
 class ConfigureBootloaderTaskTestCase(unittest.TestCase):
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.rename")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.symlink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.LOCALIZATION")
@@ -733,7 +733,7 @@ def test_btrfs_run(self, devdata_mock, storage_mock, localization_mock,
                 root=sysroot
             )
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.rename")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.symlink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.LOCALIZATION")
@@ -782,7 +782,7 @@ def test_nonbtrfs_run(self, devdata_mock, storage_mock, localization_mock,
             )
 
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.have_bootupd")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.rename")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.symlink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.LOCALIZATION")
@@ -835,7 +835,7 @@ def test_bootupd_run(self, devdata_mock, storage_mock, localization_mock, symlin
             ])
 
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.have_bootupd")
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.rename")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.symlink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.LOCALIZATION")
@@ -887,7 +887,7 @@ def test_bootupd_run_with_leavebootorder(self, devdata_mock, storage_mock, local
                 )
             ])
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.rename")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.os.symlink")
     @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.conf")
@@ -915,7 +915,7 @@ def test_dir_run(self, conf_mock, symlink_mock, rename_mock, exec_mock):
 
 
 class DeployOSTreeTaskTestCase(unittest.TestCase):
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_run(self, exec_mock):
         """Test OSTree deploy task"""
         exec_mock.return_value = 0
@@ -932,7 +932,7 @@ def test_run(self, exec_mock):
         ])
         # no need to mock RpmOstree.varsubst_basearch(), since "ref" won't change
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_container_run(self, exec_mock):
         """Test OSTree deploy task ostreecontainer"""
         exec_mock.return_value = 0
@@ -953,7 +953,7 @@ def test_container_run(self, exec_mock):
         ])
         # no need to mock RpmOstree.varsubst_basearch(), since "ref" won't change
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_container_run_with_no_stateroot(self, exec_mock):
         """Test OSTree deploy task ostreecontainer without stateroot."""
         exec_mock.return_value = 0
@@ -973,7 +973,7 @@ def test_container_run_with_no_stateroot(self, exec_mock):
                             "set", "sysroot.readonly", "true"])
         ])
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_container_run_with_no_transport(self, exec_mock):
         """Test OSTree deploy task ostreecontainer without transport."""
         exec_mock.return_value = 0
@@ -993,7 +993,7 @@ def test_container_run_with_no_transport(self, exec_mock):
                             "set", "sysroot.readonly", "true"])
         ])
 
-    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execWithRedirect")
+    @patch("pyanaconda.modules.payloads.payload.rpm_ostree.installation.execProgram")
     def test_container_run_with_no_verification(self, exec_mock):
         """Test OSTree deploy task ostreecontainer without signature verification."""
         exec_mock.return_value = 0