Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access modes for single node single/multi writer #476

Merged
merged 1 commit into from
Jun 7, 2021
Merged

Access modes for single node single/multi writer #476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 32 additions & 0 deletions csi.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,18 @@ message VolumeCapability {
// Can be published as read/write at multiple nodes
// simultaneously.
MULTI_NODE_MULTI_WRITER = 5;

// Can only be published once as read/write at a single workload
Copy link
Contributor

@humblec humblec May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@chrishenzie can we mount this same volume as READONLY in the same node ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Access modes are meant to be a floor/minimum level of access requested. If a user wants a volume to have read-only ceiling/maximum level of access, that is enforced at NodePublish time using the readonly flag, which is completely orthagonal to access modes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, let me rephrase the clarification which I am looking for, suppose a SP driver declared only SINGLE_NODE_SINGLE_WRITER, can we still publish a volume in READONLY ? because minimum level of access is "allowed writing on single node" in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the choice whether to ControllerPublish or NodePublish any given volume readonly ALWAYS lies with the CO. What the access mode means is that the SP MUST support write access if the CO chooses to request it. Access mode is a promise that the SP makes to the CO, but the CO doesn't have to take advantage of it.

// on a single node, at any given time. SHOULD be used instead of
// SINGLE_NODE_WRITER for COs using the experimental
// SINGLE_NODE_MULTI_WRITER capability.
Copy link
Contributor

@humblec humblec Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@chrishenzie this should be SINGLE_NODE_SINGLE_WRITER ? or we are kind of defaulting SINGLE_NODE_MULTI_WRITER as a replacement for SINGLE_NODE_WRITER ? if latter is the case, please ignore the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both SINGLE_NODE_SINGLE_WRITER and SINGLE_NODE_MULTI_WRITER are replacements for SINGLE_NODE_WRITER depending on the number of intended writers. The capability SINGLE_NODE_MULTI_WRITER implies support for SINGLE_NODE_MULTI_WRITER and/or SINGLE_NODE_SINGLE_WRITER. The reason for naming the capability like so was because it was the access mode that was missing from the CSI spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

The capability SINGLE_NODE_MULTI_WRITER implies support for SINGLE_NODE_MULTI_WRITER and/or SINGLE_NODE_SINGLE_WRITER -> yeah, this aligns with my thought too or what I meant with replacement as a default in above comment, so we are good to mention this over over here ! Thanks @chrishenzie. 👍

SINGLE_NODE_SINGLE_WRITER = 6 [(alpha_enum_value) = true];

// Can be published as read/write at multiple workloads on a
// single node simultaneously. SHOULD be used instead of
// SINGLE_NODE_WRITER for COs using the experimental
// SINGLE_NODE_MULTI_WRITER capability.
SINGLE_NODE_MULTI_WRITER = 7 [(alpha_enum_value) = true];
Copy link

Choose a reason for hiding this comment

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

with this change, I am trying to reason on MULTI_NODE_SINGLE_WRITER. There are two possible interoperation

  1. only a single pod from a node can access it,
  2. multiple pods on the same single node can access it

From the comments of this mode, it seems refer to 2. Is that the intention? Do we want to support both? @saad-ali

// Can be published at multiple nodes simultaneously. Only one of
// the node can be used as read/write. The rest will be readonly.

Copy link
Contributor Author

@chrishenzie chrishenzie May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think because the name mentions multiple nodes, it's saying this volume is attachable to multiple machines and readable by multiple machines, but only one workload on one of the machines has write access.

Copy link

Choose a reason for hiding this comment

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

i don't see it mentioned "one workload on one of the machines has write access" in the doc?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 99% sure that Kubernetes never uses the MULTI_NODE_SINGLE_WRITER access mode for anything, so while it suffers from the same vagueness issues as other single writer access modes, nobody actually experiences problems related to this. Ideally we could sharpen up whether the single writer for that access mode is a single workload or all the workloads on a single node, but I don't see that as a priority.

}

// This field is REQUIRED.
Expand Down Expand Up @@ -1044,6 +1056,16 @@ message ControllerServiceCapability {
// This enables COs to, for example, fetch per volume
// condition after a volume is provisioned.
GET_VOLUME = 12 [(alpha_enum_value) = true];

// Indicates the SP supports the SINGLE_NODE_SINGLE_WRITER and/or
// SINGLE_NODE_MULTI_WRITER access modes.
// These access modes are intended to replace the
// SINGLE_NODE_WRITER access mode to clarify the number of writers
// for a volume on a single node. Plugins MUST accept and allow
// use of the SINGLE_NODE_WRITER access mode when either
// SINGLE_NODE_SINGLE_WRITER and/or SINGLE_NODE_MULTI_WRITER are
// supported, in order to permit older COs to continue working.
SINGLE_NODE_MULTI_WRITER = 13 [(alpha_enum_value) = true];
}

Type type = 1;
Expand Down Expand Up @@ -1476,6 +1498,16 @@ message NodeServiceCapability {
// Note that, for alpha, `VolumeCondition` is intended to be
// informative for humans only, not for automation.
VOLUME_CONDITION = 4 [(alpha_enum_value) = true];
// Indicates the SP supports the SINGLE_NODE_SINGLE_WRITER and/or
// SINGLE_NODE_MULTI_WRITER access modes.
// These access modes are intended to replace the
// SINGLE_NODE_WRITER access mode to clarify the number of writers
// for a volume on a single node. Plugins MUST accept and allow
// use of the SINGLE_NODE_WRITER access mode (subject to the
// processing rules for NodePublishVolume), when either
// SINGLE_NODE_SINGLE_WRITER and/or SINGLE_NODE_MULTI_WRITER are
// supported, in order to permit older COs to continue working.
SINGLE_NODE_MULTI_WRITER = 5 [(alpha_enum_value) = true];
}

Type type = 1;
Expand Down
Loading