Access modes for single node single/multi writer #476

chrishenzie · 2021-04-26T19:42:24Z

Access Modes

In the CSI spec there are conflicting definitions of the SINGLE_NODE_WRITER access mode. By definition, SINGLE_NODE_WRITER means "Can only be published once as read/write on a single node, at any given time." The problem is how this access mode is used during NodePublishVolume, which is typically where volume mounting is performed.

The CSI spec defines that when NodePublishVolume is called a second time for a volume with a non-MULTI_NODE access mode and with a different target path, the plugin should return FAILED_PRECONDITION. For CSI plugins that strictly adhere to the spec, this guarantees that a volume can only be mounted to a single target path, which means SINGLE_NODE_WRITER restricts access to a single pod on a single node. This behavior conflicts with the original definition. Due to this conflict, we do not have an access mode that represents multiple writers on the same node. See #465 (comment) for more details.

The proposed solution for this issue is to introduce two new access modes: SINGLE_NODE_SINGLE_WRITER and SINGLE_NODE_MULTI_WRITER. These access modes exist to replace SINGLE_NODE_WRITER and disambiguate the number of writers on a single node.

Capabilities

Included in this change are capability bits in the controller and node services to indicate that a driver understands these new access modes. This approach was chosen in order to preserve backwards compatibility. When a client calls a CSI driver that supports the SINGLE_NODE_MULTI_WRITER capability, the client should replace any uses of the SINGLE_NODE_WRITER access mode with the appropriate SINGLE_NODE_{SINGLE,MULTI}_WRITER access mode depending on their use case.

gnufied · 2021-04-29T18:31:16Z

spec.md

+|                                | T1=T2, P1=P2    | T1=T2, P1!=P2  | T1!=T2, P1=P2       | T1!=T2, P1!=P2      |
+|--------------------------------|-----------------|----------------|---------------------|---------------------|
+| SINGLE_NODE_SINGLE_WRITER      | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+| Non SINGLE_NODE_SINGLE_WRITER  | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |


Did Non SINGLE_NODE_SINGLE_WRITER meant -> SINGLE_NODE_MULTI_WRITER ? We are saying above that - this table applies for SINGLE_NODE_MULTI_WRITER but we don't use that word in the table - so it is bit confusing.

Yeah, I went back and forth on the wording here. I had a row for SINGLE_NODE_MULTI_WRITER but realized I would probably also need a row for "Everything else" but it would be treated in the same way as SINGLE_NODE_MULTI_WRITER so I combined them into the "Non" row. I think having a row for this to make it explicit is a good idea, I'll add this.

chrishenzie · 2021-05-12T00:20:27Z

@bswartz Would you be able to take a look at this? Thank you!

bswartz · 2021-05-12T01:17:53Z

spec.md

+|---------------------------------------|-----------------|----------------|---------------------|---------------------|
+| SINGLE_NODE_SINGLE_WRITER             | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+| SINGLE_NODE_MULTI_WRITER              | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| Non SINGLE_NODE_{SINGLE,MULTI}_WRITER | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |


This catch all isn't clear to me. It suggests that everything else should succeed, but I think that multi node should succeed and non-multi-node should not. Either we should expand this second table to cover all the cases, or explicitly refer back to the first table in the 3rd row.

Also don't forget that even for SPs that support the new capability, older COs could continue to use SINGLE_NODE_WRITER and SPs are required to continue to support the old semantics forever alongside the new ones (unless we go through format deprecation).

Done, expanded the table to include the MULTI_NODE and Non MULTI_NODE in the above table, and left a note requiring continued support for SINGLE_NODE_WRITER

gnufied · 2021-05-12T02:37:39Z

spec.md

@@ -887,6 +887,14 @@ 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 worklad on


typo in word worklad.

gnufied · 2021-05-12T02:40:59Z

spec.md

+
+      // Indicates the SP supports the SINGLE_NODE_MULTI_WRITER access
+      // mode.
+      SINGLE_NODE_MULTI_WRITER = 13 [(alpha_enum_value) = true];
    }


Why does this exists as a control-plane capability?

COs require this capability in order to determine which access mode to supply for controller service operations that take VolumeCapability, like CreateVolume(). This capability will be used in the same way as the capability on the node service.

@chrishenzie just to understand in bit more depth, but for COs with controller service operations, its just about knowing SINGLE_NODE or MULTI_NODE , isnt it ? in that sense, writer thing ( single or multiple) is not relevant for CO's control plane operations ?

Yes, the number of writers here does not make a difference.

gnufied · 2021-05-12T02:41:44Z

spec.md

@@ -2232,6 +2244,17 @@ The following table shows what the Plugin SHOULD return when receiving a second
 | MULTI_NODE     | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                 |
 | Non MULTI_NODE | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION|

+NOTE: If the Plugin supports the `SINGLE_NODE_MULTI_WRITER` capability, use the following table instead for what the Plugin SHOULD return when receiving a second `NodePublishVolume` on the same volume on the same node:
+


So I think we might have a problem here - can you query control-plane capabilities from node service?

The node service has an equivalent capability bit that is used for determining which access mode to supply for node operations that take VolumeCapability.

bswartz · 2021-05-12T14:45:44Z

/lgtm

gnufied · 2021-05-12T14:48:34Z

spec.md

+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` access modes are intended to replace the `SINGLE_NODE_WRITER` access mode in order to clarify the number of writers for a volume on a single node. Plugins MUST continue to support `SINGLE_NODE_WRITER` to allow older COs to continue working.
+


Do we need to explicitly deprecate SINGLE_NODE_WRITER at some point? I guess we need to wait for new modes to go GA before we can deprecate older one.

I think it would be a good idea, but there are no deprecation plans as of yet.

gnufied · 2021-05-12T14:48:48Z

lgtm

saad-ali · 2021-05-13T06:25:58Z

spec.md

+| MULTI_NODE                            | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` access modes are intended to replace the `SINGLE_NODE_WRITER` access mode in order to clarify the number of writers for a volume on a single node. Plugins MUST continue to support `SINGLE_NODE_WRITER` to allow older COs to continue working.


Plugins MUST continue to support SINGLE_NODE_WRITER to allow older COs to continue working.

Can you detail what "support" means here.

Done, here is the new wording

Plugins MUST continue to accept and allow use of the SINGLE_NODE_WRITER access mode to support older COs to continue working.

saad-ali · 2021-05-13T06:27:00Z

spec.md

@@ -2232,6 +2244,17 @@ The following table shows what the Plugin SHOULD return when receiving a second
 | MULTI_NODE     | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                 |
 | Non MULTI_NODE | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION|

+NOTE: If the Plugin supports the `SINGLE_NODE_MULTI_WRITER` capability, use the following table instead for what the Plugin SHOULD return when receiving a second `NodePublishVolume` on the same volume on the same node:
+
+|                                       | T1=T2, P1=P2    | T1=T2, P1!=P2  | T1!=T2, P1=P2       | T1!=T2, P1!=P2      |


nit: Can you add an explanation before or after these tables explaining what T1, T2, P1, P2 are in this context (I realize existing table doesn't do this, but let's fix it while we're here).

Done. There was a little section on that below the table. I moved it to the top of the first table so it's the first thing the user sees.

jingxu97 · 2021-05-19T19:15:00Z

csi.proto

+
+      // Can be published as read/write at multiple workloads on a
+      // single node simultaneously.
+      SINGLE_NODE_MULTI_WRITER = 7 [(alpha_enum_value) = true];


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

only a single pod from a node can access it,

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.

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.

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

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.

saad-ali

/lgtm
/approve

mod nit

@ddebroy can you PTAL?

saad-ali · 2021-05-24T07:40:24Z

spec.md

+| MULTI_NODE                            | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 continue to accept and allow use of the `SINGLE_NODE_WRITER` access mode to support older COs to continue working.


Suggested change

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 continue to accept and allow use of the `SINGLE_NODE_WRITER` access mode to support older COs to continue working.

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 continue to accept and allow use of the `SINGLE_NODE_WRITER` access mode to permit older COs to continue working.

Fixed this.

ddebroy

Just one suggestion for a potential clarification. Otherwise LGTM.

ddebroy · 2021-05-26T14:15:52Z

spec.md

@@ -2227,12 +2239,23 @@ If this RPC failed, or the CO does not know if it failed or not, it MAY choose t
 This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).


Should the the volume has MULTI_NODE capability bit in the sentence above be amended to also include SINGLE_NODE_MULTI_WRITER?

Yes this is a good idea. Added, here is the amended sentence. Is this understandable?

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different target_path and/or other arguments if the volume has MULTI_NODE or SINGLE_NODDE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include MULTI_NODE_READER_ONLY, MULTI_NODE_SINGLE_WRITER or MULTI_NODE_MULTI_WRITER.

jdef · 2021-05-26T18:22:40Z

thanks for putting this together, looking now..

jdef · 2021-05-26T18:22:19Z

spec.md

@@ -2224,15 +2236,26 @@ If the volume corresponding to the `volume_id` has already been published at the

 If this RPC failed, or the CO does not know if it failed or not, it MAY choose to call `NodePublishVolume` again, or choose to call `NodeUnpublishVolume`.

-This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).
+This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODDE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.


Suggested change

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODDE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

Thanks for catching this

jdef · 2021-05-26T18:28:48Z

spec.md

+
+      // Can only be published once as read/write at a single workload
+      // on a single node, at any given time.
+      SINGLE_NODE_SINGLE_WRITER = 6 [(alpha_enum_value) = true];


CO's using the experimental capability SINGLE_NODE_MULTI_WRITER should probably avoid specifying SINGLE_NODE_WRITER, yes? if for no other reason than for clarity. if so, please mention that here.

Yes, COs should not use SINGLE_NODE_WRITER in this case. Added this to the description.

jdef · 2021-05-26T18:29:44Z

spec.md

+
+      // Can be published as read/write at multiple workloads on a
+      // single node simultaneously.
+      SINGLE_NODE_MULTI_WRITER = 7 [(alpha_enum_value) = true];


@chhsia0 any initial thoughts re: Mesos compatibility?

This seems okay at a first glance. At least this is the same as how Mesos shared persistent volumes work: http://mesos.apache.org/documentation/latest/shared-resources/

BTW I'm no longer actively involved in Mesos development; this is just my personal opinion.

jdef · 2021-05-26T18:37:07Z

spec.md

+| MULTI_NODE                            | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 continue to accept and allow use of the `SINGLE_NODE_WRITER` access mode to permit older COs to continue working.


Suggested change

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 continue to accept and allow use of the `SINGLE_NODE_WRITER` access mode to permit older COs to continue working.

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.

^ is this more precise? or do I not understand what the goal is?

I feel like this should probably be called out somewhere in the generated .proto too

Yes this is useful, thank you.

I called this out on the SINGLE_NODE_MULTI_WRITER access mode capabilities in the node and controller services

I updated the SINGLE_NODE_MULTI_WRITER capability to explain it implies support for either or both access modes

The node service capability description contains "(subject to the processing rules for NodePublishVolume)", I left it out for the controller service capability description since these access modes are also used in other RPCs like CreateVolume where these rules don't apply

jdef · 2021-05-27T02:25:21Z

spec.md

@@ -2224,15 +2246,26 @@ If the volume corresponding to the `volume_id` has already been published at the

 If this RPC failed, or the CO does not know if it failed or not, it MAY choose to call `NodePublishVolume` again, or choose to call `NodeUnpublishVolume`.

-This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).
+This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.


Suggested change

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes.

Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

^ nit: one sentence per line, please

jdef · 2021-05-27T02:27:39Z

spec.md

+| MULTI_NODE                            | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.


Suggested change

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.

The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.

one sentence per line

gnufied · 2021-05-29T18:37:04Z

spec.md

+|                                       | T1=T2, P1=P2    | T1=T2, P1!=P2  | T1!=T2, P1=P2       | T1!=T2, P1!=P2      |
+|---------------------------------------|-----------------|----------------|---------------------|---------------------|
+| SINGLE_NODE_SINGLE_WRITER             | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+| SINGLE_NODE_MULTI_WRITER              | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |


For many volume types - I suspect they may not support consuming same volume with different capability on same node. For example - would they allow mounting volume with different mount flags or would they allow consuming volume both as block and fs volume at same time?

So I am bit doubtful that if "P1!=P2" and "T1!=T2" - node-publish will be okay (assuming driver really enforces supplied capabilities). I suspect this will lead to drivers ignoring certain capabilities and allowing node-publish anyways. It may be okay, but strictly speaking they are not spec compliant then.

We currently make the clarification that Pn refers to other arguments passed to NodePublishVolume except secrets. We could extend this to also include volume_capability. If we want to play it safe we could make this change just for the new table. Does this sound reasonable?

I think the except secrets wording does not make it clear, if calling NodePublish multiple times on same node has to be done with same secret or secrets can be different. I always found that wording to be ambiguous, so when we add volume_capability to it - does it mean the capabilities has to be same? If yes - may be we should add more fields to the table. Something like:

T1!=T2, P1!=P2, Cap1!=Cap2 -> FAILED_PRECONDITION T1!=T2, P1!=P2, Cap1==Cap2 -> OK

Discussed this in the CSI standup, we're adding a sentence to make this explicit to avoid adding another column and more rows to the table. Added this sentence:

COs SHOULD NOT call NodePublishVolume a second time with different volume_capability. If this happens, the Plugin SHOULD return FAILED_PRECONDITION.

@bswartz for review on this

+1 I like it

Thanks. I think having explicit wording about capability helps. +1

humblec · 2021-05-31T03:47:20Z

csi.proto

@@ -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


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

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.

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.

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.

gnufied · 2021-06-01T16:55:02Z

spec.md

+| Non MULTI_NODE                        | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+
+The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.



Do we need update NodePublish errors? Currently it says:

| Exceeds capabilities | 9 FAILED_PRECONDITION | Indicates that the CO has exceeded the volume's capabilities because the volume does not have MULTI_NODE capability. | Caller MAY choose to call ValidateVolumeCapabilities to validate the volume capabilities, or wait for the volume to be unpublished on the node. |

I would prefer not to introduce another error if possible. Since the condition is capabilities being exceeded, I think this description could be generalized to any time a CO requests capabilities not supported by the volume.

Updated to use the error description for "Exceeds capabilities" used for ControllerExpandVolume and NodeExpandVolume which is more general:

Indicates that CO has specified capabilities not supported by the volume.

Also updated this for NodeStageVolume since we used the old wording there as well.

saad-ali · 2021-06-04T21:39:37Z

@jdef @humblec @gnufied PTAL

I'd like to merge and cut the next release on Monday.

jdef

LGTM mod comments

jdef · 2021-06-06T00:58:23Z

spec.md

@@ -887,6 +887,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
+      // on a single node, at any given time. Should be used instead of


Suggested change

// on a single node, at any given time. Should be used instead of

// on a single node, at any given time. SHOULD be used instead of

caps .. make it an official recommendation, yes?

Yes, thanks for the heads up

jdef · 2021-06-06T00:59:09Z

spec.md

+      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


Suggested change

// single node simultaneously. Should be used instead of

// single node simultaneously. SHOULD be used instead of

jdef · 2021-06-06T01:00:34Z

spec.md

@@ -2224,15 +2246,30 @@ If the volume corresponding to the `volume_id` has already been published at the

 If this RPC failed, or the CO does not know if it failed or not, it MAY choose to call `NodePublishVolume` again, or choose to call `NodeUnpublishVolume`.

-This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).
+This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has `MULTI_NODE` or `SINGLE_NODE_MULTI_WRITER` (see second table) access modes.


Suggested change

This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has `MULTI_NODE` or `SINGLE_NODE_MULTI_WRITER` (see second table) access modes.

This RPC MAY be called by the CO multiple times on the same node for the same volume with a possibly different `target_path` and/or other arguments if the volume supports either `MULTI_NODE_...` or `SINGLE_NODE_MULTI_WRITER` access modes (see second table).

jdef · 2021-06-06T01:03:59Z

spec.md

@@ -2224,15 +2246,30 @@ If the volume corresponding to the `volume_id` has already been published at the

 If this RPC failed, or the CO does not know if it failed or not, it MAY choose to call `NodePublishVolume` again, or choose to call `NodeUnpublishVolume`.

-This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).
+This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has `MULTI_NODE` or `SINGLE_NODE_MULTI_WRITER` (see second table) access modes.
+Example `MULTI_NODE` access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.


Suggested change

Example `MULTI_NODE` access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

The possible `MULTI_NODE_...` access modes are `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

jdef · 2021-06-06T01:04:27Z

spec.md

-This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).
+This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has `MULTI_NODE` or `SINGLE_NODE_MULTI_WRITER` (see second table) access modes.
+Example `MULTI_NODE` access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.
+COs SHOULD NOT call `NodePublishVolume` a second time with different `volume_capability`.


Suggested change

COs SHOULD NOT call `NodePublishVolume` a second time with different `volume_capability`.

COs SHOULD NOT call `NodePublishVolume` a second time with a different `volume_capability`.

jdef · 2021-06-06T01:05:59Z

spec.md

 The following table shows what the Plugin SHOULD return when receiving a second `NodePublishVolume` on the same volume on the same node:

+(`Tn`: target path of the n-th `NodePublishVolume`, `Pn`: other arguments of the n-th `NodePublishVolume` except `secrets`)


Suggested change

(`Tn`: target path of the n-th `NodePublishVolume`, `Pn`: other arguments of the n-th `NodePublishVolume` except `secrets`)

(**Tn**: target path of the nth `NodePublishVolume`; **Pn**: other arguments of the nth `NodePublishVolume` except `secrets`)

jdef · 2021-06-06T01:09:43Z

spec.md

+|---------------------------------------|-----------------|----------------|---------------------|---------------------|
+| SINGLE_NODE_SINGLE_WRITER             | OK (idempotent) | ALREADY_EXISTS | FAILED_PRECONDITION | FAILED_PRECONDITION |
+| SINGLE_NODE_MULTI_WRITER              | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |
+| MULTI_NODE                            | OK (idempotent) | ALREADY_EXISTS | OK                  | OK                  |


jdef · 2021-06-06T01:10:36Z

spec.md

@@ -2313,7 +2350,7 @@ The CO MUST implement the specified error recovery behavior when it encounters t
 |-----------|-----------|-------------|-------------------|
 | Volume does not exist | 5 NOT_FOUND | Indicates that a volume corresponding to the specified `volume_id` does not exist. | Caller MUST verify that the `volume_id` is correct and that the volume is accessible and has not been deleted before retrying with exponential back off. |
 | Volume published but is incompatible | 6 ALREADY_EXISTS | Indicates that a volume corresponding to the specified `volume_id` has already been published at the specified `target_path` but is incompatible with the specified `volume_capability` or `readonly` flag. | Caller MUST fix the arguments before retrying. |
-| Exceeds capabilities | 9 FAILED_PRECONDITION | Indicates that the CO has exceeded the volume's capabilities because the volume does not have MULTI_NODE capability. | Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. |
+| Exceeds capabilities | 9 FAILED_PRECONDITION | Indicates that CO has specified capabilities not supported by the volume. | Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. |


Suggested change

| Exceeds capabilities | 9 FAILED_PRECONDITION | Indicates that CO has specified capabilities not supported by the volume. | Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. |

| Exceeds capabilities | 9 FAILED_PRECONDITION | Indicates that the CO has specified capabilities not supported by the volume. | Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. |

chrishenzie · 2021-06-07T01:11:01Z

@jdef Done, addressed comments. Thank you for the review!

humblec · 2021-06-07T08:20:17Z

csi.proto

+      // Can only be published once as read/write at a single workload
+      // 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.


@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.

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.

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. 👍

humblec · 2021-06-07T18:15:20Z

lgtm from end. Thanks 👍

saad-ali · 2021-06-07T19:12:16Z

LGTM mod comments

lgtm from end. Thanks 👍

Thanks folks. Mergining this PR.

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from 62b8ef5 to 5156940 Compare April 28, 2021 22:54

gnufied reviewed Apr 29, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from 1bbe297 to b17438c Compare May 7, 2021 17:01

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from d77872e to eaeb3d3 Compare May 10, 2021 22:01

bswartz reviewed May 12, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from e6f921a to 2595556 Compare May 12, 2021 01:31

gnufied reviewed May 12, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch from 2595556 to cb93ef7 Compare May 12, 2021 06:20

gnufied reviewed May 12, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch from cb93ef7 to 1f3e577 Compare May 12, 2021 18:29

saad-ali reviewed May 13, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch 3 times, most recently from 4d39415 to a99f86b Compare May 13, 2021 15:00

jingxu97 reviewed May 19, 2021

View reviewed changes

saad-ali approved these changes May 24, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch from a99f86b to 2cc8efe Compare May 24, 2021 17:57

ddebroy reviewed May 26, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from 7456c03 to fb7cd94 Compare May 26, 2021 15:55

jdef reviewed May 26, 2021

View reviewed changes

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from 0faba95 to 43405ec Compare May 26, 2021 21:43

jdef reviewed May 27, 2021

View reviewed changes

gnufied mentioned this pull request May 27, 2021

Add spec change for volume mount group #468

Merged

chrishenzie force-pushed the single-node-access-modes branch from 43405ec to eb31bfb Compare May 27, 2021 16:41

gnufied reviewed May 29, 2021

View reviewed changes

humblec reviewed May 31, 2021

View reviewed changes

gnufied reviewed Jun 1, 2021

View reviewed changes

chrishenzie mentioned this pull request Jun 1, 2021

Map PV access modes to CSI access modes kubernetes-csi/csi-lib-utils#89

Merged

chrishenzie force-pushed the single-node-access-modes branch 2 times, most recently from d461b57 to fd96fc5 Compare June 2, 2021 19:47

jdef reviewed Jun 6, 2021

View reviewed changes

Single node single/multi writer access modes

7c7f05e

chrishenzie force-pushed the single-node-access-modes branch from fd96fc5 to 7c7f05e Compare June 7, 2021 01:07

humblec reviewed Jun 7, 2021

View reviewed changes

saad-ali merged commit 0cedd1b into container-storage-interface:master Jun 7, 2021

chrishenzie deleted the single-node-access-modes branch June 7, 2021 19:13

saad-ali mentioned this pull request Jun 7, 2021

Add force detach #477

Open

		\| Non MULTI_NODE \| OK (idempotent) \| ALREADY_EXISTS \| FAILED_PRECONDITION \| FAILED_PRECONDITION \|

		The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` access modes are intended to replace the `SINGLE_NODE_WRITER` access mode in order to clarify the number of writers for a volume on a single node. Plugins MUST continue to support `SINGLE_NODE_WRITER` to allow older COs to continue working.

		@@ -2227,12 +2239,23 @@ If this RPC failed, or the CO does not know if it failed or not, it MAY choose t
		This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE capability (i.e., `access_mode` is either `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`).

	This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes. Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.
	This RPC MAY be called by the CO multiple times on the same node for the same volume with possibly different `target_path` and/or other arguments if the volume has MULTI_NODE or SINGLE_NODE_MULTI_WRITER (see second table) access modes.
	Example MULTI_NODE access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

	The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.
	The `SINGLE_NODE_SINGLE_WRITER` and `SINGLE_NODE_MULTI_WRITER` 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 above), when either `SINGLE_NODE_SINGLE_WRITER` and/or `SINGLE_NODE_MULTI_WRITER` are supported, in order to permit older COs to continue working.

	// on a single node, at any given time. Should be used instead of
	// on a single node, at any given time. SHOULD be used instead of

	// single node simultaneously. Should be used instead of
	// single node simultaneously. SHOULD be used instead of

	Example `MULTI_NODE` access modes include `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.
	The possible `MULTI_NODE_...` access modes are `MULTI_NODE_READER_ONLY`, `MULTI_NODE_SINGLE_WRITER` or `MULTI_NODE_MULTI_WRITER`.

	COs SHOULD NOT call `NodePublishVolume` a second time with different `volume_capability`.
	COs SHOULD NOT call `NodePublishVolume` a second time with a different `volume_capability`.

		The following table shows what the Plugin SHOULD return when receiving a second `NodePublishVolume` on the same volume on the same node:

		(`Tn`: target path of the n-th `NodePublishVolume`, `Pn`: other arguments of the n-th `NodePublishVolume` except `secrets`)

	(`Tn`: target path of the n-th `NodePublishVolume`, `Pn`: other arguments of the n-th `NodePublishVolume` except `secrets`)
	(T<sub>n</sub>: target path of the n<sup>th</sup> `NodePublishVolume`; P<sub>n</sub>: other arguments of the n<sup>th</sup> `NodePublishVolume` except `secrets`)

	\| MULTI_NODE \| OK (idempotent) \| ALREADY_EXISTS \| OK \| OK \|
	\| MULTI_NODE_... \| OK (idempotent) \| ALREADY_EXISTS \| OK \| OK \|

	\| Exceeds capabilities \| 9 FAILED_PRECONDITION \| Indicates that CO has specified capabilities not supported by the volume. \| Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. \|
	\| Exceeds capabilities \| 9 FAILED_PRECONDITION \| Indicates that the CO has specified capabilities not supported by the volume. \| Caller MAY choose to call `ValidateVolumeCapabilities` to validate the volume capabilities, or wait for the volume to be unpublished on the node. \|

Access modes for single node single/multi writer #476

Access modes for single node single/multi writer #476

Conversation

chrishenzie commented Apr 26, 2021 • edited Loading

Access Modes

Capabilities

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishenzie commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishenzie May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bswartz commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented May 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishenzie May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishenzie May 26, 2021 • edited Loading

Choose a reason for hiding this comment

jdef commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhsia0 Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jun 4, 2021

jdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishenzie commented Apr 26, 2021 •

edited

Loading

chrishenzie May 12, 2021 •

edited

Loading

chrishenzie May 19, 2021 •

edited

Loading

chrishenzie May 26, 2021 •

edited

Loading

chhsia0 Jun 4, 2021 •

edited

Loading

humblec May 31, 2021 •

edited

Loading

humblec Jun 7, 2021 •

edited

Loading