From f9b5f9dba76f15ffc92f8e320df5a520244d263c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marijan=20Petri=C4=8Devi=C4=87?= Date: Fri, 7 Jan 2022 15:45:30 +0100 Subject: [PATCH 1/5] nixos/test-driver: use an argument instead of the out env-var --- nixos/lib/test-driver/test_driver/__init__.py | 27 +++++++++++++++++-- nixos/lib/test-driver/test_driver/driver.py | 7 +++++ nixos/lib/test-driver/test_driver/machine.py | 9 ++++--- nixos/lib/testing-python.nix | 2 +- 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/__init__.py b/nixos/lib/test-driver/test_driver/__init__.py index 5477ab5cd038..c7f2f9f3ec05 100755 --- a/nixos/lib/test-driver/test_driver/__init__.py +++ b/nixos/lib/test-driver/test_driver/__init__.py @@ -33,6 +33,18 @@ class EnvDefault(argparse.Action): setattr(namespace, self.dest, values) +class WriteableDir(argparse.Action): # type: ignore + def __call__(self, parser, namespace, values, option_string=None): # type: ignore + if not values.is_dir(): + raise argparse.ArgumentTypeError("{0} is not a directory".format(values)) + if os.access(values, os.W_OK): + setattr(namespace, self.dest, values) + else: + raise argparse.ArgumentTypeError( + "{0} is not a writeable directory".format(values) + ) + + def main() -> None: arg_parser = argparse.ArgumentParser(prog="nixos-test-driver") arg_parser.add_argument( @@ -63,6 +75,13 @@ def main() -> None: nargs="*", help="vlans to span by the driver", ) + arg_parser.add_argument( + "output_directory", + action=WriteableDir, + help="""The path to the directory where outputs copied from the VM will be placed. + By e.g. Machine.copy_from_vm or Machine.screenshot""", + type=Path, + ) arg_parser.add_argument( "testscript", action=EnvDefault, @@ -77,7 +96,11 @@ def main() -> None: rootlog.info("Machine state will be reset. To keep it, pass --keep-vm-state") with Driver( - args.start_scripts, args.vlans, args.testscript.read_text(), args.keep_vm_state + args.start_scripts, + args.vlans, + args.testscript.read_text(), + args.output_directory, + args.keep_vm_state, ) as driver: if args.interactive: ptpython.repl.embed(driver.test_symbols(), {}) @@ -94,7 +117,7 @@ def generate_driver_symbols() -> None: in user's test scripts. That list is then used by pyflakes to lint those scripts. """ - d = Driver([], [], "") + d = Driver([], [], "", Path()) test_symbols = d.test_symbols() with open("driver-symbols", "w") as fp: fp.write(",".join(test_symbols.keys())) diff --git a/nixos/lib/test-driver/test_driver/driver.py b/nixos/lib/test-driver/test_driver/driver.py index 49a42fe5fb4e..d4f799a1cbf6 100644 --- a/nixos/lib/test-driver/test_driver/driver.py +++ b/nixos/lib/test-driver/test_driver/driver.py @@ -24,9 +24,11 @@ class Driver: start_scripts: List[str], vlans: List[int], tests: str, + out_dir: Path, keep_vm_state: bool = False, ): self.tests = tests + self.out_dir = out_dir tmp_dir = Path(os.environ.get("TMPDIR", tempfile.gettempdir())) tmp_dir.mkdir(mode=0o700, exist_ok=True) @@ -46,7 +48,11 @@ class Driver: keep_vm_state=keep_vm_state, name=cmd.machine_name, tmp_dir=tmp_dir, +<<<<<<< HEAD callbacks=[self.check_polling_conditions], +======= + out_dir=self.out_dir, +>>>>>>> 68b2e235272 (nixos/test-driver: use an argument instead of the out env-var) ) for cmd in cmd(start_scripts) ] @@ -154,6 +160,7 @@ class Driver: return Machine( tmp_dir=tmp_dir, + out_dir=self.out_dir, start_command=cmd, name=name, keep_vm_state=args.get("keep_vm_state", False), diff --git a/nixos/lib/test-driver/test_driver/machine.py b/nixos/lib/test-driver/test_driver/machine.py index e050cbd7d990..a41c419ebe6a 100644 --- a/nixos/lib/test-driver/test_driver/machine.py +++ b/nixos/lib/test-driver/test_driver/machine.py @@ -297,6 +297,7 @@ class Machine: the machine lifecycle with the help of a start script / command.""" name: str + out_dir: Path tmp_dir: Path shared_dir: Path state_dir: Path @@ -325,6 +326,7 @@ class Machine: def __init__( self, + out_dir: Path, tmp_dir: Path, start_command: StartCommand, name: str = "machine", @@ -332,6 +334,7 @@ class Machine: allow_reboot: bool = False, callbacks: Optional[List[Callable]] = None, ) -> None: + self.out_dir = out_dir self.tmp_dir = tmp_dir self.keep_vm_state = keep_vm_state self.allow_reboot = allow_reboot @@ -702,10 +705,9 @@ class Machine: self.connected = True def screenshot(self, filename: str) -> None: - out_dir = os.environ.get("out", os.getcwd()) word_pattern = re.compile(r"^\w+$") if word_pattern.match(filename): - filename = os.path.join(out_dir, "{}.png".format(filename)) + filename = os.path.join(self.out_dir, "{}.png".format(filename)) tmp = "{}.ppm".format(filename) with self.nested( @@ -756,7 +758,6 @@ class Machine: all the VMs (using a temporary directory). """ # Compute the source, target, and intermediate shared file names - out_dir = Path(os.environ.get("out", os.getcwd())) vm_src = Path(source) with tempfile.TemporaryDirectory(dir=self.shared_dir) as shared_td: shared_temp = Path(shared_td) @@ -766,7 +767,7 @@ class Machine: # Copy the file to the shared directory inside VM self.succeed(make_command(["mkdir", "-p", vm_shared_temp])) self.succeed(make_command(["cp", "-r", vm_src, vm_intermediate])) - abs_target = out_dir / target_dir / vm_src.name + abs_target = self.out_dir / target_dir / vm_src.name abs_target.parent.mkdir(exist_ok=True, parents=True) # Copy the file from the shared directory outside VM if intermediate.is_dir(): diff --git a/nixos/lib/testing-python.nix b/nixos/lib/testing-python.nix index a67040468136..68c8b0eeba06 100644 --- a/nixos/lib/testing-python.nix +++ b/nixos/lib/testing-python.nix @@ -30,7 +30,7 @@ rec { # effectively mute the XMLLogger export LOGFILE=/dev/null - ${driver}/bin/nixos-test-driver + ${driver}/bin/nixos-test-driver $out ''; passthru = driver.passthru // { From 7765670c8a80695ae1b7c4e4d3ec6389ba533f1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marijan=20Petri=C4=8Devi=C4=87?= Date: Tue, 25 Jan 2022 01:15:16 +0100 Subject: [PATCH 2/5] make output_directory an optional parameter --- nixos/lib/test-driver/test_driver/__init__.py | 31 +++++++++++-------- nixos/lib/test-driver/test_driver/driver.py | 3 -- nixos/lib/testing-python.nix | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/__init__.py b/nixos/lib/test-driver/test_driver/__init__.py index c7f2f9f3ec05..3ee140894a78 100755 --- a/nixos/lib/test-driver/test_driver/__init__.py +++ b/nixos/lib/test-driver/test_driver/__init__.py @@ -33,16 +33,18 @@ class EnvDefault(argparse.Action): setattr(namespace, self.dest, values) -class WriteableDir(argparse.Action): # type: ignore - def __call__(self, parser, namespace, values, option_string=None): # type: ignore - if not values.is_dir(): - raise argparse.ArgumentTypeError("{0} is not a directory".format(values)) - if os.access(values, os.W_OK): - setattr(namespace, self.dest, values) - else: - raise argparse.ArgumentTypeError( - "{0} is not a writeable directory".format(values) - ) +def raise_if_not_writeable_dir(path: Path) -> None: + """Raises an ArgumentTypeError if the given path isn't a writeable directory + Note: We want to fail as early as possible if a directory isn't writeable, + since an executed nixos-test could fail (very late) because of the test-driver + writing in a directory without proper permissions. + """ + if not path.is_dir(): + raise argparse.ArgumentTypeError("{0} is not a directory".format(path)) + if not os.access(path, os.W_OK): + raise argparse.ArgumentTypeError( + "{0} is not a writeable directory".format(path) + ) def main() -> None: @@ -76,10 +78,11 @@ def main() -> None: help="vlans to span by the driver", ) arg_parser.add_argument( - "output_directory", - action=WriteableDir, + "-o", + "--output_directory", help="""The path to the directory where outputs copied from the VM will be placed. By e.g. Machine.copy_from_vm or Machine.screenshot""", + default=Path.cwd(), type=Path, ) arg_parser.add_argument( @@ -92,6 +95,8 @@ def main() -> None: args = arg_parser.parse_args() + raise_if_not_writeable_dir(args.output_directory) + if not args.keep_vm_state: rootlog.info("Machine state will be reset. To keep it, pass --keep-vm-state") @@ -99,7 +104,7 @@ def main() -> None: args.start_scripts, args.vlans, args.testscript.read_text(), - args.output_directory, + args.output_directory.resolve(), args.keep_vm_state, ) as driver: if args.interactive: diff --git a/nixos/lib/test-driver/test_driver/driver.py b/nixos/lib/test-driver/test_driver/driver.py index d4f799a1cbf6..656e04143c17 100644 --- a/nixos/lib/test-driver/test_driver/driver.py +++ b/nixos/lib/test-driver/test_driver/driver.py @@ -48,11 +48,8 @@ class Driver: keep_vm_state=keep_vm_state, name=cmd.machine_name, tmp_dir=tmp_dir, -<<<<<<< HEAD callbacks=[self.check_polling_conditions], -======= out_dir=self.out_dir, ->>>>>>> 68b2e235272 (nixos/test-driver: use an argument instead of the out env-var) ) for cmd in cmd(start_scripts) ] diff --git a/nixos/lib/testing-python.nix b/nixos/lib/testing-python.nix index 68c8b0eeba06..a67040468136 100644 --- a/nixos/lib/testing-python.nix +++ b/nixos/lib/testing-python.nix @@ -30,7 +30,7 @@ rec { # effectively mute the XMLLogger export LOGFILE=/dev/null - ${driver}/bin/nixos-test-driver $out + ${driver}/bin/nixos-test-driver ''; passthru = driver.passthru // { From e17fcbc9663040c932fd49664dc7b8cbd3b5894c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marijan=20Petri=C4=8Devi=C4=87?= Date: Tue, 25 Jan 2022 10:21:33 +0100 Subject: [PATCH 3/5] introduce writeable_dir argparse type --- nixos/lib/test-driver/test_driver/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/__init__.py b/nixos/lib/test-driver/test_driver/__init__.py index 3ee140894a78..498a4f56c55b 100755 --- a/nixos/lib/test-driver/test_driver/__init__.py +++ b/nixos/lib/test-driver/test_driver/__init__.py @@ -33,18 +33,20 @@ class EnvDefault(argparse.Action): setattr(namespace, self.dest, values) -def raise_if_not_writeable_dir(path: Path) -> None: - """Raises an ArgumentTypeError if the given path isn't a writeable directory +def writeable_dir(arg: str) -> Path: + """Raises an ArgumentTypeError if the given argument isn't a writeable directory Note: We want to fail as early as possible if a directory isn't writeable, since an executed nixos-test could fail (very late) because of the test-driver writing in a directory without proper permissions. """ + path = Path(arg) if not path.is_dir(): raise argparse.ArgumentTypeError("{0} is not a directory".format(path)) if not os.access(path, os.W_OK): raise argparse.ArgumentTypeError( "{0} is not a writeable directory".format(path) ) + return path def main() -> None: @@ -83,7 +85,7 @@ def main() -> None: help="""The path to the directory where outputs copied from the VM will be placed. By e.g. Machine.copy_from_vm or Machine.screenshot""", default=Path.cwd(), - type=Path, + type=writeable_dir, ) arg_parser.add_argument( "testscript", @@ -95,8 +97,6 @@ def main() -> None: args = arg_parser.parse_args() - raise_if_not_writeable_dir(args.output_directory) - if not args.keep_vm_state: rootlog.info("Machine state will be reset. To keep it, pass --keep-vm-state") From 03c90974a74fd31dc03af58e6aeedb0c9d1f3a35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marijan=20Petri=C4=8Devi=C4=87?= Date: Tue, 25 Jan 2022 11:14:57 +0100 Subject: [PATCH 4/5] add tmp_dir access check --- nixos/lib/test-driver/test_driver/driver.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/nixos/lib/test-driver/test_driver/driver.py b/nixos/lib/test-driver/test_driver/driver.py index 656e04143c17..10914a8c7aee 100644 --- a/nixos/lib/test-driver/test_driver/driver.py +++ b/nixos/lib/test-driver/test_driver/driver.py @@ -30,9 +30,22 @@ class Driver: self.tests = tests self.out_dir = out_dir - tmp_dir = Path(os.environ.get("TMPDIR", tempfile.gettempdir())) + tmp_dir = Path(tempfile.gettempdir()) tmp_dir.mkdir(mode=0o700, exist_ok=True) + if not tmp_dir.is_dir(): + raise NotADirectoryError( + "The directory defined by TMPDIR, TEMP, TMP or CWD: {0} is not a directory".format( + tmp_dir + ) + ) + if not os.access(tmp_dir, os.W_OK): + raise PermissionError( + "The directory defined by TMPDIR, TEMP, TMP or CWD: {0} is not writeable".format( + tmp_dir + ) + ) + with rootlog.nested("start all VLans"): self.vlans = [VLan(nr, tmp_dir) for nr in vlans] From 446c21fdc75c6a045bf5b715a315bdf87b5e71cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marijan=20Petri=C4=8Devi=C4=87?= Date: Tue, 25 Jan 2022 11:29:42 +0100 Subject: [PATCH 5/5] factor out tmp-dir checks --- nixos/lib/test-driver/test_driver/driver.py | 42 ++++++++++++--------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/driver.py b/nixos/lib/test-driver/test_driver/driver.py index 10914a8c7aee..880b1c5fdec0 100644 --- a/nixos/lib/test-driver/test_driver/driver.py +++ b/nixos/lib/test-driver/test_driver/driver.py @@ -10,6 +10,28 @@ from test_driver.vlan import VLan from test_driver.polling_condition import PollingCondition +def get_tmp_dir() -> Path: + """Returns a temporary directory that is defined by TMPDIR, TEMP, TMP or CWD + Raises an exception in case the retrieved temporary directory is not writeable + See https://docs.python.org/3/library/tempfile.html#tempfile.gettempdir + """ + tmp_dir = Path(tempfile.gettempdir()) + tmp_dir.mkdir(mode=0o700, exist_ok=True) + if not tmp_dir.is_dir(): + raise NotADirectoryError( + "The directory defined by TMPDIR, TEMP, TMP or CWD: {0} is not a directory".format( + tmp_dir + ) + ) + if not os.access(tmp_dir, os.W_OK): + raise PermissionError( + "The directory defined by TMPDIR, TEMP, TMP, or CWD: {0} is not writeable".format( + tmp_dir + ) + ) + return tmp_dir + + class Driver: """A handle to the driver that sets up the environment and runs the tests""" @@ -30,21 +52,7 @@ class Driver: self.tests = tests self.out_dir = out_dir - tmp_dir = Path(tempfile.gettempdir()) - tmp_dir.mkdir(mode=0o700, exist_ok=True) - - if not tmp_dir.is_dir(): - raise NotADirectoryError( - "The directory defined by TMPDIR, TEMP, TMP or CWD: {0} is not a directory".format( - tmp_dir - ) - ) - if not os.access(tmp_dir, os.W_OK): - raise PermissionError( - "The directory defined by TMPDIR, TEMP, TMP or CWD: {0} is not writeable".format( - tmp_dir - ) - ) + tmp_dir = get_tmp_dir() with rootlog.nested("start all VLans"): self.vlans = [VLan(nr, tmp_dir) for nr in vlans] @@ -157,8 +165,8 @@ class Driver: "Using legacy create_machine(), please instantiate the" "Machine class directly, instead" ) - tmp_dir = Path(os.environ.get("TMPDIR", tempfile.gettempdir())) - tmp_dir.mkdir(mode=0o700, exist_ok=True) + + tmp_dir = get_tmp_dir() if args.get("startCommand"): start_command: str = args.get("startCommand", "")