fix: set last-sync-time only on successful reconcile (#66)

internal/controller/enterprise_controller.go

@@ -59,13 +59,6 @@ func (r *EnterpriseReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, nil
 	}
 
-	annotations.SetLastSyncTime(enterprise)
-	err = r.Update(ctx, enterprise)
-	if err != nil {
-		log.Error(err, "can not set annotation")
-		return ctrl.Result{}, err
-	}
-
 	enterpriseClient := garmClient.NewEnterpriseClient()
 
 	// Handle deleted enterprises
@@ -120,6 +113,11 @@ func (r *EnterpriseReconciler) reconcileNormal(ctx context.Context, client garmC
 		return ctrl.Result{}, err
 	}
 
+	if err = annotations.SetLastSyncTime(enterprise, r.Client); err != nil {
+		log.Error(err, fmt.Sprintf("can not set annotation: %s", key.LastSyncTimeAnnotation))
+		return ctrl.Result{}, err
+	}
+
 	log.Info("reconciling enterprise successfully done")
 
 	return ctrl.Result{}, nil

internal/controller/enterprise_controller_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/cloudbase/garm/client/enterprises"
 	"github.com/cloudbase/garm/params"
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -21,6 +22,7 @@ import (
 	garmoperatorv1alpha1 "github.com/mercedes-benz/garm-operator/api/v1alpha1"
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 	"github.com/mercedes-benz/garm-operator/pkg/client/mock"
+	"github.com/mercedes-benz/garm-operator/pkg/util/annotations"
 )
 
 func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
@@ -531,6 +533,12 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 				return
 			}
 
+			// test last-sync-time
+			assert.Equal(t, annotations.HasAnnotation(enterprise, key.LastSyncTimeAnnotation), true)
+
+			// clear out annotations to avoid comparison errors
+			enterprise.ObjectMeta.Annotations = nil
+
 			// empty resource version to avoid comparison errors
 			enterprise.ObjectMeta.ResourceVersion = ""
 			if !reflect.DeepEqual(enterprise, tt.expectedObject) {

internal/controller/organization_controller.go

@@ -58,13 +58,6 @@ func (r *OrganizationReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return ctrl.Result{}, nil
 	}
 
-	annotations.SetLastSyncTime(organization)
-	err = r.Update(ctx, organization)
-	if err != nil {
-		log.Error(err, "can not set annotation")
-		return ctrl.Result{}, err
-	}
-
 	organizationClient := garmClient.NewOrganizationClient()
 
 	// Handle deleted organizations
@@ -119,6 +112,11 @@ func (r *OrganizationReconciler) reconcileNormal(ctx context.Context, client gar
 		return ctrl.Result{}, err
 	}
 
+	if err = annotations.SetLastSyncTime(organization, r.Client); err != nil {
+		log.Error(err, "can not set annotation")
+		return ctrl.Result{}, err
+	}
+
 	log.Info("reconciling organization successfully done")
 
 	return ctrl.Result{}, nil

internal/controller/organization_controller_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/cloudbase/garm/client/organizations"
 	"github.com/cloudbase/garm/params"
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -21,6 +22,7 @@ import (
 	garmoperatorv1alpha1 "github.com/mercedes-benz/garm-operator/api/v1alpha1"
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 	"github.com/mercedes-benz/garm-operator/pkg/client/mock"
+	"github.com/mercedes-benz/garm-operator/pkg/util/annotations"
 )
 
 func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
@@ -531,6 +533,12 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 				return
 			}
 
+			// test last-sync-time
+			assert.Equal(t, annotations.HasAnnotation(organization, key.LastSyncTimeAnnotation), true)
+
+			// clear out annotations to avoid comparison errors
+			organization.ObjectMeta.Annotations = nil
+
 			// empty resource version to avoid comparison errors
 			organization.ObjectMeta.ResourceVersion = ""
 			if !reflect.DeepEqual(organization, tt.expectedObject) {

internal/controller/pool_controller.go

@@ -69,13 +69,6 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		return ctrl.Result{}, nil
 	}
 
-	annotations.SetLastSyncTime(pool)
-	err := r.Update(ctx, pool)
-	if err != nil {
-		log.Error(err, "can not set annotation")
-		return ctrl.Result{}, err
-	}
-
 	poolClient := garmClient.NewPoolClient()
 
 	instanceClient := garmClient.NewInstanceClient()
@@ -196,6 +189,12 @@ func (r *PoolReconciler) reconcileUpdate(ctx context.Context, garmClient garmCli
 		}
 	}
 
+	err = annotations.SetLastSyncTime(pool, r.Client)
+	if err != nil {
+		log.Error(err, "can not set annotation")
+		return ctrl.Result{}, err
+	}
+
 	return ctrl.Result{}, nil
 }
 
@@ -311,13 +310,21 @@ func (r *PoolReconciler) handleUpdateError(ctx context.Context, pool *garmoperat
 }
 
 func (r *PoolReconciler) handleSuccessfulUpdate(ctx context.Context, pool *garmoperatorv1alpha1.Pool, garmPool params.Pool) (ctrl.Result, error) {
+	log := log.FromContext(ctx)
+
 	pool.Status.ID = garmPool.ID
 	pool.Status.IdleRunners = garmPool.MinIdleRunners
 	pool.Status.LastSyncError = ""
 
 	if err := r.updatePoolCRStatus(ctx, pool); err != nil {
 		return ctrl.Result{}, err
 	}
+
+	if err := annotations.SetLastSyncTime(pool, r.Client); err != nil {
+		log.Error(err, "can not set annotation")
+		return ctrl.Result{}, err
+	}
+
 	return ctrl.Result{}, nil
 }
 

internal/controller/pool_controller_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/cloudbase/garm/client/instances"
 	"github.com/cloudbase/garm/client/pools"
 	"github.com/cloudbase/garm/params"
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -25,6 +26,7 @@ import (
 	garmoperatorv1alpha1 "github.com/mercedes-benz/garm-operator/api/v1alpha1"
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 	"github.com/mercedes-benz/garm-operator/pkg/client/mock"
+	"github.com/mercedes-benz/garm-operator/pkg/util/annotations"
 )
 
 const namespaceName = "test-namespace"
@@ -679,6 +681,12 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 				return
 			}
 
+			// test last-sync-time
+			assert.Equal(t, annotations.HasAnnotation(pool, key.LastSyncTimeAnnotation), true)
+
+			// clear out annotations to avoid comparison errors
+			pool.ObjectMeta.Annotations = nil
+
 			// empty resource version to avoid comparison errors
 			pool.ObjectMeta.ResourceVersion = ""
 			if !reflect.DeepEqual(pool, tt.expectedObject) {

internal/controller/repository_controller.go

@@ -58,13 +58,6 @@ func (r *RepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, nil
 	}
 
-	annotations.SetLastSyncTime(repository)
-	err = r.Update(ctx, repository)
-	if err != nil {
-		log.Error(err, "can not set annotation")
-		return ctrl.Result{}, err
-	}
-
 	repositoryClient := garmClient.NewRepositoryClient()
 
 	// Handle deleted repositories
@@ -118,6 +111,11 @@ func (r *RepositoryReconciler) reconcileNormal(ctx context.Context, client garmC
 		return ctrl.Result{}, err
 	}
 
+	if err = annotations.SetLastSyncTime(repository, r.Client); err != nil {
+		log.Error(err, "can not set annotation")
+		return ctrl.Result{}, err
+	}
+
 	log.Info("reconciling repository successfully done")
 
 	return ctrl.Result{}, nil

internal/controller/repository_controller_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/cloudbase/garm/client/repositories"
 	"github.com/cloudbase/garm/params"
+	"github.com/stretchr/testify/assert"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -21,6 +22,7 @@ import (
 	garmoperatorv1alpha1 "github.com/mercedes-benz/garm-operator/api/v1alpha1"
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 	"github.com/mercedes-benz/garm-operator/pkg/client/mock"
+	"github.com/mercedes-benz/garm-operator/pkg/util/annotations"
 )
 
 func TestRepositoryReconciler_reconcileNormal(t *testing.T) {
@@ -558,6 +560,12 @@ func TestRepositoryReconciler_reconcileNormal(t *testing.T) {
 				return
 			}
 
+			// test last-sync-time
+			assert.Equal(t, annotations.HasAnnotation(repository, key.LastSyncTimeAnnotation), true)
+
+			// clear out annotations to avoid comparison errors
+			repository.ObjectMeta.Annotations = nil
+
 			// empty resource version to avoid comparison errors
 			repository.ObjectMeta.ResourceVersion = ""
 			if !reflect.DeepEqual(repository, tt.expectedObject) {

internal/controller/runner_controller.go

@@ -54,7 +54,6 @@ func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 }
 
 func (r *RunnerReconciler) reconcile(ctx context.Context, req ctrl.Request, instanceClient garmClient.InstanceClient) (ctrl.Result, error) {
-	log := log.FromContext(ctx)
 	// try fetch runner instance in garm db with events coming from reconcile loop events of RunnerCR or from manually enqueued events of garm api.
 	garmRunner, err := r.getGarmRunnerInstance(instanceClient, req.Name)
 	if err != nil {
@@ -67,13 +66,6 @@ func (r *RunnerReconciler) reconcile(ctx context.Context, req ctrl.Request, inst
 		return r.handleCreateRunnerCR(ctx, req, err, garmRunner)
 	}
 
-	annotations.SetLastSyncTime(runner)
-	err = r.Update(ctx, runner)
-	if err != nil {
-		log.Error(err, "can not set annotation")
-		return ctrl.Result{}, err
-	}
-
 	// delete runner in garm db
 	if !runner.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, instanceClient, garmRunner)
@@ -101,8 +93,8 @@ func (r *RunnerReconciler) createRunnerCR(ctx context.Context, garmRunner *param
 		},
 		Spec: garmoperatorv1alpha1.RunnerSpec{},
 	}
-	err := r.Create(ctx, runnerObj)
-	if err != nil {
+
+	if err := r.Create(ctx, runnerObj); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -189,6 +181,12 @@ func (r *RunnerReconciler) updateRunnerStatus(ctx context.Context, runner *garmo
 		log.Error(err, "unable to update Runner status")
 		return ctrl.Result{}, err
 	}
+
+	if err = annotations.SetLastSyncTime(runner, r.Client); err != nil {
+		log.Error(err, "can not set annotation")
+		return ctrl.Result{}, err
+	}
+
 	return ctrl.Result{}, nil
 }
 

internal/controller/runner_controller_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 	"github.com/mercedes-benz/garm-operator/pkg/client/mock"
 	"github.com/mercedes-benz/garm-operator/pkg/config"
+	"github.com/mercedes-benz/garm-operator/pkg/util/annotations"
 )
 
 func TestRunnerReconciler_reconcileCreate(t *testing.T) {
@@ -133,6 +134,12 @@ func TestRunnerReconciler_reconcileCreate(t *testing.T) {
 				return
 			}
 
+			// test last-sync-time
+			assert.Equal(t, annotations.HasAnnotation(runner, key.LastSyncTimeAnnotation), true)
+
+			// clear out annotations to avoid comparison errors
+			runner.ObjectMeta.Annotations = nil
+
 			// empty resource version to avoid comparison errors
 			runner.ObjectMeta.ResourceVersion = ""
 			if !reflect.DeepEqual(runner, tt.expectedObject) {
@@ -451,6 +458,7 @@ func TestRunnerReconciler_reconcileDeleteCR(t *testing.T) {
 				close(fakeChan)
 			}()
 
+			// receive events in fake channel and compare if expected event occurs by filtering received events in fake channel by expected event
 			var eventCount int
 			for obj := range fakeChan {
 				t.Logf("Received Event: %s", obj.Object.GetName())

pkg/util/annotations/helpers.go

@@ -3,20 +3,22 @@
 package annotations
 
 import (
+	"context"
 	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/mercedes-benz/garm-operator/pkg/client/key"
 )
 
 // IsPaused returns true if the object has the `paused` annotation.
 func IsPaused(o metav1.Object) bool {
-	return hasAnnotation(o, key.PausedAnnotation)
+	return HasAnnotation(o, key.PausedAnnotation)
 }
 
-// hasAnnotation returns true if the object has the specified annotation.
-func hasAnnotation(o metav1.Object, annotation string) bool {
+// HasAnnotation returns true if the object has the specified annotation.
+func HasAnnotation(o metav1.Object, annotation string) bool {
 	annotations := o.GetAnnotations()
 	if annotations == nil {
 		return false
@@ -25,10 +27,11 @@ func hasAnnotation(o metav1.Object, annotation string) bool {
 	return ok
 }
 
-func SetLastSyncTime(o metav1.Object) {
+func SetLastSyncTime(o client.Object, client client.Client) error {
 	now := time.Now().UTC()
 	newAnnotations := appendAnnotations(o, key.LastSyncTimeAnnotation, now.Format(time.RFC3339))
 	o.SetAnnotations(newAnnotations)
+	return client.Update(context.Background(), o)
 }
 
 func appendAnnotations(o metav1.Object, kayValuePair ...string) map[string]string {