fix: remove last-sync-time annotation (#91)

removes last-sync-time annotation which should prevent countless unnecessary reconciles to be triggered.
mercedes-benz · Feb 29, 2024 · b028770 · b028770
1 parent ca4d50d
commit b028770
Show file tree

Hide file tree

Showing 11 changed files with 28 additions and 134 deletions.
diff --git a/internal/controller/enterprise_controller.go b/internal/controller/enterprise_controller.go
@@ -136,11 +136,6 @@ 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
 }

diff --git a/internal/controller/enterprise_controller_test.go b/internal/controller/enterprise_controller_test.go
@@ -10,7 +10,6 @@ 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"
@@ -23,7 +22,6 @@ 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"
 	"github.com/mercedes-benz/garm-operator/pkg/util/conditions"
 )
 
@@ -32,13 +30,12 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 	defer mockCtrl.Finish()
 
 	tests := []struct {
-		name                         string
-		object                       runtime.Object
-		expectGarmRequest            func(m *mock.MockEnterpriseClientMockRecorder)
-		runtimeObjects               []runtime.Object
-		wantErr                      bool
-		expectedObject               *garmoperatorv1alpha1.Enterprise
-		expectLastSyncTimeAnnotation bool
+		name              string
+		object            runtime.Object
+		expectGarmRequest func(m *mock.MockEnterpriseClientMockRecorder)
+		runtimeObjects    []runtime.Object
+		wantErr           bool
+		expectedObject    *garmoperatorv1alpha1.Enterprise
 	}{
 		{
 			name: "enterprise exist - update",
@@ -137,7 +134,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "enterprise exist but spec has changed - update",
@@ -236,7 +232,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "enterprise exist but pool status has changed - update",
@@ -339,7 +334,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "enterprise does not exist - create and update",
@@ -445,7 +439,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "enterprise already exist in garm - update",
@@ -537,7 +530,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "enterprise does not exist in garm - create update",
@@ -644,7 +636,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "secret ref not found condition",
@@ -700,9 +691,8 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 					},
 				},
 			},
-			expectGarmRequest:            func(m *mock.MockEnterpriseClientMockRecorder) {},
-			expectLastSyncTimeAnnotation: false,
-			wantErr:                      true,
+			expectGarmRequest: func(m *mock.MockEnterpriseClientMockRecorder) {},
+			wantErr:           true,
 		},
 	}
 	for _, tt := range tests {
@@ -736,9 +726,6 @@ func TestEnterpriseReconciler_reconcileNormal(t *testing.T) {
 				return
 			}
 
-			// test last-sync-time
-			assert.Equal(t, tt.expectLastSyncTimeAnnotation, annotations.HasAnnotation(enterprise, key.LastSyncTimeAnnotation))
-
 			// clear out annotations to avoid comparison errors
 			enterprise.ObjectMeta.Annotations = nil
 

diff --git a/internal/controller/organization_controller.go b/internal/controller/organization_controller.go
@@ -135,11 +135,6 @@ 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

diff --git a/internal/controller/organization_controller_test.go b/internal/controller/organization_controller_test.go
@@ -10,7 +10,6 @@ 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"
@@ -23,7 +22,6 @@ 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"
 	"github.com/mercedes-benz/garm-operator/pkg/util/conditions"
 )
 
@@ -32,13 +30,12 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 	defer mockCtrl.Finish()
 
 	tests := []struct {
-		name                         string
-		object                       runtime.Object
-		runtimeObjects               []runtime.Object
-		expectGarmRequest            func(m *mock.MockOrganizationClientMockRecorder)
-		wantErr                      bool
-		expectedObject               *garmoperatorv1alpha1.Organization
-		expectLastSyncTimeAnnotation bool
+		name              string
+		object            runtime.Object
+		runtimeObjects    []runtime.Object
+		expectGarmRequest func(m *mock.MockOrganizationClientMockRecorder)
+		wantErr           bool
+		expectedObject    *garmoperatorv1alpha1.Organization
 	}{
 		{
 			name: "organization exist - update",
@@ -137,7 +134,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "organization exist but spec has changed - update",
@@ -236,7 +232,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "organization exist but pool status has changed - update",
@@ -339,7 +334,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "organization does not exist - create and update",
@@ -445,7 +439,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "organization already exist in garm - update",
@@ -537,7 +530,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "organization does not exist in garm - create and update",
@@ -644,7 +636,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				}, nil)
 			},
-			expectLastSyncTimeAnnotation: true,
 		},
 		{
 			name: "secret ref not found condition",
@@ -700,9 +691,8 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 					},
 				},
 			},
-			expectGarmRequest:            func(m *mock.MockOrganizationClientMockRecorder) {},
-			expectLastSyncTimeAnnotation: false,
-			wantErr:                      true,
+			expectGarmRequest: func(m *mock.MockOrganizationClientMockRecorder) {},
+			wantErr:           true,
 		},
 	}
 	for _, tt := range tests {
@@ -736,9 +726,6 @@ func TestOrganizationReconciler_reconcileNormal(t *testing.T) {
 				return
 			}
 
-			// test last-sync-time
-			assert.Equal(t, tt.expectLastSyncTimeAnnotation, annotations.HasAnnotation(organization, key.LastSyncTimeAnnotation))
-
 			// clear out annotations to avoid comparison errors
 			organization.ObjectMeta.Annotations = nil
 

diff --git a/internal/controller/pool_controller.go b/internal/controller/pool_controller.go
@@ -326,19 +326,12 @@ func (r *PoolReconciler) handleUpdateError(ctx context.Context, pool *garmoperat
 }
 
 func (r *PoolReconciler) handleSuccessfulUpdate(ctx context.Context, pool *garmoperatorv1alpha1.Pool) (ctrl.Result, error) {
-	log := log.FromContext(ctx)
-
 	conditions.MarkTrue(pool, conditions.ReadyCondition, conditions.SuccessfulReconcileReason, "")
 
 	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
 }
 

diff --git a/internal/controller/pool_controller_test.go b/internal/controller/pool_controller_test.go
@@ -14,7 +14,6 @@ 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"
@@ -29,7 +28,6 @@ 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"
 	"github.com/mercedes-benz/garm-operator/pkg/util/conditions"
 )
 
@@ -51,11 +49,10 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 		// a list of objects to initialize the fake client with
 		// this can be used to define other existing objects that are referenced by the object to reconcile
 		// e.g. images or other pools ..
-		runtimeObjects               []runtime.Object
-		expectGarmRequest            func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder)
-		wantErr                      bool
-		expectLastSyncTimeAnnotation bool
-		expectedObject               *garmoperatorv1alpha1.Pool
+		runtimeObjects    []runtime.Object
+		expectGarmRequest func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder)
+		wantErr           bool
+		expectedObject    *garmoperatorv1alpha1.Pool
 	}{
 		{
 			name: "pool does not exist in garm - create",
@@ -197,7 +194,6 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					},
 				},
 			},
-			expectLastSyncTimeAnnotation: true,
 			expectGarmRequest: func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder) {
 				poolClient.ListAllPools(pools.NewListPoolsParams()).Return(&pools.ListPoolsOK{Payload: params.Pools{}}, nil)
 
@@ -407,7 +403,6 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					},
 				},
 			},
-			expectLastSyncTimeAnnotation: true,
 			expectGarmRequest: func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder) {
 				poolClient.GetPool(pools.NewGetPoolParams().WithPoolID(outdatedPoolID)).Return(&pools.GetPoolOK{Payload: params.Pool{}}, nil)
 
@@ -596,7 +591,6 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					},
 				},
 			},
-			expectLastSyncTimeAnnotation: true,
 			expectGarmRequest: func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder) {
 				poolClient.GetPool(pools.NewGetPoolParams().WithPoolID(poolID)).Return(&pools.GetPoolOK{Payload: params.Pool{
 					RunnerPrefix: params.RunnerPrefix{
@@ -776,7 +770,6 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					// LastSyncError:          "",
 				},
 			},
-			expectLastSyncTimeAnnotation: true,
 			expectedObject: &garmoperatorv1alpha1.Pool{
 				TypeMeta: metav1.TypeMeta{
 					Kind:       "Pool",
@@ -1269,8 +1262,7 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					},
 				},
 			},
-			wantErr:                      true,
-			expectLastSyncTimeAnnotation: false,
+			wantErr: true,
 			expectGarmRequest: func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder) {
 			},
 		},
@@ -1416,8 +1408,7 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 					},
 				},
 			},
-			expectLastSyncTimeAnnotation: false,
-			wantErr:                      true,
+			wantErr: true,
 			expectGarmRequest: func(poolClient *mock.MockPoolClientMockRecorder, instanceClient *mock.MockInstanceClientMockRecorder) {
 				poolClient.GetPool(pools.NewGetPoolParams().WithPoolID(poolID)).Return(&pools.GetPoolOK{Payload: params.Pool{
 					RunnerPrefix: params.RunnerPrefix{
@@ -1500,9 +1491,6 @@ func TestPoolController_ReconcileCreate(t *testing.T) {
 				return
 			}
 
-			// test last-sync-time
-			assert.Equal(t, annotations.HasAnnotation(pool, key.LastSyncTimeAnnotation), tt.expectLastSyncTimeAnnotation)
-
 			// clear out annotations to avoid comparison errors
 			pool.ObjectMeta.Annotations = nil
 

diff --git a/internal/controller/repository_controller.go b/internal/controller/repository_controller.go
@@ -135,11 +135,6 @@ 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