diff --git a/pungi/checks.py b/pungi/checks.py index d262f313..66f852b0 100644 --- a/pungi/checks.py +++ b/pungi/checks.py @@ -654,6 +654,7 @@ def make_schema(): "gather_allow_reuse": {"type": "boolean", "default": False}, "pkgset_allow_reuse": {"type": "boolean", "default": True}, "createiso_allow_reuse": {"type": "boolean", "default": True}, + "extraiso_allow_reuse": {"type": "boolean", "default": True}, "pkgset_source": {"type": "string", "enum": ["koji", "repos"]}, "createrepo_c": {"type": "boolean", "default": True}, "createrepo_checksum": { diff --git a/pungi/phases/extra_isos.py b/pungi/phases/extra_isos.py index 5db71a6f..31139159 100644 --- a/pungi/phases/extra_isos.py +++ b/pungi/phases/extra_isos.py @@ -14,6 +14,8 @@ # along with this program; if not, see . import os +import hashlib +import json from kobo.shortcuts import force_list from kobo.threads import ThreadPool, WorkerThread @@ -28,8 +30,16 @@ from pungi.phases.createiso import ( copy_boot_images, run_createiso_command, load_and_tweak_treeinfo, + compare_packages, + OldFileLinker, +) +from pungi.util import ( + failable, + get_format_substs, + get_variant_data, + get_volid, + read_json_file, ) -from pungi.util import failable, get_format_substs, get_variant_data, get_volid from pungi.wrappers import iso from pungi.wrappers.scm import get_dir_from_scm, get_file_from_scm @@ -37,9 +47,10 @@ from pungi.wrappers.scm import get_dir_from_scm, get_file_from_scm class ExtraIsosPhase(PhaseLoggerMixin, ConfigGuardedPhase, PhaseBase): name = "extra_isos" - def __init__(self, compose): + def __init__(self, compose, buildinstall_phase): super(ExtraIsosPhase, self).__init__(compose) self.pool = ThreadPool(logger=self.logger) + self.bi = buildinstall_phase def validate(self): for variant in self.compose.get_variants(types=["variant"]): @@ -65,13 +76,17 @@ class ExtraIsosPhase(PhaseLoggerMixin, ConfigGuardedPhase, PhaseBase): commands.append((config, variant, arch)) for (config, variant, arch) in commands: - self.pool.add(ExtraIsosThread(self.pool)) + self.pool.add(ExtraIsosThread(self.pool, self.bi)) self.pool.queue_put((self.compose, config, variant, arch)) self.pool.start() class ExtraIsosThread(WorkerThread): + def __init__(self, pool, buildinstall_phase): + super(ExtraIsosThread, self).__init__(pool) + self.bi = buildinstall_phase + def process(self, item, num): self.num = num compose, config, variant, arch = item @@ -127,24 +142,30 @@ class ExtraIsosThread(WorkerThread): buildinstall_method=compose.conf["buildinstall_method"] ) - script_file = os.path.join( - compose.paths.work.tmp_dir(arch, variant), "extraiso-%s.sh" % filename - ) - with open(script_file, "w") as f: - createiso.write_script(opts, f) + # Check if it can be reused. + hash = hashlib.sha256() + hash.update(json.dumps(config, sort_keys=True).encode("utf-8")) + config_hash = hash.hexdigest() - run_createiso_command( - self.num, - compose, - bootable, - arch, - ["bash", script_file], - [compose.topdir], - log_file=compose.paths.log.log_file( - arch, "extraiso-%s" % os.path.basename(iso_path) - ), - with_jigdo=compose.conf["create_jigdo"], - ) + if not self.try_reuse(compose, variant, arch, config_hash, opts): + script_file = os.path.join( + compose.paths.work.tmp_dir(arch, variant), "extraiso-%s.sh" % filename + ) + with open(script_file, "w") as f: + createiso.write_script(opts, f) + + run_createiso_command( + self.num, + compose, + bootable, + arch, + ["bash", script_file], + [compose.topdir], + log_file=compose.paths.log.log_file( + arch, "extraiso-%s" % os.path.basename(iso_path) + ), + with_jigdo=compose.conf["create_jigdo"], + ) img = add_iso_to_metadata( compose, @@ -156,8 +177,153 @@ class ExtraIsosThread(WorkerThread): ) img._max_size = config.get("max_size") + save_reuse_metadata(compose, variant, arch, config_hash, opts, iso_path) + self.pool.log_info("[DONE ] %s" % msg) + def try_reuse(self, compose, variant, arch, config_hash, opts): + # Check explicit config + if not compose.conf["extraiso_allow_reuse"]: + return + + log_msg = "Cannot reuse ISO for %s.%s" % (variant, arch) + + if opts.buildinstall_method and not self.bi.reused(variant, arch): + # If buildinstall phase was not reused for some reason, we can not + # reuse any bootable image. If a package change caused rebuild of + # boot.iso, we would catch it here too, but there could be a + # configuration change in lorax template which would remain + # undetected. + self.pool.log_info("%s - boot configuration changed", log_msg) + return False + + # Check old compose configuration: extra_files and product_ids can be + # reflected on ISO. + old_config = compose.load_old_compose_config() + if not old_config: + self.pool.log_info("%s - no config for old compose", log_msg) + return False + # Convert current configuration to JSON and back to encode it similarly + # to the old one + config = json.loads(json.dumps(compose.conf)) + for opt in compose.conf: + # Skip a selection of options: these affect what packages can be + # included, which we explicitly check later on. + config_whitelist = set( + [ + "gather_lookaside_repos", + "pkgset_koji_builds", + "pkgset_koji_scratch_tasks", + "pkgset_koji_module_builds", + ] + ) + if opt in config_whitelist: + continue + + if old_config.get(opt) != config.get(opt): + self.pool.log_info("%s - option %s differs", log_msg, opt) + return False + + old_metadata = load_old_metadata(compose, variant, arch, config_hash) + if not old_metadata: + self.pool.log_info("%s - no old metadata found", log_msg) + return False + + # Test if volume ID matches - volid can be generated dynamically based on + # other values, and could change even if nothing else is different. + if opts.volid != old_metadata["opts"]["volid"]: + self.pool.log_info("%s - volume ID differs", log_msg) + return False + + # Compare packages on the ISO. + if compare_packages( + old_metadata["opts"]["graft_points"], + opts.graft_points, + ): + self.pool.log_info("%s - packages differ", log_msg) + return False + + try: + self.perform_reuse( + compose, + variant, + arch, + opts, + old_metadata["opts"]["output_dir"], + old_metadata["opts"]["iso_name"], + ) + return True + except Exception as exc: + self.pool.log_error( + "Error while reusing ISO for %s.%s: %s", variant, arch, exc + ) + compose.traceback("extraiso-reuse-%s-%s-%s" % (variant, arch, config_hash)) + return False + + def perform_reuse(self, compose, variant, arch, opts, old_iso_dir, old_file_name): + """ + Copy all related files from old compose to the new one. As a last step + add the new image to metadata. + """ + linker = OldFileLinker(self.pool._logger) + old_iso_path = os.path.join(old_iso_dir, old_file_name) + iso_path = os.path.join(opts.output_dir, opts.iso_name) + try: + # Hardlink ISO and manifest + for suffix in ("", ".manifest"): + linker.link(old_iso_path + suffix, iso_path + suffix) + # Copy log files + # The log file name includes filename of the image, so we need to + # find old file with the old name, and rename it to the new name. + log_file = compose.paths.log.log_file(arch, "extraiso-%s" % opts.iso_name) + old_log_file = compose.paths.old_compose_path( + compose.paths.log.log_file(arch, "extraiso-%s" % old_file_name) + ) + linker.link(old_log_file, log_file) + # Copy jigdo files + if opts.jigdo_dir: + old_jigdo_dir = compose.paths.old_compose_path(opts.jigdo_dir) + for suffix in (".template", ".jigdo"): + linker.link( + os.path.join(old_jigdo_dir, old_file_name) + suffix, + os.path.join(opts.jigdo_dir, opts.iso_name) + suffix, + ) + except Exception: + # A problem happened while linking some file, let's clean up + # everything. + linker.abort() + raise + + +def save_reuse_metadata(compose, variant, arch, config_hash, opts, iso_path): + """ + Save metadata for possible reuse of this image. The file name is determined + from the hash of a configuration snippet for this image. Any change in that + configuration in next compose will change the hash and thus reuse will be + blocked. + """ + metadata = {"opts": opts._asdict()} + metadata_path = compose.paths.log.log_file( + arch, + "extraiso-reuse-%s-%s-%s" % (variant.uid, arch, config_hash), + ext="json", + ) + with open(metadata_path, "w") as f: + json.dump(metadata, f, indent=2) + + +def load_old_metadata(compose, variant, arch, config_hash): + metadata_path = compose.paths.log.log_file( + arch, + "extraiso-reuse-%s-%s-%s" % (variant.uid, arch, config_hash), + ext="json", + ) + old_path = compose.paths.old_compose_path(metadata_path) + try: + return read_json_file(old_path) + except Exception: + return None + def get_extra_files(compose, variant, arch, extra_files): """Clone the configured files into a directory from where they can be diff --git a/pungi/scripts/config_validate.py b/pungi/scripts/config_validate.py index d4b9b5b5..b4bdb1eb 100644 --- a/pungi/scripts/config_validate.py +++ b/pungi/scripts/config_validate.py @@ -127,7 +127,7 @@ def run(config, topdir, has_old, offline, defined_variables, schema_overrides): pungi.phases.OstreeInstallerPhase(compose, buildinstall_phase), pungi.phases.OSTreePhase(compose), pungi.phases.CreateisoPhase(compose, buildinstall_phase), - pungi.phases.ExtraIsosPhase(compose), + pungi.phases.ExtraIsosPhase(compose, buildinstall_phase), pungi.phases.LiveImagesPhase(compose), pungi.phases.LiveMediaPhase(compose), pungi.phases.ImageBuildPhase(compose), diff --git a/pungi/scripts/pungi_koji.py b/pungi/scripts/pungi_koji.py index 9570e3c5..e393220d 100644 --- a/pungi/scripts/pungi_koji.py +++ b/pungi/scripts/pungi_koji.py @@ -403,7 +403,7 @@ def run_compose( ) ostree_phase = pungi.phases.OSTreePhase(compose, pkgset_phase) createiso_phase = pungi.phases.CreateisoPhase(compose, buildinstall_phase) - extra_isos_phase = pungi.phases.ExtraIsosPhase(compose) + extra_isos_phase = pungi.phases.ExtraIsosPhase(compose, buildinstall_phase) liveimages_phase = pungi.phases.LiveImagesPhase(compose) livemedia_phase = pungi.phases.LiveMediaPhase(compose) image_build_phase = pungi.phases.ImageBuildPhase(compose, buildinstall_phase) diff --git a/tests/test_extra_isos_phase.py b/tests/test_extra_isos_phase.py index e38ec9da..7b7c7346 100644 --- a/tests/test_extra_isos_phase.py +++ b/tests/test_extra_isos_phase.py @@ -1,12 +1,13 @@ # -*- coding: utf-8 -*- - +import logging import mock import six import os from tests import helpers +from pungi.createiso import CreateIsoOpts from pungi.phases import extra_isos @@ -19,7 +20,7 @@ class ExtraIsosPhaseTest(helpers.PungiTestCase): } compose = helpers.DummyCompose(self.topdir, {"extra_isos": {"^Server$": [cfg]}}) - phase = extra_isos.ExtraIsosPhase(compose) + phase = extra_isos.ExtraIsosPhase(compose, mock.Mock()) phase.validate() self.assertEqual(len(compose.log_warning.call_args_list), 1) @@ -30,7 +31,7 @@ class ExtraIsosPhaseTest(helpers.PungiTestCase): } compose = helpers.DummyCompose(self.topdir, {"extra_isos": {"^Server$": [cfg]}}) - phase = extra_isos.ExtraIsosPhase(compose) + phase = extra_isos.ExtraIsosPhase(compose, mock.Mock()) phase.run() self.assertEqual(len(ThreadPool.return_value.add.call_args_list), 3) @@ -51,7 +52,7 @@ class ExtraIsosPhaseTest(helpers.PungiTestCase): } compose = helpers.DummyCompose(self.topdir, {"extra_isos": {"^Server$": [cfg]}}) - phase = extra_isos.ExtraIsosPhase(compose) + phase = extra_isos.ExtraIsosPhase(compose, mock.Mock()) phase.run() self.assertEqual(len(ThreadPool.return_value.add.call_args_list), 2) @@ -71,7 +72,7 @@ class ExtraIsosPhaseTest(helpers.PungiTestCase): } compose = helpers.DummyCompose(self.topdir, {"extra_isos": {"^Server$": [cfg]}}) - phase = extra_isos.ExtraIsosPhase(compose) + phase = extra_isos.ExtraIsosPhase(compose, mock.Mock()) phase.run() self.assertEqual(len(ThreadPool.return_value.add.call_args_list), 2) @@ -106,7 +107,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gvi.return_value = "my volume id" gic.return_value = "/tmp/iso-graft-points" - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -182,7 +183,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gvi.return_value = "my volume id" gic.return_value = "/tmp/iso-graft-points" - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -256,7 +257,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gvi.return_value = "my volume id" gic.return_value = "/tmp/iso-graft-points" - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -330,7 +331,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gvi.return_value = "my volume id" gic.return_value = "/tmp/iso-graft-points" - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -405,7 +406,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gvi.return_value = "my volume id" gic.return_value = "/tmp/iso-graft-points" - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "src"), 1) @@ -476,7 +477,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gic.return_value = "/tmp/iso-graft-points" rcc.side_effect = helpers.mk_boom() - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -494,7 +495,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): gic.return_value = "/tmp/iso-graft-points" rcc.side_effect = helpers.mk_boom(RuntimeError) - t = extra_isos.ExtraIsosThread(mock.Mock()) + t = extra_isos.ExtraIsosThread(mock.Mock(), mock.Mock()) with self.assertRaises(RuntimeError): with mock.patch("time.sleep"): t.process((compose, cfg, server, "x86_64"), 1) @@ -1061,3 +1062,215 @@ class PrepareMetadataTest(helpers.PungiTestCase): ), ], ) + + +class ExtraisoTryReusePhaseTest(helpers.PungiTestCase): + def setUp(self): + super(ExtraisoTryReusePhaseTest, self).setUp() + self.logger = logging.getLogger() + self.logger.setLevel(logging.DEBUG) + self.logger.addHandler(logging.NullHandler()) + + def test_disabled(self): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": False}) + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + opts = CreateIsoOpts() + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + def test_buildinstall_changed(self): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + thread.bi = mock.Mock() + thread.bi.reused.return_value = False + opts = CreateIsoOpts(buildinstall_method="lorax") + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + def test_no_old_config(self): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + opts = CreateIsoOpts() + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + def test_old_config_changed(self): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + old_config = compose.conf.copy() + old_config["release_version"] = "2" + compose.load_old_compose_config.return_value = old_config + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + opts = CreateIsoOpts() + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + def test_no_old_metadata(self): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + compose.load_old_compose_config.return_value = compose.conf.copy() + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + opts = CreateIsoOpts() + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + @mock.patch("pungi.phases.extra_isos.read_json_file") + def test_volume_id_differs(self, read_json_file): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + compose.load_old_compose_config.return_value = compose.conf.copy() + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + + opts = CreateIsoOpts(volid="new-volid") + + read_json_file.return_value = {"opts": {"volid": "old-volid"}} + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + @mock.patch("pungi.phases.extra_isos.read_json_file") + def test_packages_differ(self, read_json_file): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + compose.load_old_compose_config.return_value = compose.conf.copy() + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + + new_graft_points = os.path.join(self.topdir, "new_graft_points") + helpers.touch(new_graft_points, "Packages/f/foo-1-1.x86_64.rpm\n") + opts = CreateIsoOpts(graft_points=new_graft_points, volid="volid") + + old_graft_points = os.path.join(self.topdir, "old_graft_points") + helpers.touch(old_graft_points, "Packages/f/foo-1-2.x86_64.rpm\n") + read_json_file.return_value = { + "opts": {"graft_points": old_graft_points, "volid": "volid"} + } + + self.assertFalse( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + + @mock.patch("pungi.phases.extra_isos.read_json_file") + def test_runs_perform_reuse(self, read_json_file): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + compose.load_old_compose_config.return_value = compose.conf.copy() + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + thread.logger = self.logger + thread.perform_reuse = mock.Mock() + + new_graft_points = os.path.join(self.topdir, "new_graft_points") + helpers.touch(new_graft_points) + opts = CreateIsoOpts(graft_points=new_graft_points, volid="volid") + + old_graft_points = os.path.join(self.topdir, "old_graft_points") + helpers.touch(old_graft_points) + dummy_iso_path = "dummy-iso-path/dummy.iso" + read_json_file.return_value = { + "opts": { + "graft_points": old_graft_points, + "volid": "volid", + "output_dir": os.path.dirname(dummy_iso_path), + "iso_name": os.path.basename(dummy_iso_path), + }, + } + + self.assertTrue( + thread.try_reuse( + compose, compose.variants["Server"], "x86_64", "abcdef", opts + ) + ) + self.assertEqual( + thread.perform_reuse.call_args_list, + [ + mock.call( + compose, + compose.variants["Server"], + "x86_64", + opts, + "dummy-iso-path", + "dummy.iso", + ) + ], + ) + + +@mock.patch("pungi.phases.extra_isos.OldFileLinker") +class ExtraIsoPerformReusePhaseTest(helpers.PungiTestCase): + def test_success(self, OldFileLinker): + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + opts = CreateIsoOpts(output_dir="new/path", iso_name="new.iso") + + thread.perform_reuse( + compose, + compose.variants["Server"], + "x86_64", + opts, + "old", + "image.iso", + ) + + self.assertEqual( + OldFileLinker.return_value.mock_calls, + [ + mock.call.link("old/image.iso", "new/path/new.iso"), + mock.call.link("old/image.iso.manifest", "new/path/new.iso.manifest"), + # The old log file doesn't exist in the test scenario. + mock.call.link( + None, + os.path.join( + self.topdir, "logs/x86_64/extraiso-new.iso.x86_64.log" + ), + ), + ], + ) + + def test_failure(self, OldFileLinker): + OldFileLinker.return_value.link.side_effect = helpers.mk_boom() + compose = helpers.DummyCompose(self.topdir, {"extraiso_allow_reuse": True}) + thread = extra_isos.ExtraIsosThread(compose, mock.Mock()) + opts = CreateIsoOpts(output_dir="new/path", iso_name="new.iso") + + with self.assertRaises(Exception): + thread.perform_reuse( + compose, + compose.variants["Server"], + "x86_64", + opts, + "old", + "image.iso", + ) + + self.assertEqual( + OldFileLinker.return_value.mock_calls, + [ + mock.call.link("old/image.iso", "new/path/new.iso"), + mock.call.abort(), + ], + )