buildinstall: No copy if task fails

Even a failed lorax task can leave behind some in-progress images. Pungi
needs to copy the files into compose/ subdirectory only if the task
finished successfully.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1485021
Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

pungi/phases/buildinstall.py

@@ -41,6 +41,9 @@ class BuildinstallPhase(PhaseBase):
     def __init__(self, compose):
         PhaseBase.__init__(self, compose)
         self.pool = ThreadPool(logger=self.compose._logger)
+        # A set of (variant_uid, arch) pairs that completed successfully. This
+        # is needed to skip copying files for failed tasks.
+        self.pool.finished_tasks = set()
 
     def skip(self):
         if PhaseBase.skip(self):
@@ -138,18 +141,25 @@ class BuildinstallPhase(PhaseBase):
         buildinstall_method = self.compose.conf["buildinstall_method"]
         disc_type = self.compose.conf['disc_types'].get('dvd', 'dvd')
 
+        used_lorax = buildinstall_method == 'lorax'
+
         # copy buildinstall files to the 'os' dir
         kickstart_file = get_kickstart_file(self.compose)
         for arch in self.compose.get_arches():
             for variant in self.compose.get_variants(arch=arch, types=["self", "variant"]):
                 if variant.is_empty:
                     continue
+                if (variant.uid if used_lorax else None, arch) not in self.pool.finished_tasks:
+                    self.compose.log_debug(
+                        'Buildinstall: skipping copying files for %s.%s due to failed runroot task'
+                        % (variant.uid, arch))
+                    continue
 
                 buildinstall_dir = self.compose.paths.work.buildinstall_dir(arch)
 
                 # Lorax runs per-variant, so we need to tweak the source path
                 # to include variant.
-                if buildinstall_method == 'lorax':
+                if used_lorax:
                     buildinstall_dir = os.path.join(buildinstall_dir, variant.uid)
 
                 if not os.path.isdir(buildinstall_dir) or not os.listdir(buildinstall_dir):
@@ -415,4 +425,5 @@ class BuildinstallThread(WorkerThread):
         rpms = get_buildroot_rpms(compose, task_id)
         open(log_file, "w").write("\n".join(rpms))
 
+        self.pool.finished_tasks.add((variant.uid if variant else None, arch))
         self.pool.log_info("[DONE ] %s" % msg)

tests/test_buildinstall.py

@@ -339,6 +339,7 @@ class TestCopyFiles(PungiTestCase):
         get_kickstart_file.return_value = 'kickstart'
 
         phase = BuildinstallPhase(compose)
+        phase.pool.finished_tasks = set([(None, 'x86_64'), (None, 'amd64')])
         phase.copy_files()
 
         self.assertItemsEqual(
@@ -366,6 +367,22 @@ class TestCopyFiles(PungiTestCase):
              mock.call(compose, 'amd64', compose.variants['Client'], False),
              mock.call(compose, 'amd64', compose.variants['Server'], False)])
 
+    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
+    @mock.patch('pungi.phases.buildinstall.get_volid')
+    def test_copy_files_buildinstall_failed_task(self, get_volid, tweak_buildinstall, link_boot_iso):
+        compose = BuildInstallCompose(self.topdir, {
+            'buildinstall_method': 'buildinstall'
+        })
+
+        phase = BuildinstallPhase(compose)
+        phase.pool.finished_tasks = set()
+        phase.copy_files()
+
+        self.assertItemsEqual(get_volid.mock_calls, [])
+        self.assertItemsEqual(tweak_buildinstall.mock_calls, [])
+        self.assertItemsEqual(link_boot_iso.mock_calls, [])
+
     @mock.patch('pungi.phases.buildinstall.link_boot_iso')
     @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
     @mock.patch('pungi.phases.buildinstall.get_volid')
@@ -384,6 +401,7 @@ class TestCopyFiles(PungiTestCase):
         get_kickstart_file.return_value = 'kickstart'
 
         phase = BuildinstallPhase(compose)
+        phase.pool.finished_tasks = set([('Server', 'x86_64'), ('Server', 'amd64'), ('Client', 'amd64')])
         phase.copy_files()
 
         self.assertItemsEqual(
@@ -411,6 +429,22 @@ class TestCopyFiles(PungiTestCase):
              mock.call(compose, 'amd64', compose.variants['Client'], False),
              mock.call(compose, 'amd64', compose.variants['Server'], False)])
 
+    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
+    @mock.patch('pungi.phases.buildinstall.get_volid')
+    def test_copy_files_lorax_failed_task(self, get_volid, tweak_buildinstall, link_boot_iso):
+        compose = BuildInstallCompose(self.topdir, {
+            'buildinstall_method': 'lorax'
+        })
+
+        phase = BuildinstallPhase(compose)
+        phase.pool.finished_tasks = set()
+        phase.copy_files()
+
+        self.assertItemsEqual(get_volid.mock_calls, [])
+        self.assertItemsEqual(tweak_buildinstall.mock_calls, [])
+        self.assertItemsEqual(link_boot_iso.mock_calls, [])
+
     @mock.patch('pungi.phases.buildinstall.link_boot_iso')
     @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
     @mock.patch('pungi.phases.buildinstall.get_volid')
@@ -433,6 +467,7 @@ class TestCopyFiles(PungiTestCase):
         tweak_buildinstall.side_effect = boom
 
         phase = BuildinstallPhase(compose)
+        phase.pool.finished_tasks = set([('Server', 'x86_64'), ('Server', 'amd64'), ('Client', 'amd64')])
         phase.copy_files()
 
         self.assertItemsEqual(
@@ -459,6 +494,11 @@ class TestCopyFiles(PungiTestCase):
 
 class BuildinstallThreadTestCase(PungiTestCase):
 
+    def setUp(self):
+        super(BuildinstallThreadTestCase, self).setUp()
+        self.pool = mock.Mock(finished_tasks=set())
+        self.cmd = mock.Mock()
+
     @mock.patch('pungi.phases.buildinstall.KojiWrapper')
     @mock.patch('pungi.phases.buildinstall.get_buildroot_rpms')
     @mock.patch('pungi.phases.buildinstall.run')
@@ -472,8 +512,6 @@ class BuildinstallThreadTestCase(PungiTestCase):
         })
 
         get_buildroot_rpms.return_value = ['bash', 'zsh']
-        pool = mock.Mock()
-        cmd = mock.Mock()
 
         get_runroot_cmd = KojiWrapperMock.return_value.get_runroot_cmd
 
@@ -484,14 +522,14 @@ class BuildinstallThreadTestCase(PungiTestCase):
             'task_id': 1234,
         }
 
-        t = BuildinstallThread(pool)
+        t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', compose.variants['Server'], cmd), 0)
+            t.process((compose, 'x86_64', compose.variants['Server'], self.cmd), 0)
 
         self.assertItemsEqual(
             get_runroot_cmd.mock_calls,
-            [mock.call('rrt', 'x86_64', cmd, channel=None,
+            [mock.call('rrt', 'x86_64', self.cmd, channel=None,
                        use_shell=True, task_id=True,
                        packages=['strace', 'lorax'], mounts=[self.topdir], weight=123)])
         self.assertItemsEqual(
@@ -501,6 +539,7 @@ class BuildinstallThreadTestCase(PungiTestCase):
         with open(self.topdir + '/logs/x86_64/buildinstall-Server-RPMs.x86_64.log') as f:
             rpms = f.read().strip().split('\n')
         self.assertItemsEqual(rpms, ['bash', 'zsh'])
+        self.assertItemsEqual(self.pool.finished_tasks, [('Server', 'x86_64')])
 
     @mock.patch('pungi.phases.buildinstall.KojiWrapper')
     @mock.patch('pungi.phases.buildinstall.get_buildroot_rpms')
@@ -514,8 +553,6 @@ class BuildinstallThreadTestCase(PungiTestCase):
         })
 
         get_buildroot_rpms.return_value = ['bash', 'zsh']
-        pool = mock.Mock()
-        cmd = mock.Mock()
 
         get_runroot_cmd = KojiWrapperMock.return_value.get_runroot_cmd
 
@@ -526,14 +563,14 @@ class BuildinstallThreadTestCase(PungiTestCase):
             'task_id': 1234,
         }
 
-        t = BuildinstallThread(pool)
+        t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', None, cmd), 0)
+            t.process((compose, 'x86_64', None, self.cmd), 0)
 
         self.assertItemsEqual(
             get_runroot_cmd.mock_calls,
-            [mock.call('rrt', 'x86_64', cmd, channel=None,
+            [mock.call('rrt', 'x86_64', self.cmd, channel=None,
                        use_shell=True, task_id=True,
                        packages=['strace', 'anaconda'], mounts=[self.topdir], weight=None)])
         self.assertItemsEqual(
@@ -543,6 +580,7 @@ class BuildinstallThreadTestCase(PungiTestCase):
         with open(self.topdir + '/logs/x86_64/buildinstall-RPMs.x86_64.log') as f:
             rpms = f.read().strip().split('\n')
         self.assertItemsEqual(rpms, ['bash', 'zsh'])
+        self.assertItemsEqual(self.pool.finished_tasks, [(None, 'x86_64')])
 
     @mock.patch('pungi.phases.buildinstall.KojiWrapper')
     @mock.patch('pungi.phases.buildinstall.get_buildroot_rpms')
@@ -559,8 +597,6 @@ class BuildinstallThreadTestCase(PungiTestCase):
         })
 
         get_buildroot_rpms.return_value = ['bash', 'zsh']
-        pool = mock.Mock()
-        cmd = mock.Mock()
 
         run_runroot_cmd = KojiWrapperMock.return_value.run_runroot_cmd
         run_runroot_cmd.return_value = {
@@ -569,16 +605,17 @@ class BuildinstallThreadTestCase(PungiTestCase):
             'task_id': 1234,
         }
 
-        t = BuildinstallThread(pool)
+        t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', None, cmd), 0)
+            t.process((compose, 'x86_64', None, self.cmd), 0)
 
         compose._logger.info.assert_has_calls([
             mock.call('[FAIL] Buildinstall (variant None, arch x86_64) failed, but going on anyway.'),
             mock.call('Runroot task failed: 1234. See %s/logs/x86_64/buildinstall.x86_64.log for more details.'
                       % self.topdir)
         ])
+        self.assertItemsEqual(self.pool.finished_tasks, [])
 
     @mock.patch('pungi.phases.buildinstall.KojiWrapper')
     @mock.patch('pungi.phases.buildinstall.get_buildroot_rpms')
@@ -595,8 +632,6 @@ class BuildinstallThreadTestCase(PungiTestCase):
         })
 
         get_buildroot_rpms.return_value = ['bash', 'zsh']
-        pool = mock.Mock()
-        cmd = mock.Mock()
 
         run_runroot_cmd = KojiWrapperMock.return_value.run_runroot_cmd
         run_runroot_cmd.return_value = {
@@ -605,15 +640,16 @@ class BuildinstallThreadTestCase(PungiTestCase):
             'task_id': 1234,
         }
 
-        t = BuildinstallThread(pool)
+        t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', compose.variants['Server'], cmd), 0)
+            t.process((compose, 'x86_64', compose.variants['Server'], self.cmd), 0)
 
         compose._logger.info.assert_has_calls([
             mock.call('[FAIL] Buildinstall (variant Server, arch x86_64) failed, but going on anyway.'),
             mock.call('Runroot task failed: 1234. See %s/logs/x86_64/buildinstall-Server.x86_64.log for more details.' % self.topdir)
         ])
+        self.assertItemsEqual(self.pool.finished_tasks, [])
 
     @mock.patch('pungi.phases.buildinstall.KojiWrapper')
     @mock.patch('pungi.phases.buildinstall.get_buildroot_rpms')
@@ -630,20 +666,19 @@ class BuildinstallThreadTestCase(PungiTestCase):
         })
 
         get_buildroot_rpms.return_value = ['bash', 'zsh']
-        pool = mock.Mock()
-        cmd = mock.Mock()
 
         dummy_file = os.path.join(self.topdir, 'work/x86_64/buildinstall/Server/dummy')
         touch(os.path.join(dummy_file))
 
-        t = BuildinstallThread(pool)
+        t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', compose.variants['Server'], cmd), 0)
+            t.process((compose, 'x86_64', compose.variants['Server'], self.cmd), 0)
 
         self.assertEqual(0, len(run.mock_calls))
 
         self.assertTrue(os.path.exists(dummy_file))
+        self.assertItemsEqual(self.pool.finished_tasks, [])
 
 
 class TestSymlinkIso(PungiTestCase):