Add volume capability to volume expansion calls #381

gnufied · 2019-08-14T18:52:40Z

Add volume capability to volume expansion calls.

cc @msau42 @wongma7 @saad-ali @davidz627

Fixes #380

spec.md

jdef · 2019-08-15T18:15:20Z

It's possible that a confused CO might send along capabilities beyond the ability of the volume (and/or plugin) to support. What error code should a CO expect in such circumstances? InvalidArgument? FailedPrecondition? Aborted? How does a CO recover from such?

gnufied · 2019-08-15T18:25:00Z

I think that - if CO sends along capabilities which are not supported by the volume or plugin, then we should return FailedPreCondition. If that happens CO should verify the capabilities by calling ValidateVolumeCapabilities and retry with right capabilities.

I will update the spec to reflect that.

leakingtapan · 2019-08-15T21:36:00Z

csi.proto

+  // filesystem. If volume_capability is omitted the SP MAY determine
+  // access_type from given volume_path for the volume and perform
+  // node expansion. This is an OPTIONAL field.
+  VolumeCapability volume_capability = 4;


for If volume_capability is omitted the SP MAY determine access_type from given volume_path for the volume and perform node expansion, may be add some more details about how does SP determine this?

I am thinking of adding some examples that SP MAY query "volume_path" and determine if given volume_path is a block device or mounted filesystem. But being too specific here requires us bring in operating system and file system specific calls (Stat), which may not be suitable for higher level specs.

davidz627 · 2019-08-15T23:50:31Z

spec.md

+  // device or mounted file system. For example - if volume is being
+  // used as a block device the SP MAY choose to skip expanding the
+  // filesystem. If volume_capability is omitted the SP MAY determine
+  // access_type from given volume_path for the volume and perform


If volume_capability is omitted the SP MAY determine
// access_type from given volume_path for the volume and perform
// node expansion.

I know this is MAY but it feels a little bit prescriptive. Do we want to give this suggestion if this is not something a SP "should" be doing? I feel like it's a specific workaround that may not always be possible to do from the volume_path

can you give me some examples where it may not be possible to determine access_type from given volume_path?

Do we want to give this suggestion if this is not something a SP "should" be doing?

I think the only way to convey that is to convert this field from OPTIONAL to REQUIRED and MAY to MUST.

But then we will have to introduce a new node capability for being backward compatible.

can you give me some examples where it may not be possible to determine access_type from given volume_path

I don't have any :) Ack on the backward compatibility, think thats ok

saad-ali

This overlaps with Ben's NodeExpand for multi-attach -- should coordinate that work with this.

saad-ali · 2019-08-21T16:45:19Z

spec.md

+  // NodeExpandVolume on the node by setting node_expansion_required
+  // to false in ControllerExpandVolumeResponse.
+  // This is an OPTIONAL field.
+  VolumeCapability volume_capability = 4;


A k8s PV may have multiple access modes. How do we map that to a single VolumeCapability?

This was discussed in CSI meeting. Resizing is only dependent on access_type and not on access_mode when it comes to whether volume can be resized(how) and hence just having singular capability should be enough.

Should we say that in the spec here. That only the access_type will be set not access_mode?

The way VolumeCapability is defined access_type and access_mode both will be always set. It is another thing that expansion calls don't care about access_mode but there is just no way to instantiate VolumeCapability and not have access_type set on it.

Added additional note.

gnufied · 2019-10-10T20:36:19Z

@jdef updated the spec to return error code when mis-matching volume capabilities are specified.

davidz627 · 2019-10-16T00:38:28Z

spec.md

+  // Volume capability describing how the CO intends to use this volume.
+  // This allows SP to determine if volume is being used as a block
+  // device or mounted file system. For example - if volume is being
+  // used as a block device the SP MAY choose to skip expanding the


It looks like whether the NodeExpand can do work is psuedo-determined in two places

here, based on the volume capability

setting node_expansion_required on the response to ControllerExpand

Can you explain why you might want to set one or not the other, or when they would differ? Basically: why do we need both, can we simplify the spec to use only one of them?

The call to NodeExpand is still made if node_expansion_required is set to true. It is upto SP to process a block device node expansion and determine how expansion should work. For example:

In case of EBS or GCE-PD raw block devices, NodeExpand call might be a NO-OP and as such may never be called.

For something like iSCSI raw block devices though, even if there is no file system on the device it may still require rescan of the LUN, so as OS can see updated size.

Since it is the SP that processes ControllerExpand and has Block/FS information at that level and sets the node_expansion_required. In case 1) couldn't those drivers set node_expansion_required=false and in case 2) those drivers set node_expansion_required=true. When node_expansion_required=false the NodeExpand should never get called right?

Then the NodeExpand doesn't really need the capability to decide anything anymore. I feel like maybe I'm misunderstanding something here. Could you explain why node_expansion_required is not enough of a signal for doing a NodeExpand? Is there the possibility of node_expansion_required=true and there will be different behavior for either Block or FS

Is there the possibility of node_expansion_required=true and there will be different behavior for either Block or FS

Yes there is! For example - an iSCSI implementation may return node_expansion_required as true in ControllerExpandVolume call for raw block devices but actual implementation of NodeExpandVolume call for iSCSI might be different between whether device is raw block or FS. For example - if device is raw block, it could just do a rescan of the LUN. But device has FS on it, it would rescan the LUN and also expand the filesystem.

We must also consider case of volume types that don't necessarily implement a control-plane volume expansion (such as ceph-csi). In that case, actual implementation of NodeExpandVolume might be different based on block or FS.

ah I see, this answers my question. Thanks! LGTM

saad-ali · 2019-10-16T18:12:53Z

This will need a rebase

gnufied · 2019-10-16T20:06:57Z

@saad-ali rebased.

saad-ali · 2019-10-18T00:40:40Z

/lgtm
/approve

@jdef or @julian-hj or @ddebroy or @jieyu can you PTAL

ddebroy

Just a couple of minor suggestions around comments. Otherwise lgtm.

Is there a way to capture #381 (comment) is some form? It's pretty subtle but important.

ddebroy · 2019-10-18T00:58:31Z

csi.proto

+  // Volume capability describing how the CO intends to use this volume.
+  // This allows SP to determine if volume is being used as a block
+  // device or mounted file system. For example - if volume is
+  // being used as a block device - the SP MAY choose to skip calling


Minor clarification: since it's the CO that calls NodeExpandVolume, will it be clearer if the SP MAY choose to skip calling NodeExpandVolume on the node by setting node_expansion_required to false in ControllerExpandVolumeResponse. is reworded to something along the lines of the SP MAY set node_expansion_required to false in ControllerExpandVolumeResponse to skip invocation of NodeExpandVolume on the node by CO.

ddebroy · 2019-10-18T01:27:44Z

spec.md

@@ -1805,6 +1805,8 @@ This RPC allows the CO to expand the size of a volume.
 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.
 If plugin has `EXPAND_VOLUME` node capability, then `NodeExpandVolume` MUST be called after successful `ControllerExpandVolume` and `node_expansion_required` in `ControllerExpandVolumeResponse` is `true`.

+If specified the `volume_capability` in `ControllerExpandVolumeRequest` should be same as what CO would pass in `ControllerPublishVolumeRequest`.


nit: If specified, the ...

gnufied · 2019-10-18T13:38:13Z

@ddebroy I have reworded the comment for NodeExpandVolume to provide more context and capture #381 (comment) PTAL

ddebroy · 2019-10-18T15:40:01Z

Thanks!
/lgtm

Update specs and add new error code for invalid capabilities

saad-ali · 2019-10-18T18:45:28Z

Merging

gnufied force-pushed the add-volume-capability-to-expansion-calls branch from f16612f to 318163d Compare August 15, 2019 15:58

wongma7 mentioned this pull request Aug 15, 2019

Fix raw block volume resizing for EBS kubernetes-sigs/aws-ebs-csi-driver#346

Closed

wongma7 reviewed Aug 15, 2019

View reviewed changes

spec.md Outdated Show resolved Hide resolved

gnufied mentioned this pull request Aug 15, 2019

Enables resizing of block volumes. kubernetes/kubernetes#81429

Merged

leakingtapan reviewed Aug 15, 2019

View reviewed changes

davidz627 reviewed Aug 15, 2019

View reviewed changes

saad-ali reviewed Aug 21, 2019

View reviewed changes

gnufied force-pushed the add-volume-capability-to-expansion-calls branch from d2a85cb to 96dc029 Compare October 10, 2019 20:32

davidz627 reviewed Oct 16, 2019

View reviewed changes

gnufied force-pushed the add-volume-capability-to-expansion-calls branch from 3241315 to e4b23dc Compare October 16, 2019 20:06

gnufied force-pushed the add-volume-capability-to-expansion-calls branch from e4b23dc to 683b6cc Compare October 16, 2019 20:15

ddebroy reviewed Oct 18, 2019

View reviewed changes

Add volume capability to volume expansion calls

6c94335

Update specs and add new error code for invalid capabilities

gnufied force-pushed the add-volume-capability-to-expansion-calls branch from ab5b53a to 6c94335 Compare October 18, 2019 18:39

saad-ali merged commit ae4d010 into container-storage-interface:master Oct 18, 2019

frittentheke mentioned this pull request Oct 20, 2019

Switch to having Kubernetes do (EBS) volume management / resizing zalando/postgres-operator#460

Closed

davidz627 mentioned this pull request Dec 18, 2019

NodeExpand may resize fs for block volume kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#372

Closed

Add volume capability to volume expansion calls #381

Add volume capability to volume expansion calls #381

Uh oh!

Conversation

gnufied commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jdef commented Aug 15, 2019

Uh oh!

gnufied commented Aug 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saad-ali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Oct 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidz627 Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saad-ali commented Oct 16, 2019

Uh oh!

gnufied commented Oct 16, 2019

Uh oh!

saad-ali commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddebroy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Oct 18, 2019

Uh oh!

ddebroy commented Oct 18, 2019

Uh oh!

saad-ali commented Oct 18, 2019

Uh oh!

Uh oh!

gnufied commented Aug 14, 2019 •

edited

Loading

gnufied Aug 16, 2019 •

edited

Loading

davidz627 Oct 16, 2019 •

edited

Loading

saad-ali commented Oct 18, 2019 •

edited

Loading

ddebroy left a comment •

edited

Loading