Skip to content

nixos/qemu-vm: add option for named network interfaces#233350

Merged
RaitoBezarius merged 2 commits intoNixOS:masterfrom
GrahamDennis:grahamdennis/testing-networks
May 26, 2023
Merged

nixos/qemu-vm: add option for named network interfaces#233350
RaitoBezarius merged 2 commits intoNixOS:masterfrom
GrahamDennis:grahamdennis/testing-networks

Conversation

@GrahamDennis
Copy link
Contributor

Adds a new option to the virtualisation modules that enables specifying explicitly named network interfaces in QEMU VMs. The existing virtualisation.vlans is still supported for cases where the name of the network interface is irrelevant.

This feature is useful for creating VMs with network configurations that exactly match the network configuration of real-world devices for more accurate SITL testing.

Note: This is PR #178290 which was later reverted in #213519 because it was causing some hydra tests to fail: #178290 (comment).

This PR includes a fix for the "sit" and "privacy" tests that failed after the PR was merged last time, and adds a new test to cover the situation where the MAC addresses in the QEMU VM contain a character in the range [a-f].

Description of changes
  • Added virtualisation.interfaces option to qemu-vm.nix.
  • Updated the default value of virtualisation.vlans to empty if virtualisation.interfaces is not empty. This avoids a scenario where a user specifies virtualisation.interfaces, but inadvertently still has a network interface from the default virtualisation.vlans.
  • Updated build-vms.nix to create the new network interfaces using QEMU options.
  • Updated build-vms.nix to rename network interfaces using udev rules.
  • Updated testing-python.nix to use the union of the VLANs specified in virtualisation.vlans and virtualisation.interfaces when collecting the list of VLANs.
  • Updated networking tests in nixos/tests/networking.nix to use the new option where applicable. This cut down on unnecessary overrides to remove IP addresses from network interfaces.
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels May 22, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels May 22, 2023
@RaitoBezarius
Copy link
Member

Awesome, please tag me for such PRs if you don't want specialized reviewers to miss them. :)

@RaitoBezarius
Copy link
Member

Also, please format your commits according to the CONTRIBUTING guide (and same for the PR title).

@RaitoBezarius RaitoBezarius marked this pull request as draft May 22, 2023 07:49
@RaitoBezarius
Copy link
Member

I will re-evaluate your whole PR over Hydra in https://hydra.nixos.org/jobset/nixos/python-test-refactoring
This PR has to wait until this evening as we will proceed to branch-off for NixOS 23.05 today.

@GrahamDennis GrahamDennis changed the title qemu-network-interfaces nixos/qemu-vm: add option for named network interfaces May 22, 2023
@GrahamDennis GrahamDennis force-pushed the grahamdennis/testing-networks branch from a8ec846 to 91c113d Compare May 22, 2023 10:21
@GrahamDennis
Copy link
Contributor Author

Thanks @RaitoBezarius! I have rebased and squashed my commits amending the commit title (and PR title) to match the guidelines.

I'll also need to move my change to the release notes after 23.05 branches.

@GrahamDennis
Copy link
Contributor Author

Am I right in understanding that hydra hasn't run the PR yet? It looks to have unrelated evaluation errors.

@RaitoBezarius
Copy link
Member

Am I right in understanding that hydra hasn't run the PR yet? It looks to have unrelated evaluation errors.

At the moment, I see ofborg-eval cloning the project, so not running it yet.

@RaitoBezarius RaitoBezarius requested review from nikstur, roberth and tfc May 22, 2023 10:51
@GrahamDennis GrahamDennis force-pushed the grahamdennis/testing-networks branch from ef4c4c1 to aa6fe11 Compare May 22, 2023 22:41
@GrahamDennis GrahamDennis marked this pull request as ready for review May 22, 2023 22:42
@GrahamDennis
Copy link
Contributor Author

I don't understand why the manual check is failing. Does anyone have ideas?

@RaitoBezarius
Copy link
Member

Potentially because I fucked up the initialization of the manual :-)

@RaitoBezarius
Copy link
Member

If you can solve the conflicts, it'd be awesome. :)

Graham Dennis added 2 commits May 24, 2023 08:54
Adds a new option to the virtualisation modules that enables specifying explicitly named network interfaces in QEMU VMs.
The existing `virtualisation.vlans` option is still supported for cases where the name of the network interface is irrelevant.
@GrahamDennis GrahamDennis force-pushed the grahamdennis/testing-networks branch from aa6fe11 to 8e58daa Compare May 23, 2023 22:54
@GrahamDennis
Copy link
Contributor Author

Done @RaitoBezarius!

@RaitoBezarius
Copy link
Member

Done @RaitoBezarius!

I'm evaluating a recent nixos-unstable-small and we will evaluate this PR over https://hydra.nixos.org/jobset/nixos/python-test-refactoring to smoke out any regression. :)

@RaitoBezarius
Copy link
Member

You should see one of your commit here: https://hydra.nixos.org/jobset/nixos/python-test-refactoring in ~30 mn. It will take some amount of time as the queue is currently full of other jobs and we don't have any priority.

Let's rediscuss when we will have some new (non flaky) failures.

@RaitoBezarius
Copy link
Member

Please review the 4 new failures here: https://hydra.nixos.org/eval/1795393

I think aarch64 are classical spurious failures, please double check the x86_64 one.

If we are good, I think this is good to go.

@GrahamDennis
Copy link
Contributor Author

None of those failures seem related to this change. The x86_64 change is a timeout, and it looks like control never passed from the kernel to userspace (https://hydra.nixos.org/build/221399319/log). I think we are good.
I wasn't aware that NixPkgs has flaky tests... this must be very frustrating

@RaitoBezarius RaitoBezarius merged commit 435237d into NixOS:master May 26, 2023
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

(was already reviewed in the previous PR)

@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

Development

Successfully merging this pull request may close these issues.

6 participants