Skip to content

Commit

Permalink
Fix indefinite stuck Pending pod on a deleted node
Browse files Browse the repository at this point in the history
  • Loading branch information
@sunnylovestiramisu
sunnylovestiramisu committed Mar 20, 2023
1 parent f1f3e83 commit 438f665
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 30 deletions.
65 changes: 37 additions & 28 deletions controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1408,6 +1408,10 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
selectedNode, err = ctrl.client.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) // TODO (verult) cache Nodes
}
if err != nil {
// if node does not exist, reschedule and remove volume.kubernetes.io/selected-node annotation
if apierrs.IsNotFound(err) {
return ctrl.provisionVolumeErrorHandling(ctx, ProvisioningReschedule, err, claim, operation)
}
err = fmt.Errorf("failed to get target node: %v", err)
ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
return ProvisioningNoChange, err
Expand All @@ -1430,35 +1434,9 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
klog.Info(logOperation(operation, "volume provision ignored: %v", ierr))
return ProvisioningFinished, errStopProvision
}
err = fmt.Errorf("failed to provision volume with StorageClass %q: %v", claimClass, err)
ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
if _, ok := claim.Annotations[annSelectedNode]; ok && result == ProvisioningReschedule {
// For dynamic PV provisioning with delayed binding, the provisioner may fail
// because the node is wrong (permanent error) or currently unusable (not enough
// capacity). If the provisioner wants to give up scheduling with the currently
// selected node, then it can ask for that by returning ProvisioningReschedule
// as state.
//
// `selectedNode` must be removed to notify scheduler to schedule again.
if errLabel := ctrl.rescheduleProvisioning(ctx, claim); errLabel != nil {
klog.Info(logOperation(operation, "volume rescheduling failed: %v", errLabel))
// If unsetting that label fails in ctrl.rescheduleProvisioning, we
// keep the volume in the work queue as if the provisioner had
// returned ProvisioningFinished and simply try again later.
return ProvisioningFinished, err
}
// Label was removed, stop working on the volume.
klog.Info(logOperation(operation, "volume rescheduled because: %v", err))
return ProvisioningFinished, errStopProvision
}

// ProvisioningReschedule shouldn't have been returned for volumes without selected node,
// but if we get it anyway, then treat it like ProvisioningFinished because we cannot
// reschedule.
if result == ProvisioningReschedule {
result = ProvisioningFinished
}
return result, err
err = fmt.Errorf("failed to provision volume with StorageClass %q: %v", claimClass, err)
return ctrl.provisionVolumeErrorHandling(ctx, result, err, claim, operation)
}

klog.Info(logOperation(operation, "volume %q provisioned", volume.Name))
Expand All @@ -1485,6 +1463,37 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
return ProvisioningFinished, nil
}

func (ctrl *ProvisionController) provisionVolumeErrorHandling(ctx context.Context, result ProvisioningState, err error, claim *v1.PersistentVolumeClaim, operation string) (ProvisioningState, error) {
ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
if _, ok := claim.Annotations[annSelectedNode]; ok && result == ProvisioningReschedule {
// For dynamic PV provisioning with delayed binding, the provisioner may fail
// because the node is wrong (permanent error) or currently unusable (not enough
// capacity). If the provisioner wants to give up scheduling with the currently
// selected node, then it can ask for that by returning ProvisioningReschedule
// as state.
//
// `selectedNode` must be removed to notify scheduler to schedule again.
if errLabel := ctrl.rescheduleProvisioning(ctx, claim); errLabel != nil {
klog.Info(logOperation(operation, "volume rescheduling failed: %v", errLabel))
// If unsetting that label fails in ctrl.rescheduleProvisioning, we
// keep the volume in the work queue as if the provisioner had
// returned ProvisioningFinished and simply try again later.
return ProvisioningFinished, err
}
// Label was removed, stop working on the volume.
klog.Info(logOperation(operation, "volume rescheduled because: %v", err))
return ProvisioningFinished, errStopProvision
}

// ProvisioningReschedule shouldn't have been returned for volumes without selected node,
// but if we get it anyway, then treat it like ProvisioningFinished because we cannot
// reschedule.
if result == ProvisioningReschedule {
result = ProvisioningFinished
}
return result, err
}

// deleteVolumeOperation attempts to delete the volume backing the given
// volume. Returns error, which indicates whether deletion should be retried
// (requeue the volume) or not
Expand Down
41 changes: 39 additions & 2 deletions controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,14 +542,33 @@ func TestController(t *testing.T) {
},
{
name: "do not remove selectedNode after final error, only the claim",
objs: []runtime.Object{
newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
newNode("node-1"),
},
provisionerName: "foo.bar/baz",
provisioner: newBadTestProvisioner(),
expectedClaims: []v1.PersistentVolumeClaim{
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
},
expectedClaimsInProgress: nil, // not in progress anymore
expectedMetrics: testMetrics{
provisioned: counts{
"class-1": count{failed: 1},
},
},
},
{
name: "remove selectedNode if no node exists",
objs: []runtime.Object{
newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
},
provisionerName: "foo.bar/baz",
provisioner: newBadTestProvisioner(),
expectedClaims: []v1.PersistentVolumeClaim{
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz"}),
},
expectedClaimsInProgress: nil, // not in progress anymore
expectedMetrics: testMetrics{
Expand All @@ -560,14 +579,32 @@ func TestController(t *testing.T) {
},
{
name: "do not remove selectedNode if nothing changes",
objs: []runtime.Object{
newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
newNode("node-1"),
},
provisionerName: "foo.bar/baz",
provisioner: newNoChangeTestProvisioner(),
expectedClaims: []v1.PersistentVolumeClaim{
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
},
expectedMetrics: testMetrics{
provisioned: counts{
"class-1": count{failed: 1},
},
},
},
{
name: "remove selectedNode if nothing changes and no node exists",
objs: []runtime.Object{
newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
},
provisionerName: "foo.bar/baz",
provisioner: newNoChangeTestProvisioner(),
expectedClaims: []v1.PersistentVolumeClaim{
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz"}),
},
expectedMetrics: testMetrics{
provisioned: counts{
Expand Down

0 comments on commit 438f665

Please sign in to comment.