diff --git a/controllers/binding_controller.go b/controllers/binding_controller.go index 5c3ba454..a817fe8e 100644 --- a/controllers/binding_controller.go +++ b/controllers/binding_controller.go @@ -37,6 +37,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -278,7 +279,7 @@ func (r *BindingReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) return r.updateStatusError(instance, bindingStateFailed, err) } - return r.updateStatusOnline(session, instance, serviceInstance, serviceClassType) + return r.updateStatusOnline(session, instance) } // The KeyInstanceID has been set (or is still inProgress), verify that the key and secret still exist @@ -313,7 +314,7 @@ func (r *BindingReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) logt.Info("Error creating secret", instance.Name, err.Error()) return r.updateStatusError(instance, bindingStateFailed, err) } - return r.updateStatusOnline(session, instance, serviceInstance, serviceClassType) + return r.updateStatusOnline(session, instance) } // The secret exists, make sure it has the right content @@ -335,9 +336,9 @@ func (r *BindingReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) logt.Info("Error re-creating secret", instance.Name, err.Error()) return r.updateStatusError(instance, bindingStateFailed, err) } - return r.updateStatusOnline(session, instance, serviceInstance, serviceClassType) + return r.updateStatusOnline(session, instance) } - return r.updateStatusOnline(session, instance, serviceInstance, serviceClassType) + return r.updateStatusOnline(session, instance) } func (r *BindingReconciler) getServiceInstance(instance *ibmcloudv1.Binding) (*ibmcloudv1.Service, error) { @@ -507,19 +508,23 @@ func (r *BindingReconciler) createSecret(instance *ibmcloudv1.Binding, keyConten return nil } -func (r *BindingReconciler) updateStatusOnline(session *session.Session, instance *ibmcloudv1.Binding, serviceInstance *ibmcloudv1.Service, serviceClassType string) (ctrl.Result, error) { - instance.Status.State = bindingStateOnline - instance.Status.Message = bindingStateOnline - instance.Status.SecretName = getSecretName(instance) - err := r.Status().Update(context.Background(), instance) - if err != nil { - r.Log.Info("Failed to update online status, will delete external resource ", instance.ObjectMeta.Name, err.Error()) - err = r.deleteCredentials(session, instance, serviceClassType) +func (r *BindingReconciler) updateStatusOnline(session *session.Session, instance *ibmcloudv1.Binding) (ctrl.Result, error) { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + currentBindingInstance := &ibmcloudv1.Binding{} + err := r.Get(context.Background(), types.NamespacedName{Namespace: instance.Namespace, Name: instance.Name}, currentBindingInstance) if err != nil { - r.Log.Info("Failed to delete external resource, operator state and external resource might be in an inconsistent state", instance.ObjectMeta.Name, err.Error()) + r.Log.Error(err, "Failed to fetch binding instance", "namespace", instance.Namespace, "name", instance.Name) + return err } + currentBindingInstance.Status.State = bindingStateOnline + currentBindingInstance.Status.Message = bindingStateOnline + currentBindingInstance.Status.SecretName = getSecretName(currentBindingInstance) + return r.Status().Update(context.Background(), currentBindingInstance) + }) + if err != nil { + r.Log.Error(err, "Failed to update binding instance after retry", "namespace", instance.Namespace, "name", instance.Name) + return ctrl.Result{Requeue: true}, err } - return ctrl.Result{Requeue: true, RequeueAfter: config.Get().SyncPeriod}, nil } diff --git a/controllers/binding_controller_test.go b/controllers/binding_controller_test.go index 47b549ec..be5280ab 100644 --- a/controllers/binding_controller_test.go +++ b/controllers/binding_controller_test.go @@ -2252,7 +2252,7 @@ func TestBindingDeleteCredentials(t *testing.T) { } } -func TestBindingUpdateStatusOnlineFailed(t *testing.T) { +func TestBindingUpdateStatusOnlineFailedWithConflictError(t *testing.T) { t.Parallel() scheme := schemas(t) binding := &ibmcloudv1.Binding{ @@ -2264,9 +2264,19 @@ func TestBindingUpdateStatusOnlineFailed(t *testing.T) { Spec: ibmcloudv1.ServiceSpec{}, } + errChan := make(chan error, 10) + + //It return conflict error at first so retry will be triggered and succeed with no error and the function will succeed + errChan <- &k8sErrors.StatusError{metav1.Status{ + Status: metav1.StatusFailure, + Code: 409, + Reason: metav1.StatusReasonConflict, + }} + errChan <- nil + client := newMockClient( fake.NewFakeClientWithScheme(scheme, binding, service), - MockConfig{StatusUpdateErr: fmt.Errorf("status failed")}, + MockConfig{ErrChan: errChan}, ) r := &BindingReconciler{ Client: client, @@ -2278,21 +2288,48 @@ func TestBindingUpdateStatusOnlineFailed(t *testing.T) { }, } - result, err := r.updateStatusOnline(nil, binding, service, "") + result, err := r.updateStatusOnline(nil, binding) assert.Equal(t, ctrl.Result{ Requeue: true, RequeueAfter: config.Get().SyncPeriod, }, result) assert.NoError(t, err) - assert.Equal(t, &ibmcloudv1.Binding{ +} + +func TestBindingUpdateStatusOnlineFailedWithOtherErrror(t *testing.T) { + t.Parallel() + scheme := schemas(t) + binding := &ibmcloudv1.Binding{ ObjectMeta: metav1.ObjectMeta{Name: "myservice", Namespace: "mynamespace"}, - Status: ibmcloudv1.BindingStatus{ - State: bindingStateOnline, - Message: bindingStateOnline, - SecretName: "myservice", + Spec: ibmcloudv1.BindingSpec{}, + } + service := &ibmcloudv1.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "myservice", Namespace: "mynamespace"}, + Spec: ibmcloudv1.ServiceSpec{}, + } + + errChan := make(chan error, 10) + + errChan <- fmt.Errorf("status failed") + client := newMockClient( + fake.NewFakeClientWithScheme(scheme, binding, service), + MockConfig{ErrChan: errChan}, + ) + r := &BindingReconciler{ + Client: client, + Log: testLogger(t), + Scheme: scheme, + + DeleteResourceServiceKey: func(session *session.Session, keyID string) error { + return fmt.Errorf("failed") }, - Spec: ibmcloudv1.BindingSpec{}, - }, client.LastStatusUpdate()) + } + + result, err := r.updateStatusOnline(nil, binding) + assert.Equal(t, ctrl.Result{ + Requeue: true, + }, result) + assert.Error(t, err) } func TestBindingSetupWithManager(t *testing.T) { diff --git a/controllers/mock_client_test.go b/controllers/mock_client_test.go index 0dacee5e..8f112990 100644 --- a/controllers/mock_client_test.go +++ b/controllers/mock_client_test.go @@ -45,6 +45,8 @@ type MockConfig struct { StatusPatchErr error StatusUpdateErr error UpdateErr error + + ErrChan chan error } func newMockClient(client client.Client, config MockConfig) MockClient { @@ -107,7 +109,13 @@ func (m *mockClient) Status() client.StatusWriter { func (s *mockStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { s.lastStatusUpdate = obj.DeepCopyObject() - return s.StatusUpdateErr + if s.ErrChan != nil { + err := <-s.ErrChan + return err + } else { + return s.StatusUpdateErr + + } } func (m *mockClient) LastStatusUpdate() runtime.Object {