fix: use pv annotation to trigger filesystem resize when necessary #99326

sunpa93 · 2021-02-23T00:16:23Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix: when newly binding pvc to a pv, adjust pvc.status.capacity to pv's annotation that denotes the pre-resize capacity of the original pvc that pv was bound to if it has one

test: confirm that pvc's status capacity is adjusted if pv has a pre-resize capacity annotation

Which issue(s) this PR fixes:

Fixes #88683

Special notes for your reviewer:

This PR is a sister PR of kubernetes-csi/external-resizer#140

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-02-23T00:16:30Z

Welcome @sunpa93!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-02-23T00:16:30Z

Hi @sunpa93. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gnufied · 2021-02-23T19:58:27Z

/ok-to-test

gnufied · 2021-02-23T19:58:37Z

/assign

sunpa93 · 2021-02-25T00:33:46Z

/retest

pkg/volume/util/resize_util.go

pkg/controller/volume/persistentvolume/BUILD

gnufied · 2021-03-01T19:49:57Z

I also want @msau42 and @jsafrane to take a look at the pv_controller change.

/assign @msau42 @jsafrane

pkg/controller/volume/persistentvolume/pv_controller.go

gnufied · 2021-03-05T20:37:41Z

We can do this in a follow up but I think we should be able to test this with e2e and mock driver.

Define mock driver to have offline only expansion capability.
Create a PVC and then expand it.
change reclaim policy on PV to retain and delete the PVC
bind the PVC again and create a pod and ensure that PVC reports accurate size after expansion.

sunpa93 · 2021-03-08T16:59:40Z

/test pull-kubernetes-unit

…'s annotation that denotes the pre-resize capacity of the original pvc that pv was bound to if it has one test: confirm that pvc's status capacity is adjusted if pv has a pre-resize capacity annotation

sunpa93 · 2021-03-09T11:24:23Z

/retest

gnufied · 2021-03-09T15:37:20Z

/priority important-soon
/triage accepted
/lgtm

jsafrane · 2021-03-09T15:38:23Z

/approve

k8s-ci-robot · 2021-03-09T15:38:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, sunpa93

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~~ [jsafrane]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [jsafrane]
~~pkg/volume/util/OWNERS~~ [jsafrane]

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

gnufied · 2021-03-09T16:14:32Z

/retest

msau42

Sorry for the delayed review. I started reviewing the external-resizer one and came to this for some background.

General question. This annotation is alpha. What are our future plans for this? In general we have tried to move away from using alpha annotations because it's difficult to enforce alpha-level guarantees around them.

msau42 · 2021-04-14T04:56:10Z

pkg/volume/util/resize_util.go

+	// AnnPreResizeCapacity annotation is added to a PV when expanding volume.
+	// Its value is status capacity of the PVC prior to the volume expansion
+	// Its value will be set by the external-resizer when it deems that filesystem resize is required after resizing volume.
+	// Its value will be used by pv_controller to determine pvc's status capacity when binding pvc and pv.


Can you add a comment when the annotation is supposed to be removed?

msau42 · 2021-04-14T04:57:22Z

pkg/volume/util/operationexecutor/operation_generator.go

@@ -1551,6 +1551,14 @@ func (og *operationGenerator) GenerateExpandVolumeFunc(
 				klog.Warning(detailedErr)
 				return volumetypes.NewOperationContext(nil, nil, migrated)
 			}
+			oldCapacity := pvc.Status.Capacity[v1.ResourceStorage]
+			err = util.AddAnnPreResizeCapacity(pv, oldCapacity, og.kubeClient)


Is there a race here? Should the annotation be added before marking for fsresize?

I can't think of scenarios where there can be a race from top of my head. Could you point to when race could happen?

k8s-ci-robot requested review from jingxu97 and saad-ali February 23, 2021 00:17

sunpa93 mentioned this pull request Feb 23, 2021

fix: use pv annotation to trigger filesystem resize when necessary kubernetes-csi/external-resizer#140

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2021

k8s-ci-robot assigned gnufied Feb 23, 2021

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 23, 2021

sunpa93 force-pushed the fs_resize_fix branch from aab9820 to b80b302 Compare February 23, 2021 23:43

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 23, 2021

sunpa93 force-pushed the fs_resize_fix branch 3 times, most recently from 401594b to cc29214 Compare February 24, 2021 01:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2021

gnufied reviewed Mar 1, 2021

View reviewed changes

pkg/volume/util/resize_util.go Show resolved Hide resolved

pkg/controller/volume/persistentvolume/BUILD Outdated Show resolved Hide resolved

k8s-ci-robot assigned jsafrane and msau42 Mar 1, 2021

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 1, 2021

sunpa93 force-pushed the fs_resize_fix branch from 8d5d9bf to 58785e9 Compare March 3, 2021 22:43

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2021

gnufied reviewed Mar 5, 2021

View reviewed changes

pkg/controller/volume/persistentvolume/pv_controller.go Show resolved Hide resolved

sunpa93 force-pushed the fs_resize_fix branch from 80e7173 to 5bf891e Compare March 7, 2021 23:06

fix: when newly binding pvc to a pv, adjust pvc.status.capacity to pv…

5f69cf7

…'s annotation that denotes the pre-resize capacity of the original pvc that pv was bound to if it has one test: confirm that pvc's status capacity is adjusted if pv has a pre-resize capacity annotation

sunpa93 force-pushed the fs_resize_fix branch from 5bf891e to 5f69cf7 Compare March 9, 2021 07:56

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

k8s-ci-robot merged commit 5155865 into kubernetes:master Mar 9, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021

xing-yang mentioned this pull request Mar 30, 2021

v2.2.0 release doc and removal of Supervisor and TKG Features kubernetes-sigs/vsphere-csi-driver#769

Merged

msau42 reviewed Apr 14, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use pv annotation to trigger filesystem resize when necessary #99326

fix: use pv annotation to trigger filesystem resize when necessary #99326

sunpa93 commented Feb 23, 2021 •

edited

k8s-ci-robot commented Feb 23, 2021

k8s-ci-robot commented Feb 23, 2021

gnufied commented Feb 23, 2021

gnufied commented Feb 23, 2021

sunpa93 commented Feb 25, 2021

gnufied commented Mar 1, 2021

gnufied commented Mar 5, 2021

sunpa93 commented Mar 8, 2021

sunpa93 commented Mar 9, 2021

gnufied commented Mar 9, 2021

jsafrane commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

gnufied commented Mar 9, 2021

msau42 left a comment

msau42 Apr 14, 2021

sunpa93 Apr 14, 2021

msau42 Apr 14, 2021

sunpa93 Apr 14, 2021

fix: use pv annotation to trigger filesystem resize when necessary #99326

fix: use pv annotation to trigger filesystem resize when necessary #99326

Conversation

sunpa93 commented Feb 23, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 23, 2021

k8s-ci-robot commented Feb 23, 2021

gnufied commented Feb 23, 2021

gnufied commented Feb 23, 2021

sunpa93 commented Feb 25, 2021

gnufied commented Mar 1, 2021

gnufied commented Mar 5, 2021

sunpa93 commented Mar 8, 2021

sunpa93 commented Mar 9, 2021

gnufied commented Mar 9, 2021

jsafrane commented Mar 9, 2021

k8s-ci-robot commented Mar 9, 2021

gnufied commented Mar 9, 2021

msau42 left a comment

Choose a reason for hiding this comment

msau42 Apr 14, 2021

Choose a reason for hiding this comment

sunpa93 Apr 14, 2021

Choose a reason for hiding this comment

msau42 Apr 14, 2021

Choose a reason for hiding this comment

sunpa93 Apr 14, 2021

Choose a reason for hiding this comment

sunpa93 commented Feb 23, 2021 •

edited