buildinstall: Copy files in thread

Instead of running the copy from the main script explicitly, make it
part of the thread.

This should make things very slightly faster, and the code is much
simpler.

Fixes: https://pagure.io/pungi/issue/959
JIRA: COMPOSE-2604
Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

bin/pungi-koji

@@ -384,9 +384,6 @@ def run_compose(compose, create_latest_link=True, latest_link_status=None):
     essentials_phase.start()
     essentials_phase.stop()
 
-    if not buildinstall_phase.skip():
-        buildinstall_phase.copy_files()
-
     productimg_phase.start()
     productimg_phase.stop()
 

pungi/phases/buildinstall.py

@@ -143,6 +143,9 @@ class BuildinstallPhase(PhaseBase):
         release = self.compose.conf["release_version"]
         disc_type = self.compose.conf['disc_types'].get('dvd', 'dvd')
 
+        # Prepare kickstart file for final images.
+        self.pool.kickstart_file = get_kickstart_file(self.compose)
+
         for arch in self.compose.get_arches():
             commands = []
 
@@ -199,40 +202,6 @@ class BuildinstallPhase(PhaseBase):
         return (super(BuildinstallPhase, self).skip()
                 or (variant.uid if self.used_lorax else None, arch) in self.pool.finished_tasks)
 
-    def copy_files(self):
-        disc_type = self.compose.conf['disc_types'].get('dvd', 'dvd')
-
-        # 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 not self.succeeded(variant, arch):
-                    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 self.used_lorax:
-                    buildinstall_dir = os.path.join(buildinstall_dir, variant.uid)
-
-                if not os.path.isdir(buildinstall_dir) or not os.listdir(buildinstall_dir):
-                    continue
-
-                os_tree = self.compose.paths.compose.os_tree(arch, variant)
-                # TODO: label is not used
-                label = ""
-                volid = get_volid(self.compose, arch, variant, disc_type=disc_type)
-                can_fail = self.compose.can_fail(variant, arch, 'buildinstall')
-                with failable(self.compose, can_fail, variant, arch, 'buildinstall'):
-                    tweak_buildinstall(self.compose, buildinstall_dir, os_tree, arch, variant.uid, label, volid, kickstart_file)
-                    link_boot_iso(self.compose, arch, variant, can_fail)
-
 
 def get_kickstart_file(compose):
     scm_dict = compose.conf.get("buildinstall_kickstart")
@@ -494,4 +463,37 @@ class BuildinstallThread(WorkerThread):
             f.write("\n".join(rpms))
 
         self.pool.finished_tasks.add((variant.uid if variant else None, arch))
+
+        self.copy_files(compose, variant, arch)
+
         self.pool.log_info("[DONE ] %s" % msg)
+
+    def copy_files(self, compose, variant, arch):
+        disc_type = compose.conf['disc_types'].get('dvd', 'dvd')
+
+        buildinstall_dir = compose.paths.work.buildinstall_dir(arch)
+
+        # Lorax runs per-variant, so we need to tweak the source path
+        # to include variant.
+        if variant:
+            buildinstall_dir = os.path.join(buildinstall_dir, variant.uid)
+
+        # Find all relevant variants if lorax is not used.
+        variants = [variant] if variant else compose.get_variants(arch=arch, types=["self", "variant"])
+        for var in variants:
+            os_tree = compose.paths.compose.os_tree(arch, var)
+            # TODO: label is not used
+            label = ""
+            volid = get_volid(compose, arch, var, disc_type=disc_type)
+            can_fail = compose.can_fail(var, arch, 'buildinstall')
+            tweak_buildinstall(
+                compose,
+                buildinstall_dir,
+                os_tree,
+                arch,
+                var.uid,
+                label,
+                volid,
+                self.pool.kickstart_file,
+            )
+            link_boot_iso(compose, arch, var, can_fail)

tests/test_buildinstall.py

@@ -15,7 +15,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
 
 from pungi.phases.buildinstall import (BuildinstallPhase, BuildinstallThread, link_boot_iso,
                                        BOOT_CONFIGS, tweak_configs)
-from tests.helpers import DummyCompose, PungiTestCase, touch, boom
+from tests.helpers import DummyCompose, PungiTestCase, touch
 
 
 class BuildInstallCompose(DummyCompose):
@@ -555,179 +555,9 @@ class TestBuildinstallPhase(PungiTestCase):
                        log_dir=self.topdir + '/logs/amd64/buildinstall-Client-logs')])
 
 
-class TestCopyFiles(PungiTestCase):
+@mock.patch(
-
+    'pungi.phases.buildinstall.get_volid', new=lambda *args, **kwargs: "dummy-volid"
-    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+)
-    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
-    @mock.patch('pungi.phases.buildinstall.get_volid')
-    @mock.patch('os.listdir')
-    @mock.patch('os.path.isdir')
-    @mock.patch('pungi.phases.buildinstall.get_kickstart_file')
-    def test_copy_files_buildinstall(self, get_kickstart_file, isdir, listdir,
-                                     get_volid, tweak_buildinstall, link_boot_iso):
-        compose = BuildInstallCompose(self.topdir, {
-            'buildinstall_method': 'buildinstall'
-        })
-
-        get_volid.side_effect = (
-            lambda compose, arch, variant, disc_type: "%s.%s" % (variant.uid, arch)
-        )
-        get_kickstart_file.return_value = 'kickstart'
-
-        phase = BuildinstallPhase(compose)
-        phase.pool.finished_tasks = set([(None, 'x86_64'), (None, 'amd64')])
-        phase.copy_files()
-
-        self.assertItemsEqual(
-            get_volid.mock_calls,
-            [mock.call(compose, 'x86_64', compose.variants['Server'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Client'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Server'], disc_type='dvd')])
-        self.assertItemsEqual(
-            tweak_buildinstall.mock_calls,
-            [mock.call(compose,
-                       self.topdir + '/work/x86_64/buildinstall',
-                       self.topdir + '/compose/Server/x86_64/os',
-                       'x86_64', 'Server', '', 'Server.x86_64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall',
-                       self.topdir + '/compose/Server/amd64/os',
-                       'amd64', 'Server', '', 'Server.amd64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall',
-                       self.topdir + '/compose/Client/amd64/os',
-                       'amd64', 'Client', '', 'Client.amd64', 'kickstart')])
-        self.assertItemsEqual(
-            link_boot_iso.mock_calls,
-            [mock.call(compose, 'x86_64', compose.variants['Server'], False),
-             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')
-    @mock.patch('os.listdir')
-    @mock.patch('os.path.isdir')
-    @mock.patch('pungi.phases.buildinstall.get_kickstart_file')
-    def test_copy_files_lorax(self, get_kickstart_file, isdir, listdir,
-                              get_volid, tweak_buildinstall, link_boot_iso):
-        compose = BuildInstallCompose(self.topdir, {
-            'buildinstall_method': 'lorax'
-        })
-
-        get_volid.side_effect = (
-            lambda compose, arch, variant, disc_type: "%s.%s" % (variant.uid, arch)
-        )
-        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(
-            get_volid.mock_calls,
-            [mock.call(compose, 'x86_64', compose.variants['Server'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Client'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Server'], disc_type='dvd')])
-        self.assertItemsEqual(
-            tweak_buildinstall.mock_calls,
-            [mock.call(compose,
-                       self.topdir + '/work/x86_64/buildinstall/Server',
-                       self.topdir + '/compose/Server/x86_64/os',
-                       'x86_64', 'Server', '', 'Server.x86_64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall/Server',
-                       self.topdir + '/compose/Server/amd64/os',
-                       'amd64', 'Server', '', 'Server.amd64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall/Client',
-                       self.topdir + '/compose/Client/amd64/os',
-                       'amd64', 'Client', '', 'Client.amd64', 'kickstart')])
-        self.assertItemsEqual(
-            link_boot_iso.mock_calls,
-            [mock.call(compose, 'x86_64', compose.variants['Server'], False),
-             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')
-    @mock.patch('os.listdir')
-    @mock.patch('os.path.isdir')
-    @mock.patch('pungi.phases.buildinstall.get_kickstart_file')
-    def test_copy_fail(self, get_kickstart_file, isdir, listdir,
-                       get_volid, tweak_buildinstall, link_boot_iso):
-        compose = BuildInstallCompose(self.topdir, {
-            'buildinstall_method': 'lorax',
-            'failable_deliverables': [
-                ('^.+$', {'*': ['buildinstall']})
-            ],
-        })
-
-        get_volid.side_effect = (
-            lambda compose, arch, variant, disc_type: "%s.%s" % (variant.uid, arch)
-        )
-        get_kickstart_file.return_value = 'kickstart'
-        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(
-            get_volid.mock_calls,
-            [mock.call(compose, 'x86_64', compose.variants['Server'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Client'], disc_type='dvd'),
-             mock.call(compose, 'amd64', compose.variants['Server'], disc_type='dvd')])
-        self.assertItemsEqual(
-            tweak_buildinstall.mock_calls,
-            [mock.call(compose,
-                       self.topdir + '/work/x86_64/buildinstall/Server',
-                       self.topdir + '/compose/Server/x86_64/os',
-                       'x86_64', 'Server', '', 'Server.x86_64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall/Server',
-                       self.topdir + '/compose/Server/amd64/os',
-                       'amd64', 'Server', '', 'Server.amd64', 'kickstart'),
-             mock.call(compose,
-                       self.topdir + '/work/amd64/buildinstall/Client',
-                       self.topdir + '/compose/Client/amd64/os',
-                       'amd64', 'Client', '', 'Client.amd64', 'kickstart')])
-        self.assertItemsEqual(link_boot_iso.mock_calls, [])
-
-
 class BuildinstallThreadTestCase(PungiTestCase):
 
     def setUp(self):
@@ -735,10 +565,14 @@ class BuildinstallThreadTestCase(PungiTestCase):
         self.pool = mock.Mock(finished_tasks=set())
         self.cmd = mock.Mock()
 
+    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
     @mock.patch('pungi.wrappers.kojiwrapper.KojiWrapper')
     @mock.patch('pungi.wrappers.kojiwrapper.get_buildroot_rpms')
     @mock.patch('pungi.phases.buildinstall.run')
-    def test_buildinstall_thread_with_lorax_in_runroot(self, run, get_buildroot_rpms, KojiWrapperMock):
+    def test_buildinstall_thread_with_lorax_in_runroot(
+        self, run, get_buildroot_rpms, KojiWrapperMock, mock_tweak, mock_link
+    ):
         compose = BuildInstallCompose(self.topdir, {
             'buildinstall_method': 'lorax',
             'runroot': True,
@@ -763,13 +597,14 @@ class BuildinstallThreadTestCase(PungiTestCase):
         with mock.patch('time.sleep'):
             t.process((compose, 'x86_64', compose.variants['Server'], self.cmd), 0)
 
+        destdir = os.path.join(self.topdir, "work/x86_64/buildinstall/Server")
         self.assertItemsEqual(
             get_runroot_cmd.mock_calls,
             [mock.call(
                 'rrt', 'x86_64', self.cmd, channel=None,
                 use_shell=True, task_id=True,
                 packages=['lorax'], mounts=[self.topdir], weight=123,
-                destdir=os.path.join(self.topdir, "work/x86_64/buildinstall/Server"),
+                destdir=destdir,
             )])
         self.assertItemsEqual(
             run_runroot_cmd.mock_calls,
@@ -780,10 +615,34 @@ class BuildinstallThreadTestCase(PungiTestCase):
         self.assertItemsEqual(rpms, ['bash', 'zsh'])
         self.assertItemsEqual(self.pool.finished_tasks, [('Server', 'x86_64')])
 
+        self.assertEqual(
+            mock_tweak.call_args_list,
+            [
+                mock.call(
+                    compose,
+                    destdir,
+                    os.path.join(self.topdir, "compose/Server/x86_64/os"),
+                    "x86_64",
+                    "Server",
+                    "",
+                    "dummy-volid",
+                    self.pool.kickstart_file,
+                )
+            ],
+        )
+        self.assertEqual(
+            mock_link.call_args_list,
+            [mock.call(compose, "x86_64", compose.variants["Server"], False)],
+        )
+
+    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
     @mock.patch('pungi.wrappers.kojiwrapper.KojiWrapper')
     @mock.patch('pungi.wrappers.kojiwrapper.get_buildroot_rpms')
     @mock.patch('pungi.phases.buildinstall.run')
-    def test_buildinstall_thread_with_buildinstall_in_runroot(self, run, get_buildroot_rpms, KojiWrapperMock):
+    def test_buildinstall_thread_with_buildinstall_in_runroot(
+        self, run, get_buildroot_rpms, KojiWrapperMock, mock_tweak, mock_link
+    ):
         compose = BuildInstallCompose(self.topdir, {
             'buildinstall_method': 'buildinstall',
             'runroot': True,
@@ -805,24 +664,48 @@ class BuildinstallThreadTestCase(PungiTestCase):
         t = BuildinstallThread(self.pool)
 
         with mock.patch('time.sleep'):
-            t.process((compose, 'x86_64', None, self.cmd), 0)
+            t.process((compose, "amd64", None, self.cmd), 0)
 
+        destdir = os.path.join(self.topdir, "work/amd64/buildinstall")
         self.assertItemsEqual(
             get_runroot_cmd.mock_calls,
             [mock.call(
-                'rrt', 'x86_64', self.cmd, channel=None,
+                "rrt", "amd64", self.cmd, channel=None,
                 use_shell=True, task_id=True,
                 packages=['anaconda'], mounts=[self.topdir], weight=None,
-                destdir=os.path.join(self.topdir, "work/x86_64/buildinstall"),
+                destdir=destdir,
             )])
         self.assertItemsEqual(
             run_runroot_cmd.mock_calls,
             [mock.call(get_runroot_cmd.return_value,
-                       log_file=self.topdir + '/logs/x86_64/buildinstall.x86_64.log')])
+                       log_file=self.topdir + "/logs/amd64/buildinstall.amd64.log")])
-        with open(self.topdir + '/logs/x86_64/buildinstall-RPMs.x86_64.log') as f:
+        with open(self.topdir + "/logs/amd64/buildinstall-RPMs.amd64.log") as f:
             rpms = f.read().strip().split('\n')
         self.assertItemsEqual(rpms, ['bash', 'zsh'])
-        self.assertItemsEqual(self.pool.finished_tasks, [(None, 'x86_64')])
+        self.assertItemsEqual(self.pool.finished_tasks, [(None, 'amd64')])
+        self.assertItemsEqual(
+            mock_tweak.call_args_list,
+            [
+                mock.call(
+                    compose,
+                    destdir,
+                    os.path.join(self.topdir, "compose", var, "amd64/os"),
+                    "amd64",
+                    var,
+                    "",
+                    "dummy-volid",
+                    self.pool.kickstart_file,
+                )
+                for var in ["Client", "Server"]
+            ],
+        )
+        self.assertItemsEqual(
+            mock_link.call_args_list,
+            [
+                mock.call(compose, "amd64", compose.variants["Client"], False),
+                mock.call(compose, "amd64", compose.variants["Server"], False),
+            ],
+        )
 
     @mock.patch('pungi.wrappers.kojiwrapper.KojiWrapper')
     @mock.patch('pungi.wrappers.kojiwrapper.get_buildroot_rpms')
@@ -922,12 +805,15 @@ class BuildinstallThreadTestCase(PungiTestCase):
         self.assertTrue(os.path.exists(dummy_file))
         self.assertItemsEqual(self.pool.finished_tasks, [])
 
+    @mock.patch('pungi.phases.buildinstall.link_boot_iso')
+    @mock.patch('pungi.phases.buildinstall.tweak_buildinstall')
     @mock.patch('pungi.wrappers.kojiwrapper.KojiWrapper')
     @mock.patch('pungi.wrappers.kojiwrapper.get_buildroot_rpms')
     @mock.patch('pungi.phases.buildinstall.run')
     @mock.patch('pungi.phases.buildinstall.copy_all')
     def test_buildinstall_thread_with_lorax_custom_buildinstall_topdir(
-            self, copy_all, run, get_buildroot_rpms, KojiWrapperMock):
+            self, copy_all, run, get_buildroot_rpms, KojiWrapperMock, mock_tweak, mock_link
+    ):
         compose = BuildInstallCompose(self.topdir, {
             'buildinstall_method': 'lorax',
             'runroot': True,
@@ -980,6 +866,26 @@ class BuildinstallThreadTestCase(PungiTestCase):
                        os.path.join(self.topdir, 'logs/x86_64/buildinstall-Server-logs'))]
         )
 
+        self.assertEqual(
+            mock_tweak.call_args_list,
+            [
+                mock.call(
+                    compose,
+                    os.path.join(self.topdir, "work/x86_64/buildinstall/Server"),
+                    os.path.join(self.topdir, "compose/Server/x86_64/os"),
+                    "x86_64",
+                    "Server",
+                    "",
+                    "dummy-volid",
+                    self.pool.kickstart_file,
+                )
+            ],
+        )
+        self.assertEqual(
+            mock_link.call_args_list,
+            [mock.call(compose, "x86_64", compose.variants["Server"], False)],
+        )
+
 
 class TestSymlinkIso(PungiTestCase):