[manila-csi-plugin] Add support for online volume expansion #1579

gman0 · 2021-06-25T12:14:27Z

What this PR does / why we need it:

This PR implements online EXPAND_VOLUME controller capability.

Which issue this PR fixes(if applicable):
fixes #1475

Special notes for reviewers:

Release note:

[manila-csi-plugin] Add support for online volume expansion

gman0 · 2021-06-25T13:16:54Z

PTAL @gouthampacha @tombarron @ramineni

gouthampacha

/ok-to-test

pkg/csi/manila/controllerserver.go

gman0 · 2021-06-28T09:05:40Z

@lingxiankong is the CI disabled? No acceptance tests were run.

lingxiankong · 2021-06-28T10:25:09Z

/retest

lingxiankong · 2021-06-28T10:25:34Z

@lingxiankong is the CI disabled? No acceptance tests were run.

No idea, I checked the http://status.openlabtesting.org/status, seems fine

lingxiankong · 2021-06-28T10:26:17Z

Maybe it was restarted, now I see the job is running.

theopenlab-ci · 2021-06-28T10:29:21Z

Build succeeded.

cloud-provider-openstack-unittest : SUCCESS in 3m 12s

theopenlab-ci · 2021-06-28T10:30:20Z

Build succeeded.

cloud-provider-openstack-format : SUCCESS in 3m 22s

gman0 · 2021-06-28T10:37:00Z

Weird... Thank you @lingxiankong !

theopenlab-ci · 2021-06-28T11:49:16Z

Build succeeded.

cloud-provider-openstack-acceptance-test-csi-manila : SUCCESS in 1h 21m 47s

gouthampacha

/lgtm

charts/manila-csi-plugin/templates/controllerplugin-statefulset.yaml

ramineni · 2021-06-30T05:34:59Z

pkg/csi/manila/controllerserver.go

+		// Share is already larger than requested size
+
+		return &csi.ControllerExpandVolumeResponse{
+			CapacityBytes: currentSizeInBytes,


Isnt it better to add NodeExpansionRequired: false, if node expansion not required? It says required field in csi spec.

// Whether node expansion is required for the volume. When true // the CO MUST make NodeExpandVolume RPC call on the node. This field // is REQUIRED. bool node_expansion_required = 2;

NodeExpansionRequired bool is initialized to false by default

ok fine then

ramineni · 2021-06-30T05:35:31Z

pkg/csi/manila/controllerserver.go

+	}
+
+	return &csi.ControllerExpandVolumeResponse{
+		CapacityBytes: int64(desiredSizeInGiB * bytesInGiB),


ramineni · 2021-06-30T05:36:46Z

pkg/csi/manila/controllerserver.go

+	if _, isPending := pendingVolumes.LoadOrStore(share.Name, true); isPending {
+		return nil, status.Errorf(codes.Aborted, "volume named %s is already being processed", share.Name)
+	}
+	defer pendingVolumes.Delete(share.Name)


@gman0 Just wondering here, why we need to delete volumes here if some operation is in progress ?

pendingVolumes is a sync.Map. It acts like a try-lock: it tries to lock a resource (with share name as the key), protecting it against concurrent use. The Delete() just deletes a key from the map, freeing the lock for that volume.

Ah, got it. Thanks

ramineni · 2021-06-30T05:39:17Z

pkg/csi/manila/identityserver.go

+		{
+			Type: &csi.PluginCapability_VolumeExpansion_{
+				VolumeExpansion: &csi.PluginCapability_VolumeExpansion{
+					Type: csi.PluginCapability_VolumeExpansion_ONLINE,


@gman0 Is OFFLINE capability not applicable for manila csi?

I think VolumeExpansion.OFFLINE and VolumeExpansion.ONLINE pertain to what happens when the volume is in use while its being expanded, rather than if it's allowed to be online/offline.

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.
https://github.com/container-storage-interface/spec/blob/master/spec.md#controllerexpandvolume

Manila doesn't care whether a share is in use or not, so I think advertising online expansion only is enough. I asked at the CSI slack channel to make sure though.

I'll send a comment here once somebody replies.

Got a response: external-resizer doesn't check offline/online caps anyway, and they will be even deprecated from the CSI spec. See release notes at https://github.com/container-storage-interface/spec/releases/tag/v1.5.0

Agree that manila doesn't care if share is in use or not.

tombarron

This change seems good to me if ramineni's questions have been answered.

ramineni · 2021-07-01T05:14:34Z

Looks good to me

/approve

k8s-ci-robot · 2021-07-01T05:14:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ramineni

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ramineni]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gman0 · 2021-07-01T06:56:58Z

Thank you all for reviews! I'll follow up with pull requests like we agreed: consolidating log messages and lowering verbosity levels.

…es#1579) * implemented ControllerExpandVolume with online volume expansion support * updated CSI sanity tests * updated Helm chart * updated k8s manifests * updated docs * updated examples * docs: extend -> expand

gman0 added 6 commits June 25, 2021 14:07

implemented ControllerExpandVolume with online volume expansion support

ab360f1

updated CSI sanity tests

ea79928

updated Helm chart

063b151

updated k8s manifests

2b15d5e

updated docs

1f2c599

updated examples

393c4b7

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 25, 2021

k8s-ci-robot requested review from ramineni and tsmetana June 25, 2021 12:14

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2021

docs: extend -> expand

a289914

gouthampacha reviewed Jun 25, 2021

View reviewed changes

pkg/csi/manila/controllerserver.go Show resolved Hide resolved

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 25, 2021

gouthampacha approved these changes Jun 29, 2021

View reviewed changes

k8s-ci-robot assigned gouthampacha Jun 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2021

ramineni reviewed Jun 29, 2021

View reviewed changes

charts/manila-csi-plugin/templates/controllerplugin-statefulset.yaml Show resolved Hide resolved

ramineni reviewed Jun 30, 2021

View reviewed changes

tombarron approved these changes Jun 30, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2021

k8s-ci-robot merged commit 291d04b into kubernetes:master Jul 1, 2021

This was referenced Jul 2, 2021

[manila-csi-plugin] Consolidate log and error messages #1582

Merged

[manila-csi-plugin] Use low verbosity logging by default #1583

Merged

JPRbrs mentioned this pull request Nov 16, 2021

[manila-csi-plugin] Unable to resize volumes with CSI-NFS-driver and manila-CSI-plugin #1697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[manila-csi-plugin] Add support for online volume expansion #1579

[manila-csi-plugin] Add support for online volume expansion #1579

gman0 commented Jun 25, 2021

gman0 commented Jun 25, 2021

gouthampacha left a comment

gman0 commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

gman0 commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

gouthampacha left a comment

ramineni Jun 30, 2021 •

edited

gman0 Jun 30, 2021

ramineni Jul 1, 2021

ramineni Jun 30, 2021

ramineni Jun 30, 2021

gman0 Jun 30, 2021

ramineni Jul 1, 2021

ramineni Jun 30, 2021

gman0 Jun 30, 2021 •

edited

gman0 Jun 30, 2021

gman0 Jun 30, 2021

tombarron Jun 30, 2021

ramineni Jul 1, 2021

tombarron left a comment

ramineni commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021

gman0 commented Jul 1, 2021

[manila-csi-plugin] Add support for online volume expansion #1579

[manila-csi-plugin] Add support for online volume expansion #1579

Conversation

gman0 commented Jun 25, 2021

gman0 commented Jun 25, 2021

gouthampacha left a comment

Choose a reason for hiding this comment

gman0 commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

lingxiankong commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

gman0 commented Jun 28, 2021

theopenlab-ci bot commented Jun 28, 2021

gouthampacha left a comment

Choose a reason for hiding this comment

ramineni Jun 30, 2021 • edited

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

gman0 Jun 30, 2021 • edited

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

tombarron left a comment

Choose a reason for hiding this comment

ramineni commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021

gman0 commented Jul 1, 2021

ramineni Jun 30, 2021 •

edited

gman0 Jun 30, 2021 •

edited