Fix zfs with encryption mount/unmount #1140
Conversation
- Fix typo against full dataset path argument - Make unmount with encryption keys idempotent
WalkthroughAdds idempotence checks to module-mode tests (double unmount then remount). Introduces internal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TestRunner as Module Tests
participant Disko as Disko Scripts
participant ZFS as ZFS CLI
User->>TestRunner: run module-mode tests
TestRunner->>Disko: mount
Disko->>ZFS: (create/mount as configured)
TestRunner->>Disko: unmount
Disko->>ZFS: unmount + conditional unload-key(_name)
TestRunner->>Disko: unmount (again)
Disko->>ZFS: no-op unmount if already unmounted
TestRunner->>Disko: mount (remount)
Disko->>ZFS: keystatus(_name)? load-key if needed → mount
TestRunner->>Disko: destroy/recreate...
Note over Disko,ZFS: Direct-mode flow unchanged
sequenceDiagram
autonumber
participant Script as Un/mount Script
participant ZFS as ZFS CLI
rect rgb(230,240,255)
note right of Script: Mount path
Script->>ZFS: keystatus _name
alt keystatus != "available"
Script->>ZFS: load-key _name
end
Script->>ZFS: mount dataset(s)
end
rect rgb(255,240,230)
note right of Script: Unmount path
Script->>ZFS: unmount dataset(s)
Script->>ZFS: keystatus _name
alt keystatus == "available"
Script->>ZFS: unload-key _name
else
Note over Script,ZFS: Skip unload-key
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You can cherry-pick my changes to the ZFS types, otherwise decisions have to be made about how to handle the failing tests. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/types/zfs_volume.nix (1)
53-53: Suggest using config._name for consistency.The device path construction manually concatenates
${config._parent.name}/${config.name}, which duplicates the logic inconfig._name. For consistency with the rest of the file and easier maintenance, consider usingconfig._name.Apply this diff:
- device = "/dev/zvol/${config._parent.name}/${config.name}"; + device = "/dev/zvol/${config._name}";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/tests.nix(1 hunks)lib/types/zfs_fs.nix(1 hunks)lib/types/zfs_volume.nix(4 hunks)
🔇 Additional comments (9)
lib/tests.nix (1)
348-350: LGTM! Test coverage for unmount idempotence is valuable.The addition of double unmount followed by remount properly validates that the unmount script is idempotent and that the system can be remounted after unmounting. This will help catch regressions in the ZFS key management fixes.
lib/types/zfs_fs.nix (4)
18-23: LGTM! Fully qualified dataset name fixes the core issue.The
_nameoption correctly constructs the full dataset path by combining parent pool name and dataset name. This resolves issue #1139 where only the leaf name was used, causing "dataset does not exist" errors.
122-124: LGTM! Load-key now uses fully qualified dataset name.The keystatus check and load-key operation correctly use
config._nameto reference the full dataset path, fixing mount failures with encrypted datasets.
146-150: LGTM! Conditional unload-key prevents spurious errors.The unmount logic now:
- Checks if the key is "available" before attempting to unload
- Uses the fully qualified dataset name (
config._name)This prevents "dataset does not exist" errors and avoids attempting to unload keys that aren't loaded.
94-114: No action needed for_createparent initialization.config._parentdefaults to theparentargument (passed into the type) and_nameis declared before_create, so${config._name}(and thus${config._parent.name}) is always available at evaluation time.lib/types/zfs_volume.nix (4)
16-21: LGTM! Fully qualified dataset name for ZFS volumes.The
_nameoption mirrors the implementation inzfs_fs.nix, providing consistent fully qualified dataset names across both ZFS filesystem and volume types.
70-78: LGTM! ZFS volume creation uses fully qualified name.The create logic correctly uses
config._namefor:
- Type existence checks
- Volume creation target
- Device path in partprobe
This ensures zvol operations reference the correct dataset path.
87-89: LGTM! Mount key loading uses fully qualified name.The keystatus check and load-key operation correctly use
config._name, matching the fix pattern inzfs_fs.nix.
103-107: LGTM! Conditional key unload prevents errors.The unmount logic mirrors the fix in
zfs_fs.nix:
- Only unloads when keystatus is "available"
- Uses fully qualified dataset name (
config._name)This prevents failures when keys aren't loaded or datasets don't exist.
|
I suspect all the tests are failing because all the other types don't have idempotent unmounts |
- Fix consistent use of fully qualified dataset path - Add idempotent unmounting with encryption keys
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/types/zfs_volume.nix (1)
16-21: LGTM! Nitpick: Typo in description.The
_nameoption correctly constructs the fully qualified dataset path, which fixes the core issue in #1139.Minor typo: "Fully quantified" should be "Fully qualified":
- description = "Fully quantified name for dataset"; + description = "Fully qualified name for dataset";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/types/zfs_volume.nix(4 hunks)
🔇 Additional comments (3)
lib/types/zfs_volume.nix (3)
70-71: LGTM!The _create section correctly uses
config._namefor both existence checks and dataset creation, ensuring the full dataset path is used consistently.
87-88: LGTM!The mount logic correctly uses
config._namefor both keystatus checks and load-key operations, ensuring encrypted datasets are properly handled with the full dataset path.
103-107: LGTM! Core fix for issue #1139.The unmount logic now correctly uses
config._namefor both keystatus checks and unload-key operations. This fixes the "cannot open 'root': dataset does not exist" error by passing the fully qualified dataset path (e.g.,zroot/root) instead of just the leaf name (root). The conditional keystatus check also makes unmounting idempotent.
|
i removed the change to the partprobe command in zfs_volume create step. The argument to partprobe is not related to the dataset path but to the device path EDIT; in the disko abstraction layer.
Yeah, I understand i'm pushing on invariants/guarantees with this PR. But the lack of tests for unmounting practically comes down to "unmounting is not guaranteed", let alone that it happens clean/idempotent (to match naive user expectations). That's why I commented to cherry-pick or lay out decisions, because this is just a drive-by contribution and I'm not a maintainer. I can adjust the PR. |
|
If you're interested in updating the rest of the types to support idempotent unmounts, that would be great 👍 |
Fixes #1139
Currently running all tests locally, new/other issues might pop up due to change to test script.
Summary by CodeRabbit