Skip to content

nixos/virtualisation: add option for explicitly named network interfaces#178290

Merged
RaitoBezarius merged 1 commit intoNixOS:masterfrom
andrew-hoff:ahh/qemu-interfaces
Jan 25, 2023
Merged

nixos/virtualisation: add option for explicitly named network interfaces#178290
RaitoBezarius merged 1 commit intoNixOS:masterfrom
andrew-hoff:ahh/qemu-interfaces

Conversation

@andrew-hoff
Copy link
Contributor

@andrew-hoff andrew-hoff commented Jun 20, 2022 •
edited
Loading

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.

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/)
  • 22.11 Release Notes (or backporting 22.05 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • 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 Jun 20, 2022
@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch from 881ebac to 337f2d9 Compare June 20, 2022 01:56
@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 Jun 20, 2022
@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch from 337f2d9 to be93afb Compare June 20, 2022 14:28
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 20, 2022
andrew-hoff
andrew-hoff commented Jun 20, 2022
View reviewed changes
nixos/lib/build-vms.nix Outdated
Comment on lines 87 to 88
Copy link
Contributor Author

@andrew-hoff andrew-hoff Jun 20, 2022

Choose a reason for hiding this comment

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

I chose to use udev rules rather than systemd.network.links because in the latter case, systemd ignores all but one file that applies to a given interface. I did not want to preclude users from using that module elsewhere.

@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch 2 times, most recently from ce0059f to db98006 Compare June 20, 2022 15:58
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 20, 2022
@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch from db98006 to 8580287 Compare June 20, 2022 17:30
@andrew-hoff andrew-hoff marked this pull request as ready for review June 21, 2022 23:28
@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jun 23, 2022

cc @roberth since I see you as the last committer to build-vms.nix

@roberth
Copy link
Member

roberth commented Jun 23, 2022

Oh, I'm actually replacing build-vms.nix by modules in

The changes in build-vms.nix can be applied to the testing/network.nix file instead.

I'm arranging to run aforementioned PR on Hydra. I think this PR can be the next one to run there.

roberth
roberth reviewed Jun 23, 2022
View reviewed changes
@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jun 23, 2022

Oh, I'm actually replacing build-vms.nix by modules in

* [nixosTest: modularity and matrix #176557](https://github.com/NixOS/nixpkgs/pull/176557)

The changes in build-vms.nix can be applied to the testing/network.nix file instead.

I'm arranging to run aforementioned PR on Hydra. I think this PR can be the next one to run there.

Got it. Should I wait until that gets merged and rebase? Or should I just cut a branch off of yours?

@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch from 8580287 to 7e1a7f3 Compare June 23, 2022 19:59
@roberth
Copy link
Member

roberth commented Jun 30, 2022

tl;dr: This PR is not blocked and can be reviewed by any maintainer who is familiar with NixOS networking.

Oh, I'm actually replacing build-vms.nix by modules in

* [nixosTest: modularity and matrix #176557](https://github.com/NixOS/nixpkgs/pull/176557)

The changes in build-vms.nix can be applied to the testing/network.nix file instead.
I'm arranging to run aforementioned PR on Hydra. I think this PR can be the next one to run there.

Got it. Should I wait until that gets merged and rebase? Or should I just cut a branch off of yours?

Ok, while the big refactor is in a good shape, we won't be able to get it properly reviewed and merged in the coming 3 weeks.
The listed maintainer is occupied during this time, and as for myself, I already went far over my time budget with my own contributions as well, so this PR isn't going to receive a review from the two of us until around that time at least.

That all said, this PR isn't anything that can't be ported to my branch, and if another maintainer can review and merge this in the meanwhile, that is fine by me. I don't see any issues with how the code is structured.

@roberth
Copy link
Member

roberth commented Jun 30, 2022

Perhaps @fpletz could review the changes, as it touches the networking.nix test?

We can also run all tests on hydra after the current https://hydra.nixos.org/jobset/nixos/python-test-refactoring completes.

@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jul 4, 2022 •
edited
Loading

I just opened up a new PR against your branch here. If you have a chance to review and merge it, I'd really appreciate it!

@roberth
Copy link
Member

roberth commented Nov 8, 2022

Hi, the module changes have been merged, so you can rebase the PR.

I was looking for a qemu networking expert just now. What do you think of #200225?

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Dec 22, 2022

I am interested into helping getting this merged, can you fix the conflicts @andrew-hoff and move the release notes to 23.05 ?

@andrew-hoff andrew-hoff force-pushed the ahh/qemu-interfaces branch 3 times, most recently from 311ffb4 to f162d9d Compare January 7, 2023 20:57
@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jan 7, 2023

@RaitoBezarius I updated the PR and moved release notes to 23.05. Sorry for the slow response!

@RaitoBezarius RaitoBezarius requested a review from roberth January 15, 2023 17:02
@RaitoBezarius RaitoBezarius requested a review from nikstur January 15, 2023 17:03
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jan 22, 2023

@andrew-hoff Thank you for maintaining this conflict-less, it seems you introduced a merge commit recently, can you get rid of it?
I think if we don't have a review, I would merge this and we can always revert it in case of problems.

@andrew-hoff
nixos/virtualisation: add option for explicitly named network interfaces
6ae3e76 
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.
@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jan 22, 2023

@andrew-hoff Thank you for maintaining this conflict-less, it seems you introduced a merge commit recently, can you get rid of it? I think if we don't have a review, I would merge this and we can always revert it in case of problems.

Ah, sorry about that. Just cleaned it up.

@RaitoBezarius RaitoBezarius merged commit 8803f1d into NixOS:master Jan 25, 2023
@andrew-hoff
Copy link
Contributor Author

andrew-hoff commented Jan 25, 2023

Thank you @RaitoBezarius !

@andrew-hoff andrew-hoff deleted the ahh/qemu-interfaces branch January 25, 2023 16:33
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jan 25, 2023

Thank you @RaitoBezarius !

Thank YOU for your incredible patience. :)

@vcunat
Copy link
Member

vcunat commented Jan 28, 2023

#213118
Can you perhaps look again if other things could go wrong when resurrecting the PR after months?

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jan 28, 2023

#213118 Can you perhaps look again if other things could go wrong when resurrecting the PR after months?

Apologies for that breakage, I will run more tests to check.

Artturin
Artturin reviewed Jan 28, 2023
View reviewed changes
nixos/tests/networking.nix
Comment on lines +953 to +955
} else {
name = "RenameInterface";
testScript = "";
Copy link
Member

@Artturin Artturin Jan 28, 2023

Choose a reason for hiding this comment

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

this is unfinished

> nixosTests.networking.scripted.rename
error: The option `nodes' is used but not defined.

Copy link
Contributor

@raphaelr raphaelr Jan 29, 2023

Choose a reason for hiding this comment

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

Fixed by #213371

@vcunat
Copy link
Member

vcunat commented Jan 29, 2023

Well, while evaluation got fixed, some of the tests fail or time out. (And they work if I revert this PR and the fixups.) And so the nixos-unstable channel is still blocked. https://hydra.nixos.org/build/207314144#tabs-constituents

@RaitoBezarius
Copy link
Member

RaitoBezarius commented Jan 29, 2023

Well, while evaluation got fixed, some of the tests fail or time out. (And they work if I revert this PR and the fixups.) And so the nixos-unstable channel is still blocked. https://hydra.nixos.org/build/207314144#tabs-constituents

I'm fine with reverting this PR as it requires clearly more work at the moment I guess.

vcunat added a commit to vcunat/nixpkgs that referenced this pull request Jan 30, 2023
@vcunat
Revert NixOS#178290: nixos/virtualisation: add option
a8947a3 
...for explicitly named network interfaces

This reverts commit 6ae3e76.
(and evaluation fixups 08d26bb 7aed90a)
Some of the tests fail or time out after the merge.
@vcunat
Copy link
Member

vcunat commented Jan 30, 2023

Opened as #213519
(if noone has a fix)

cole-h pushed a commit that referenced this pull request Jan 30, 2023
@vcunat @cole-h
Revert #178290: nixos/virtualisation: add option
23ce77d 
...for explicitly named network interfaces

This reverts commit 6ae3e76.
(and evaluation fixups 08d26bb 7aed90a)
Some of the tests fail or time out after the merge.
@GrahamDennis GrahamDennis mentioned this pull request May 22, 2023
Merged
12 tasks
@Janik-Haag Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
@jian-lin
Copy link
Contributor

jian-lin commented Feb 6, 2026 •
edited
Loading

Found an interesting issue, maybe people here will be interested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Artturin Artturin Artturin left review comments

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

@roberth roberth Awaiting requested review from roberth

@nikstur nikstur Awaiting requested review from nikstur

+1 more reviewer

@raphaelr raphaelr raphaelr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

QEMU VM tests refactors
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@andrew-hoff @roberth @RaitoBezarius @vcunat @jian-lin @raphaelr @Artturin @Janik-Haag