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

Add support to resize Portworx volume #62308

Merged
merged 1 commit into from Apr 19, 2018

Conversation

@harsh-px
Contributor

harsh-px commented Apr 10, 2018

Signed-off-by: Harsh Desai harsh@portworx.com

What this PR does / why we need it:

This PR adds support in the Portworx volume plugin to expand an existing PVC.

Which issue(s) this PR fixes:

Closes #62305

Release note:

Add support to resize Portworx volumes.
@lpabon

lpabon approved these changes Apr 10, 2018

LGTM

spec *volume.Spec,
newSize resource.Quantity,
oldSize resource.Quantity) (resource.Quantity, error) {
glog.Infof("Expanding: %s from %v to %v", spec.Name(), oldSize, newSize)

This comment has been minimized.

@dixudx

dixudx Apr 10, 2018

Member

using level 4 log?

This comment has been minimized.

@harsh-px

harsh-px Apr 12, 2018

Contributor

Switched to level 4 logging.

return oldSize, err
}
glog.Infof("Successfully resized %s to %v", spec.Name(), newSize)

This comment has been minimized.

@dixudx

dixudx Apr 10, 2018

Member

Ditto.

This comment has been minimized.

@harsh-px

harsh-px Apr 12, 2018

Contributor

Fixed in latest incremental

@@ -38,6 +41,7 @@ const (
pvcNamespaceLabel = "namespace"
pxServiceName = "portworx-service"
pxDriverName = "pxd-sched"
oneGig = 1024 * 1024 * 1024

This comment has been minimized.

@dixudx

dixudx Apr 10, 2018

Member

use oneGi instead?

This comment has been minimized.

@harsh-px
return fmt.Errorf("failed to inspect Portworx volume: %s. Found: %d volumes", spec.Name(), len(vols))
}
vol := vols[0]

This comment has been minimized.

@dixudx

dixudx Apr 10, 2018

Member

only the first volume will be resized?

This comment has been minimized.

@harsh-px

harsh-px Apr 12, 2018

Contributor

Yes, because we inspected one volume above. So only one volume is expected is the response. Line 200 also explicitly makes that check. ResizeVolume is a per-volume call.

func capacityToGB(capacity resource.Quantity) int {
return int(volutil.RoundUpSize(capacity.Value(), oneGig))
}

This comment has been minimized.

@gnufied

gnufied Apr 10, 2018

Member

there is already https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/util.go#L372

This comment has been minimized.

@harsh-px

harsh-px Apr 12, 2018

Contributor

Thanks. Fixed.

@gnufied

This comment has been minimized.

Member

gnufied commented Apr 10, 2018

/ok-to-test

@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 12, 2018

@dixudx / @gnufied I have fixed the review comments and the tests are passing.

@dixudx

This comment has been minimized.

Member

dixudx commented Apr 13, 2018

@harsh-px Would you please rebase your commits. Thanks.

I'll lgtm.

@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 13, 2018

@dixudx done.

@dixudx

This comment has been minimized.

Member

dixudx commented Apr 16, 2018

/lgtm

ping @msau42 @jsafrane for approval. Thanks.
/cc @kubernetes/sig-storage-api-reviews

@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 16, 2018

@deads2k and @gnufied can you approve?

@@ -56,7 +59,7 @@ func (util *PortworxVolumeUtil) CreateVolume(p *portworxVolumeProvisioner) (stri
capacity := p.options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)]
// Portworx Volumes are specified in GB

This comment has been minimized.

@gnufied

gnufied Apr 17, 2018

Member

Why does this comment says Portworx volumes are specified in GB and then code proceeds to use GiB ?

This comment has been minimized.

@harsh-px

harsh-px Apr 18, 2018

Contributor

Fixed in latest incremental.

newSize resource.Quantity,
oldSize resource.Quantity) (resource.Quantity, error) {
glog.V(4).Infof("Expanding: %s from %v to %v", spec.Name(), oldSize, newSize)
err := plugin.util.ResizeVolume(spec, newSize, plugin.host)

This comment has been minimized.

@gnufied

gnufied Apr 17, 2018

Member

This function should be idempotent. If portworx volume already has the user requested size, ExpandVolumeDevice should be a no-op followed by a successful response. Please see other volume plugins as an example.

This comment has been minimized.

@harsh-px

harsh-px Apr 18, 2018

Contributor

@gnufied the Portworx driver implementation on the server side is already idempotent. So invoking it multiple times will have the same effect as it internally checks if the volume has the size. So I don't need any logic here on the client side. Best to keep it simple.

This comment has been minimized.

@gnufied

gnufied Apr 19, 2018

Member

There is another reason for keeping this idempotent in Kubernetes rather than in Portworx server side. Lets say:

  1. User created a PVC of 5GB size and is matched with a PV which is of 7GiB size.
  2. Later on User edits the PVC to be 6GiB. Kuberenetes may still call ExpandVolumeDevice even though underlying PV already matches the requested size.
  3. What happens in portworx server side now? Will it shrink existing 7GiB disk to 6GiB or will it return an error?

Obviously shrinking will be bad. We never shrink volumes in Kubernetes. Returning an error will be bad too. Even if theoretically Portworx in Server side does the "right thing" and returns successful response - there is no guarantee that the behaviour will remain the same always. That is why - it is important to not rely on Portworx behaviour in server side.

This comment has been minimized.

@harsh-px

harsh-px Apr 19, 2018

Contributor

@gnufied good point ! Updated the review with check to see if existing volume is already equal or greater than requested size.

vol := vols[0]
vol.Spec.Size = uint64(volutil.RoundUpToGiB(newSize))
return driver.Set(spec.Name(), vol.Locator, vol.Spec)

This comment has been minimized.

@gnufied

gnufied Apr 17, 2018

Member

Is there a way to get size after resizing? The reason most volume plugins returns "new size" from this function is because - after alignment to GB or GiB boundaries, actual volume size may be different than user requested size(but as long as it is more than user requested size we are fine)

This comment has been minimized.

@harsh-px

harsh-px Apr 18, 2018

Contributor

Done. Added check in the latest incremental if the size actually got updated.

@gnufied

This comment has been minimized.

Member

gnufied commented Apr 17, 2018

Nearly there. Please make sure plugin uses one unit throughout the code (GB or GiB). Also - ideally ExpandVolumeDevice should be idempotent, if somehow we can't do that for portworx then the idempotency should be handled by portworx itself. Under no circumstance - if user requested 15GB space and volume is of >=15GB size, an error should be thrown from ExpandVolumeDevice function.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 17, 2018

}
updatedVol := vols[0]
if updatedVol.Spec.Size != vol.Spec.Size {

This comment has been minimized.

@gnufied

gnufied Apr 19, 2018

Member

I don't think we need to check exact equality, may be just updatedVol.Size >= vol.Size should be sufficient?

This comment has been minimized.

@harsh-px

harsh-px Apr 19, 2018

Contributor

Done. Now I'm allowing the updated volume to be greater than the requested size.

@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 19, 2018

/retest

@gnufied

This comment has been minimized.

Member

gnufied commented Apr 19, 2018

@harsh-px please squash the commits.

Add support to resize Portworx volume
Closes #62305

Signed-off-by: Harsh Desai <harsh@portworx.com>

update comment and variable references to GiB

Signed-off-by: Harsh Desai <harsh@portworx.com>

explicitly check volume size after resize and fix size volume spec

Signed-off-by: Harsh Desai <harsh@portworx.com>

If Portworx volume is already greater than new size, skip resize

Signed-off-by: Harsh Desai <harsh@portworx.com>

Allow updated volume to be greater than requested size

Signed-off-by: Harsh Desai <harsh@portworx.com>
@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 19, 2018

@gnufied Squashed the changes. It seems the tests for failing due to an automation issue.

ERROR: (gcloud.auth.activate-service-account) There was a problem refreshing your current auth tokens: Unable to find the server at accounts.google.com

I'm going to retry these.

/retest

@harsh-px

This comment has been minimized.

Contributor

harsh-px commented Apr 19, 2018

/retest

@gnufied

This comment has been minimized.

Member

gnufied commented Apr 19, 2018

Looks good to me. Assinging @deads2k for approval

/lgtm
/assign @deads2k

@deads2k

This comment has been minimized.

Contributor

deads2k commented Apr 19, 2018

/approve

approving the admission changes.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, dixudx, gnufied, harsh-px

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 19, 2018

Automatic merge from submit-queue (batch tested with PRs 59592, 62308, 62523, 62635, 62243). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 607edb8 into kubernetes:master Apr 19, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation harsh-px 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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce 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
@gnufied

This comment has been minimized.

Member

gnufied commented Jul 10, 2018

@harsh-px I was just reviewing some of the stuff for a blog post and noticed that Portworx still returns false from RequiresFSReisze function. Is that an oversight? Does this needs fixing. I don't know much about portworx.. cc @lpabon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment