nixos/tests/installer: port to python #73237

betaboon · 2019-11-11T18:00:55Z

Motivation for this change

#72828

Prepare for changes mentioned in #58121 (comment)

Things done

Notify maintainers

cc @

nixos/lib/test-driver/test-driver.py

nixos/tests/installer.nix

samueldr

One change, two bits brought forward that were already questionable.

nixos/tests/installer.nix

samueldr · 2019-11-11T18:02:48Z

nixos/tests/installer.nix

-        (optionalString (system == "aarch64-linux") "-enable-kvm -machine virt,gic-version=host -cpu host ");
+      makeMachineConfig = name: pythonDict ({
+        inherit name;
+        # FIXME don't duplicate the -enable-kvm etc. flags here yet again!


Do you know what that comment was all about? Couldn't figure it out yesterday, though I did not dive into git blame and the history.

no i don't know for sure.
git blame just yields this: e58624a which doesn't clear it up any further.

i am assuming this refers to qemu-flags being defined here as well:
https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/qemu-flags.nix

(btw, the change can go through without this being resolved, just wanted to point it out as it's being ported as-is.)

i double checked. the options set here, are exactly the ones in qemu-flags.nix except the ram (-m) option.

The bigger picture situation is as follows:

the vm used initially is set up using qemu-vm.nix which respects the options from qemu-flags.nix and thus does not use the flags defined in installer.nix

only the machines created with create_machine in installer.nix do not use the flags defined in qemu-flags.nix as they use a completly different codepath, namely create_startcommand from test-driver.py which uses qemu-kvm only

@samueldr what do we wanna do about this?

samueldr · 2019-11-11T18:03:33Z

nixos/tests/installer.nix

+        qemuFlags =
+          (if system == "x86_64-linux" then "-m 768 " else "-m 512 ") +
+          (optionalString (system == "x86_64-linux") "-cpu kvm64 ") +
+          (optionalString (system == "aarch64-linux") "-enable-kvm -machine virt,gic-version=host -cpu host ");


Especially since we have -enable-kvm here!

tfc · 2019-12-01T09:42:43Z

nixos/tests/installer.nix

-      $machine->waitForUnit("nixos-manual");
+      machine.succeed("echo hello")
+      # machine.wait_for_unit("getty@tty2")
+      # machine.wait_for_unit("rogue")


it looks like "echo hello" is a leftover from earlier experiments. is this and the commented out lines going to vanish before this is merged?

tfc · 2019-12-01T09:44:29Z

nixos/tests/installer.nix

          "${ makeConfig { inherit bootLoader grubVersion grubDevice grubIdentifier grubUseEfi extraConfig; } }",
-          "/mnt/etc/nixos/configuration.nix");
+          "/mnt/etc/nixos/configuration.nix",


wouldn't this path be visible in the VM anyway as it's from the nix store path? wouldn't then machine.succeed("cp ${makeConfig ...} /mnt/etc/nixos/configuration.nix") work or is this suggestion stupid becasue i am missing out anything really fundamental?

i dont think it is available in the VMs nix-store.

tfc · 2019-12-01T09:45:02Z

nixos/tests/installer.nix

-      $machine->succeed("sync");
+      machine.succeed("umount /mnt/boot || true")
+      machine.succeed("umount /mnt")
+      machine.succeed("sync")


all these groups of steps would be perfect candidates for descriptive with subtest(....): sections.

yeah i thought so to. just wanted to do a straight port first without refactoring at the same time.

We can do a followup PR, this shouldn't be a reason to block this PR.

tfc · 2019-12-01T09:45:59Z

nixos/tests/installer.nix

      }

      # Check whether /root has correct permissions.
-      $machine->succeed("stat -c '%a' /root") =~ /700/ or die;
+      machine.succeed("stat -c '%a' /root | grep 700")


why not assert "700" in machine.succeed(...)

might be possible. It's just done this way everywhere else. If it would be preferable to use assert i am fine with it.

Yeah, let's use assert where possible.

tfc · 2019-12-01T09:46:24Z

nixos/tests/installer.nix

-      $machine->waitForUnit("swap.target");
-      $machine->succeed("cat /proc/swaps | grep -q /dev");
+      machine.wait_for_unit("swap.target")
+      machine.succeed("cat /proc/swaps | grep -q /dev")


many grep commands could be substituted by python asserts

tfc · 2019-12-01T09:47:45Z

nixos/tests/installer.nix

-      $machine->succeed("nixos-option boot.initrd.kernelModules | grep qemu-guest.nix");
+      machine.succeed("nixos-option boot.initrd.kernelModules | grep virtio_console")
+      machine.succeed("nixos-option boot.initrd.kernelModules | grep 'List of modules'")
+      machine.succeed("nixos-option boot.initrd.kernelModules | grep qemu-guest.nix")


why not:

modules = machine.succeed("nixos-option boot.initrd.kernelModules") for line in "virtio_console", "List of modules", "qemu-guest.nix": assert line in modules

true. looks better.

tfc · 2019-12-01T09:49:29Z

nixos/tests/installer.nix

+      machine.wait_for_unit("multi-user.target")
+      # Booted configuration name should be Work
+      machine.succeed("cat /run/booted-system/configuration-name >&2")
+      machine.succeed("cat /run/booted-system/configuration-name | grep Work")


isn't this somehow redundant to:

assert "Work" in machine.succeed("cat /run/booted-system/configuration-name >&2")

tfc · 2019-12-01T09:50:10Z

nixos/tests/installer.nix

+              "flock /dev/vda parted --script /dev/vda -- mklabel msdos"
+              " mkpart primary ext2 1M 50MB"  # /boot
+              " mkpart primary linux-swap 50M 1024M"
+              " mkpart primary 1024M -1s",  # LUKS


it's not necessary, but might be helpful while reading to prefix those multiline-lines with +

good point. might try that

flokli

Some comments, but already looking quite nice so far! 👍

flokli · 2019-12-03T23:38:23Z

nixos/lib/test-driver/test-driver.py

@@ -710,6 +711,13 @@ def unblock(self):
        """
        self.send_monitor_command("set_link virtio-net-pci.1 on")

+    def copy_file_from_host(self, from_path, to_path):


This probably needs type annotations and a rebase, as the test driver got annotations too.

flokli · 2019-12-03T23:38:38Z

nixos/tests/installer.nix

-      $machine->waitForUnit("nixos-manual");
+      machine.succeed("echo hello")
+      # machine.wait_for_unit("getty@tty2")
+      # machine.wait_for_unit("rogue")


flokli · 2019-12-03T23:39:06Z

nixos/tests/installer.nix

-      $machine->succeed("sync");
+      machine.succeed("umount /mnt/boot || true")
+      machine.succeed("umount /mnt")
+      machine.succeed("sync")


We can do a followup PR, this shouldn't be a reason to block this PR.

flokli · 2019-12-03T23:39:55Z

nixos/tests/installer.nix

      }

      # Check whether /root has correct permissions.
-      $machine->succeed("stat -c '%a' /root") =~ /700/ or die;
+      machine.succeed("stat -c '%a' /root | grep 700")


Yeah, let's use assert where possible.

betaboon · 2020-02-18T18:26:33Z

seems like this is now obsolete as #78670 has been merged.

closing.

ofborg bot added 6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 11, 2019

FRidh reviewed Nov 11, 2019

View reviewed changes

nixos/lib/test-driver/test-driver.py Outdated Show resolved Hide resolved

nixos/tests/installer.nix Outdated Show resolved Hide resolved

betaboon force-pushed the tests-installer-python-port branch from b0f33f8 to 8a48bc5 Compare November 11, 2019 18:25

samueldr requested changes Nov 11, 2019

View reviewed changes

betaboon force-pushed the tests-installer-python-port branch from 8a48bc5 to 496db01 Compare November 11, 2019 19:49

nixos/tests/installer: port to python

d07ba9b

betaboon force-pushed the tests-installer-python-port branch from 496db01 to d07ba9b Compare November 18, 2019 18:58

betaboon mentioned this pull request Nov 19, 2019

[WIP] nixos/refind: init #58121

Closed

10 tasks

tfc requested changes Dec 1, 2019

View reviewed changes

flokli reviewed Dec 3, 2019

View reviewed changes

betaboon closed this Feb 18, 2020

betaboon mentioned this pull request Feb 20, 2020

[WIP] nixos/tests/installer: support useEFIBoot #73768

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/tests/installer: port to python #73237

nixos/tests/installer: port to python #73237

betaboon commented Nov 11, 2019 •

edited

samueldr left a comment

samueldr Nov 11, 2019

betaboon Nov 11, 2019

samueldr Nov 11, 2019

betaboon Nov 19, 2019

betaboon Nov 21, 2019

samueldr Nov 11, 2019

tfc Dec 1, 2019

flokli Dec 3, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

flokli Dec 3, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

flokli Dec 3, 2019

tfc Dec 1, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

tfc Dec 1, 2019

betaboon Dec 1, 2019

flokli left a comment

flokli Dec 3, 2019

flokli Dec 3, 2019

flokli Dec 3, 2019

flokli Dec 3, 2019

betaboon commented Feb 18, 2020

nixos/tests/installer: port to python #73237

nixos/tests/installer: port to python #73237

Conversation

betaboon commented Nov 11, 2019 • edited

Motivation for this change

Things done

Notify maintainers

samueldr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betaboon commented Feb 18, 2020

betaboon commented Nov 11, 2019 •

edited