Add staging path to NodeExpand/GetStats calls #390

bswartz · 2019-09-19T18:20:48Z

Allow COs to explicitly pass the staging path to node plugins when
invoking NodeGetVolumeStats and NodeExpandVolume. Previously the
CO could choose to pass either the staging or publish path, but
there was no way for the node plugin to easily know which path it
was.

This change allows COs to pass both paths (if it wishes) or just
the staging path, and in either case, the node plugin can easily
figure out what it's being given. This makes its significantly
easier for node plugins to implement the NodeGetVolumeStats and
NodeExpandVolume RPCs, in particular it relieves the need to
store some state on the node.

For backwards compatibility, the field is not required, but it
should be trivially easy for any CO to populate the field, so we
encourage clients to set it.

Release Note:

Added optional staging_target_path field to NodeExpandVolume()
and NodeGetVolumeStats(). CO implementers should populate this
new field if possible when calling these RPCs.

pure-yesmat · 2019-09-19T21:05:40Z

csi.proto

+  // If not empty, it MUST be an absolute path in the root
+  // filesystem of the process serving this request.
+  // This field is OPTIONAL.
+  string staging_path = 3;


Thank you very much for this change!
If we want to remain consistent with other places this is used, its actually called: staging_target_path
eg:

spec/csi.proto

Line 1152 in 9e773d2

string staging_target_path = 3;

spec/csi.proto

Line 1129 in 9e773d2

string staging_target_path = 2;

I like your suggestion, I have implemented it.

I thought this was going to be renamed from staging_path to staging_target_path for consistency .. what happened?

gnufied · 2019-09-23T20:56:13Z

lgtm

julian-hj

👍

davidz627 · 2019-10-16T00:33:54Z

spec.md

@@ -2339,6 +2350,11 @@ Otherwise `NodeExpandVolume` MUST be called after successful `NodePublishVolume`

 If a plugin only supports expansion via the `VolumeExpansion.OFFLINE` capability, then the volume MUST first be taken offline and expanded via `ControllerExpandVolume` (see `ControllerExpandVolume` for more details), and then node-staged or node-published before it can be expanded on the node via `NodeExpandVolume`.

+The `staging_target_path` field is not required, for backwards compatibility, but the CO SHOULD supply it


if this is an optional field drivers will have to continue to contain logic for if staging_target_path is unset (since we cant assume all callers will actually use this new field) and thus not actually fixing the problem specified in the description:

in particular it relieves the need to store some state on the node.

This is a good point.

If we assume that good COs will always set staging_target_path for plugins that implement STAGE_UNSTAGE_VOLUME then SP logic can be:

If staging_target_path is set (which it should be for all good COs going forward), you know it is the staging path, and you can compare it to target_path if it is different you can assume it is the publish path.

If staging_target_path is unset, but you absolutely need it you can fail the operation and say you require this field to operate. This means you will only support resizing for newer clients (given volume expansion is still beta in k8s, and SPs can control what external-resizer sidecar they pair with, this seems like a reasonable approach).

But the question is do we have a case where a good CO wants to legitimately not set staging_target_path AND SP will want it?

saad-ali · 2019-10-16T01:02:07Z

spec.md

@@ -2170,6 +2170,10 @@ A Node plugin MUST implement this RPC call if it has GET_VOLUME_STATS node capab
 If the volume is being used in `BlockVolume` mode then `used` and `available` MAY be omitted from `usage` field of `NodeGetVolumeStatsResponse`.
 Similarly, inode information MAY be omitted from `NodeGetVolumeStatsResponse` when unavailable.

+The `staging_target_path` field is not required, for backwards compatibility, but the CO SHOULD supply it
+when possible. Plugins can use this field to determine if `volume_path` is where the volume is


Drop when possible.

saad-ali · 2019-10-16T01:13:03Z

spec.md

@@ -2339,6 +2350,11 @@ Otherwise `NodeExpandVolume` MUST be called after successful `NodePublishVolume`

 If a plugin only supports expansion via the `VolumeExpansion.OFFLINE` capability, then the volume MUST first be taken offline and expanded via `ControllerExpandVolume` (see `ControllerExpandVolume` for more details), and then node-staged or node-published before it can be expanded on the node via `NodeExpandVolume`.

+The `staging_target_path` field is not required, for backwards compatibility, but the CO SHOULD supply it


This is a good point.

If we assume that good COs will always set staging_target_path for plugins that implement STAGE_UNSTAGE_VOLUME then SP logic can be:

If staging_target_path is set (which it should be for all good COs going forward), you know it is the staging path, and you can compare it to target_path if it is different you can assume it is the publish path.

If staging_target_path is unset, but you absolutely need it you can fail the operation and say you require this field to operate. This means you will only support resizing for newer clients (given volume expansion is still beta in k8s, and SPs can control what external-resizer sidecar they pair with, this seems like a reasonable approach).

But the question is do we have a case where a good CO wants to legitimately not set staging_target_path AND SP will want it?

saad-ali · 2019-10-16T01:15:32Z

spec.md

+The `staging_target_path` field is not required, for backwards compatibility, but the CO SHOULD supply it
+when possible. Plugins can use this field to determine if `volume_path` is where the volume is
+published or staged, and setting this field to non-empty allows plugins to function with less
+stored state on the node.


We should make this more explicit -- if staging_target_path is specified, then volume_path MUST contain the publish path NOT stage path.

Allow COs to explicitly pass the staging path to node plugins when invoking NodeGetVolumeStats and NodeExpandVolume. Previously the CO could choose to pass either the staging or publish path, but there was no way for the node plugin to easily know which path it was. This change allows COs to pass both paths (if it wishes) or just the staging path, and in either case, the node plugin can easily figure out what it's being given. This makes its significantly easier for node plugins to implement the NodeGetVolumeStats and NodeExpandVolume RPCs, in particular it relieves the need to store some state on the node. For backwards compatibility, the field is not required, but it should be trivially easy for any CO to populate the field, so we encourage clients to set it. Release Note: ``` Added optional staging_target_path field to NodeExpandVolume() and NodeGetVolumeStats(). CO implementers should populate this new field if possible when calling these RPCs. ```

saad-ali

/lgtm
/approve

saad-ali

/lgtm
/approve

pure-yesmat reviewed Sep 19, 2019

View reviewed changes

bswartz force-pushed the staging_path branch from aaebe34 to b700261 Compare September 20, 2019 01:20

bswartz changed the title ~~Add staging_path to NodeExpand/GetStats calls~~ Add staging path to NodeExpand/GetStats calls Sep 20, 2019

pure-yesmat approved these changes Sep 20, 2019

View reviewed changes

jdef approved these changes Sep 20, 2019

View reviewed changes

julian-hj approved these changes Oct 10, 2019

View reviewed changes

davidz627 reviewed Oct 16, 2019

View reviewed changes

saad-ali requested changes Oct 16, 2019

View reviewed changes

bswartz force-pushed the staging_path branch from b700261 to 0294da9 Compare October 16, 2019 17:19

saad-ali approved these changes Oct 16, 2019

View reviewed changes

saad-ali merged commit 4e23257 into container-storage-interface:master Oct 16, 2019

bswartz deleted the staging_path branch December 30, 2019 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add staging path to NodeExpand/GetStats calls #390

Add staging path to NodeExpand/GetStats calls #390

bswartz commented Sep 19, 2019 •

edited

pure-yesmat Sep 19, 2019

bswartz Sep 20, 2019

jdef Oct 21, 2019

gnufied commented Sep 23, 2019

julian-hj left a comment

davidz627 Oct 16, 2019

saad-ali Oct 16, 2019

saad-ali Oct 16, 2019

saad-ali Oct 16, 2019

saad-ali Oct 16, 2019

saad-ali left a comment

saad-ali left a comment

		@@ -2339,6 +2350,11 @@ Otherwise `NodeExpandVolume` MUST be called after successful `NodePublishVolume`

		If a plugin only supports expansion via the `VolumeExpansion.OFFLINE` capability, then the volume MUST first be taken offline and expanded via `ControllerExpandVolume` (see `ControllerExpandVolume` for more details), and then node-staged or node-published before it can be expanded on the node via `NodeExpandVolume`.

		The `staging_target_path` field is not required, for backwards compatibility, but the CO SHOULD supply it

Add staging path to NodeExpand/GetStats calls #390

Add staging path to NodeExpand/GetStats calls #390

Conversation

bswartz commented Sep 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Sep 23, 2019

julian-hj 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

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali left a comment

Choose a reason for hiding this comment

bswartz commented Sep 19, 2019 •

edited