Merge pull request #526 from sttts/sttts-managed-reconcile-avoid-temporary-data-loss

reconciler/managed: avoid temporary data loss to managed on annotation update

Author: negz

pkg/reconciler/managed/api.go

@@ -22,6 +22,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
@@ -167,18 +168,19 @@ func NewRetryingCriticalAnnotationUpdater(c client.Client) *RetryingCriticalAnno
 
 // UpdateCriticalAnnotations updates (i.e. persists) the annotations of the
 // supplied Object. It retries in the face of any API server error several times
-// in order to ensure annotations that contain critical state are persisted. Any
-// pending changes to the supplied Object's spec, status, or other metadata are
-// reset to their current state according to the API server.
+// in order to ensure annotations that contain critical state are persisted.
+// Pending changes to the supplied Object's spec, status, or other metadata
+// might get reset to their current state according to the API server, e.g. in
+// case of a conflict error.
 func (u *RetryingCriticalAnnotationUpdater) UpdateCriticalAnnotations(ctx context.Context, o client.Object) error {
 	a := o.GetAnnotations()
 	err := retry.OnError(retry.DefaultRetry, resource.IsAPIError, func() error {
-		nn := types.NamespacedName{Name: o.GetName()}
-		if err := u.client.Get(ctx, nn, o); err != nil {
-			return err
+		err := u.client.Update(ctx, o)
+		if kerrors.IsConflict(err) {
+			err = u.client.Get(ctx, types.NamespacedName{Name: o.GetName()}, o)
+			meta.AddAnnotations(o, a)
 		}
-		meta.AddAnnotations(o, a)
-		return u.client.Update(ctx, o)
+		return err
 	})
 	return errors.Wrap(err, errUpdateCriticalAnnotations)
 }

pkg/reconciler/managed/api_test.go

@@ -21,8 +21,10 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
@@ -356,59 +358,85 @@ func TestResolveReferences(t *testing.T) {
 }
 
 func TestRetryingCriticalAnnotationUpdater(t *testing.T) {
-
 	errBoom := errors.New("boom")
 
 	type args struct {
 		ctx context.Context
 		o   client.Object
 	}
+	type want struct {
+		err error
+		o   client.Object
+	}
+
+	setLabels := func(obj client.Object) error {
+		obj.SetLabels(map[string]string{"getcalled": "true"})
+		return nil
+	}
+	objectReturnedByGet := &fake.Managed{}
+	setLabels(objectReturnedByGet)
 
 	cases := map[string]struct {
 		reason string
-		c      client.Client
+		c      *test.MockClient
 		args   args
-		want   error
+		want   want
 	}{
-		"GetError": {
+		"UpdateConflictGetError": {
 			reason: "We should return any error we encounter getting the supplied object",
 			c: &test.MockClient{
-				MockGet: test.NewMockGetFn(errBoom),
+				MockGet: test.NewMockGetFn(errBoom, setLabels),
+				MockUpdate: test.NewMockUpdateFn(kerrors.NewConflict(schema.GroupResource{
+					Group:    "foo.com",
+					Resource: "bars",
+				}, "abc", errBoom)),
 			},
 			args: args{
 				o: &fake.Managed{},
 			},
-			want: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+			want: want{
+				err: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+				o:   objectReturnedByGet,
+			},
 		},
 		"UpdateError": {
 			reason: "We should return any error we encounter updating the supplied object",
 			c: &test.MockClient{
-				MockGet:    test.NewMockGetFn(nil),
+				MockGet:    test.NewMockGetFn(nil, setLabels),
 				MockUpdate: test.NewMockUpdateFn(errBoom),
 			},
 			args: args{
 				o: &fake.Managed{},
 			},
-			want: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+			want: want{
+				err: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+				o:   &fake.Managed{},
+			},
 		},
 		"Success": {
 			reason: "We should return without error if we successfully update our annotations",
 			c: &test.MockClient{
-				MockGet:    test.NewMockGetFn(nil),
+				MockGet:    test.NewMockGetFn(nil, setLabels),
 				MockUpdate: test.NewMockUpdateFn(errBoom),
 			},
 			args: args{
 				o: &fake.Managed{},
 			},
-			want: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+			want: want{
+				err: errors.Wrap(errBoom, errUpdateCriticalAnnotations),
+				o:   &fake.Managed{},
+			},
 		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			u := NewRetryingCriticalAnnotationUpdater(tc.c)
 			got := u.UpdateCriticalAnnotations(tc.args.ctx, tc.args.o)
-			if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" {
+			if diff := cmp.Diff(tc.want.err, got, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.o, tc.args.o); diff != "" {
 				t.Errorf("\n%s\nu.UpdateCriticalAnnotations(...): -want, +got:\n%s", tc.reason, diff)
 			}
 		})