Preflight check for blkid failing to read the disk, plus an additiona… - #481
Conversation
…l annotation which presents the on-disk format
There was a problem hiding this comment.
🟡 Changes recommended
The annotation override path in stageVolume mounts without formatting (and keeps old fsType/flags), which will fail for blank devices and breaks the intended behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enhances the CSI node staging flow by (1) adding a strict preflight that refuses to format disks when blkid cannot confidently identify the on-disk state, and (2) introducing a PVC annotation to both request and later record the volume’s on-disk filesystem format.
Changes:
- Add a preflight probe/classification step around
blkidresults to avoid formatting devices whose contents are uncertain. - Introduce a PVC annotation (
storage.simplyblock.io/on-disk-filesystem) for filesystem selection (when blank) and for recording the filesystem after staging. - Track the staged filesystem in
VolumeContext(stagedFsType) to keep restage mount behavior consistent.
File summaries
| File | Description |
|---|---|
| csi-driver/pkg/spdk/nodeserver.go | Adds disk-format preflight checks, PVC annotation read/write for on-disk filesystem, and propagates staged filesystem to restage logic. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| mntFlags := stagingMountFlags(fsType, req.GetVolumeCapability()) | ||
| mounter := mount.SafeFormatAndMount{Interface: ns.mounter, Exec: exec.New()} | ||
| if fs == "" { | ||
| if annotated := ns.annotatedFilesystem(ctx, req.GetVolumeId(), volumeContext); annotated != "" { | ||
| if annotated != fsType { | ||
| klog.Warningf("wrong filesystem: mount %s to %s, requested fstype %s but formatted with %s", devicePath, stagingPath, fsType, annotated) | ||
| } | ||
| volumeContext[stagedFsTypeKey] = annotated | ||
| return mounter.Mount( | ||
| devicePath, | ||
| stagingPath, | ||
| fsType, | ||
| mntFlags, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| // Record what was actually staged: a later restage remounts an existing | ||
| // filesystem, and the volume capability alone no longer answers which one it | ||
| // is once the annotation has overridden it. | ||
| volumeContext[stagedFsTypeKey] = fsType | ||
|
|
When blkid reports nothing and the claim records a filesystem, staging mounts the device rather than formatting it, which is what keeps a volume whose device could not be read from being reformatted. It was mounting with the filesystem and the mount flags the volume asked for, not the ones the claim recorded. Those two disagree exactly when the branch matters. A volume formatted before its StorageClass changed is the case the annotation exists to describe, and mounting an ext4 filesystem as xfs fails. The flags follow the same filesystem for the same reason: stagingMountFlags adds nouuid for xfs, and deriving the flags from the request drops it whenever the recorded filesystem is the xfs one. Also fixes the two findings that failed the lint job: the warning line exceeded the column limit, and getDiskFormat's if/else chain over cs[0] is a tagged switch. Verified against a real xfs filesystem under a dm-flakey error_reads table, where blkid exits 2 having read nothing: with the claim recording xfs and the volume asking for ext4, staging now mounts as xfs with nouuid rather than as ext4 without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed a125246 — the RBAC grant this PR needs to work on a real cluster.
Both charts that grant the node plugin claim access now grant
Worth noting how this was missed: the test cluster's live role reads One piece still outstanding for an upgrade: existing volumes have no annotation until their next successful stage, so there is a window after upgrading where a degraded path still formats. |
recordOnDiskFilesystem patches the claim after staging, and annotatedFilesystem reads that patch back on a later stage to decide what a device blkid could not read holds. The node plugin's ClusterRole grants get, list, and watch on persistentvolumeclaims and nothing more, so the patch is refused. The refusal is swallowed by design, since a volume has to mount when the API is unreachable, which means the annotation silently never appears and the volume it was meant to protect stays exposed with nothing in the logs but a warning. Both charts that grant the node plugin any claim access now grant patch as well. The raw manifests under csi-driver/deploy/kubernetes grant no claim access at all, so that deployment path cannot use the annotation either way and is left alone. patch rather than update: the write is a merge patch of one key, so a concurrent writer of any other annotation on the claim is left alone. Cluster-wide because claims live in whichever namespace a workload chose, and unnamed because their names are not known ahead of time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
95c605a to
97d6940
Compare
#481) * Preflight check for blkid failing to read the disk, plus an additional annotation which presents the on-disk format * fix(csi-driver): mount the recorded filesystem, not the requested one When blkid reports nothing and the claim records a filesystem, staging mounts the device rather than formatting it, which is what keeps a volume whose device could not be read from being reformatted. It was mounting with the filesystem and the mount flags the volume asked for, not the ones the claim recorded. Those two disagree exactly when the branch matters. A volume formatted before its StorageClass changed is the case the annotation exists to describe, and mounting an ext4 filesystem as xfs fails. The flags follow the same filesystem for the same reason: stagingMountFlags adds nouuid for xfs, and deriving the flags from the request drops it whenever the recorded filesystem is the xfs one. Also fixes the two findings that failed the lint job: the warning line exceeded the column limit, and getDiskFormat's if/else chain over cs[0] is a tagged switch. Verified against a real xfs filesystem under a dm-flakey error_reads table, where blkid exits 2 having read nothing: with the claim recording xfs and the volume asking for ext4, staging now mounts as xfs with nouuid rather than as ext4 without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(charts): let the node plugin write the on-disk-filesystem annotation recordOnDiskFilesystem patches the claim after staging, and annotatedFilesystem reads that patch back on a later stage to decide what a device blkid could not read holds. The node plugin's ClusterRole grants get, list, and watch on persistentvolumeclaims and nothing more, so the patch is refused. The refusal is swallowed by design, since a volume has to mount when the API is unreachable, which means the annotation silently never appears and the volume it was meant to protect stays exposed with nothing in the logs but a warning. Both charts that grant the node plugin any claim access now grant patch as well. The raw manifests under csi-driver/deploy/kubernetes grant no claim access at all, so that deployment path cannot use the annotation either way and is left alone. patch rather than update: the write is a merge patch of one key, so a concurrent writer of any other annotation on the claim is left alone. Cluster-wide because claims live in whichever namespace a workload chose, and unnamed because their names are not known ahead of time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Manohar Reddy <manohar@simplyblock.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit c7e0166)
…l annotation which presents the on-disk format