-
-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/tests/installer: port to python #73237
Conversation
b0f33f8
to
8a48bc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One change, two bits brought forward that were already questionable.
(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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(btw, the change can go through without this being resolved, just wanted to point it out as it's being ported as-is.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inqemu-flags.nix
as they use a completly different codepath, namelycreate_startcommand
from test-driver.py which usesqemu-kvm
only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samueldr what do we wanna do about this?
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 "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially since we have -enable-kvm
here!
8a48bc5
to
496db01
Compare
496db01
to
d07ba9b
Compare
$machine->waitForUnit("nixos-manual"); | ||
machine.succeed("echo hello") | ||
# machine.wait_for_unit("getty@tty2") | ||
# machine.wait_for_unit("rogue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
"${ makeConfig { inherit bootLoader grubVersion grubDevice grubIdentifier grubUseEfi extraConfig; } }", | ||
"/mnt/etc/nixos/configuration.nix"); | ||
"/mnt/etc/nixos/configuration.nix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think it is available in the VMs nix-store.
$machine->succeed("sync"); | ||
machine.succeed("umount /mnt/boot || true") | ||
machine.succeed("umount /mnt") | ||
machine.succeed("sync") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these groups of steps would be perfect candidates for descriptive with subtest(....):
sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i thought so to. just wanted to do a straight port first without refactoring at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do a followup PR, this shouldn't be a reason to block this PR.
} | ||
|
||
# Check whether /root has correct permissions. | ||
$machine->succeed("stat -c '%a' /root") =~ /700/ or die; | ||
machine.succeed("stat -c '%a' /root | grep 700") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not assert "700" in machine.succeed(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be possible. It's just done this way everywhere else. If it would be preferable to use assert
i am fine with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's use assert
where possible.
$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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
many grep commands could be substituted by python asserts
$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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. looks better.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this somehow redundant to:
assert "Work" in machine.succeed("cat /run/booted-system/configuration-name >&2")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly
"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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not necessary, but might be helpful while reading to prefix those multiline-lines with +
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. might try that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but already looking quite nice so far! 👍
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs type annotations and a rebase, as the test driver got annotations too.
$machine->waitForUnit("nixos-manual"); | ||
machine.succeed("echo hello") | ||
# machine.wait_for_unit("getty@tty2") | ||
# machine.wait_for_unit("rogue") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
$machine->succeed("sync"); | ||
machine.succeed("umount /mnt/boot || true") | ||
machine.succeed("umount /mnt") | ||
machine.succeed("sync") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do a followup PR, this shouldn't be a reason to block this PR.
} | ||
|
||
# Check whether /root has correct permissions. | ||
$machine->succeed("stat -c '%a' /root") =~ /700/ or die; | ||
machine.succeed("stat -c '%a' /root | grep 700") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's use assert
where possible.
seems like this is now obsolete as #78670 has been merged. closing. |
Motivation for this change
#72828
Prepare for changes mentioned in #58121 (comment)
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @