[ALPHA] Add volume health proposal#604
Conversation
gnufied
force-pushed
the
implement-volume-health
branch
3 times, most recently
from
8789044 to
456214c
Compare
|
I was thinking about this use case, but where would we report it? In `CSIDriver` object? I do not think, CSI driver object was designed for such use case, but idea is sound in theory.
I am intentionally keeping some of the health reporting as future enhancement, so as we can ship something rather than being bogged down by designing everything upfront. I am adding a section in kubernetes/enhancements#6138 to keep track of future enhancements. I will update
Thanks for the consideration. There is no need to include such a feature with this PR, but it would be nice if we can add it at a later time.
The (currently) only location where this makes sense to report, is at the StorageClass level. Depending on the parameters/secrets, a part of a storage backend could be unhealthy (like a pool of disks, HDD vs NVMe, or network connection type, or the like).
|
nixpanic
left a comment
nixpanic
left a comment
There was a problem hiding this comment.
The current proposal looks good to me, and it should be workable with the CSI-drivers for Ceph.
gnufied
changed the title
| // being accessible from that node. | ||
| INACCESSIBLE = 2; | ||
|
|
||
| // Data loss is known or strongly suspected on underlying volume. |
There was a problem hiding this comment.
Permanent data loss is known or suspected.....
This is to distinguish from situations where data was lost but self-healing will eventually repair the problem.
We may also want to add text here indicating that we recommend discarding this volume and rebuilding from other (higher level) replicas if possible, because this is supposed to be one of the actionable health states and we don't want there to be any confusion about what its intended use is.
There was a problem hiding this comment.
Permanent data loss is known or suspected.....
+1
We may also want to add text here indicating that we recommend discarding this volume and rebuilding from other (higher level) replicas if possible, because this is supposed to be one of the actionable health states and we don't want there to be any confusion about what its intended use is.
So far we have avoided being too prescriptive on remediation (notice inaccessible doesn't say do not schedule workload that uses this volume on this node). I think that returned health condition is basically a suggestion to the CO and the workload (managed via some kind of operator), at the end of day - it is impossible to enforce this one way or the other from CSI perspective, so being very prescriptive IMO does not add much value.
| // COs MAY ignore unknown VolumeHealthErrorType values, that they | ||
| // don't know about. |
There was a problem hiding this comment.
I would say they must ignore? What are you thinking of leaving the door open for?
My fear is that if we leave this door open, and some CO starts to react to health states it doesn't understand, that may prevent us from safely adding new health states where very particular handling is required when we know that olders COs will do something by default which happens to be wrong for that case.
There was a problem hiding this comment.
updated the wording.
|
|
||
| // Indicates the SP supports the ControllerListVolumeHealth RPC. | ||
| // This enables COs to fetch volume health information from | ||
| // the Controller Plugin's perspective. | ||
| // A SP that supports ControllerListVolumeHealth MUST also | ||
| // support ControllerGetVolumeHealth. | ||
| LIST_VOLUME_HEALTH = 16 [(alpha_enum_value) = true]; | ||
|
|
||
| // Indicates the SP supports the ControllerGetVolumeHealth RPC. | ||
| // This enables COs to fetch health information for a single | ||
| // volume from the Controller Plugin's perspective. | ||
| GET_VOLUME_HEALTH = 17 [(alpha_enum_value) = true]; |
There was a problem hiding this comment.
I though we agreed one capability would govern both RPCs, so it's only possible to have both or none. Please combine these.
There was a problem hiding this comment.
I think we kinda did. LIST_VOLUME_HEALTH implies GET_VOLUME_HEALTH, but GET_VOLUME_HEALTH does not imply LIST_VOLUME_HEALTH.
Do we want to force providers who only can support getting health of individual volumes to also support LIST_VOLUME_HEALTH?
There was a problem hiding this comment.
Oh that's a fair point. I see the utility in allowing just Get but not List. In that case we should make GET come before LIST so that the dependency points backwards instead of forewards.
| // The path where the volume is or was expected to be | ||
| // published on the node. If not empty, it MUST be an absolute path |
There was a problem hiding this comment.
"was expected to be" is a little too vague. AIUI, the only case when this might be called on an unpublished volume is AFTER a failed call to PublishVolume. If that's the case, we should say so, otherwise SPs will pull their hair out trying to figure out what to do with this path in the case it's called on a volume where a publish was never attempted, and thus the SP never saw the publish context/volume context.
| // The storage backend is completely unreachable from this node. | ||
| // Volumes using this backend are expected to be unavailable. | ||
| STORAGE_UNREACHABLE = 1; | ||
|
|
||
| // The storage backend is operating in a degraded | ||
| // state (e.g. reduced path count, high latency). | ||
| // Volumes using this backend may experience reduced performance. | ||
| STORAGE_DEGRADED = 2; |
There was a problem hiding this comment.
Please reorder these so that degraded comes first, assuming it's a similar wildcard to the volume degraded state.
|
|
||
| // Indicates the SP supports the NodeGetVolumeHealth RPC. | ||
| // This enables COs to fetch per-volume health information from | ||
| // the Node Plugin's perspective. | ||
| GET_VOLUME_HEALTH = 7 [(alpha_enum_value) = true]; | ||
|
|
||
| // Indicates the SP supports the NodeGetStorageHealth RPC. | ||
| // This enables COs to fetch health of entire storage subsystem | ||
| // from node's perspective. | ||
| GET_STORAGE_HEALTH = 8 [(alpha_enum_value) = true]; |
There was a problem hiding this comment.
Why split these? Having 2 capabilities means the CO will need to deal with all 4 SP cases (has neither, has both, has GET_VOLUME_HEALTH only, and has GET_STORAGE_HEALTH only) which is extra code on the CO side. Do we really expect some SPs to implement one or the other? Is it a big burden to make them implement both, or to just return always healthy for the one they don't want to implement?
There was a problem hiding this comment.
Do we really expect some SPs to implement one or the other.
Yes it could be easily that a lot of cloudproviders based SP will not have GET_STORAGE_HEALTH capability but will definitely want GET_VOLUME_HEALTH on the node. On the other hand - providers like TopoLVM(https://github.com/topolvm/topolvm) will have both capabilities.
Having 2 capabilities means the CO will need to deal with all 4 SP cases (has neither, has both, has GET_VOLUME_HEALTH only, and has GET_STORAGE_HEALTH only) which is extra code on the CO side.
So these 2 have very different lifecycle from CO perspective. Volume Health of a individual volume is reported in the pod and after scheduling the pod to the node, whereas storage susbsytem health from the node is reported in CSINode and regardless of pod scheduling concerns. IMO - it makes sense to keep them separate.
Make ControllerGetVolumeHealth required if LIST_VOLUME_HEALTH is implemented
gnufied
force-pushed
the
implement-volume-health
branch
from
85d1755 to
72e2e56
Compare
|
@bswartz addressed the review comments PTAL |
5 participants
No description provided.