Skip to content

Commit ae05361

Browse files
kibbles-n-bytespmorie
kibbles-n-bytes
authored and
pmorie
committed
retry failed unbind requests (openshift#1653)
* retry failed unbind requests * add integration test
1 parent 7eae845 commit ae05361

File tree

4 files changed

+114
-43
lines changed

4 files changed

+114
-43
lines changed

Diff for: ‎pkg/controller/controller_binding.go

+18-34
Original file line numberDiff line numberDiff line change
@@ -824,34 +824,6 @@ func (c *controller) serviceBindingRequestUnbinding(binding *v1beta1.ServiceBind
824824

825825
response, err := brokerClient.Unbind(unbindRequest)
826826
if err != nil {
827-
if httpErr, ok := osb.IsHTTPError(err); ok {
828-
s := fmt.Sprintf(
829-
`Error unbinding from %s: %s`,
830-
pretty.FromServiceInstanceOfClusterServiceClassAtBrokerName(instance, serviceClass, brokerName), httpErr.Error(),
831-
)
832-
glog.Warning(pcb.Message(s))
833-
c.recorder.Event(binding, corev1.EventTypeWarning, errorUnbindCallReason, s)
834-
setServiceBindingCondition(
835-
toUpdate,
836-
v1beta1.ServiceBindingConditionReady,
837-
v1beta1.ConditionUnknown,
838-
errorUnbindCallReason,
839-
"Unbind call failed. "+s)
840-
if !toUpdate.Status.OrphanMitigationInProgress {
841-
setServiceBindingCondition(
842-
toUpdate,
843-
v1beta1.ServiceBindingConditionFailed,
844-
v1beta1.ConditionTrue,
845-
errorUnbindCallReason,
846-
"Unbind call failed. "+s)
847-
}
848-
clearServiceBindingCurrentOperation(toUpdate)
849-
toUpdate.Status.UnbindStatus = v1beta1.ServiceBindingUnbindStatusFailed
850-
if _, err := c.updateServiceBindingStatus(toUpdate); err != nil {
851-
return false, err
852-
}
853-
return false, nil
854-
}
855827
s := fmt.Sprintf(
856828
`Error unbinding from %s: %s`,
857829
pretty.FromServiceInstanceOfClusterServiceClassAtBrokerName(instance, serviceClass, brokerName), err,
@@ -1593,27 +1565,39 @@ func (c *controller) pollServiceBinding(binding *v1beta1.ServiceBinding) error {
15931565
message,
15941566
)
15951567

1596-
if !mitigatingOrphan {
1568+
if !deleting {
15971569
setServiceBindingCondition(
15981570
binding,
15991571
v1beta1.ServiceBindingConditionFailed,
16001572
v1beta1.ConditionTrue,
16011573
reason,
16021574
message,
16031575
)
1604-
}
1576+
clearServiceBindingCurrentOperation(binding)
16051577

1606-
clearServiceBindingCurrentOperation(binding)
1578+
if _, err := c.updateServiceBindingStatus(binding); err != nil {
1579+
return err
1580+
}
16071581

1608-
if deleting {
1609-
binding.Status.UnbindStatus = v1beta1.ServiceBindingUnbindStatusFailed
1582+
return c.finishPollingServiceBinding(binding)
1583+
}
1584+
if !time.Now().Before(binding.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) {
1585+
return c.reconciliationRetryDurationExceededFinishPollingServiceBinding(binding)
16101586
}
16111587

1588+
// we must trigger a new unbind attempt entirely (as opposed to
1589+
// retrying querying the failed operation endpoint). Finish
1590+
// polling, and return an error in order to requeue in the
1591+
// standard binding queue.
1592+
binding.Status.AsyncOpInProgress = false
1593+
binding.Status.LastOperation = nil
1594+
16121595
if _, err := c.updateServiceBindingStatus(binding); err != nil {
16131596
return err
16141597
}
16151598

1616-
return c.finishPollingServiceBinding(binding)
1599+
c.finishPollingServiceBinding(binding)
1600+
return fmt.Errorf(message)
16171601
default:
16181602
glog.Warning(pcb.Messagef("Got invalid state in LastOperationResponse: %q", response.State))
16191603

Diff for: ‎pkg/controller/controller_binding_test.go

+59-9
Original file line numberDiff line numberDiff line change
@@ -1813,8 +1813,8 @@ func TestReconcileUnbindingWithClusterServiceBrokerHTTPError(t *testing.T) {
18131813
if err := scmeta.AddFinalizer(binding, v1beta1.FinalizerServiceCatalog); err != nil {
18141814
t.Fatalf("Finalizer error: %v", err)
18151815
}
1816-
if err := testController.reconcileServiceBinding(binding); err != nil {
1817-
t.Fatalf("reconcileServiceBinding should not have returned an error: %v", err)
1816+
if err := testController.reconcileServiceBinding(binding); err == nil {
1817+
t.Fatalf("reconcileServiceBinding should have returned an error")
18181818
}
18191819

18201820
actions := fakeCatalogClient.Actions()
@@ -1825,7 +1825,7 @@ func TestReconcileUnbindingWithClusterServiceBrokerHTTPError(t *testing.T) {
18251825
assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false)
18261826

18271827
updatedServiceBinding = assertUpdateStatus(t, actions[1], binding)
1828-
assertServiceBindingRequestFailingError(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, errorUnbindCallReason, errorUnbindCallReason, binding)
1828+
assertServiceBindingRequestRetriableError(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, errorUnbindCallReason, binding)
18291829
assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false)
18301830

18311831
events := getRecordedEvents(testController)
@@ -3068,6 +3068,7 @@ func TestPollServiceBinding(t *testing.T) {
30683068
validateBrokerActionsFunc func(t *testing.T, actions []fakeosb.Action)
30693069
validateKubeActionsFunc func(t *testing.T, actions []clientgotesting.Action)
30703070
validateConditionsFunc func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding)
3071+
shouldError bool
30713072
shouldFinishPolling bool
30723073
expectedEvents []string
30733074
}{
@@ -3371,7 +3372,7 @@ func TestPollServiceBinding(t *testing.T) {
33713372
expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"},
33723373
},
33733374
{
3374-
name: "unbind - failed",
3375+
name: "unbind - failed (retries)",
33753376
binding: getTestServiceBindingAsyncUnbinding(testOperation),
33763377
pollReaction: &fakeosb.PollBindingLastOperationReaction{
33773378
Response: &osb.LastOperationResponse{
@@ -3381,15 +3382,15 @@ func TestPollServiceBinding(t *testing.T) {
33813382
},
33823383
validateBrokerActionsFunc: validatePollBindingLastOperationAction,
33833384
validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) {
3384-
assertServiceBindingRequestFailingError(
3385+
assertServiceBindingRequestRetriableError(
33853386
t,
33863387
updatedBinding,
33873388
v1beta1.ServiceBindingOperationUnbind,
33883389
errorUnbindCallReason,
3389-
errorUnbindCallReason,
33903390
originalBinding,
33913391
)
33923392
},
3393+
shouldError: true,
33933394
shouldFinishPolling: true,
33943395
expectedEvents: []string{corev1.EventTypeWarning + " " + errorUnbindCallReason + " " + "Unbind call failed: " + lastOperationDescription},
33953396
},
@@ -3453,6 +3454,32 @@ func TestPollServiceBinding(t *testing.T) {
34533454
shouldFinishPolling: true,
34543455
expectedEvents: []string{corev1.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason + " " + "Stopping reconciliation retries because too much time has elapsed"},
34553456
},
3457+
{
3458+
name: "unbind - failed - retry duration exceeded",
3459+
binding: getTestServiceBindingAsyncUnbindingRetryDurationExceeded(testOperation),
3460+
pollReaction: &fakeosb.PollBindingLastOperationReaction{
3461+
Response: &osb.LastOperationResponse{
3462+
State: osb.StateFailed,
3463+
Description: strPtr(lastOperationDescription),
3464+
},
3465+
},
3466+
validateBrokerActionsFunc: validatePollBindingLastOperationAction,
3467+
validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) {
3468+
assertServiceBindingRequestFailingError(
3469+
t,
3470+
updatedBinding,
3471+
v1beta1.ServiceBindingOperationUnbind,
3472+
errorUnbindCallReason,
3473+
errorReconciliationRetryTimeoutReason,
3474+
originalBinding,
3475+
)
3476+
},
3477+
shouldFinishPolling: true,
3478+
expectedEvents: []string{
3479+
corev1.EventTypeWarning + " " + errorUnbindCallReason + " " + "Unbind call failed: " + lastOperationDescription,
3480+
corev1.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason + " " + "Stopping reconciliation retries because too much time has elapsed",
3481+
},
3482+
},
34563483
// Unbind as part of orphan mitigation
34573484
{
34583485
name: "orphan mitigation - succeeded",
@@ -3513,7 +3540,7 @@ func TestPollServiceBinding(t *testing.T) {
35133540
expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"},
35143541
},
35153542
{
3516-
name: "orphan mitigation - failed",
3543+
name: "orphan mitigation - failed (retries)",
35173544
binding: getTestServiceBindingAsyncOrphanMitigation(testOperation),
35183545
pollReaction: &fakeosb.PollBindingLastOperationReaction{
35193546
Response: &osb.LastOperationResponse{
@@ -3523,8 +3550,9 @@ func TestPollServiceBinding(t *testing.T) {
35233550
},
35243551
validateBrokerActionsFunc: validatePollBindingLastOperationAction,
35253552
validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) {
3526-
assertServiceBindingOrphanMitigationFailure(t, updatedBinding, originalBinding)
3553+
assertServiceBindingRequestRetriableOrphanMitigation(t, updatedBinding, originalBinding)
35273554
},
3555+
shouldError: true,
35283556
shouldFinishPolling: true,
35293557
expectedEvents: []string{corev1.EventTypeWarning + " " + errorOrphanMitigationFailedReason + " " + "Orphan mitigation failed: " + lastOperationDescription},
35303558
},
@@ -3574,6 +3602,25 @@ func TestPollServiceBinding(t *testing.T) {
35743602
shouldFinishPolling: true,
35753603
expectedEvents: []string{corev1.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason + " " + "Stopping reconciliation retries because too much time has elapsed"},
35763604
},
3605+
{
3606+
name: "orphan mitigation - failed - retry duration exceeded",
3607+
binding: getTestServiceBindingAsyncOrphanMitigationRetryDurationExceeded(testOperation),
3608+
pollReaction: &fakeosb.PollBindingLastOperationReaction{
3609+
Response: &osb.LastOperationResponse{
3610+
State: osb.StateFailed,
3611+
Description: strPtr(lastOperationDescription),
3612+
},
3613+
},
3614+
validateBrokerActionsFunc: validatePollBindingLastOperationAction,
3615+
validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) {
3616+
assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding)
3617+
},
3618+
shouldFinishPolling: true,
3619+
expectedEvents: []string{
3620+
corev1.EventTypeWarning + " " + errorOrphanMitigationFailedReason + " " + "Orphan mitigation failed: " + lastOperationDescription,
3621+
corev1.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason + " " + "Stopping reconciliation retries because too much time has elapsed",
3622+
},
3623+
},
35773624
}
35783625

35793626
for _, tc := range cases {
@@ -3595,7 +3642,10 @@ func TestPollServiceBinding(t *testing.T) {
35953642

35963643
bindingKey := tc.binding.Namespace + "/" + tc.binding.Name
35973644

3598-
if err := testController.pollServiceBinding(tc.binding); err != nil {
3645+
err := testController.pollServiceBinding(tc.binding)
3646+
if tc.shouldError && err == nil {
3647+
t.Fatalf("expected error when polling service binding but there was none")
3648+
} else if !tc.shouldError && err != nil {
35993649
t.Fatalf("unexpected error when polling service binding: %v", err)
36003650
}
36013651

Diff for: ‎pkg/controller/controller_test.go

+10
Original file line numberDiff line numberDiff line change
@@ -2658,6 +2658,16 @@ func assertServiceBindingRequestRetriableErrorWithParameters(t *testing.T, obj r
26582658
assertServiceBindingUnbindStatus(t, obj, v1beta1.ServiceBindingUnbindStatusRequired)
26592659
}
26602660

2661+
func assertServiceBindingRequestRetriableOrphanMitigation(t *testing.T, obj runtime.Object, originalBinding *v1beta1.ServiceBinding) {
2662+
assertServiceBindingReadyCondition(t, obj, v1beta1.ConditionUnknown, errorOrphanMitigationFailedReason)
2663+
assertServiceBindingCurrentOperation(t, obj, v1beta1.ServiceBindingOperationBind)
2664+
assertServiceBindingOperationStartTimeSet(t, obj, true)
2665+
assertServiceBindingReconciledGeneration(t, obj, originalBinding.Status.ReconciledGeneration)
2666+
assertServiceBindingInProgressPropertiesParameters(t, obj, nil, "")
2667+
assertServiceBindingExternalPropertiesUnchanged(t, obj, originalBinding)
2668+
assertServiceBindingUnbindStatus(t, obj, v1beta1.ServiceBindingUnbindStatusRequired)
2669+
}
2670+
26612671
func assertServiceBindingAsyncInProgress(t *testing.T, obj runtime.Object, operation v1beta1.ServiceBindingOperation, reason string, operationKey string, originalBinding *v1beta1.ServiceBinding) {
26622672
assertServiceBindingReadyFalse(t, obj, reason)
26632673
assertServiceBindingLastOperation(t, obj, operationKey)

Diff for: ‎test/integration/controller_binding_test.go

+27
Original file line numberDiff line numberDiff line change
@@ -458,3 +458,30 @@ func TestCreateServiceBindingWithParameters(t *testing.T) {
458458
})
459459
}
460460
}
461+
462+
// TestDeleteServiceBindingRetry tests whether deletion of a service binding
463+
// retries after failing.
464+
func TestDeleteServiceBindingFailureRetry(t *testing.T) {
465+
const NumberOfUnbindFailures = 2
466+
numberOfAttempts := 0
467+
ct := &controllerTest{
468+
t: t,
469+
broker: getTestBroker(),
470+
instance: getTestInstance(),
471+
binding: getTestBinding(),
472+
setup: func(ct *controllerTest) {
473+
ct.osbClient.UnbindReaction = fakeosb.DynamicUnbindReaction(
474+
func(_ *osb.UnbindRequest) (*osb.UnbindResponse, error) {
475+
numberOfAttempts++
476+
if numberOfAttempts > NumberOfUnbindFailures {
477+
return &osb.UnbindResponse{}, nil
478+
}
479+
return nil, osb.HTTPStatusCodeError{
480+
StatusCode: 500,
481+
Description: strPtr("test error unbinding"),
482+
}
483+
})
484+
},
485+
}
486+
ct.run(func(_ *controllerTest) {})
487+
}

0 commit comments

Comments
 (0)