Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions csi.proto
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,23 @@ message ControllerPublishVolumeResponse {
map<string, string> publish_context = 1;
}
message ControllerUnpublishVolumeRequest {
enum Reason {
UNKNOWN = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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.
Comment on lines +878 to +882

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

NODE_CLEANUP_TIMEOUT = 1;

// 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;
Comment on lines +885 to +889

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ID of the volume. This field is REQUIRED.
string volume_id = 1;

Expand All @@ -888,6 +905,14 @@ message ControllerUnpublishVolumeRequest {
// This field is OPTIONAL. Refer to the `Secrets Requirements`
// section on how to use this field.
map<string, string> secrets = 3 [(csi_secret) = true];

// 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.
Comment on lines +909 to +914

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Reason reason = 4 [(alpha_field) = true];
}

message ControllerUnpublishVolumeResponse {
Expand Down
Loading