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

nixos-container: Use new configuration & state directories #87268

Merged
merged 6 commits into from Apr 27, 2022
Merged

nixos-container: Use new configuration & state directories #87268

merged 6 commits into from Apr 27, 2022

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented May 8, 2020 •
edited

Motivation for this change

We need to move NixOS containers somewhere else so these don't clash with Podman, Skopeo & other container software in the libpod & cri-o/cri-u/libcontainer ecosystems.

The state directory move is not strictly a requirement but is good for consistency.

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.

Closes #77925

@flokli
Copy link
Contributor

flokli commented May 8, 2020

This is only really gonna fix podman etc. for all new NixOS installations.

Existing installations will still stay on /var/lib/containers, so podman will still not work properly.

I assume we don't want to only be able to provide podman for all new installations… Instead of making this conditional on stateVersion, could we ship some "migrate from /var/lib/containers -> /var/lib/nixos-containers code for 1-2 releases (if we can clearly detect nixos-container-generated files?)

@adisbladis
Copy link
Member Author

could we ship some "migrate from /var/lib/containers -> /var/lib/nixos-containers code for 1-2 releases

I thought about it but opted not to because:

  1. Automatic migrations of user data feels scary
  2. They would break rollbacks to previous releases unless we also backported a back-migration

I could add migrations if we are in agreement that it's the correct solution.

@flokli
Copy link
Contributor

flokli commented May 11, 2020

At least the /etc/[nixos-]containers migration should be possible to do independent of stateVersion, and fix #77925, should it?

I agree migrating container root directories in /var/lib/containers is scary, and indeed shouldn't be done automatically (as it contains all data inside these containers too).

But I'm also not a fan of keeping users upgrading from an older NixOS with state in /var/lib/containers indefinitely - we might run into hard-to-debug issues with other container software if things are still left in /var/lib/containers.

I'm not sure what's the best approach there, but feel like the first part (new configuration files) should be less problematic and handled first.

@adisbladis adisbladis mentioned this pull request Aug 17, 2020
Closed
@FRidh
Copy link
Member

FRidh commented Aug 17, 2020

Do I understand correctly that we were using the generic /var/lib/containers, but now because podman et al. also use that generic name, we feel like we should change our container implementation to a different location?

@Ma27
Copy link
Member

Ma27 commented Aug 17, 2020

Why did those clash in the first place though? Shouldn't new modules use a better named state-directory from the beginning?

@adisbladis
Copy link
Member Author

Do I understand correctly that we were using the generic /var/lib/containers, but now because podman et al. also use that generic name, we feel like we should change our container implementation to a different location?

Yes. Both /var/lib/containers and /etc/containers are clashing.

Why did those clash in the first place though? Shouldn't new modules use a better named state-directory from the beginning?

These locations are often hard-coded and not provided by the NixOS module system.
This becomes extra problematic as it's a whole ecosystem of tools and not just one or two packages that we could patch.
But.. Even if we could patch those tools we would break those tools on non-NixOS so that wouldn't be a great solution even if we could.

The whole situation is very unfortunate but I don't see a way forward except moving NixOS-containers elsewhere.

@Ma27
Copy link
Member

Ma27 commented Aug 17, 2020

This becomes extra problematic as it's a whole ecosystem of tools and not just one or two packages that we could patch

I see. It's fine then :)

The whole situation is very unfortunate but I don't see a way forward except moving NixOS-containers elsewhere.

Have you checked whether one can migrate NixOS containers to the new location without too much inconveniences? If one has a system with a stateVersion <20.09 and wants to use e.g. podman, they'd have to migrate anyway, right?

@edolstra
Copy link
Member

I agree that using stateVersion is the most practical solution if we don't want to migrate containers automatically. We could issue a warning if you try to enable nixos-containers and podman and stateVersion < 20.09.

@adisbladis adisbladis force-pushed the nixos-containers-state-directories branch 2 times, most recently from 537e93d to 783900c Compare September 1, 2020 13:01
@worldofpeace
Copy link
Contributor

@adisbladis News?

@adisbladis
Copy link
Member Author

@worldofpeace No news. This is ready for review/merge.

worldofpeace
worldofpeace approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Someone more in tune with these components should review it though.

@worldofpeace worldofpeace requested review from edolstra, cole-h, Ma27 and flokli and removed request for edolstra September 9, 2020 15:17
Ma27
Ma27 reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

First of all thanks a lot for taking care of this! 🎉

I really hope I didn't bring up too much stuff that was discussed in the past. If I forget about this again, please feel free to hit me up on e.g. IRC or Matrix :)

Note to self: before merging I'd like to test a container's migration from /var/lib/containers to /var/lib/nixos-containers just to make sure there are no edge-cases to address.

nixos/doc/manual/release-notes/rl-2009.xml Outdated Show resolved Hide resolved
pkgs/tools/virtualization/nixos-container/default.nix Outdated Show resolved Hide resolved
nixos/modules/virtualisation/nixos-containers.nix Outdated Show resolved Hide resolved
@Ma27
Copy link
Member

Ma27 commented Oct 4, 2020

As stated previously, I'd like to see the state-dir being more configurable to make sure that I don't have to bump the state-version of an existing system when I want to migrate my containers to the new location - apart from the other comments I left (/cc @adisbladis).

Optionally, I'd add a warning to the documentation about the current state.

@adisbladis adisbladis force-pushed the nixos-containers-state-directories branch from 783900c to 78bbfdc Compare October 13, 2020 09:33
@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@critbase
Copy link
Contributor

Still matters to me

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@stale
Copy link

stale bot commented Apr 19, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 19, 2022
@adisbladis adisbladis force-pushed the nixos-containers-state-directories branch from 78bbfdc to 0865c7f Compare April 26, 2022 20:56
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 26, 2022
@adisbladis adisbladis force-pushed the nixos-containers-state-directories branch 2 times, most recently from 3add074 to 3dbd6e5 Compare April 26, 2022 21:41
@adisbladis
nixos-container: Make configuration and state directories configurable
7d9a979 
/etc/containers is also used by Podman, Skopeo & other popular
container tooling so we need to be able to move to another
configuration directory.

The state move is not strictly a requirement but is good for consistency.
@adisbladis
nixos-container: Use new configuration & state directories
f535d6f 
We need to move NixOS containers somewhere else so these don't clash
with Podman, Skopeo & other container software in the libpod &
cri-o/cri-u/libcontainer ecosystems.

The state directory move is not strictly a requirement but is good for
consistency.
@adisbladis
nixos-container: Add compatibility hack for pre-22.05 state/config di…
81d192c 
…rectories
@adisbladis
release-notes: Add release note about nixos containers directory moves
01e35db
@adisbladis
nixos/nixos-containers: Add warning on unsupported state version combo
3c49151
@adisbladis
nixos-container: Add tests to passthru for CI
dc26602
@adisbladis adisbladis force-pushed the nixos-containers-state-directories branch from 3dbd6e5 to dc26602 Compare April 27, 2022 06:36
@adisbladis
Copy link
Member Author

Years later and this is still relevant..

I have rebased this, moved the release notes to 22.05 & fixed up some breakage.
I think this is ready and should go in 22.05.

This was referenced Apr 27, 2022
Closed
Closed
@flokli flokli merged commit 9b71167 into NixOS:master Apr 27, 2022
@flokli
Copy link
Contributor

flokli commented Apr 27, 2022

I ran all container-related tests, and gave this another look. LGTM, merged!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-container-update-flake-fails/20755/4

@samhh samhh mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@Ma27 Ma27 Ma27 left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@cole-h cole-h Awaiting requested review from cole-h

@flokli flokli Awaiting requested review from flokli

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Installing any libcontainer related software (runc, podman, cri-o) bricks nixos-container