Handle case of recovery from resize failures by gnufied · Pull Request #187 · kubernetes-csi/external-resizer

gnufied · 2022-01-13T16:55:17Z

Handle the case of recovery from resize failure.

This is mostly same implementation from intree controller code - https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/operationexecutor/operation_generator.go#L1782 with minor changes because of node-only expansion handling as described in KEP.

Manually tested following scenarios (with hostpath driver):

If controller-expansion failed, it allows recovery to a size > pvc.Status.Cap
if Node expansion is pending, it does not try expansion to new size until previous expansion is finished
Tested with consecutive changes which can cause resize success and failure
Tested with node expansion failing (control-plane succeeded) and users reducing size (no recovery is possible)
Tested with node expansion failing but no controller-expansion is available (recovery is possible)

Add support to allow users to recover from expansion failures

k8s-ci-robot · 2022-01-13T16:55:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

gnufied · 2022-01-13T17:00:47Z

/assign @jsafrane

gnufied · 2022-01-13T21:07:03Z

/assign @jingxu97 @xing-yang

fix bug with switch..case statement

pkg/controller/expand_and_recover.go

pkg/controller/resize_status.go

pkg/controller/expand_and_recover_test.go

jsafrane · 2022-01-20T13:42:58Z

I tested it with AWS EBS (it allows one expansion of a volume in 6 hours, so it can throw nice errors).
/lgtm
/hold
(for squash)

jingxu97 · 2022-01-20T15:45:21Z

pkg/controller/expand_and_recover.go

+// checks if pv can be expanded
+func (ctrl *resizeController) pvCanBeExpanded(pv *v1.PersistentVolume, pvc *v1.PersistentVolumeClaim) bool {
+	if !ctrl.resizer.CanSupport(pv, pvc) {
+		klog.V(4).Infof("Resizer %q doesn't support PV %q", ctrl.name, pv.Name)


maybe it can be a warning because you are calling resize, but resizer does not support it?

it can get called on all sort of PVs. such as intree pv,pvc etc, which are not resizable by external-resizer. I think Info is suitable since most people will be running both intree expand_controller and this controller side-by-side for now.

jingxu97 · 2022-01-20T15:50:11Z

pkg/controller/controller.go

+		_, _, err, _ := ctrl.expandAndRecover(pvc, pv)
+		return err
+	} else {
+		if !ctrl.pvNeedResize(pvc, pv) {


seem pvNeedResize can be called before checking feature is enabled?

is pvNeedResize more like pv cannot be resized due to condition is not met? Does it consider an error?

this is basically old flow - which checks if PV is resizable (such as, whether it is bound and bound to PVC which is being resized and whether it was already controller expanded and node expansion is pending). It is not a hard error if PV can not expanded at this moment. But anyways - this is old code and has kinda always worked okay.

oh I confused about pvCanBeExpanded and pvNeedResize, thinking they are the same function.

yeah I am sorry about similar names. I am hoping to delete pvNeedResize when this feature goes beta.

jingxu97 · 2022-01-20T15:55:46Z

pkg/controller/controller.go


-	return ctrl.resizePVC(pvc, pv)
+	if utilfeature.DefaultFeatureGate.Enabled(features.RecoverVolumeExpansionFailure) {
+		_, _, err, _ := ctrl.expandAndRecover(pvc, pv)


why return pv, pvc, but without using use objects?

for now, mainly for tests.

xing-yang · 2022-01-20T19:21:02Z

pkg/controller/controller.go

-		return nil, err
+		return updatedPVC, err
 	}
 	return updatedPVC, nil


Since updatedPVC is always returned now, we can remove the "if err != nil" code block and just have "return updatedPVC, err" as this will cover the case when err is nil as well.

jingxu97 · 2022-01-20T19:47:37Z

pkg/controller/controller.go


-func (ctrl *resizeController) patchClaim(oldPVC, newPVC *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
-	patchBytes, err := util.GetPVCPatchData(oldPVC, newPVC)
+func (ctrl *resizeController) patchClaim(oldPVC, newPVC *v1.PersistentVolumeClaim, addResourceVersionCheck bool) (*v1.PersistentVolumeClaim, error) {


could you add some comments about this option addResourceVersionCheck?

gnufied · 2022-01-20T22:54:30Z

@jingxu97 @xing-yang addressed your comments. can you PTAL?

xing-yang · 2022-01-20T23:37:05Z

/lgtm

jingxu97 · 2022-01-20T23:45:07Z

pkg/controller/resize_status.go

+	newPVC := pvc.DeepCopy()
+	newPVC.Status.ResizeStatus = &expansionFailedOnController
+
+	updatedPVC, err := ctrl.patchClaim(pvc, newPVC, false /* addResourceVersionCheck */)


the reason no need to check resource version here?

The general idea I had was if expansion failed we record the error even if we get a potential conflict. Since we are only patching resize status we should be okay.

what is the result of conflict? It might overwrite some fields which are updated by other places?
how about we also check addResourceVersionCheck here? Any potential issue?

The conflict when addResourceVersionCheck is present has nothing to do with content of the patch, but is about whether patch we are applying is with latest version of object or not. i.e - if two updates were performed on PVC concurrently and they updated different fields, then if addResourceVersionCheck is present with second patch - you will still get error (even if second update changes entirely different field of PVC).

how about we also check addResourceVersionCheck here? Any potential issue?

We can, but if somehow our version of PVC was older then, the entire resizing operation has to be restarted before ExpansionFailedOnController can be set for resizeStatus. As such it does not affect the design but skipping addResourceVersionCheck is an optimization - so as we set this field if expansion fails on controller and even if PVC our version of PVC was slightly older somehow.

Added as a comment in the code.

jingxu97 · 2022-01-21T02:05:02Z

/lgtm

gnufied · 2022-01-21T02:16:25Z

/hold cancel

gnufied added 3 commits January 13, 2022 10:44

Fix code to handling recovery from expansion failure

ff87c34

start adding tests for expand and recover

dea6889

Fix tests for recovery from expansion

4127d62

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 13, 2022

k8s-ci-robot requested review from ggriffiths and j-griffith January 13, 2022 16:55

k8s-ci-robot assigned jsafrane Jan 13, 2022

k8s-ci-robot assigned jingxu97 and xing-yang Jan 13, 2022

gnufied force-pushed the fix-recover-resize branch from 2aa9ed0 to 4cdc2b7 Compare January 14, 2022 13:21

Add more tests for pending expansion

9f08c69

fix bug with switch..case statement

gnufied force-pushed the fix-recover-resize branch from 4cdc2b7 to 9f08c69 Compare January 14, 2022 19:15

jsafrane reviewed Jan 18, 2022

View reviewed changes

jsafrane reviewed Jan 20, 2022

View reviewed changes

pkg/controller/expand_and_recover_test.go Show resolved Hide resolved

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 20, 2022

jingxu97 reviewed Jan 20, 2022

View reviewed changes

xing-yang reviewed Jan 20, 2022

View reviewed changes

jingxu97 reviewed Jan 20, 2022

View reviewed changes

gnufied force-pushed the fix-recover-resize branch from 15fcebc to 7ca824f Compare January 20, 2022 22:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2022

jingxu97 reviewed Jan 20, 2022

View reviewed changes

Fix review comments and add tests

8c552c3

gnufied force-pushed the fix-recover-resize branch from 7ca824f to 8c552c3 Compare January 21, 2022 00:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2022

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

k8s-ci-robot merged commit 9f5f2c2 into kubernetes-csi:master Jan 21, 2022

Conversation

gnufied commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 13, 2022

Uh oh!

gnufied commented Jan 13, 2022

Uh oh!

gnufied commented Jan 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsafrane commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied Jan 20, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied commented Jan 20, 2022

Uh oh!

xing-yang commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied Jan 21, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

jingxu97 commented Jan 21, 2022

Uh oh!

gnufied commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gnufied commented Jan 13, 2022 •

edited

Loading

gnufied Jan 20, 2022 •

edited

Loading

gnufied Jan 21, 2022 •

edited

Loading