rbd: (filesystem) add async health checks to NodeGetVolumeStats - #6382
rbd: (filesystem) add async health checks to NodeGetVolumeStats#6382SanjalKatiyar wants to merge 3 commits into
Conversation
b4aa66c to
97ec187
Compare
23620ff to
df76b06
Compare
df76b06 to
cc5ca83
Compare
| } else if !isNotMnt { | ||
| log.DebugLog(ctx, "rbd: volume %s is already mounted to %s, skipping", volID, stagingTargetPath) | ||
|
|
||
| ns.startSharedHealthChecker(ctx, volID, stagingTargetPath) |
There was a problem hiding this comment.
Don't start the health-checker when the volume-mode is Block.
One way to prevent that, is by adding the volume-mode as parameter to startSharedHealthChecker() and conditionally select the StatCheckerType for filesystem mode, with no checker to start otherwise. This makes it easy to extend later on for Block mode.
There was a problem hiding this comment.
@nixpanic initially, I was doing that, but the issue is that even if we block the shared checker in the NodeStageVolume RPC, NodeGetVolumeStats will still end up starting a non-shared checker later. Right now we use a synchronous os.Stat call to determine whether the volume is block-mode or filesystem-mode, and that call is what hangs when the volume is unhealthy.
For now, though, as you suggested, I will update and at least prevent the shared checker from being started.
There was a problem hiding this comment.
But, in order to extend later, we will need a better way for NodeGetVolumeStats to determine the volume mode without relying on os.Stat.
There was a problem hiding this comment.
Yes. When we have a Block-mode health-checker we can start it in startSharedHealthChecker() too. Preparing for that, and logging a warning that Block-mode is not supported would be nice.
Filesystem-mode or Block-mode is part of the CSI Volume specification. There are statements in nodeserver.go like these already:
isBlock := req.GetVolumeCapability().GetBlock() != nil
cc5ca83 to
1b24596
Compare
1b24596 to
2356f5c
Compare
| // NOTE: rbd.getStagingPath() uses os.Stat() internally which | ||
| // if called synchronously, could block indefinitely. | ||
|
|
||
| // TODO: Add health checker for block-mode volumes. |
There was a problem hiding this comment.
I think we always run into this on block-mode volumes?
Maybe we can introduce a no-op health checker, that just always returns healthy with a message?
There was a problem hiding this comment.
@nixpanic actually this is what I was concerned about here in this comment: #6382 (comment).
We determine whether the volume is block-mode or filesystem-mode later in the flow, using os.Stat (synchronously).
At this point in the flow, we are still determining the volume's health and don't yet know its mode, so we can't rely on os.Stat for that info. We need a better way to determine the volume mode earlier in the flow, right ?? Which is why I left this as a TODO comment.
There was a problem hiding this comment.
os.Stat() will always work on block-mode volumes. The information that is returned is about the local device node (/dev/rdbX) and does not trigger any I/O towards the Ceph cluster.
For filesystem-mode volumes, os.Stat() will not be reached if the volume is unhealthy (baring races between checking in the background and NodeGetVolumeStat calls).
Starting a No-Op checker for block-mode should not make the situation any worse than that it is now? But a No-Op checker should always return true, nil so that it isn't getting started on every NodeGetVolumeStat request.
There was a problem hiding this comment.
My doubt is how would we know whether to start a no-op checker or a stat checker ??
Ex: err = ns.healthChecker.StartChecker(volumeId, targetPath, hc.StatCheckerType)
Here we need to pass an argument hc.NoOpCheckerType and in order to do that we need to know if the volume is block-mode or filesystem-mode. We can't do synchronous os.Stat yet, both filesystem and block volumes shares the same flow, so even if os.Stat works for block, it will hang for filesystem.
There was a problem hiding this comment.
right, that is indeed a good point.
This code is expected to only get executed when the node-plugin restarted while a volume was already attached (no health-checker running yet). In this case, I think it it acceptable to call os.Stat() to figure out what type of volume it is. There is an unlikely/rare case where the volume is not healthy and the node-plugin is restarted, we can live with this potential hang.
There was a problem hiding this comment.
We can do one more thing, since we can determine the mode during NodeStageVolume, we can start a shared no-op checker at that point.
For NodeGetVolumeStats, we can keep it as currently proposed in this PR, that is, always a non-shared StatChecker for now (as u pointed out that this flow will be triggered less often) to avoid performing an early os.Stat check, which could potentially hang (though, I know it might look like introducing several workarounds that make the code more complex and reduce its overall readability).
There was a problem hiding this comment.
If there is a real issue on the Ceph cluster, Ceph-CSI won't work correctly in that case anyway (not only NodeGetVolumeStat, but any CSI procedure). SlowGRPCRestart causes a complete cleanup/retry of everything, I don't think we need to care about it specifically.
There is an alternative where you can store/retrieve the mode of the volume. Extend stashRBDImageMetadata() and struct rbdImageMetadataStash. Those details are stored during NodeStageVolume on a host-path, and can be retrieved after a restart of the node-plugin. This introduces a little more work, but that way you can start a selected health-checker without calling os.Stat().
There was a problem hiding this comment.
@nixpanic thanks for the suggestion. I checked, but it won't solve our problem, issue is currently metadata is stashed at "stagingPath", but NodeGetVolumeStats won't have access to "stagingPath" as easily as expected.
I know we have getStagingPath util already, but that function internally uses os.Stat itself, so we are looping back to the same issue again which we were discussing above :)
If it's okay, I will stash the volume mode at new path indexed at "volumeID" ??
There was a problem hiding this comment.
using the volume-id makes sense to me. But what would the full path be then? The staging path is a known location that can be used, not sure what other location suits the purpose.
It all gets quite tricky... Maybe get a simple approach (with known issues) in with this PR, and improve with a next one?
There was a problem hiding this comment.
There are now 3 commits:
- no-op checker (cherry-picked from your fork).
- updating rbd server to use no-op checker for block based on os.Stat, with a comment about known bug.
- fixing the known bug via stashing the mode on the host, indexed by volumeID.
I still need to test the changes, will let you know once done.
Block-mode volumes can not be health-checked yet, the implementation is not trivial. Because a health-checker needs to be started for all volumes managed by a backend, this no-op checker can be started for RBD block-mode volumes until we have a better way. Assisted-by: AskBob <askbob@ibm.com> Signed-off-by: Niels de Vos <ndevos@ibm.com>
Use the shared health-check manager in the RBD node server so NodeGetVolumeStats reports async health state before calling os.Stat(). Avoids stat hangs and aligns RBD behaviour with CephFS. Start/Stop shared checkers during stage/unstage and incorporated fallback in NodeGetVolumeStats for node-plugin restart scenarios. Only supported for filesystem-mode volumes. Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
2356f5c to
0ba92e8
Compare
NodeStageVolume stashes each volume's block/filesystem mode to /csi/volume-info/<volumeID>.json. NodeGetVolumeStats uses this on node-plugin restart to pick the correct checker type (NoOp for block, Stat for filesystem) without a synchronous os.Stat() on the target path, which could hang. NodeUnstageVolume removes the stash on both of its success paths, including the early "already unstaged" return, to avoid leaking stale files. Assisted-by: Claude Sonnet 5 and Claude Opus 4.6 Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
Pre-requisite PR: #6418
Use the shared health-check manager in the RBD node server so
NodeGetVolumeStatsreports async health state before callingos.Stat().Avoids stat hangs and aligns RBD behaviour with CephFS.
Start/Stop shared checkers during stage/unstage and incorporated fallback in NodeGetVolumeStats for node-plugin restart scenarios.
Only supported for filesystem-mode volumes.
Testing:
Logs of
openshift-storage.rbd.csi.ceph.com-nodeplugin-<HASH>pod:Returns
"volume_condition":{"message":"block-mode health checking is not supported"}for RBD block.Returns
"volume_condition":{"message":"volume is in a healthy condition"}for RBD filesystem.