modify: support rollback #513

huww98 · 2025-07-20T09:23:41Z

What type of PR is this?

/kind feature

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Support rollback to VAC A if modifying from A to B failed with a final error.
This works just like we modifying it again to C on final error.

The significant changes in the sync logic:

Always retry if pvc.Status.ModifyVolumeStatus is not nil, which means the
last transation does not finish successfully.
Keep reconciling to the previous target if spec is rolled back to nil, until
it succeeds or we get an infeasible error. Then we just leave it at its
current state and stop reconciling for it, since user may not care about it
now.

See kubernetes/enhancements#5482 for details

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The first 2 commits are already included in my other PRs. Please review the last commit. I will rebase when the previous PRs are merged.

Does this PR introduce a user-facing change?:

Support rolling back when modify failed

k8s-ci-robot · 2025-07-20T09:23:51Z

Hi @huww98. Thanks for your PR.

I'm waiting for a kubernetes-csi 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-sigs/prow repository.

huww98 · 2025-08-17T17:13:17Z

Implementation of kubernetes/enhancements#5482

sunnylovestiramisu · 2025-09-02T19:53:13Z

/ok-to-test

sunnylovestiramisu · 2025-09-04T17:54:23Z

pkg/modifycontroller/modify_volume.go

+
+	// Check if we should change our target
+	_, inUncertainState := ctrl.uncertainPVCs.Load(pvcKey)
+	if (status != nil && status.Status == v1.PersistentVolumeClaimModifyVolumeInProgress && inUncertainState) || pvcSpecVacName == "" {


If status.Status == v1.PersistentVolumeClaimModifyVolumeInProgress alone should be a sufficient condition to not modify volume anymore?

that condition exists to ensure that for transient failing modifications we keep trying to reconcile to whatever value was recorded in target and do not start with new value of vac.

sunnylovestiramisu · 2025-09-22T18:55:45Z

pkg/modifycontroller/modify_volume_test.go

+		t.Fatalf("expected error to be %v, got %v", finalErr, err)
+	}
+	// should clear uncertain state
+	assertUncertain(false)


Can we also add cases to verify what the conditions should be in each step?

Now The ModifyVolumeError condition will overwrite ModifyingVolume, we will always get only ModifyVolumeError after a failed modification. So I think it maybe not very interesting to check it here.

Added stronger assertions to TestMarkControllerModifyVolumeStatus.

gnufied · 2025-09-23T18:38:35Z

@huww98 let me know when this PR is ready for another round of review. thanks.

torredil

Validated that the change introduced in this PR does not break existing modify volume workflows by building a custom resizer image including this change and deploying latest version of EBS CSI Driver. I ran the external storage test suite and manually tested a few edge cases.

lgtm besides the outstanding feedback from @gnufied and @sunnylovestiramisu that needs to be addressed.

We should keep retry the previously specified target.

Support rollback to VAC A if modifying from A to B failed with a final error. This works just like we modifying it again to C on final error. The significant changes in the sync logic: - Always retry if pvc.Status.ModifyVolumeStatus is not nil, which means the last transation does not finish successfully. - Keep reconciling to the previous target if spec is rolled back to nil, until it succeeds or we get an infeasible error. Then we just leave it at its current state and stop reconciling for it, since user may not care about it now.

pvcModifier should return independent object from the start of the call chain to avoid interference between test cases.

Start modify @ t1 Type Status LastProbeTime LastTransitionTime Reason Message ---- ------ ------------- ------------------ ------ ------- ModifyingVolume True t1 t1 Modifying volume to <VAC name> is in progress. final error @ t2: - Use gPRC code for Reason - update LastTransitionTime only if Reason changes Type Status LastProbeTime LastTransitionTime Reason Message ---- ------ ------------- ------------------ ------ ------- ModifyingVolume True t2 t1 Modifying volume to <VAC name> failed. Waiting for retry. ModifyVolumeError True t2 t2 Internal Final error. Retry @ t3: - update ModifyingVolume LastProbeTime Type Status LastProbeTime LastTransitionTime Reason Message ---- ------ ------------- ------------------ ------ ------- ModifyingVolume True t3 t1 Modifying volume to <VAC name> is in progress. ModifyVolumeError True t2 t2 Internal Final error. non-final error @ t4: Type Status LastProbeTime LastTransitionTime Reason Message ---- ------ ------------- ------------------ ------ ------- ModifyingVolume True t4 t1 Modifying volume to <VAC name> is still in progress. ModifyVolumeError True t4 t4 DeadlineExceeded Progress: 10%.

gnufied · 2025-09-29T14:48:51Z

@huww98 you didn't address:

When I apply a invalid VAC (which doesn't exist) and then revert back to empty/nil VAC the status.modifyStatus is not cleared. IMO it should be cleared:

huww98 · 2025-09-29T15:22:45Z

@gnufied I think we have reached agreement about this: the status.modifyStatus should not be cleared

Yes. I think we can clear the conditions. But status.modifyStatus should not be cleared.

That is fine. I was mainly thinking about conditions.

gnufied · 2025-09-29T17:11:17Z

@gnufied I think we have reached agreement about this: the status.modifyStatus should not be cleared

Okay, lets move past this issue for now. I was mainly talking about case of VAC being pending.

But I think I found a bug in handling of non-existing VACs which can cause users to abuse quota trivially. Example flow:

A PVC starts with a nil VAC.
User applies VAC B to the PVC, which fails with final error.
User applies VAC C to the PVC, which doesn't exist, now since this we are recording VAC C in target's status, any record of B being ever applied is erased.

Make it harder to abuse VAC quota by change spec to an non-existing VAC when the volume is partially modified. Basically reverting 6c8c10f.

huww98 · 2025-09-29T18:37:11Z

It is a bit weird if we left TargetVolumeAttributesClassName empty while setting Status to "Pending". We want them to be set or cleared at the same time since we put them in a struct, right? But as you wish, I reverted to the previous behavior, it is more correct in terms of quota.

I'm thinking that can we just deprecate the Pending status? Maybe we should left modifyVolumeStatus untouched if VAC not exists, and just communicate this situation with condition and event. Maybe we can have condition with type: ModifyingVolume, status: "False" for this.

gnufied · 2025-10-01T19:36:51Z

/lgtm
/approve

k8s-ci-robot · 2025-10-01T19:37:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, huww98

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~~ [gnufied]

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

huww98 · 2025-10-07T06:58:44Z

/test pull-kubernetes-csi-external-resizer-1-33-on-kubernetes-master

k8s-ci-robot requested review from humblec and jingxu97 July 20, 2025 09:23

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 20, 2025

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 20, 2025

huww98 force-pushed the rollback branch from 1f7a07b to cab9251 Compare July 20, 2025 09:45

This was referenced Jul 26, 2025

Collapse the INVALID_ARGUMENTS error rows and clarify container-storage-interface/spec#597

Merged

fix uncertain cache #512

Closed

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

huww98 force-pushed the rollback branch from cab9251 to 98ffaa2 Compare August 17, 2025 17:12

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

huww98 force-pushed the rollback branch 2 times, most recently from 56ff79c to 19cef74 Compare August 24, 2025 12:47

carlory mentioned this pull request Sep 4, 2025

Promote VolumeAttributesClass to GA #515

Merged

huww98 mentioned this pull request Sep 4, 2025

KEP-3751: add error handling kubernetes/enhancements#5482

Open

sunnylovestiramisu reviewed Sep 4, 2025

View reviewed changes

huww98 force-pushed the rollback branch from d9709c5 to 38f3d52 Compare September 16, 2025 02:37

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

gnufied mentioned this pull request Sep 18, 2025

Address various linter (staticcheck, modernize, etc.) comments. #517

Open

sunnylovestiramisu reviewed Sep 22, 2025

View reviewed changes

torredil reviewed Sep 24, 2025

View reviewed changes

don't change target in uncertain state

e8f399a

We should keep retry the previously specified target.

huww98 force-pushed the rollback branch 2 times, most recently from ef281b6 to eff5e89 Compare September 24, 2025 17:12

huww98 added 5 commits September 25, 2025 02:16

modify: cancel retry deleted PVC

41bf573

modify: only update status once when failed

96f7c35

make non-final error event a little different

6688de6

stronger assertion to verify conditions

852486a

pvcModifier should return independent object from the start of the call chain to avoid interference between test cases.

huww98 force-pushed the rollback branch from eff5e89 to 852486a Compare September 24, 2025 18:29

huww98 added 3 commits September 25, 2025 21:26

clear modifying condition when set spec VAC to empty

eed270e

simplify clearModifyVolumeConditions

d5cdab8

Preserve previous target VAC when new VAC not found

d03a1e7

Make it harder to abuse VAC quota by change spec to an non-existing VAC when the volume is partially modified. Basically reverting 6c8c10f.

k8s-ci-robot assigned gnufied Oct 1, 2025

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

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

k8s-ci-robot merged commit 099f112 into kubernetes-csi:master Oct 1, 2025
6 checks passed

sunnylovestiramisu mentioned this pull request Oct 6, 2025

PVC being modified to infeasible VAC should have modifyvolume AND modifyvolumeerror conditions #483

Closed

huww98 mentioned this pull request Oct 7, 2025

REQUEST: New membership for huww98 kubernetes/org#5903

Closed

11 tasks

modify: support rollback #513

modify: support rollback #513

Conversation

huww98 commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 20, 2025

Uh oh!

huww98 commented Aug 17, 2025

Uh oh!

sunnylovestiramisu commented Sep 2, 2025

Uh oh!

sunnylovestiramisu Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

gnufied Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

sunnylovestiramisu Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

huww98 Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Sep 23, 2025

Uh oh!

torredil left a comment

Choose a reason for hiding this comment

Uh oh!

gnufied commented Sep 29, 2025

Uh oh!

huww98 commented Sep 29, 2025

Uh oh!

gnufied commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huww98 commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnufied commented Oct 1, 2025

Uh oh!

k8s-ci-robot commented Oct 1, 2025

Uh oh!

Uh oh!

huww98 commented Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

huww98 commented Jul 20, 2025 •

edited

Loading

huww98 Sep 24, 2025 •

edited

Loading

gnufied commented Sep 29, 2025 •

edited

Loading

huww98 commented Sep 29, 2025 •

edited

Loading