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

Add OverlayFs Multiple lower layers support #118573

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

augu5te
Copy link

@augu5te augu5te commented Apr 5, 2021

Motivation for this change

OverlayFs Multiple lower layers is incorrectly addressed by the prefixing (add /mnt-root) step in stage-1-init.sh:

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.

OverlayFs Multiple lower layers is incorrectly addressed by the prefixing (add /mnt-root) step in stage-1-init.sh:

Example:
optionsFiltered=lowerdir=/mnt1:/mnt2,upperdir=/mnt3,workdir=/mnt4

Actual prefix command (in nixos/modules/system/boot/stage-1-init.sh:):
echo "$optionsFiltered" | sed -E 's#\<(lowerdir|upperdir|workdir)=#\1=/mnt-root#g'

Obtained WRONG prefix:
lowerdir=/mnt-root/mnt1:/mnt2:/mnt3,upperdir=/mnt-root/mnt3,workdir=/mnt-root/mnt4

Proposed prefix command:
echo "$optionsFiltered" | sed -E 's#\<(lowerdir|upperdir|workdir)=#\1=/mnt-root#g; s#:#:/mnt-root#g'

Correct and EXPECTED prefix
lowerdir=/mnt-root/mnt1:/mnt-root/mnt2:/mnt-root/mnt3,upperdir=/mnt-root/mnt3,workdir=/mnt-root/mnt4

Added if [ "$fsType" = overlay ] to guard this prefixing step

@FRidh I'll apprecitate your opinion/review

@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 Apr 5, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 5, 2021
@FRidh FRidh requested review from cole-h, samueldr and flokli April 5, 2021 10:56
@augu5te augu5te mentioned this pull request May 7, 2021
@stale
Copy link

stale bot commented Oct 12, 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 Oct 12, 2021
@augu5te
Copy link
Author

augu5te commented Oct 13, 2021

This commit is really simple to review

@SuperSandro2000
Copy link
Member

@cole-h @samueldr @flokli please take a look at this. I don't know enough about the boot stage to review this.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 15, 2021
@cole-h
Copy link
Member

cole-h commented Nov 15, 2021

I have no knowledge of overlayfs.

While it may be simple to review the contents of the change, it's not as simple to verify functionality. Do we have an overlayfs test in NixOS? If not, maybe it would be a good idea to write one? (Otherwise, a simple but explicit "I ran this and it works fine" would satisfy me, at least.)

@samueldr
Copy link
Member

I'm missing a test, an example configuration or scenario to test this.

@augu5te
Copy link
Author

augu5te commented Feb 20, 2022

One scenario is a cluster where the nodes are booting from the network with the base system being in ramdisk and an nfs shared nix store is added (this additional layer revealed this issue).

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2022
@RaitoBezarius
Copy link
Member

Can a NixOS test be added to this so we can merge it?

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 1, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 1, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 1, 2025
@SigmaSquadron SigmaSquadron removed the awaiting_changes (old Marvin label, do not use) label Jan 5, 2025
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 5, 2025
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: 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
Development

Successfully merging this pull request may close these issues.

8 participants