Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nixosTests.virtualbox: Port to python #94858

Merged
merged 2 commits into from Aug 26, 2020
Merged

nixosTests.virtualbox: Port to python #94858

merged 2 commits into from Aug 26, 2020

Conversation

liff
Copy link
Contributor

@liff liff commented Aug 7, 2020

Motivation for this change

Making progress on #72828.

I can only get these to run with use64bitGuest=true. The 32-bit VMs just boot very slowly and then stall completely. Not sure if this makes these tests broken?

Includes a fix for network interface names in net-hostonlyif test in the first commit.

cc @blitz since you said you were also working on this.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@liff liff requested a review from tfc August 7, 2020 10:38
@tfc tfc mentioned this pull request Aug 14, 2020
Closed
@flokli
Copy link
Contributor

flokli commented Aug 15, 2020

@liff thanks for the PR!

@blitz, @tfc, could you take a look at this?

aszlig
aszlig reviewed Aug 16, 2020
View reviewed changes
nixos/tests/virtualbox.nix
@@ -205,96 +205,105 @@ let
};

testSubs = ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty ugly and the reason why I used the name attribute back then was to work around lack of serialisation. With Python however, json is part of the kitchen sink, so we can simply toJSON the attributes above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt that it would be more appropriate to keep this a simple port/translation rather than start refactoring. I can change this to use a dict, though, if you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also make this as much of a 1:1 translation as possible for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally also in favor of starting with a 1:1 translation generally. but @aszlig's suggestion is too good to not do it right now.

aszlig
aszlig reviewed Aug 16, 2020
View reviewed changes
nixos/tests/virtualbox.nix Outdated
@@ -314,10 +323,10 @@ let
${pkgs.dhcp}/bin/dhclient \
-lf /run/dhcp.leases \
-pf /run/dhclient.pid \
-v eth0 eth1
-v enp0s3 enp0s8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this should be inside the guest without systemd, so there shouldn't be predictable interface names. Did you actually test this?

Copy link
Contributor Author

@liff liff Aug 17, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stage 1 runs udev, which renames the interfaces, so it seems something has changed along the way.

However, I realized this can be changed to just networking.usePredictableInterfaceNames = false in testVMConfig above. I’ll go ahead and do that.

aszlig
aszlig reviewed Aug 16, 2020
View reviewed changes
nixos/tests/virtualbox.nix Outdated

otherIP="$(${pkgs.netcat}/bin/nc -l 1234 || :)"
${pkgs.iputils}/bin/ping -I eth1 -c1 "$otherIP"
${pkgs.iputils}/bin/ping -I enp0s8 -c1 "$otherIP"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@blitz
Copy link
Contributor

blitz commented Aug 16, 2020

@liff Awesome that you took this on! I wouldn't worry too much about 32-bit tests. If 64-bit guests work with nested virtualization enabled on the host, I'm good. (This should be the default since Linux 5.1)

@flokli I'm without a laptop for a week. Maybe @tfc can find someone to review in the meantime.

@flokli
Copy link
Contributor

flokli commented Aug 23, 2020

@tfc can you take a look at this?

@blitz
Copy link
Contributor

blitz commented Aug 23, 2020

This worked for me on the second try. On the first one of the tests hung on shutting down the VM. (This also happened to me for the Perl tests IIRC).

% nix-build -A nixosTests.virtualbox
/nix/store/z1gb3scy4hh1fbpkk57v0qhqkabk8nnm-vm-test-run-virtualbox-headless
/nix/store/xmrjijnz7f0j4d8511w0m384gkhakp5j-vm-test-run-virtualbox-host-usb-permissions
/nix/store/1mppl36a05i5w9yz1b2jypj9ksm3ir4i-vm-test-run-virtualbox-net-hostonlyif
/nix/store/qvsj04n44i8cjj78wihavgw8vin4ikcv-vm-test-run-virtualbox-simple-cli
/nix/store/0dwrmq6zjkrgmj5wvzswm7lvv5jy2wia-vm-test-run-virtualbox-simple-gui
/nix/store/ahqy6n0lhbjhaf28hxx8x76s1bqbdbgf-vm-test-run-virtualbox-systemd-detect-virt

@blitz
Copy link
Contributor

blitz commented Aug 23, 2020

This also worked for me on the second try:

% nix-build nixos/tests/virtualbox.nix --arg useKvmNestedVirt true --arg use64bitGuest true
/nix/store/fr2hmzn4dccyfbmk1nq7anxymikpb109-vm-test-run-virtualbox-headless
/nix/store/cswc29ihiv1q2ivn1a0zz0slc1pxffhb-vm-test-run-virtualbox-host-usb-permissions
/nix/store/998fp42hqbglb6lqdhj9dh8aq9qpnsp6-vm-test-run-virtualbox-net-hostonlyif
/nix/store/87v9ib4lxbq94xx7ap4c80hq15ipxhca-vm-test-run-virtualbox-simple-cli
/nix/store/qq1mp1m1h9hjdj7dzc01xkqpliqbgwwp-vm-test-run-virtualbox-simple-gui
/nix/store/32r5j94y6jdi0vcj286jcnpxfb7ydvca-vm-test-run-virtualbox-systemd-detect-virt

blitz
blitz approved these changes Aug 23, 2020
View reviewed changes
Copy link
Contributor

@blitz blitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an excellent first step towards rescuing this important test. We can improve on it later on to make it more reliable.

I'm personally in favor of using the 64-bit / nested KVM options by default, but that can be a different PR.

@flokli
Copy link
Contributor

flokli commented Aug 25, 2020 •
edited

For some reason this both stalls on my Intel Laptop, as well as AMD workstation, even after multiple tries.

On the workstation, I also saw some resets of the (virtual) ATA port in the guests' dmesg:

machine # [    8.192255] su[1038]: Successful su for alice by root
machine # [    8.194012] su[1038]: pam_unix(su:session): session opened for user alice by (uid=0)
machine # [    8.302174] SUPR0GipMap: fGetGipCpu=0x1b
machine # [    9.650657] vboxdrv: 0000000000000000 VMMR0.r0
machine # [    9.737208] vboxdrv: 0000000000000000 VBoxDDR0.r0
machine # [    9.710336] systemd[1]: Created slice system-vboxtestlog\x2dsimple.slice.
machine # [    9.712283] systemd[1]: Condition check resulted in FUSE Control File System being skipped.
machine # [    9.715217] systemd[1]: Condition check resulted in Kernel Configuration File System being skipped.
machine # [    9.717112] systemd[1]: Condition check resulted in File System Check on Root Device being skipped.
machine # [    9.719067] systemd[1]: Started VirtualBox Test Machine Log For simple (PID 1045/UID 1000).
machine # [    9.782617] VMMR0InitVM: eflags=246 fKernelFeatures=0x0 (SUPKERNELFEATURES_SMAP=0)
machine # [    9.752719] su[1038]: pam_unix(su:session): session closed for user alice
(1.57 seconds)
machine # [   48.240716] ata2: lost interrupt (Status 0x58)
machine # [   53.858766] ata2.00: qc timeout (cmd 0xa0)
machine # [   53.859113] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
machine # [   59.490792] ata2.00: qc timeout (cmd 0xa0)
machine # [   59.491142] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
machine # [   59.491609] ata2.00: limiting speed to MWDMA2:PIO3
machine # [   65.122815] ata2.00: qc timeout (cmd 0xa0)
machine # [   65.123185] ata2.00: TEST_UNIT_READY failed (err_mask=0x5)
machine # [   65.123643] ata2.00: disabled

I also tried toggling useKvmNestedVirt and use64bitGuest, but it didn't change much.

@flokli
Copy link
Contributor

flokli commented Aug 26, 2020

Apparently I had more luck with --arg useKvmNestedVirt true --arg use64bitGuest true on the intel box, and got tests to succeed there. 🎉

Let's merge this, we can improve these things later.

@flokli flokli merged commit df2f22d into NixOS:master Aug 26, 2020
@flokli
Copy link
Contributor

flokli commented Aug 26, 2020

Thanks a lot for all the work!

@alyssais alyssais mentioned this pull request Jun 26, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig left review comments

@blitz blitz blitz approved these changes

@tfc tfc Awaiting requested review from tfc

Assignees
No one assigned
Labels
6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@liff @flokli @blitz @tfc @aszlig