Flexvolume resize implementation #67851

aniket-s-kulkarni · 2018-08-25T00:03:49Z

What this PR does / why we need it

This PR adds support for expanding Flex Persistent Volumes - kubernetes/enhancements#304

This PR allows the expansion code flow to execute on the kubelet for flex volumes rather than the controller as the flex plugins are not installed on the controller.
It allows the flex volume plugin to implement ExpandVolumeDevice method as well as a new ExpandFS method to allow for expanding in use Flex PersistentVolumes
Includes e2e tests

Flex volume plugins now support expandvolume (to increase underlying volume capacity) and expanfs (resize filesystem) commands that Flex plugin authors can implement to support expanding in use Flex PersistentVolumes

/sig storage
/assign @gnufied @chakri-nelluri

neolit123

@aniket-s-kulkarni
thank you for your PR!
added a couple of small comments about formatting.

neolit123 · 2018-08-27T13:34:17Z

pkg/volume/util/operationexecutor/operation_generator.go

+					pvcWithResizeRequest.CurrentSize)
+
+				if expandErr != nil {
+					detailedErr := fmt.Errorf("error expanding volume %q of plugin %s: %v", pvcWithResizeRequest.QualifiedName(), volumePlugin.GetPluginName(), expandErr)


...of plugin %q...

neolit123 · 2018-08-27T13:35:43Z

pkg/controller/volume/expand/expand_controller.go

+		if volumeSpec := volume.NewSpecFromPersistentVolume(pv, false); volumeSpec.PersistentVolume.Spec.FlexVolume == nil {
+			// A flex volume is expandable but since it resides on the kubelet not the controller
+			// and is dynamically probed plugin, FindExpandablePluginBySpec will never find it
+			// Therefore explicitly allow flex volumes to resize


please add punctuation in the comment.

...find it.
Therefore explicitly allow flex volumes to resize.

the comment seems to repeat multiple times in the diff.

neolit123 · 2018-08-27T13:36:22Z

pkg/controller/volume/expand/expand_controller.go

+			// and is dynamically probed plugin, FindExpandablePluginBySpec will never find it
+			// Therefore explicitly allow flex volumes to resize
+			volumePlugin, err := expc.volumePluginMgr.FindExpandablePluginBySpec(volumeSpec)
+			glog.Warningf("plugin=%v err=%v fv=%v", volumePlugin != nil, err, volumeSpec.PersistentVolume.Spec.FlexVolume)


should this be without glog.V(x)?

neolit123 · 2018-08-27T13:37:00Z

pkg/volume/flexvolume/plugin.go

+	call.Append(deviceMountPath)
+
+	_, err := call.Run()
+


please remove this empty line.

neolit123 · 2018-08-27T13:38:21Z

pkg/volume/plugins.go

+	if err != nil {
+		return nil, err
+	}
+


best to trim some of the empty lines that follow a line that only has }

aniket-s-kulkarni · 2018-08-27T14:58:36Z

@neolit123 I made the suggested changes.

brahmaroutu · 2018-08-27T17:15:48Z

Docs: kubernetes/website#10097
@zparnold

dims · 2018-08-27T17:32:50Z

/ok-to-test

neolit123 · 2018-08-27T17:48:58Z

/uncc

aniket-s-kulkarni · 2018-08-27T20:37:07Z

/ok-to-test

aniket-s-kulkarni · 2018-08-27T20:42:56Z

/retest

linyouchong · 2018-08-28T03:13:17Z

pkg/controller/volume/expand/expand_controller.go

-				util.GetPersistentVolumeClaimQualifiedName(newPVC), newPVC.UID, err)
-			return
+		if volumeSpec := volume.NewSpecFromPersistentVolume(pv, false); volumeSpec.PersistentVolume.Spec.FlexVolume == nil {
+			// A flex volume is expandable but since it resides on the kubelet not the controller


the volume driver shall be installed on master node.
According to https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/grow-flexvolume-size.md#expandfs

If flex volume driver supports resizing, it shall implement expandvolume method and at least, the volume driver shall be installed on master node.

@linyouchong after discussions with @gnufied and @chakri-nelluri, installing the flex driver on the master didn't seem to be ideal. Maybe we need to update that point in the design document - will do so.

Meanwhile, I do realize that if the flex driver doesn't support expansion and it does return an error via ExpandVolumeDevice- the faked expansion status on the PV and the PVC condition isn't cleared. I will fix that up so that its in line with the design.

Did we finalize on that? I don't particularly like special casing here for flex. Can we not make it a hard requirement that for flex volume resizing the plugin must be at least installed on the controller so as controller is aware of plugin capabilities before allowing volume expansion?

@gnufied I was under the impression there was discussion around not make that (installing flex plugin on master) a hard requirement. Maybe we need to revisit it?

If we do make it a hard requirement, it would lead to issues for certain uses cases. Esp. for "managed Kubernetes service" like Amazon EKS or IBM CKS or AKS - in each cases the control plane management is hidden from the end user making it tough to install the flex driver on the master.

How about an alternative -

The volume-expand controller sets the PVC condition to ExternalResizing instead of Resizing for plugins (only Flex today) that don't reside on the master/controller

The kubelet reconciler calls ExpandVolumeDevice of the plugin (Flex) for such volumes and then

if it succeeds the condition is cleared prompting the volume-expand controller to finish and update PV and move onto fs resizing as it does today

if it fails the condition.Reason set to Error and the .Message has the actual reason; this prompts the volume-expand to abort resizing with appropriate error

This kind of controller <-> kubelet feedback loop is necessary since only the controller is guaranteed to have privileges to update PV, the kubelet may not be able to do it.

@saad-ali @chakri-nelluri what you guys think about this? I think that - controller to kubelet and then back to controller feedback loop is problematic. Expand Controller must reprocess such PVCs.

@saad-ali @chakri-nelluri - @gnufied and I were discussing this on slack, and there maybe a simpler way.

This discussion got underway because the design mentioned the need to install Flex on the controller so that a Flex author can signal expansion is unsupported by returning an error from ExpandVolumeDevice.

Since only dynamically provisioned volumes with allowVolumeExpansion = true in their storage classes are allowed to be expanded, a Flex plugin that doesn't support expansion can simply set allowVolumeExpansion = false and we don't need to come up with a complicated flow to handle it.

So is that a good enough option?

@aniket-s-kulkarni if the plugin is not installed on master, assume it to be true and mark the volume "RequiresFSResize" and let the Kubelet side code manage it. Let me know if you see any issues with it?

@chakri-nelluri you are right. That works - and that's how the PR is doing it. And with what @gnufied mentioned above - it covers all the cases. I will address your comment about abstracting the .FlexVolume != nil checks

linyouchong · 2018-08-28T03:15:51Z

pkg/controller/volume/expand/expand_controller.go

+			// Therefore explicitly allow flex volumes to resize
+			volumePlugin, err := expc.volumePluginMgr.FindExpandablePluginBySpec(volumeSpec)
+			glog.V(4).Infof("plugin=%v err=%v fv=%v", volumePlugin != nil, err, volumeSpec.PersistentVolume.Spec.FlexVolume)
+			if err != nil || (volumePlugin == nil && volumeSpec.PersistentVolume.Spec.FlexVolume == nil) {


volumeSpec.PersistentVolume.Spec.FlexVolume == nil always be true at this line, should delete it.

sounds good.

aniket-s-kulkarni · 2018-08-28T15:28:44Z

/retest

chakri-nelluri · 2018-08-28T21:01:30Z

pkg/volume/util/operationexecutor/operation_generator.go

@@ -1265,26 +1281,54 @@ func (og *operationGenerator) GenerateExpandVolumeFunc(

 	volumePlugin, err := og.volumePluginMgr.FindExpandablePluginBySpec(volumeSpec)

-	if err != nil {
+	if err != nil && volumeSpec.PersistentVolume.Spec.FlexVolume == nil {


Is there a way we can abstract Flexvolume type here? We should not bring knowledge of Flexvolume into operationexcutor

@chakri-nelluri yes, maybe we can do some abstraction there. Let me look into it.

pkg/volume/util/operationexecutor/operation_generator.go

chakri-nelluri · 2018-08-28T21:12:27Z

pkg/volume/flexvolume/plugin.go

+	call := plugin.NewDriverCall(expandFSCmd)
+	call.AppendSpec(spec, plugin.host, nil)
+	call.Append(devicePath)
+	call.Append(deviceMountPath)


We need the new size of the volume. Some plugins might need this information.

@chakri-nelluri do we need the new size? The size has already been passed in the prior call (ExpandVolumeDevice) and has been acted on by the plugin.

@aniket-s-kulkarni Yes, there can be plugins which can support FS resize, but have no corresponding block resize calls. They expand dynamically and can ignore previous device resize call.

Ok I can look up the size from spec.PersistentVolume.Status.Capacity and add that in here for the call.

chakri-nelluri · 2018-08-28T21:31:43Z

pkg/volume/flexvolume/plugin.go

+	call.AppendSpec(spec, plugin.host, nil)
+	call.Append(strconv.FormatInt(newSize.Value(), 10))
+	call.Append(strconv.FormatInt(oldSize.Value(), 10))
+


Let us also add device path for this call.

gnufied · 2018-08-29T01:42:45Z

pkg/volume/util/operationexecutor/operation_generator.go

+			if fsResizablePlugin, callExpandFs := expandableVolumePlugin.(volume.FSResizableVolumePlugin); callExpandFs {
+				// special case for flex volumes
+				if _, resizeErr = fsResizablePlugin.ExpandVolumeDevice(volumeToMount.VolumeSpec, pvSpecCap, pvcStatusCap); resizeErr == nil {
+					resizeErr = fsResizablePlugin.ExpandFS(volumeToMount.VolumeSpec, devicePath, deviceMountPath)


ExpandFS call should be extended to all volume plugins that require file system resize rather than special casing this for Flex.

@gnufied so you would like to see all existing plugins have a default ExpandFS implementation that calls the existing resize FS ?

@aniket-s-kulkarni Yes this might be required for all plugins going forward.

OK so we go ahead and add this to all existing plugins.

yeah. Also all the existing plugins will simply call out to existing util resize function.

gnufied · 2018-08-29T01:44:47Z

pkg/controller/volume/expand/expand_controller.go

-				util.GetPersistentVolumeClaimQualifiedName(newPVC), newPVC.UID, err)
-			return
+		if volumeSpec := volume.NewSpecFromPersistentVolume(pv, false); volumeSpec.PersistentVolume.Spec.FlexVolume == nil {
+			// A flex volume is expandable but since it resides on the kubelet not the controller


Did we finalize on that? I don't particularly like special casing here for flex. Can we not make it a hard requirement that for flex volume resizing the plugin must be at least installed on the controller so as controller is aware of plugin capabilities before allowing volume expansion?

chakri-nelluri · 2018-09-01T14:01:51Z

@aniket-s-kulkarni The code freeze is very close. Ping me on slack if you need a quick review.

chakri-nelluri · 2018-10-24T19:37:35Z

/LGTM

gnufied · 2018-10-25T16:32:01Z

test/e2e/storage/flexvolume_resize.go

+	return stKlass, err
+}
+
+var _ = utils.SIGDescribe("Mounted flexvolume volume expand [Slow] [Feature:ExpandInUsePersistentVolumes]", func() {


This e2e should not have the feature tag since this feature does not tie itself to ONLINE resizing.

chakri-nelluri · 2018-10-25T18:36:57Z

/LGTM

gnufied · 2018-10-25T20:00:03Z

/approve

gnufied · 2018-10-25T20:00:10Z

/lgtm

aniket-s-kulkarni · 2018-10-25T20:03:53Z

/assign @saad-ali

@saad-ali assigning you as approver, since this has a test case change that needs approval.

chakri-nelluri · 2018-10-25T20:05:50Z

/LGTM

gnufied · 2018-10-25T20:10:33Z

Please don't forget to open follow up PR that adds e2e that does not depend on online resizing feature.

gnufied · 2018-10-25T20:15:06Z

pkg/volume/flexvolume/expander.go

+	"strconv"
+)
+
+func (plugin *flexVolumePlugin) ExpandVolumeDevice(spec *volume.Spec, newSize resource.Quantity, oldSize resource.Quantity) (resource.Quantity, error) {


Will this implementation EVER get called? It looks like, even if flex volume plugin is installed on the controller, the code still returns a no-op version of flex plugin.

nvm. looks like it works as expected. thanks.

gnufied · 2018-10-25T20:15:57Z

Putting the PR on hold for a bit.
/hold

gnufied · 2018-10-25T20:18:54Z

/hold cancel

saad-ali

/approve

k8s-ci-robot · 2018-11-02T16:29:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aniket-s-kulkarni, gnufied, saad-ali

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:

~~pkg/controller/volume/expand/OWNERS~~ [gnufied,saad-ali]
~~pkg/volume/OWNERS~~ [gnufied,saad-ali]
~~test/OWNERS~~ [saad-ali]

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

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 25, 2018

k8s-ci-robot assigned chakri-nelluri and gnufied Aug 25, 2018

k8s-ci-robot requested review from gnufied and ivan4th August 25, 2018 00:04

aniket-s-kulkarni mentioned this pull request Aug 26, 2018

Add resize support for FlexVolume kubernetes/enhancements#304

Closed

neolit123 reviewed Aug 27, 2018

View reviewed changes

brahmaroutu mentioned this pull request Aug 27, 2018

Flexvolume resize #67666

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 27, 2018

linyouchong reviewed Aug 28, 2018

View reviewed changes

aniket-s-kulkarni force-pushed the flexvolume-resize-implementation branch from 2109a87 to ce8d3f0 Compare August 28, 2018 15:26

chakri-nelluri reviewed Aug 28, 2018

View reviewed changes

pkg/volume/util/operationexecutor/operation_generator.go Outdated Show resolved Hide resolved

chakri-nelluri reviewed Aug 28, 2018

View reviewed changes

gnufied reviewed Aug 29, 2018

View reviewed changes

aniket-s-kulkarni force-pushed the flexvolume-resize-implementation branch from ce8d3f0 to 848f7e7 Compare September 2, 2018 04:46

aniket-s-kulkarni force-pushed the flexvolume-resize-implementation branch from cb8ae8b to bfc0d45 Compare October 24, 2018 19:31

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2018

gnufied reviewed Oct 25, 2018

View reviewed changes

Incorporating review comments

45aec04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2018

Updating build files after review comments

82f83a8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2018

k8s-ci-robot assigned saad-ali Oct 25, 2018

gnufied reviewed Oct 25, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2018

saad-ali reviewed Nov 2, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2018

k8s-ci-robot merged commit 6813ebb into kubernetes:master Nov 2, 2018

This was referenced Nov 5, 2018

adding test case for flex volume resize on pod restart #70667

Merged

Add "ExpandFS" interface to "ExpandableVolumePlugin" #58786

Closed

This was referenced Aug 21, 2024

remove unneeded permissions for volume controllers #125995

Merged

Volume expand controller doesn't have a support for the flexvolume plugin #126881

Open

Flexvolume resize implementation #67851

Flexvolume resize implementation #67851

Conversation

aniket-s-kulkarni commented Aug 25, 2018

neolit123 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

aniket-s-kulkarni commented Aug 27, 2018

brahmaroutu commented Aug 27, 2018

dims commented Aug 27, 2018

neolit123 commented Aug 27, 2018

aniket-s-kulkarni commented Aug 27, 2018

aniket-s-kulkarni commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aniket-s-kulkarni Aug 29, 2018 • 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

linyouchong Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aniket-s-kulkarni commented Aug 28, 2018

chakri-nelluri Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakri-nelluri Aug 28, 2018 • 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

aniket-s-kulkarni Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakri-nelluri Sep 3, 2018 • 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

chakri-nelluri commented Sep 1, 2018

chakri-nelluri commented Oct 24, 2018

Choose a reason for hiding this comment

chakri-nelluri commented Oct 25, 2018

gnufied commented Oct 25, 2018

gnufied commented Oct 25, 2018

aniket-s-kulkarni commented Oct 25, 2018

chakri-nelluri commented Oct 25, 2018

gnufied commented Oct 25, 2018

gnufied Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Oct 25, 2018

gnufied commented Oct 25, 2018

saad-ali left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 2, 2018

aniket-s-kulkarni Aug 29, 2018 •

edited

Loading

linyouchong Aug 28, 2018 •

edited

Loading

chakri-nelluri Aug 28, 2018 •

edited

Loading

chakri-nelluri Aug 28, 2018 •

edited

Loading

aniket-s-kulkarni Sep 2, 2018 •

edited

Loading

chakri-nelluri Sep 3, 2018 •

edited

Loading

gnufied Oct 25, 2018 •

edited

Loading