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

Add support for RFC108 imperative containers #216025

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Add support for RFC108 imperative containers #216025

wants to merge 4 commits into from

Conversation

m1cr0man
Copy link
Contributor

@m1cr0man m1cr0man commented Feb 12, 2023 •
edited
Loading

Description of changes

This is a fork of #140669 in a minified and refactored form covering the changes which cannot be placed in a separate flake.

I have been working on python-nixos-nspawn which implements all the components for managing imperative RFC108 style containers. I have been able to refactor almost all parts of the original POC to work within this repo/flake however I could not find a satisfactory way to otherwise apply the changes covered in this PR.

  • Skip some more mountpoints within containers which are covered by systemd-nspawn
  • Avoid altering imperative containers and add the necessary logic to handle declarative containers in switch-to-configuration.pl

With this, it would be possible to use python-nixos-nspawn as a tool (and hopefully as a module soon) to manage RFC108 containers on any system with networkd enabled + configured appropriately.

TODO
  • Figure out how to make imperative containers start on boot + whether that requires modifying this PR or the python tool.
  • Write a test suite
  • Test these changes with the existing containers module (specifically check if those mountpoint changes would break them)
  • Add some docs and examples
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.

CC @Ma27

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 12, 2023
@m1cr0man m1cr0man mentioned this pull request Jun 29, 2023
Draft
12 tasks
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels Jul 16, 2023
@m1cr0man m1cr0man mentioned this pull request Aug 16, 2023
Open
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Aug 19, 2023
@m1cr0man m1cr0man force-pushed the rfc108-minimal branch 2 times, most recently from 103994b to f8be66a Compare November 22, 2023 22:36
@m1cr0man m1cr0man force-pushed the rfc108-minimal branch 2 times, most recently from 9ab1e9a to 4a920bd Compare May 7, 2024 21:15
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Oct 2, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024
@m1cr0man m1cr0man mentioned this pull request Dec 29, 2024
Open
@Ma27
Copy link
Member

Ma27 commented Dec 31, 2024

cc @dasJ who is probably the most qualified person for a review of the code here.

I think the first commit is a mixture of my and your code. Would be appreciated if you could add a Co-authored-by tag to the commit :)
(If I'm mistaken, ignore)

@m1cr0man
Copy link
Contributor Author

Would be appreciated if you could add a Co-authored-by tag to the commit :)

I'll do that for sure next time I push! I actually didn't know about the feature. I will also attribute a chunk of commits on python-nixos-nspawn to yourself too, as I've moved a lot of code to that repo.

@m1cr0man
nixos/systemd-nspawn: Updates to switch-to-configuration.pl
339b8b0 
This is a refactor of the necessary changes for RFC108 to
reduce the delta with current master and conform it to code
changes made to switch-to-configuration.pl since the project
started. I'll try to summarise the changes:

- Camel case to snake case
- Try to conform compare_nspawn_units to match the style and
 logic of compare_units for long term maintainability.
- Remove fingerprintNspawnUnits and use comp_array over deepCmp.
 By using parse_unit instead of parseNspawn, override confs will
 be factored in to the data loading and comparison so I don't see
 a need to do the fingerprinting.
- Remove use of smartmatch in favour of hash map membership,
 see L331/%section_cmp as an example of this being done elsewhere.
- @systemd@ -> $new_systemd

Overall, I hope this makes the RFC108 component changes to the
script easier to maintain in nixpkgs.

This also contains work from squashed commits:

- Query active nspawn instance names via DBus

The original implementation is pretty hacky and doesn't work correctly
if the name of one running instance is the prefix of the name another
instance (e.g. `foo` and `foobar`).

Using `ListMachines` from machined's DBus API instead to get some
structure data seems more reasonable here. This is also the way how
`machienctl list` retrieves running machines.
@m1cr0man
nixos/systemd-nspawn: Skip more filesystems for containers
ab1c963 
Within systemd nspawn, even less default filesystems are
required than what is currently assumed in filesystem.nix.
This PR wraps more mounts in conditional toggles to respect this.
@m1cr0man
nixos/systemd-nspawn: Write individual units to etc
9b1acc8 
This allows imperative container management to create unit
files in /etc/systemd/nspawn
@m1cr0man
nixos/logrotate: Copy default configFile to /etc
d5c8ec9 
When running logrotate in a user namespaced environment, such as
an nspawn container with PrivateUsers=pick, logrotate may refuse to
start as its config file is not owned by root.

Using environment.etc we can copy the file from the store and
set the proper permissions during activation.
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@m1cr0man @Ma27 @wegank