From b899126b7ec5bb0b7d61c88984304e5f12cbaa9d Mon Sep 17 00:00:00 2001 From: Bohdan Khomutskyi Date: Wed, 5 Aug 2020 15:53:58 +0200 Subject: [PATCH] Allow squashfs-only and configuration_file in lorax_options The change allows for setting the parameters as described below to Lorax. Lorax, a program called during the buildInstall phase, creates the SquashFS during the buildInstall phase. The Squash filesystem is present both on the DVD and the BOOT.ISO. squashfs_only --- (str) passes --squashfs-only option. configuration_file --- (str or scm_dict) passes -c option to Lorax. The final goal of this change is to allow for optimization of the installation medium size. This pull request is related to the Fedora change proposal, which is available at this location: https://fedoraproject.org/wiki/Category:Changes/OptimizeSquashFS See the change proposal for more information about the benefits of higher compression ratio. Jira: RHELCMP-693 Signed-off-by: Bohdan Khomutskyi --- doc/configuration.rst | 3 + pungi/checks.py | 2 + pungi/phases/buildinstall.py | 22 +++++- pungi/wrappers/lorax.py | 7 ++ pungi/wrappers/scm.py | 35 +++++++++ tests/test_buildinstall.py | 77 ++++++++++++++++++-- tests/test_lorax_wrapper.py | 6 ++ tests/test_scm.py | 137 ++++++++++++++++++++++++++++++++++- 8 files changed, 282 insertions(+), 7 deletions(-) diff --git a/doc/configuration.rst b/doc/configuration.rst index c6a817a5..41354771 100644 --- a/doc/configuration.rst +++ b/doc/configuration.rst @@ -614,6 +614,9 @@ Options other arguments, so you have to provide a full list and can not just add something. * ``skip_branding`` -- *bool* (default ``False``) + * ``squashfs_only`` -- *bool* (default ``False``) pass the --squashfs_only to Lorax. + * ``configuration_file`` -- (:ref:`scm_dict `) (default empty) pass the + specified configuration file to Lorax using the -c option. **lorax_extra_sources** (*list*) -- a variant/arch mapping with urls for extra source repositories added to Lorax command line. Either one repo or a list can be specified. diff --git a/pungi/checks.py b/pungi/checks.py index 4adf2f64..8f23a672 100644 --- a/pungi/checks.py +++ b/pungi/checks.py @@ -1121,6 +1121,8 @@ def make_schema(): "version": {"type": "string"}, "dracut_args": {"$ref": "#/definitions/list_of_strings"}, "skip_branding": {"type": "boolean"}, + "squashfs_only": {"type": "boolean"}, + "configuration_file": {"$ref": "#/definitions/str_or_scm_dict"}, }, "additionalProperties": False, } diff --git a/pungi/phases/buildinstall.py b/pungi/phases/buildinstall.py index dfaac45a..0eba8fad 100644 --- a/pungi/phases/buildinstall.py +++ b/pungi/phases/buildinstall.py @@ -34,6 +34,7 @@ from pungi.util import get_file_size, get_mtime, failable, makedirs from pungi.util import copy_all, translate_path, move_all from pungi.wrappers.lorax import LoraxWrapper from pungi.wrappers import iso +from pungi.wrappers.scm import get_file from pungi.wrappers.scm import get_file_from_scm from pungi.wrappers import kojiwrapper from pungi.phases.base import PhaseBase @@ -87,6 +88,9 @@ class BuildinstallPhase(PhaseBase): dracut_args = [] rootfs_size = None skip_branding = False + squashfs_only = False + configuration_file = None + configuration_file_source = None version = self.compose.conf.get( "treeinfo_version", self.compose.conf["release_version"] ) @@ -107,6 +111,8 @@ class BuildinstallPhase(PhaseBase): add_arch_template_var.extend(data.get("add_arch_template_var", [])) dracut_args.extend(data.get("dracut_args", [])) skip_branding = data.get("skip_branding", False) + configuration_file_source = data.get("configuration_file") + squashfs_only = data.get("squashfs_only", False) if "version" in data: version = data["version"] output_dir = os.path.join(output_dir, variant.uid) @@ -115,6 +121,17 @@ class BuildinstallPhase(PhaseBase): # The paths module will modify the filename (by inserting arch). But we # only care about the directory anyway. log_dir = _get_log_dir(self.compose, variant, arch) + # Place the lorax.conf as specified by + # the configuration_file parameter of lorax_options to the log directory. + if configuration_file_source: + configuration_file_destination = os.path.join(log_dir, "lorax.conf") + # Obtain lorax.conf for the buildInstall phase + get_file( + configuration_file_source, + configuration_file_destination, + compose=self.compose, + ) + configuration_file = configuration_file_destination repos = repo_baseurl[:] repos.extend( @@ -150,6 +167,8 @@ class BuildinstallPhase(PhaseBase): "dracut-args": dracut_args, "skip_branding": skip_branding, "outputdir": output_dir, + "squashfs_only": squashfs_only, + "configuration_file": configuration_file, } else: # If the buildinstall_topdir is set, it means Koji is used for @@ -182,6 +201,8 @@ class BuildinstallPhase(PhaseBase): log_dir=log_dir, dracut_args=dracut_args, skip_branding=skip_branding, + squashfs_only=squashfs_only, + configuration_file=configuration_file, ) return "rm -rf %s && %s" % ( shlex_quote(output_topdir), @@ -219,7 +240,6 @@ class BuildinstallPhase(PhaseBase): repo_baseurls = [translate_path(self.compose, r) for r in repo_baseurls] if self.buildinstall_method == "lorax": - buildarch = get_valid_arches(arch)[0] for variant in self.compose.get_variants(arch=arch, types=["variant"]): if variant.is_empty: diff --git a/pungi/wrappers/lorax.py b/pungi/wrappers/lorax.py index c5e6badb..7a0ac1b8 100644 --- a/pungi/wrappers/lorax.py +++ b/pungi/wrappers/lorax.py @@ -44,6 +44,8 @@ class LoraxWrapper(object): log_dir=None, dracut_args=None, skip_branding=False, + squashfs_only=False, + configuration_file=None, ): cmd = ["lorax"] cmd.append("--product=%s" % product) @@ -94,6 +96,11 @@ class LoraxWrapper(object): if skip_branding: cmd.append("--skip-branding") + if squashfs_only: + cmd.append("--squashfs-only") + + if configuration_file: + cmd.append("-c=%s" % configuration_file) output_dir = os.path.abspath(output_dir) cmd.append(output_dir) diff --git a/pungi/wrappers/scm.py b/pungi/wrappers/scm.py index f0c22815..1f464295 100644 --- a/pungi/wrappers/scm.py +++ b/pungi/wrappers/scm.py @@ -376,6 +376,41 @@ def get_file_from_scm(scm_dict, target_path, compose=None): return files_copied +def get_file(source, destination, compose, overwrite=False): + """ + Download from SCM or copy a file to the target ``destination``. + The (str) ``destination`` path is returned. + :param source: Either a (str) location of the file or an scm_dict. + :param destination: string. + :param compose: The compose object. + :param overwrite: Whether or not to overwrite the destination file if exists. + + :return: (str) A path to the copied file. + """ + if not source: + return None + + msg = "Getting the configuration file %s" % source + if not overwrite and os.path.exists(destination): + compose.log_warning("[SKIP ] %s" % msg) + return destination + + compose.log_info("[BEGIN] %s" % msg) + if isinstance(source, dict): + if source["scm"] == "file": + source["file"] = os.path.join(compose.config_dir, source["file"]) + with temp_dir(prefix="get_file_") as temporary_directory: + temporary_configuration_location = get_file_from_scm( + source, temporary_directory, compose=compose + ) + temporary_configuration_location_abs = os.path.join( + temporary_directory, temporary_configuration_location[0] + ) + shutil.move(temporary_configuration_location_abs, destination) + compose.log_info("[DONE ] %s" % msg) + return destination + + def get_dir_from_scm(scm_dict, target_path, compose=None): """ Copy a directory from source control to a target path. A list of files diff --git a/tests/test_buildinstall.py b/tests/test_buildinstall.py index 05a64d03..e9d27de9 100644 --- a/tests/test_buildinstall.py +++ b/tests/test_buildinstall.py @@ -161,6 +161,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/x86_64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -188,6 +190,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -215,6 +219,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Client-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), ], ) @@ -294,6 +300,8 @@ class TestBuildinstallPhase(PungiTestCase): "dracut-args": [], "skip_branding": False, "outputdir": self.topdir + "/work/amd64/buildinstall/Server", + "squashfs_only": False, + "configuration_file": None, }, { "product": "Test", @@ -320,6 +328,8 @@ class TestBuildinstallPhase(PungiTestCase): "dracut-args": [], "skip_branding": False, "outputdir": self.topdir + "/work/amd64/buildinstall/Client", + "squashfs_only": False, + "configuration_file": None, }, { "product": "Test", @@ -346,6 +356,8 @@ class TestBuildinstallPhase(PungiTestCase): "dracut-args": [], "skip_branding": False, "outputdir": self.topdir + "/work/x86_64/buildinstall/Server", + "squashfs_only": False, + "configuration_file": None, }, ] @@ -443,6 +455,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Client-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ) ], any_order=True, @@ -523,10 +537,11 @@ class TestBuildinstallPhase(PungiTestCase): ], ) + @mock.patch("pungi.phases.buildinstall.get_file") @mock.patch("pungi.phases.buildinstall.ThreadPool") @mock.patch("pungi.phases.buildinstall.LoraxWrapper") @mock.patch("pungi.phases.buildinstall.get_volid") - def test_uses_lorax_options(self, get_volid, loraxCls, poolCls): + def test_uses_lorax_options(self, get_volid, loraxCls, poolCls, get_file): compose = BuildInstallCompose( self.topdir, { @@ -536,7 +551,6 @@ class TestBuildinstallPhase(PungiTestCase): "release_version": "1", "buildinstall_method": "lorax", "lorax_options": [ - ("^.*$", {"*": {}}), ( "^Server$", { @@ -549,16 +563,20 @@ class TestBuildinstallPhase(PungiTestCase): "rootfs_size": 3, "version": "1.2.3", "dracut_args": ["--xz", "--install", "/.buildstamp"], + "configuration_file": "/tmp/lorax.conf", }, - "amd64": {"noupgrade": False}, + "amd64": {"noupgrade": False, "squashfs_only": True}, }, ), ("^Client$", {"*": {"nomacboot": False}}), - ("^.*$", {"*": {}}), ], }, ) + def _mocked_get_file(source, destination, compose): + return destination + + get_file.side_effect = _mocked_get_file get_volid.return_value = "vol_id" loraxCls.return_value.get_lorax_cmd.return_value = ["lorax", "..."] @@ -607,9 +625,17 @@ class TestBuildinstallPhase(PungiTestCase): add_arch_template_var=["quux=2"], bugurl="http://example.com", rootfs_size=3, - log_dir=self.topdir + "/logs/x86_64/buildinstall-Server-logs", + log_dir=os.path.join( + self.topdir, "logs/x86_64/buildinstall-Server-logs" + ), dracut_args=["--xz", "--install", "/.buildstamp"], skip_branding=False, + squashfs_only=False, + configuration_file=os.path.join( + self.topdir, + "logs/x86_64/buildinstall-Server-logs", + "lorax.conf", + ), ), mock.call( "Test", @@ -636,6 +662,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=True, + configuration_file=None, ), mock.call( "Test", @@ -662,6 +690,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Client-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), ], ) @@ -689,6 +719,25 @@ class TestBuildinstallPhase(PungiTestCase): ), ], ) + """ + There should be one get_file call. This is because the configuration_file + option was used only once in the above configuration. + """ + six.assertCountEqual( + self, + get_file.mock_calls, + [ + mock.call( + "/tmp/lorax.conf", + os.path.join( + compose.topdir, + "logs/x86_64/buildinstall-Server-logs", + "lorax.conf", + ), + compose=compose, + ) + ], + ) @mock.patch("pungi.phases.buildinstall.ThreadPool") @mock.patch("pungi.phases.buildinstall.LoraxWrapper") @@ -762,6 +811,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/x86_64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -788,6 +839,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -814,6 +867,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Client-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), ], ) @@ -915,6 +970,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=buildinstall_topdir + "/x86_64/Server/logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -941,6 +998,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=buildinstall_topdir + "/amd64/Server/logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -967,6 +1026,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=buildinstall_topdir + "/amd64/Client/logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), ], ) @@ -1060,6 +1121,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/x86_64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -1086,6 +1149,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Server-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), mock.call( "Test", @@ -1114,6 +1179,8 @@ class TestBuildinstallPhase(PungiTestCase): log_dir=self.topdir + "/logs/amd64/buildinstall-Client-logs", dracut_args=[], skip_branding=False, + squashfs_only=False, + configuration_file=None, ), ], ) diff --git a/tests/test_lorax_wrapper.py b/tests/test_lorax_wrapper.py index 5eb4fb16..d4fc7e85 100644 --- a/tests/test_lorax_wrapper.py +++ b/tests/test_lorax_wrapper.py @@ -53,6 +53,9 @@ class LoraxWrapperTest(unittest.TestCase): add_arch_template_var=["va1", "va2"], log_dir="/tmp", dracut_args=["--foo", "bar"], + squashfs_only=True, + configuration_file="/storage/RHEL-7.8-20200731.n.0/" + + "logs/x86_64/buildinstall-Server-logs/lorax.conf", ) self.assertEqual(cmd[0], "lorax") @@ -84,6 +87,9 @@ class LoraxWrapperTest(unittest.TestCase): "--logfile=/tmp/lorax.log", "--dracut-arg=--foo", "--dracut-arg=bar", + "--squashfs-only", + "-c=/storage/RHEL-7.8-20200731.n.0/" + + "logs/x86_64/buildinstall-Server-logs/lorax.conf", "/mnt/output_dir", ], ) diff --git a/tests/test_scm.py b/tests/test_scm.py index 2d184cf1..b97ce4ec 100644 --- a/tests/test_scm.py +++ b/tests/test_scm.py @@ -8,12 +8,14 @@ except ImportError: import unittest import shutil import tempfile +import random import os import six from pungi.wrappers import scm from tests.helpers import touch +from kobo.shortcuts import run class SCMBaseTest(unittest.TestCase): @@ -139,6 +141,28 @@ class GitSCMTestCase(SCMBaseTest): self.assertStructure(retval, ["some_file.txt"]) self.assertCalls(run, "git://example.com/git/repo.git", "master") + @mock.patch("pungi.wrappers.scm.run") + def test_get_file_function(self, run): + compose = mock.Mock(conf={}) + + def process(cmd, workdir=None, **kwargs): + touch(os.path.join(workdir, "some_file.txt")) + touch(os.path.join(workdir, "other_file.txt")) + + run.side_effect = process + destination = os.path.join(self.destdir, "other_file.txt") + retval = scm.get_file( + { + "scm": "git", + "repo": "git://example.com/git/repo.git", + "file": "other_file.txt", + }, + os.path.join(self.destdir, destination), + compose=compose, + ) + self.assertEqual(retval, destination) + self.assertCalls(run, "git://example.com/git/repo.git", "master") + @mock.patch("pungi.wrappers.scm.run") def test_get_file_fetch_fails(self, run): url = "git://example.com/git/repo.git" @@ -161,7 +185,13 @@ class GitSCMTestCase(SCMBaseTest): [call[0][0] for call in run.call_args_list], [ ["git", "init"], - ["git", "fetch", "--depth=1", url, "master"], + [ + "git", + "fetch", + "--depth=1", + "git://example.com/git/repo.git", + "master", + ], ["git", "remote", "add", "origin", url], ["git", "remote", "update", "origin"], ["git", "checkout", "master"], @@ -250,6 +280,111 @@ class GitSCMTestCase(SCMBaseTest): self.assertCalls(run, "git://example.com/git/repo.git", "master", "make") +class GitSCMTestCaseReal(SCMBaseTest): + def setUp(self): + super(GitSCMTestCaseReal, self).setUp() + self.compose = mock.Mock(conf={}) + self.gitRepositoryLocation = tempfile.mkdtemp() + run( + ["git", "-C", self.gitRepositoryLocation, "init"], + workdir=self.gitRepositoryLocation, + ) + fileOneLocation = os.path.join(self.gitRepositoryLocation, "some_file.txt") + fileTwoLocation = os.path.join(self.gitRepositoryLocation, "other_file.txt") + self.files = { + fileOneLocation: str(random.randrange(100000000000000000000)), + fileTwoLocation: str(random.randrange(100000000000000000000)), + } + for fileLocation, fileContents in self.files.items(): + with open(fileLocation, "w") as fileHandle: + fileHandle.write(fileContents) + run( + [ + "git", + "-C", + self.gitRepositoryLocation, + "add", + "some_file.txt", + "other_file.txt", + ], + workdir=self.gitRepositoryLocation, + ) + # Must set the user.name and user.email, otherwise an error may be returned. + run( + [ + "git", + "-c", + "user.name=Pungi Test Engineer", + "-c", + "user.email=ptestengineer@example.com", + "-C", + self.gitRepositoryLocation, + "commit", + "-m", + "Initial commit", + ], + workdir=self.gitRepositoryLocation, + ) + + def tearDown(self): + super(GitSCMTestCaseReal, self).tearDown() + shutil.rmtree(self.gitRepositoryLocation) + + def test_get_file_function(self): + sourceFileLocation = random.choice(list(self.files.keys())) + sourceFilename = os.path.basename(sourceFileLocation) + destinationFileLocation = os.path.join(self.destdir, "other_file.txt") + destinationFileActualLocation = scm.get_file( + { + "scm": "git", + "repo": "file:///%s" % self.gitRepositoryLocation, + "file": sourceFilename, + }, + os.path.join(self.destdir, destinationFileLocation), + compose=self.compose, + ) + self.assertEqual(destinationFileActualLocation, destinationFileLocation) + self.assertTrue(os.path.isfile(destinationFileActualLocation)) + + # Comparing the contents of source to the destination file. + with open(sourceFileLocation) as sourceFileHandle: + sourceFileContent = sourceFileHandle.read() + with open(destinationFileActualLocation) as destinationFileHandle: + destinationFileContent = destinationFileHandle.read() + self.assertEqual(sourceFileContent, destinationFileContent) + + def test_get_file_function_with_overwrite(self): + sourceFileLocation = random.choice(list(self.files.keys())) + sourceFilename = os.path.basename(sourceFileLocation) + destinationFileLocation = os.path.join(self.destdir, "other_file.txt") + # Writing pre-existing content to the file, that should be overwritten + preExistingContent = "This line should be overwritten." + with open(destinationFileLocation, "w") as destinationFileHandle: + destinationFileHandle.write(preExistingContent) + destinationFileActualLocation = scm.get_file( + { + "scm": "git", + "repo": "file:///%s" % self.gitRepositoryLocation, + "file": sourceFilename, + }, + os.path.join(self.destdir, destinationFileLocation), + compose=self.compose, + overwrite=True, + ) + self.assertEqual(destinationFileActualLocation, destinationFileLocation) + self.assertTrue(os.path.isfile(destinationFileActualLocation)) + + # Reading the contents of both files to compare later. + with open(sourceFileLocation) as sourceFileHandle: + sourceFileContent = sourceFileHandle.read() + with open(destinationFileActualLocation) as destinationFileHandle: + destinationFileContent = destinationFileHandle.read() + # Ensuring that the file was in fact overwritten + self.assertNotEqual(preExistingContent, destinationFileContent) + # Comparing the contents of source to the destination file. + self.assertEqual(sourceFileContent, destinationFileContent) + + class RpmSCMTestCase(SCMBaseTest): def setUp(self): super(RpmSCMTestCase, self).setUp()