New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flexvolume resize implementation #67851

Merged
merged 4 commits into from Nov 2, 2018

Conversation

@aniket-s-kulkarni
Contributor

aniket-s-kulkarni commented Aug 25, 2018

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.

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

This comment has been minimized.

@neolit123

neolit123 Aug 27, 2018

Member

...of plugin %q...

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

This comment has been minimized.

@neolit123

neolit123 Aug 27, 2018

Member

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.

// 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)

This comment has been minimized.

@neolit123

neolit123 Aug 27, 2018

Member

should this be without glog.V(x)?

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

This comment has been minimized.

@neolit123

neolit123 Aug 27, 2018

Member

please remove this empty line.

if err != nil {
return nil, err
}

This comment has been minimized.

@neolit123

neolit123 Aug 27, 2018

Member

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

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Aug 27, 2018

@neolit123 I made the suggested changes.

@brahmaroutu brahmaroutu referenced this pull request Aug 27, 2018

Closed

Flexvolume resize #67666

@brahmaroutu

This comment has been minimized.

Contributor

brahmaroutu commented Aug 27, 2018

@dims

This comment has been minimized.

Member

dims commented Aug 27, 2018

/ok-to-test

@neolit123

This comment has been minimized.

Member

neolit123 commented Aug 27, 2018

/uncc

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Aug 27, 2018

/ok-to-test

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Aug 27, 2018

/retest

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

This comment has been minimized.

@linyouchong

linyouchong Aug 28, 2018

Member

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.

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Aug 28, 2018

Contributor

@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.

This comment has been minimized.

@gnufied

gnufied Aug 29, 2018

Member

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?

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Aug 29, 2018

Contributor

@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 -

  1. 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
  2. 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.

This comment has been minimized.

@gnufied

gnufied Sep 1, 2018

Member

@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.

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 1, 2018

Contributor

@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?

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Sep 1, 2018

Contributor

@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?

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 1, 2018

Contributor

@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

// 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) {

This comment has been minimized.

@linyouchong

linyouchong Aug 28, 2018

Member

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

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Aug 28, 2018

Contributor

sounds good.

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Aug 28, 2018

/retest

@@ -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 {

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Aug 28, 2018

Contributor

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

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Aug 29, 2018

Contributor

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

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

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Aug 28, 2018

Contributor

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

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 2, 2018

Contributor

@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.

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Sep 3, 2018

Contributor

@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.

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 4, 2018

Contributor

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

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

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Aug 28, 2018

Contributor

Let us also add device path for this call.

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 2, 2018

Contributor

Will do

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)

This comment has been minimized.

@gnufied

gnufied Aug 29, 2018

Member

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

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Aug 29, 2018

Contributor

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

This comment has been minimized.

@chakri-nelluri

chakri-nelluri Sep 3, 2018

Contributor

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

This comment has been minimized.

@aniket-s-kulkarni

aniket-s-kulkarni Sep 4, 2018

Contributor

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

This comment has been minimized.

@gnufied

gnufied Sep 4, 2018

Member

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

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

This comment has been minimized.

@gnufied

gnufied Aug 29, 2018

Member

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

This comment has been minimized.

Contributor

chakri-nelluri commented Sep 1, 2018

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

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Sep 2, 2018

@chakri-nelluri I have addressed your comments

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Sep 2, 2018

/retest

@chakri-nelluri

This comment has been minimized.

Contributor

chakri-nelluri commented Oct 24, 2018

/LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 24, 2018

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

This comment has been minimized.

@gnufied

gnufied Oct 25, 2018

Member

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

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 25, 2018

@chakri-nelluri

This comment has been minimized.

Contributor

chakri-nelluri commented Oct 25, 2018

/LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 25, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 25, 2018

@gnufied

This comment has been minimized.

Member

gnufied commented Oct 25, 2018

/approve

@gnufied

This comment has been minimized.

Member

gnufied commented Oct 25, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 25, 2018

@aniket-s-kulkarni

This comment has been minimized.

Contributor

aniket-s-kulkarni commented Oct 25, 2018

/assign @saad-ali

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

@chakri-nelluri

This comment has been minimized.

Contributor

chakri-nelluri commented Oct 25, 2018

/LGTM

@gnufied

This comment has been minimized.

Member

gnufied commented Oct 25, 2018

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

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

This comment has been minimized.

@gnufied

gnufied Oct 25, 2018

Member

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.

This comment has been minimized.

@gnufied

gnufied Oct 25, 2018

Member

nvm. looks like it works as expected. thanks.

@gnufied

This comment has been minimized.

Member

gnufied commented Oct 25, 2018

Putting the PR on hold for a bit.
/hold

@gnufied

This comment has been minimized.

Member

gnufied commented Oct 25, 2018

/hold cancel

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 2, 2018

[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:

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 k8s-ci-robot merged commit 6813ebb into kubernetes:master Nov 2, 2018

18 checks passed

cla/linuxfoundation aniket-s-kulkarni authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment