Deprecates volume plugin’s ability to declare offline/online expansion capability #429

gnufied · 2020-05-07T20:03:33Z

Ability for a plugin to delcare online and offline controller expansion
capabilities via GetPluginCapabilities is deprecated. A plugin may
support either mode of operation without having to declare them
in GetPluginCapabilities call.

If a plugin can not support controller expansion of published volume
it may return Volume-in-use error and CO should ensure that volume is
not published before retrying with exponential backoff.

gnufied · 2020-05-13T02:44:56Z

cc @jdef @saad-ali

gnufied · 2020-05-14T02:00:33Z

cc @xing-yang

spec.md

xing-yang · 2020-05-14T02:40:25Z

spec.md

@@ -644,6 +644,7 @@ message PluginCapability {
  oneof type {
    // Service that the plugin supports.
    Service service = 1;
+    // deprecated


"deprecated" should also be added in front of the "message VolumeExpansion" on line 598 as well?

xing-yang · 2020-05-22T18:07:41Z

spec.md

@@ -1930,24 +1945,15 @@ This RPC allows the CO to expand the size of a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified volume ID is already larger than or equal to the target capacity of the expansion request, the plugin SHOULD reply 0 OK.

-This call MAY be made by the CO during any time in the lifecycle of the volume after creation if plugin has `VolumeExpansion.ONLINE` capability.
+This call MAY be made by the CO during any time in the lifecycle of the volume after creation but an SP may not permit controller expansion of volumes which are controller published or available on a node. In which case - the plugin may return gRPC error code `FAILED_PRECONDITION` (Volume in use) and CO SHOULD ensure that volume is not published before retrying with exponential backoff.
+


Line 659 says "published and available volumes on a node". Here it says "published or available on a node". Is it "and" or "or"? Should be consistent in both places.

or makes more sense I think

xing-yang · 2020-05-22T18:08:12Z

spec.md


 Examples:
-* Offline Volume Expansion:
+* Offline Volume Expansion(SP does not permit controller expansion of controller published volume):


nit: Add a space before (

saad-ali · 2020-05-27T06:10:51Z

spec.md

+  // Deprecated - Ability for a plugin to delcare online and offline
+  // controller expansion capabilities via PluginCapability
+  // is deprecated. A plugin may support either mode of operation
+  // without having to declare them in PluginCapability.
  message VolumeExpansion {
    enum Type {
      UNKNOWN = 0;


Should we also remove the comments on ONLINE = 1; and OFFLINE = 2; below and replace them with // Deprecated + [deprecated=true] annotation?

saad-ali · 2020-05-27T06:13:58Z

spec.md

@@ -1930,24 +1945,15 @@ This RPC allows the CO to expand the size of a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified volume ID is already larger than or equal to the target capacity of the expansion request, the plugin SHOULD reply 0 OK.

-This call MAY be made by the CO during any time in the lifecycle of the volume after creation if plugin has `VolumeExpansion.ONLINE` capability.
+This call MAY be made by the CO during any time in the lifecycle of the volume after creation but an SP may not permit controller expansion of volumes which are controller published or available on a node. In which case - the plugin may return gRPC error code `FAILED_PRECONDITION` (Volume in use) and CO SHOULD ensure that volume is not published before retrying with exponential backoff.


Suggested change

This call MAY be made by the CO during any time in the lifecycle of the volume after creation but an SP may not permit controller expansion of volumes which are controller published or available on a node. In which case - the plugin may return gRPC error code `FAILED_PRECONDITION` (Volume in use) and CO SHOULD ensure that volume is not published before retrying with exponential backoff.

This call MAY be made by the CO during any time in the lifecycle of the volume after creation. If an SP does not permit controller expansion of volumes which are controller published or available on a node, the SP may return a gRPC error code `FAILED_PRECONDITION` (Volume in use) and the CO SHOULD ensure that volume is not published on controller or node before retrying with exponential backoff.

saad-ali · 2020-05-27T06:14:39Z

spec.md

@@ -1930,24 +1945,15 @@ This RPC allows the CO to expand the size of a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified volume ID is already larger than or equal to the target capacity of the expansion request, the plugin SHOULD reply 0 OK.

-This call MAY be made by the CO during any time in the lifecycle of the volume after creation if plugin has `VolumeExpansion.ONLINE` capability.
+This call MAY be made by the CO during any time in the lifecycle of the volume after creation but an SP may not permit controller expansion of volumes which are controller published or available on a node. In which case - the plugin may return gRPC error code `FAILED_PRECONDITION` (Volume in use) and CO SHOULD ensure that volume is not published before retrying with exponential backoff.
+


or makes more sense I think

saad-ali · 2020-05-27T06:16:42Z

spec.md


 Examples:
-* Offline Volume Expansion:
+* Offline Volume Expansion(SP does not permit controller expansion of controller published volume):


Should we remove the offline example altogether?

I think this example is very important for CSI drivers that support offline volume expansion. It clearly explains how this case would work.

saad-ali · 2020-05-27T06:19:55Z

spec.md

@@ -2003,7 +2009,7 @@ message ControllerExpandVolumeResponse {
 |-----------|-----------|-------------|-------------------|
 | Exceeds capabilities | 3 INVALID_ARGUMENT | Indicates that CO has specified capabilities not supported by the volume. | Caller MAY verify volume capabilities by calling ValidateVolumeCapabilities and retry with matching capabilities. |
 | 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 in use | 9 FAILED_PRECONDITION | Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a node but the plugin does not have ONLINE expansion capability. | Caller SHOULD ensure that volume is not published and retry with exponential back off. |
+| Volume in use | 9 FAILED_PRECONDITION | Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a node. | Caller SHOULD ensure that volume is not published and retry with exponential back off. |


Suggested change

| Volume in use | 9 FAILED_PRECONDITION | Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a node. | Caller SHOULD ensure that volume is not published and retry with exponential back off. |

| Volume in use | 9 FAILED_PRECONDITION | Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a controller or node and the SP does not support expansion while published. | Caller SHOULD ensure that volume is not published to controller or node before retrying with exponential back off. |

saad-ali · 2020-05-27T06:26:05Z

Also, please add a

 ```release-note

block with a draft of the intended release note so we can all review and agree on that in this PR as well.

…lared in plugin capabilities. Ability for a plugin to delcare online and offline controller expansion capabilities via GetPluginCapabilities is deprecated. A plugin may support either mode of operation without having to declare them in GetPluginCapabilities call. If a plugin can not support controller expansion of published volume it may return Volume-in-use error and CO should ensure that volume is not published before retrying with exponential backoff.

gnufied force-pushed the deprecate-online-offline branch from 6eb7c8c to 027e794 Compare May 13, 2020 02:44

gnufied force-pushed the deprecate-online-offline branch from 027e794 to df3747d Compare May 13, 2020 02:47

xing-yang reviewed May 14, 2020

View reviewed changes

gnufied changed the title ~~Support online and offline expansion without requiring them to be declared~~ Deprecates volume plugin’s ability to declare offline/online expansion capability May 14, 2020

gnufied mentioned this pull request May 22, 2020

[WIP] Relax offline criteria #428

Open

xing-yang reviewed May 22, 2020

View reviewed changes

saad-ali reviewed May 27, 2020

View reviewed changes

gnufied added 2 commits July 15, 2020 07:09

Mark VolumeExpansion capability in PluginCapability deprecated

226ec73

gnufied force-pushed the deprecate-online-offline branch from 5523a1b to 226ec73 Compare July 15, 2020 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecates volume plugin’s ability to declare offline/online expansion capability #429

Deprecates volume plugin’s ability to declare offline/online expansion capability #429

gnufied commented May 7, 2020

gnufied commented May 13, 2020

gnufied commented May 14, 2020

xing-yang May 14, 2020

gnufied May 22, 2020

xing-yang May 22, 2020

saad-ali May 27, 2020

xing-yang May 22, 2020

saad-ali May 27, 2020

saad-ali May 27, 2020

saad-ali May 27, 2020

saad-ali May 27, 2020

xing-yang May 27, 2020

saad-ali May 27, 2020

saad-ali commented May 27, 2020

	This call MAY be made by the CO during any time in the lifecycle of the volume after creation but an SP may not permit controller expansion of volumes which are controller published or available on a node. In which case - the plugin may return gRPC error code `FAILED_PRECONDITION` (Volume in use) and CO SHOULD ensure that volume is not published before retrying with exponential backoff.
	This call MAY be made by the CO during any time in the lifecycle of the volume after creation. If an SP does not permit controller expansion of volumes which are controller published or available on a node, the SP may return a gRPC error code `FAILED_PRECONDITION` (Volume in use) and the CO SHOULD ensure that volume is not published on controller or node before retrying with exponential backoff.

	\| Volume in use \| 9 FAILED_PRECONDITION \| Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a node. \| Caller SHOULD ensure that volume is not published and retry with exponential back off. \|
	\| Volume in use \| 9 FAILED_PRECONDITION \| Indicates that the volume corresponding to the specified `volume_id` could not be expanded because it is currently published on a controller or node and the SP does not support expansion while published. \| Caller SHOULD ensure that volume is not published to controller or node before retrying with exponential back off. \|

Deprecates volume plugin’s ability to declare offline/online expansion capability #429

Are you sure you want to change the base?

Deprecates volume plugin’s ability to declare offline/online expansion capability #429

Conversation

gnufied commented May 7, 2020

gnufied commented May 13, 2020

gnufied commented May 14, 2020

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

saad-ali commented May 27, 2020