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.

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/containers: allow containers with long names to create private networks #100433

Merged
merged 2 commits into from Feb 26, 2021

Conversation

@Patryk27
Copy link
Member

@Patryk27 Patryk27 commented Oct 13, 2020

Motivation for this change

Launching a container with a private network requires creating a
dedicated networking interface for it; name of that interface is derived
from the container name itself - e.g. a container named foo gets
attached to an interface named ve-foo.

An interface name can span up to IFNAMSIZ characters, which means that a
container name must contain at most IFNAMSIZ - 3 - 1 = 11 characters;
it's a limit that we validate using a build-time assertion.

This limit has been upgraded with Linux 5.8, as it allows for an
interface to contain a so-called altname, which can be much longer,
while remaining treated as a first-class citizen.

Since altnames have been supported natively by systemd for a while now,
due diligence on our side ends with dropping the name-assertion on newer
kernels.

This merge request closes #38509.

systemd/systemd#14467
systemd/systemd#17220
https://lwn.net/Articles/794289/

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.
@Patryk27
Copy link
Member Author

@Patryk27 Patryk27 commented Oct 13, 2020

TBH, I'm not sure why the pipeline failed :/

@flokli
Copy link
Contributor

@flokli flokli commented Oct 14, 2020

As this only checks for the booted kernel (which can differ from the one currently running) how does this behave when switching to a new system with 5.8+ configured, but having booted an older kernel? Is there an error message printed by nspawn?

Or are the links just named differently?

@Patryk27
Copy link
Member Author

@Patryk27 Patryk27 commented Oct 15, 2020

Nice catch - in that case switching fails with:

machine # [    4.283293] container really-long-name[741]: Failed to set alternative interface name 've-really-long-name' to 've-really-l3QgY', ignoring: Operation not supported
machine # [    8.726324] container really-long-name[2492]: Cannot find device "ve-really-long-name"

The ethernet device itself does get created - it's just that its alternative name remains unspecified, so it's only reachable via ve-really-l3QgY, instead of both ve-really-l3QgY and ve-really-long-name.

Maybe our container-startup-script (the one that crashes with Cannot find device above) could check kernel's version too and print more fine-grained error message telling user to reboot the machine?

@flokli
Copy link
Contributor

@flokli flokli commented Oct 15, 2020

Hmm - I wasn't entirely sure, just thinking out loudly.

If this is shown inside the activation script, it already gives quite a lot of information.

Maybe if we combine it with a 21.03 release note entry explaining the change, and a "make sure you're running a kernel with support for alternative interface names (5.8+)" at the end, that should be enough to connect the dots?

@Patryk27
Copy link
Member Author

@Patryk27 Patryk27 commented Oct 15, 2020

Release note seems alright for me :-)

Changing kernel versions requires users to make conscious effort (i.e. to modify some part of their configuration), so I consider nixos-rebuild boot instead of nixos-rebuild switch a natural order of things (and only doing nixos-rebuild switch with different kernel version yields that assertion useless).

Maybe we could add a warning (like please don't forget to reboot) for when someone does nixos-rebuild switch that changes kernel version?

@tomberek
Copy link
Contributor

@tomberek tomberek commented Feb 24, 2021

Ran into this issue and would be nice to have this. What remains to be done/reviewed?

@Patryk27
Copy link
Member Author

@Patryk27 Patryk27 commented Feb 24, 2021

tl;dr as far as I'm concerned, this feature is ready 🙂 (though I'm not sure who I could ping to get it merged)

As @flokli noticed, we've got a small shortcoming that when you upgrade from an older kernel to 5.8+, the kernel version assertion passes and thus Nix compiles a configuration which then fails at runtime during nixos-rebuild switch (because the system is still running on the older kernel); imho trying to somehow solve this is not worth the hassle, as upgrading the kernel is anyway a "big change" that almost always requires reboot (which will work correctly), but feel free to add your 5 cents.

@flokli
Copy link
Contributor

@flokli flokli commented Feb 25, 2021

The problem here is that even on latest master, linuxPackages.kernel points to 5.4.x, which is < 5.8+.

@Patryk27
Copy link
Member Author

@Patryk27 Patryk27 commented Feb 25, 2021

The new test already contains:

{
    boot.kernelPackages = pkgs.linuxPackages_latest;    
}

... plus it passes on my machine, so I wouldn't bet my money on an older kernel being loaded.

I've just rebased my branch and we'll see now - maybe it was just some spurious failure.

@flokli
Copy link
Contributor

@flokli flokli commented Feb 25, 2021

Aaargh, sorry, I'm dumb. It's already too long ago - I forgot long names are only enabled on new kernels.

I locally cherry-picked the commit into master and successfully ran the test.

Can you:

  • Add the test to nixos/tests/all-tests.nix, so it appears in the nixosTests attrset
  • put the comment removal in a separate commit

nixos/tests/containers-names.nix Outdated Show resolved Hide resolved
Patryk27 added 2 commits Feb 26, 2021
…networks

Launching a container with a private network requires creating a
dedicated networking interface for it; name of that interface is derived
from the container name itself - e.g. a container named `foo` gets
attached to an interface named `ve-foo`.

An interface name can span up to IFNAMSIZ characters, which means that a
container name must contain at most IFNAMSIZ - 3 - 1 = 11 characters;
it's a limit that we validate using a build-time assertion.

This limit has been upgraded with Linux 5.8, as it allows for an
interface to contain a so-called altname, which can be much longer,
while remaining treated as a first-class citizen.

Since altnames have been supported natively by systemd for a while now,
due diligence on our side ends with dropping the name-assertion on newer
kernels.

This commit closes NixOS#38509.

systemd/systemd#14467
systemd/systemd#17220
https://lwn.net/Articles/794289/
@Patryk27 Patryk27 changed the title nixos/containers: allow containers with long names to use private networks nixos/containers: allow containers with long names to create private networks Feb 26, 2021
@flokli flokli merged commit 1624ae8 into NixOS:master Feb 26, 2021
18 checks passed
@flokli
Copy link
Contributor

@flokli flokli commented Feb 26, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants