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
zfs: refactor - adding new option for support of new native encryption format #34559
Conversation
other encrypted mounts, you will probably need to convert them to new | ||
format as well first. | ||
If you have encrypted your root dataset you will need to nuke it | ||
and re-create completely anew. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this should not be in the option description. The option description should specify what it does and the caveat about backwards compatibility, but then link elsewhere for migration details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to invert option instead to help users to migrate: #34559 (comment)
Thanks for your PR. I'm not so sure this is the right approach. This format change is in upstream master and will sooner or later land in the |
My thinking is that the format change requires quite some action... existing datasets can't be used anymore. E.g. someone has the nixos root installation in an encrypted dataset. At some point in the boot process it will stop booting (after entering password though). So if it gets added to unstable now and someone just upgrades they might now know where the problem is. Hence I think some time should be given for those that do not want to make the format change just now. E.g. in my situation I only have about 100 GB left on my disk but one dataset is 500GB in size... I need to send it first somewhere else, delete the existing one and reimport it... it just takes time and it could take a lot of time for bigger setups. |
@@ -75,6 +79,23 @@ in | |||
''; | |||
}; | |||
|
|||
enableCryptoStability = mkOption { | |||
type = types.bool; | |||
default = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options should be inverted here and provide an option to use the old version instead. We want new users to receive the new version by default - otherwise they have to migrate later.
Therefore, enableCryptoStability
should be named enableLegacyCryptoVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did make those changes now. I also altered description for the config options accordingly.
@@ -175,4 +176,28 @@ in { | |||
|
|||
spl = splUnstable; | |||
}; | |||
|
|||
zfsCryptoStability = common { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be zfsUnstable
. The other version should be called zfsLegacyCrypto
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have switched between unstable and CryptoStability (now LegacyCrypto)
I will write a mailing-list announcement before merging this. |
default = false; | ||
description = '' | ||
Enabling this option will allow you to continue to use the old format for | ||
encrypted datasets. With the inclusion of stabiity patches the format of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/stabiity/stability/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed :)
I just udpate the legacy versions to their latest commit before the merge of the encryption stability patch. |
extraPatches = [ | ||
# Mic92's patch updated for current zfs master | ||
(fetchpatch { | ||
url = "https://raw.githubusercontent.com/sjau/nix-expressions/master/customPatches/nixos-zfs-2018-02-02.patch"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will refactor this into my branch.
As we already removed the legacy zfs version. I will also cleanup the wiki text for it. In case someone still needs it. I leave a copy here:
|
Sounds good. |
Motivation for this change
In zfsUnstable there's native encryption available for zfs. However this has some issues and there was a stability patch applied to it on feb 2. That stability patch however forces a new dataset format. Meaning old datasets can only be mounted in read-only mode.
Because of that I did add a new option for the configuration.nix:
boot.zfs.enableCryptoStability = true;
(probably should use a better name but couldn't think of it - feel free to make sugestions). This will use latest zfs/spl master with the included stability patches.Also, I had to update Mic92's patch for NixOS.
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)