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

Jump to bottom

nixos/zfs: Fix boot.zfs.forceImportAll #95751

Merged
merged 3 commits into from Nov 12, 2020
Merged

nixos/zfs: Fix boot.zfs.forceImportAll #95751

merged 3 commits into from Nov 12, 2020

Conversation

srhb
Copy link
Contributor

@srhb srhb commented Aug 18, 2020 •
edited

According to the docs of boot.zfs.forceImportAll this option makes it so that non-root pools are forcibly imported. As far as I can figure out, and as the test included in this PR demonstrates, it actually does nothing 🥽

Fix it so that the zpool pool import service forcibly imports if the option is true (the default.)

cc @adisbladis maybe? 🙂

Motivation for this change
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.

@srhb
Copy link
Contributor Author

srhb commented Aug 19, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Aug 19, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@asdf8dfafjk
Copy link
Contributor

Might I suggest changing the default to false seeing as users of zfs might be used to current behavior?

@numinit
Copy link
Contributor

numinit commented Sep 4, 2020

@asdf8dfafjk Looks like it's enabled by default for backwards compatibility reasons? Shouldn't be a huge deal, I don't think.

@numinit numinit self-requested a review September 4, 2020 05:59
numinit
numinit approved these changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@numinit numinit left a comment

Choose a reason for hiding this comment

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

Looks like a pretty simple change. I think this more closely matches intended behavior, so +1 to this.

@asdf8dfafjk
Copy link
Contributor

You guys are much more experienced but the "new backward compatibility" is that by default non-root pools are not forcefully imported. I'll defer to you guys anyway.

@srhb
Copy link
Contributor Author

srhb commented Sep 4, 2020

@asdf8dfafjk I agree with you. It used to be false, and now it would be true. I will fix this once I'm back from vacation. It should continue being false.

@numinit
Copy link
Contributor

numinit commented Sep 8, 2020

I'm good with it either way.

@jonringer
Copy link
Contributor

jonringer commented Sep 14, 2020 •
edited

do you mind writing in the 2103 release notes that this is a potential breaking change for some users

https://github.com/NixOS/nixpkgs/blob/master/nixos/doc/manual/release-notes/rl-2103.xml

vikanezrimaya
vikanezrimaya approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@vikanezrimaya vikanezrimaya left a comment

Choose a reason for hiding this comment

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

LGTM.

Also off-topic, but: can I just say that I love to read our new Python-based tests? 😸

/status needs_merger

nixos/doc/manual/release-notes/rl-2103.xml Outdated
previously did nothing, but has been fixed. However its default has been
changed to <literal>false</literal> to preserve the existing default
behaviour. If you have this explicitly set to <literal>true</literal>,
please note that your non-root pools will now be force imported.
Copy link
Member

Choose a reason for hiding this comment

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

"forcibly imported" might sound a little bit better, but the documentation itself is clear enough and understandable. I had to read a little bit of ZFS documentation to understand why this might even be needed and what safeguards are bypassed by this option, but, well, we all learn something new every day, don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. I'll merge once this is updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've changed the wording. I understand the concern wrt. having to read ZFS documentation, but I'm a bit wary of including too much here, so I didn't make any other changes for now. :)

@marvin-mk2 marvin-mk2 bot requested a review from kevincox November 11, 2020 03:23
@vikanezrimaya vikanezrimaya mentioned this pull request Nov 11, 2020
Closed
@lzhang10 lzhang10 mentioned this pull request Nov 11, 2020
Closed
9 tasks
@kevincox kevincox merged commit 66c98ec into NixOS:master Nov 12, 2020
@deviant deviant mentioned this pull request May 6, 2021
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@numinit numinit numinit approved these changes

@vikanezrimaya vikanezrimaya vikanezrimaya approved these changes

@adisbladis adisbladis Awaiting requested review from adisbladis

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@mmahut mmahut Awaiting requested review from mmahut

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@kevincox kevincox Awaiting requested review from kevincox

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 awaiting_reviewer marvin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@srhb @asdf8dfafjk @numinit @jonringer @kevincox @vikanezrimaya @ryantm