fix issue Credential was deleted due to "Failed to update online stat…

…us" #217 (#226)

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
 }
 

controllers/binding_controller_test.go

@@ -23,6 +23,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/client/fake"
@@ -2252,7 +2253,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 +2265,19 @@ func TestBindingUpdateStatusOnlineFailed(t *testing.T) {
 		Spec:       ibmcloudv1.ServiceSpec{},
 	}
 
+	errChan := make(chan error, retry.DefaultRetry.Steps)
+
+	conflictErr := k8sErrors.StatusError{
+		ErrStatus: metav1.Status{
+			Status: metav1.StatusFailure,
+			Code:   409,
+			Reason: metav1.StatusReasonConflict,
+		},
+	}
+
 	client := newMockClient(
 		fake.NewFakeClientWithScheme(scheme, binding, service),
-		MockConfig{StatusUpdateErr: fmt.Errorf("status failed")},
+		MockConfig{ErrChan: errChan},
 	)
 	r := &BindingReconciler{
 		Client: client,
@@ -2278,21 +2289,87 @@ func TestBindingUpdateStatusOnlineFailed(t *testing.T) {
 		},
 	}
 
-	result, err := r.updateStatusOnline(nil, binding, service, "")
+	//It return conflict error at first so retry will be triggered and succeed with no error and the function will succeed
+	errChan <- &conflictErr
+	errChan <- nil
+	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{
+
+	//It keeps returning conflict error so retry will fail
+	for i := 0; i < retry.DefaultRetry.Steps; i++ {
+		errChan <- &conflictErr
+
+	}
+	result, err = r.updateStatusOnline(nil, binding)
+	assert.Equal(t, ctrl.Result{
+		Requeue: true,
+	}, result)
+	assert.Error(t, err)
+}
+
+func TestBindingUpdateStatusOnlineFailedWithOtherUpdateErrror(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{},
-	}, client.LastStatusUpdate())
+		Spec:       ibmcloudv1.BindingSpec{},
+	}
+	service := &ibmcloudv1.Service{
+		ObjectMeta: metav1.ObjectMeta{Name: "myservice", Namespace: "mynamespace"},
+		Spec:       ibmcloudv1.ServiceSpec{},
+	}
+
+	errChan := make(chan error, retry.DefaultRetry.Steps)
+
+	errChan <- fmt.Errorf("status failed")
+	client := newMockClient(
+		fake.NewFakeClientWithScheme(scheme, binding, service),
+		MockConfig{ErrChan: errChan},
+	)
+	r := &BindingReconciler{
+		Client: client,
+		Log:    testLogger(t),
+		Scheme: scheme,
+	}
+
+	result, err := r.updateStatusOnline(nil, binding)
+	assert.Equal(t, ctrl.Result{
+		Requeue: true,
+	}, result)
+	assert.EqualError(t, err, "status failed")
+}
+
+func TestBindingUpdateStatusOnlineFailedWithGetError(t *testing.T) {
+	t.Parallel()
+	scheme := schemas(t)
+	binding := &ibmcloudv1.Binding{
+		ObjectMeta: metav1.ObjectMeta{Name: "myservice", Namespace: "mynamespace"},
+		Spec:       ibmcloudv1.BindingSpec{},
+	}
+	errChan := make(chan error, retry.DefaultRetry.Steps)
+	//there will be no error when do updating
+	errChan <- nil
+	client := newMockClient(
+		// the service and binding does not add so Get will return error
+		fake.NewFakeClientWithScheme(scheme),
+		MockConfig{ErrChan: errChan},
+	)
+	r := &BindingReconciler{
+		Client: client,
+		Log:    testLogger(t),
+		Scheme: scheme,
+	}
+
+	result, err := r.updateStatusOnline(nil, binding)
+	assert.Equal(t, ctrl.Result{
+		Requeue: true,
+	}, result)
+	assert.Error(t, err)
+	assert.Equal(t, true, k8sErrors.IsNotFound(err))
 }
 
 func TestBindingSetupWithManager(t *testing.T) {

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,12 @@ func (m *mockClient) Status() client.StatusWriter {
 
 func (s *mockStatusWriter) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error {
 	s.lastStatusUpdate = obj.DeepCopyObject()
+	if s.ErrChan != nil {
+		err := <-s.ErrChan
+		return err
+	}
 	return s.StatusUpdateErr
+
 }
 
 func (m *mockClient) LastStatusUpdate() runtime.Object {