From f7f1a6c81a004c106d97593a08b041b450df171e Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 22 Jul 2024 10:43:38 -0400 Subject: [PATCH] Address review comments and return nicer errors --- pkg/volume/csi/expander.go | 2 +- pkg/volume/util/operationexecutor/node_expander.go | 14 ++++++++------ test/e2e/storage/csimock/csi_volume_expansion.go | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/volume/csi/expander.go b/pkg/volume/csi/expander.go index f5e6738434c..e56627203af 100644 --- a/pkg/volume/csi/expander.go +++ b/pkg/volume/csi/expander.go @@ -121,7 +121,7 @@ func (c *csiPlugin) nodeExpandWithClient( } if isInfeasibleError(err) { - infeasibleError := fmt.Errorf("Expander.NodeExpand failed to expand the volume: %w", volumetypes.NewInfeasibleError(err.Error())) + infeasibleError := volumetypes.NewInfeasibleError(fmt.Sprintf("Expander.NodeExpand failed to expand the volume %s", err.Error())) return false, infeasibleError } return false, fmt.Errorf("Expander.NodeExpand failed to expand the volume : %w", err) diff --git a/pkg/volume/util/operationexecutor/node_expander.go b/pkg/volume/util/operationexecutor/node_expander.go index 743b5837497..19547c94e09 100644 --- a/pkg/volume/util/operationexecutor/node_expander.go +++ b/pkg/volume/util/operationexecutor/node_expander.go @@ -39,9 +39,9 @@ type NodeExpander struct { pvCap resource.Quantity resizeStatus v1.ClaimResourceStatus - // indicates that pvc spec size has actually changed since last we observed size - // on the kubelet - markExpansionInfeasible bool + // indicates that if volume expansion failed on the node, then current expansion should be marked + // as infeasible so as controller can reconcile the resizing operation by using new user requested size. + markExpansionInfeasibleOnFailure bool // pvcAlreadyUpdated if true indicates that although we are calling NodeExpandVolume on the kubelet // PVC has already been updated - possibly because expansion already succeeded on different node. @@ -84,12 +84,14 @@ func (ne *NodeExpander) runPreCheck() bool { } pvcSpecCap := ne.pvc.Spec.Resources.Requests[v1.ResourceStorage] + // usually when are performing node expansion, we expect pv size and pvc spec size // to be the same, but if user has edited pvc since then and volume expansion failed - // with final error, then we should let controller reconcile this state. + // with final error, then we should let controller reconcile this state, by marking entire + // node expansion as infeasible. if pvcSpecCap.Cmp(ne.pluginResizeOpts.NewSize) != 0 && ne.actualStateOfWorld.CheckVolumeInFailedExpansionWithFinalErrors(ne.vmt.VolumeName) { - ne.markExpansionInfeasible = true + ne.markExpansionInfeasibleOnFailure = true } // PVC is already expanded but we are still trying to expand the volume because @@ -138,7 +140,7 @@ func (ne *NodeExpander) expandOnPlugin() (bool, error, testResponseData) { if volumetypes.IsOperationFinishedError(resizeErr) { var markFailedError error ne.actualStateOfWorld.MarkVolumeExpansionFailedWithFinalError(ne.vmt.VolumeName) - if volumetypes.IsInfeasibleError(resizeErr) || ne.markExpansionInfeasible { + if volumetypes.IsInfeasibleError(resizeErr) || ne.markExpansionInfeasibleOnFailure { ne.pvc, markFailedError = util.MarkNodeExpansionInfeasible(ne.pvc, ne.kubeClient, resizeErr) if markFailedError != nil { klog.Errorf(ne.vmt.GenerateErrorDetailed("MountMount.NodeExpandVolume failed to mark node expansion as failed: %v", err).Error()) diff --git a/test/e2e/storage/csimock/csi_volume_expansion.go b/test/e2e/storage/csimock/csi_volume_expansion.go index 835fd43484d..47ae8be9884 100644 --- a/test/e2e/storage/csimock/csi_volume_expansion.go +++ b/test/e2e/storage/csimock/csi_volume_expansion.go @@ -556,7 +556,7 @@ func waitForResizeStatus(ctx context.Context, pvc *v1.PersistentVolumeClaim, c c return (actualResizeStatus == expectedState), nil }) if waitErr != nil { - return fmt.Errorf("error while waiting for resize status to sync to %v, actualStatus %s: %v", expectedState, actualResizeStatus, waitErr) + return fmt.Errorf("error while waiting for resize status to sync to %v, actualStatus %s: %w", expectedState, actualResizeStatus, waitErr) } return nil }