From 2dd30008ae1f0fe18092881d6f304c2e56d28879 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Thu, 9 May 2019 10:53:00 +0200 Subject: [PATCH] buildinstall: Copy files in thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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ář --- bin/pungi-koji | 3 - pungi/phases/buildinstall.py | 70 ++++----- tests/test_buildinstall.py | 274 ++++++++++++----------------------- 3 files changed, 126 insertions(+), 221 deletions(-) diff --git a/bin/pungi-koji b/bin/pungi-koji index 38e2fcf3..2971fedf 100755 --- a/bin/pungi-koji +++ b/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() diff --git a/pungi/phases/buildinstall.py b/pungi/phases/buildinstall.py index 9e26088b..f2ea8bda 100644 --- a/pungi/phases/buildinstall.py +++ b/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) diff --git a/tests/test_buildinstall.py b/tests/test_buildinstall.py index 176bfb1b..2a956c69 100644 --- a/tests/test_buildinstall.py +++ b/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.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, []) - - +@mock.patch( + 'pungi.phases.buildinstall.get_volid', new=lambda *args, **kwargs: "dummy-volid" +) 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')]) - with open(self.topdir + '/logs/x86_64/buildinstall-RPMs.x86_64.log') as f: + log_file=self.topdir + "/logs/amd64/buildinstall.amd64.log")]) + 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):