Repository navigation
Conversation
Signed-off-by: Eddie Torres <torredil@amazon.com>
|
How do you plan to implement this in external-attacher? Do we add a new field to |
| // The CO has taken the node out of service after determining that | ||
| // the node is shut down or otherwise unable to access the volume. | ||
| // The CO MUST NOT use this value solely because the node is | ||
| // unreachable or a timeout has expired. | ||
| NODE_OUT_OF_SERVICE = 2; |
There was a problem hiding this comment.
I think the crucial information here is that the CO is offering a guarantee to the SP that the node is dead, which the SP can rely on. Of course that promise can be violated, but if so it's the CO's fault. I propose this language:
// The CO is asserting that the node is either permanently
// removed from the cluster, powered down, or rebooting.
// This reason MUST NOT be used if it is possible that the node
// may retain any open connection to the volume such that
// stale cached reads or stale cached writes to the volume
// could occur from the node.
| // The CO stopped waiting for `NodeUnstageVolume` and | ||
| // `NodeUnpublishVolume` to succeed, for example because a timeout | ||
| // expired or the node is unreachable, and has not taken the node | ||
| // out of service. The node might still be able to access the | ||
| // volume. |
There was a problem hiding this comment.
I want to add language here discouraging COs from using this reason, since up to know we've said this behavior is spec violating. I propose this language:
// The CO stopped waiting for NodeUnstageVolume and
// NodeUnpublishVolume to succeed, for example because the node
// is unreachable and a timeout expired. The CO makes no guarantees
// about whether the node can still access the volume and the node
// MAY retain cached state which could result in stale reads or stale
// writes if the SP does not take precautions to prevent improper or
// corrupted I/O.
// This reason is inherently dangerous and COs SHOULD NOT use it.
// It is included here to allow SPs to know that the CO is doing something
// dangerous in the case when the CO can't avoid it.
| } | ||
| message ControllerUnpublishVolumeRequest { | ||
| enum Reason { | ||
| UNKNOWN = 0; |
There was a problem hiding this comment.
We need a 4th reason -- one that indicates that the CO thinks all NodeUnstage requests have succeeded. Remember that the reason field will always be present, even on the happy path.
| // Indicates why the CO is calling `ControllerUnpublishVolume` | ||
| // before all `NodeUnstageVolume` and `NodeUnpublishVolume` calls | ||
| // on the volume have succeeded. The SP MAY use this information | ||
| // when deciding how to unpublish the volume. | ||
| // This field is OPTIONAL. The CO MUST NOT set this field if | ||
| // `node_id` is unset. |
There was a problem hiding this comment.
This should be reworded to encompass the happy path case. The CO will still populate this field with a value even when NodeUnstageVolume and NodeUnpublishVolume have succeeded.
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR adds an optional reason field to
ControllerUnpublishVolumeRequestthat lets COs give SPs more context about out of sequenceControllerUnpublishVolumecalls.Which issue(s) this PR fixes:
Fixes #610
Does this PR introduce an API-breaking change?: