From 5842bbb93e2858ff425319aa0f226927dcc7bc5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Budai?= Date: Mon, 27 Jul 2020 15:29:30 +0200 Subject: [PATCH] test: make osbuild.compile method take output_dir as a parameter Previously, the osbuild executor had its internal temporary directory that served as the output directory. However, this approach gives no power to the caller to control the lifetime of the produced artifacts. When more images are built using one executor, the results will accumulate in one place possibly leading to exhaustion of disk space. This commit removes the executor's internal output directory. The output directory can now be passed to osbuild.compile, so the caller can control its lifetime. If no directory is passed in, the compile method will use its own temporary directory - this is useful in cases when the caller doesn't care about the built artifacts or the manifest doesn't have any outputs. --- test/run/test_assemblers.py | 34 ++++++++------- test/run/test_boot.py | 8 ++-- test/test.py | 87 +++++++++++++++---------------------- 3 files changed, 57 insertions(+), 72 deletions(-) diff --git a/test/run/test_assemblers.py b/test/run/test_assemblers.py index 988c3614..49aa0639 100644 --- a/test/run/test_assemblers.py +++ b/test/run/test_assemblers.py @@ -38,10 +38,10 @@ class TestAssemblers(test.TestBase): treeid = osb.treeid_from_manifest(data) assert treeid - osb.compile(data, checkpoints=[treeid]) - with osb.map_object(treeid) as tree, \ - osb.map_output(output_path) as output: - yield tree, output + with tempfile.TemporaryDirectory(dir="/var/tmp") as output_dir: + osb.compile(data, output_dir=output_dir, checkpoints=[treeid]) + with osb.map_object(treeid) as tree: + yield tree, os.path.join(output_dir, output_path) def assertImageFile(self, filename, fmt, expected_size=None): info = json.loads(subprocess.check_output(["qemu-img", "info", "--output", "json", filename])) @@ -107,18 +107,20 @@ class TestAssemblers(test.TestBase): manifest = json.load(f) data = json.dumps(manifest) - osb.compile(data) - with osb.map_output("compose.json") as filename: - with open(filename) as f: - compose = json.load(f) - commit_id = compose["ostree-commit"] - ref = compose["ref"] - rpmostree_inputhash = compose["rpm-ostree-inputhash"] - assert commit_id - assert ref - assert rpmostree_inputhash + with tempfile.TemporaryDirectory(dir="/var/tmp") as output_dir: + osb.compile(data, output_dir=output_dir) + compose_file = os.path.join(output_dir, "compose.json") + repo = os.path.join(output_dir, "repo") + + with open(compose_file) as f: + compose = json.load(f) + commit_id = compose["ostree-commit"] + ref = compose["ref"] + rpmostree_inputhash = compose["rpm-ostree-inputhash"] + assert commit_id + assert ref + assert rpmostree_inputhash - with osb.map_output("repo") as repo: md = subprocess.check_output( [ "ostree", @@ -127,7 +129,7 @@ class TestAssemblers(test.TestBase): "--print-metadata-key=rpmostree.inputhash", commit_id ], encoding="utf-8").strip() - self.assertEqual(md, f"'{rpmostree_inputhash}'") + self.assertEqual(md, f"'{rpmostree_inputhash}'") @unittest.skipUnless(test.TestBase.have_tree_diff(), "tree-diff missing") def test_qemu(self): diff --git a/test/run/test_boot.py b/test/run/test_boot.py index 3ee3fb46..f98ee904 100644 --- a/test/run/test_boot.py +++ b/test/run/test_boot.py @@ -24,11 +24,11 @@ class TestBoot(test.TestBase): "manifests/fedora-boot.json") with self.osbuild as osb: - osb.compile_file(manifest) - with osb.map_output("fedora-boot.qcow2") as qcow2, \ - tempfile.TemporaryDirectory() as d: + with tempfile.TemporaryDirectory(dir="/var/tmp") as temp_dir: + osb.compile_file(manifest, output_dir=temp_dir) + qcow2 = os.path.join(temp_dir, "fedora-boot.qcow2") + output_file = os.path.join(temp_dir, "output") - output_file = os.path.join(d, "output") subprocess.run(["qemu-system-x86_64", "-snapshot", "-m", "1024", diff --git a/test/test.py b/test/test.py index 69ee4710..28de0d84 100644 --- a/test/test.py +++ b/test/test.py @@ -221,7 +221,6 @@ class OSBuild(contextlib.AbstractContextManager): _exitstack = None _cachedir = None - _outputdir = None def __init__(self, unit_test, cache_from=None): self._unittest = unit_test @@ -243,10 +242,6 @@ class OSBuild(contextlib.AbstractContextManager): self._cachedir], check=True) - # Create a temporary output-directors for assembled artifacts. - output = tempfile.TemporaryDirectory(dir="/var/tmp") - self._outputdir = self._exitstack.enter_context(output) - # Keep our ExitStack for `__exit__()`. self._exitstack = self._exitstack.pop_all() @@ -257,7 +252,6 @@ class OSBuild(contextlib.AbstractContextManager): with self._exitstack: pass - self._outputdir = None self._cachedir = None self._exitstack = None @@ -275,52 +269,57 @@ class OSBuild(contextlib.AbstractContextManager): print(data_stderr) print("-- END ---------------------------------") - def compile(self, data_stdin, checkpoints=None): + def compile(self, data_stdin, output_dir=None, checkpoints=None): """Compile an Artifact This takes a manifest as `data_stdin`, executes the pipeline, and assembles the artifact. No intermediate steps are kept, unless you provide suitable checkpoints. - The produced artifact (if any) is stored in the output directory. Use - `map_output()` to temporarily map the file and get access. Note that - the output directory becomes invalid when you leave the context-manager - of this class. + The produced artifact (if any) is stored in the directory passed via + the output_dir parameter. If it's set to None, a temporary directory + is used and thus the caller cannot access the built artifact. """ - cmd_args = [] + if output_dir is None: + output_dir_context = tempfile.TemporaryDirectory(dir="/var/tmp") + else: + output_dir_context = contextlib.nullcontext(output_dir) - cmd_args += ["--json"] - cmd_args += ["--libdir", "."] - cmd_args += ["--output-directory", self._outputdir] - cmd_args += ["--store", self._cachedir] + with output_dir_context as osbuild_output_dir: + cmd_args = [] - for c in (checkpoints or []): - cmd_args += ["--checkpoint", c] + cmd_args += ["--json"] + cmd_args += ["--libdir", "."] + cmd_args += ["--output-directory", osbuild_output_dir] + cmd_args += ["--store", self._cachedir] - # Spawn the `osbuild` executable, feed it the specified data on - # `STDIN` and wait for completion. If we are interrupted, we always - # wait for `osbuild` to shut down, so we can clean up its file-system - # trees (they would trigger `EBUSY` if we didn't wait). - try: - p = subprocess.Popen( - ["python3", "-m", "osbuild"] + cmd_args + ["-"], - encoding="utf-8", - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - ) - data_stdout, data_stderr = p.communicate(data_stdin) - except KeyboardInterrupt: - p.wait() - raise + for c in (checkpoints or []): + cmd_args += ["--checkpoint", c] + + # Spawn the `osbuild` executable, feed it the specified data on + # `STDIN` and wait for completion. If we are interrupted, we always + # wait for `osbuild` to shut down, so we can clean up its file-system + # trees (they would trigger `EBUSY` if we didn't wait). + try: + p = subprocess.Popen( + ["python3", "-m", "osbuild"] + cmd_args + ["-"], + encoding="utf-8", + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + data_stdout, data_stderr = p.communicate(data_stdin) + except KeyboardInterrupt: + p.wait() + raise # If execution failed, print results to `STDOUT`. if p.returncode != 0: self._print_result(p.returncode, data_stdout, data_stderr) self._unittest.assertEqual(p.returncode, 0) - def compile_file(self, file_stdin, checkpoints=None): + def compile_file(self, file_stdin, output_dir=None, checkpoints=None): """Compile an Artifact This is similar to `compile()` but takes a file-path instead of raw @@ -330,7 +329,7 @@ class OSBuild(contextlib.AbstractContextManager): with open(file_stdin, "r") as f: data_stdin = f.read() - return self.compile(data_stdin, checkpoints=checkpoints) + return self.compile(data_stdin, output_dir, checkpoints=checkpoints) @staticmethod def treeid_from_manifest(manifest_data): @@ -363,19 +362,3 @@ class OSBuild(contextlib.AbstractContextManager): # as a context-manager so the caller does not retain the path for # later access. yield path - - @contextlib.contextmanager - def map_output(self, filename): - """Temporarily Map an Output Object - - This takes a filename (or relative path) and looks it up in the output - directory. It then provides the absolute path to that file back to the - caller. - """ - - path = os.path.join(self._outputdir, filename) - assert os.access(path, os.R_OK) - - # Similar to `map_object()` we provide the path through a - # context-manager so the caller does not retain the path. - yield path