Advisor: Avoid write if checktype exists (#110340)

apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer.go

@@ -7,6 +7,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/grafana/grafana-app-sdk/app"
 	"github.com/grafana/grafana-app-sdk/k8s"
 	"github.com/grafana/grafana-app-sdk/logging"
@@ -60,38 +62,6 @@ func New(cfg app.Config, log logging.Logger) (app.Runnable, error) {
 	}, nil
 }
 
-func (r *Runner) createOrUpdate(ctx context.Context, log logging.Logger, obj resource.Object) error {
-	id := obj.GetStaticMetadata().Identifier()
-	_, err := r.client.Create(ctx, id, obj, resource.CreateOptions{})
-	if err != nil {
-		if errors.IsAlreadyExists(err) {
-			// Already exists, update
-			log.Debug("Check type already exists, updating", "identifier", id)
-			// Retrieve current annotations to avoid overriding them
-			current, err := r.client.Get(ctx, obj.GetStaticMetadata().Identifier())
-			if err != nil {
-				return err
-			}
-			currentAnnotations := current.GetAnnotations()
-			if currentAnnotations == nil {
-				currentAnnotations = make(map[string]string)
-			}
-			annotations := obj.GetAnnotations()
-			maps.Copy(currentAnnotations, annotations)
-			obj.SetAnnotations(currentAnnotations) // This will update the annotations in the object
-			_, err = r.client.Update(ctx, id, obj, resource.UpdateOptions{})
-			if err != nil && !errors.IsAlreadyExists(err) {
-				// Ignore the error, it's probably due to a race condition
-				log.Info("Error updating check type, ignoring", "error", err)
-			}
-			return nil
-		}
-		return err
-	}
-	log.Debug("Check type registered successfully", "identifier", id)
-	return nil
-}
-
 func (r *Runner) Run(ctx context.Context) error {
 	logger := r.log.WithContext(ctx)
 	for _, t := range r.checkRegistry.Checks() {
@@ -121,26 +91,139 @@ func (r *Runner) Run(ctx context.Context) error {
 				Steps: stepTypes,
 			},
 		}
-		for i := 0; i < r.retryAttempts; i++ {
-			err := r.createOrUpdate(context.WithoutCancel(ctx), logger, obj)
-			if err != nil {
-				if strings.Contains(err.Error(), "apiserver is shutting down") {
-					logger.Debug("Error creating check type, not retrying", "error", err)
-					return nil
-				}
-				logger.Debug("Error creating check type, retrying", "error", err, "attempt", i+1)
-				if i == r.retryAttempts-1 {
-					logger.Error("Unable to register check type", "check_type", t.ID(), "error", err)
-				} else {
-					// Calculate exponential backoff delay: baseDelay * 2^attempt
-					delay := r.retryDelay * time.Duration(1<<i)
-					time.Sleep(delay)
-				}
-				continue
-			}
-			logger.Debug("Check type registered successfully", "check_type", t.ID())
-			break
+		err := r.registerCheckType(ctx, logger, t.ID(), obj)
+		if err != nil {
+			return err
 		}
 	}
 	return nil
 }
+
+func (r *Runner) registerCheckType(ctx context.Context, logger logging.Logger, checkType string, obj resource.Object) error {
+	for i := 0; i < r.retryAttempts; i++ {
+		current, err := r.client.Get(ctx, obj.GetStaticMetadata().Identifier())
+		if err != nil {
+			if errors.IsNotFound(err) {
+				// Check type does not exist, create it
+				err = r.create(context.WithoutCancel(ctx), logger, obj)
+				if err != nil {
+					if !r.shouldRetry(err, logger, i+1, checkType) {
+						return nil
+					}
+					// Retry
+					continue
+				}
+				// Success
+				logger.Debug("Check type created successfully", "check_type", checkType)
+				break
+			}
+			if !r.shouldRetry(err, logger, i+1, checkType) {
+				return nil
+			}
+			// Retry
+			continue
+		}
+
+		// Check type already exists, check if it's the same and update if needed
+		logger.Debug("Check type already exists, checking if it's the same", "identifier", obj.GetStaticMetadata().Identifier())
+		if r.needsUpdate(current, obj, logger) {
+			err = r.update(context.WithoutCancel(ctx), logger, obj, current)
+			if err != nil {
+				if !r.shouldRetry(err, logger, i+1, checkType) {
+					return nil
+				}
+				// Retry
+				continue
+			}
+			// Success
+			logger.Debug("Check type updated successfully", "check_type", checkType)
+			break
+		}
+
+		// Check type is the same, no need to update
+		logger.Debug("Check type already registered", "check_type", checkType)
+		break
+	}
+	return nil
+}
+
+func (r *Runner) shouldRetry(err error, logger logging.Logger, attempt int, checkType string) bool {
+	logger.Debug("Error storing check type", "error", err, "attempt", attempt)
+	if isAPIServerShuttingDown(err, logger) {
+		return false
+	}
+	if attempt == r.retryAttempts-1 {
+		logger.Error("Unable to register check type", "check_type", checkType, "error", err)
+		return false
+	}
+	// Calculate exponential backoff delay: baseDelay * 2^attempt
+	delay := r.retryDelay * time.Duration(1<<attempt)
+	time.Sleep(delay)
+	return true
+}
+
+func (r *Runner) create(ctx context.Context, log logging.Logger, obj resource.Object) error {
+	id := obj.GetStaticMetadata().Identifier()
+	_, err := r.client.Create(ctx, id, obj, resource.CreateOptions{})
+	if err != nil {
+		return err
+	}
+	log.Debug("Check type created successfully", "identifier", id)
+	return nil
+}
+
+func (r *Runner) needsUpdate(current, newObj resource.Object, log logging.Logger) bool {
+	needsUpdate := false
+	// Check if the object annotations exist in the current object
+	currentAnnotations := current.GetAnnotations()
+	if currentAnnotations == nil {
+		currentAnnotations = make(map[string]string)
+	}
+	annotations := newObj.GetAnnotations()
+	for k, v := range annotations {
+		if currentAnnotations[k] != v {
+			needsUpdate = true
+		}
+	}
+	// Compare checktype spec steps with current steps
+	currentCheckType := current.(*advisorv0alpha1.CheckType)
+	newCheckType := newObj.(*advisorv0alpha1.CheckType)
+	newSteps := newCheckType.Spec.Steps
+	currentSteps := currentCheckType.Spec.Steps
+	if !cmp.Equal(newSteps, currentSteps, cmpopts.SortSlices(func(a, b advisorv0alpha1.CheckTypeStep) bool {
+		return a.StepID < b.StepID
+	})) {
+		log.Debug("Check type step mismatch, updating", "identifier", newObj.GetStaticMetadata().Identifier())
+		needsUpdate = true
+	}
+	return needsUpdate
+}
+
+func (r *Runner) update(ctx context.Context, log logging.Logger, obj resource.Object, current resource.Object) error {
+	id := obj.GetStaticMetadata().Identifier()
+	log.Debug("Updating check type", "identifier", id)
+
+	currentAnnotations := current.GetAnnotations()
+	if currentAnnotations == nil {
+		currentAnnotations = make(map[string]string)
+	}
+	annotations := obj.GetAnnotations()
+	maps.Copy(currentAnnotations, annotations)
+	obj.SetAnnotations(currentAnnotations) // This will update the annotations in the object
+
+	_, err := r.client.Update(ctx, id, obj, resource.UpdateOptions{})
+	if err != nil && !errors.IsAlreadyExists(err) {
+		// Ignore the error, it's probably due to a race condition
+		log.Info("Error updating check type, ignoring", "error", err)
+	}
+	log.Debug("Check type updated successfully", "identifier", id)
+	return nil
+}
+
+func isAPIServerShuttingDown(err error, logger logging.Logger) bool {
+	if strings.Contains(err.Error(), "apiserver is shutting down") {
+		logger.Debug("Error creating check type, not retrying", "error", err)
+		return true
+	}
+	return false
+}

apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go

@@ -16,6 +16,54 @@ import (
 )
 
 func TestCheckTypesRegisterer_Run(t *testing.T) {
+	newMockCheck := &mockCheck{
+		id: "check1",
+		steps: []checks.Step{
+			&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
+		},
+	}
+	existingObjectDifferentAnnotations := &advisorv0alpha1.CheckType{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "check1",
+			Annotations: map[string]string{
+				checks.NameAnnotation: "existing-name", // Different to trigger update
+			},
+		},
+		Spec: advisorv0alpha1.CheckTypeSpec{
+			Name: "check1",
+			Steps: []advisorv0alpha1.CheckTypeStep{
+				{StepID: "step1", Title: "Step 1", Description: "Description 1"},
+			},
+		},
+	}
+	existingObjectDifferentSteps := &advisorv0alpha1.CheckType{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "check1",
+			Annotations: map[string]string{
+				checks.NameAnnotation: "mock", // Same as check name
+			},
+		},
+		Spec: advisorv0alpha1.CheckTypeSpec{
+			Name: "check1",
+			Steps: []advisorv0alpha1.CheckTypeStep{
+				{StepID: "step2", Title: "Step 2", Description: "Description 2"}, // Different step
+			},
+		},
+	}
+	existingObjectSameContent := &advisorv0alpha1.CheckType{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "check1",
+			Annotations: map[string]string{
+				checks.NameAnnotation: "mock", // Same as check name
+			},
+		},
+		Spec: advisorv0alpha1.CheckTypeSpec{
+			Name: "check1",
+			Steps: []advisorv0alpha1.CheckTypeStep{
+				{StepID: "step1", Title: "Step 1", Description: "Description 1"},
+			},
+		},
+	}
 	tests := []struct {
 		name        string
 		checks      []checks.Check
@@ -25,14 +73,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 		expectedErr error
 	}{
 		{
-			name: "successful create",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
+			name:   "successful create",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return nil, k8sErrs.NewNotFound(schema.GroupResource{}, id.Name)
 			},
 			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
 				return obj, nil
@@ -41,17 +85,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name: "create already exists, successful update",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
-			},
-			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
-				return nil, k8sErrs.NewAlreadyExists(schema.GroupResource{}, obj.GetName())
+			name:   "resource exists with different annotations, should update",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectDifferentAnnotations, nil
 			},
 			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
 				return obj, nil
@@ -59,27 +96,32 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name: "create already exists, with custom annotations",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
-			},
+			name:   "resource exists with different steps, should update",
+			checks: []checks.Check{newMockCheck},
 			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
-				return &advisorv0alpha1.CheckType{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "check1",
-						Annotations: map[string]string{
-							checks.IgnoreStepsAnnotationList: "step1",
-						},
-					},
-				}, nil
+				return existingObjectDifferentSteps, nil
 			},
-			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
-				return nil, k8sErrs.NewAlreadyExists(schema.GroupResource{}, obj.GetName())
+			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
+				return obj, nil
+			},
+			expectedErr: nil,
+		},
+		{
+			name:   "resource exists with same annotations and steps, should not update",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectSameContent, nil
+			},
+			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
+				return nil, errors.New("updateFunc should not be called")
+			},
+			expectedErr: nil,
+		},
+		{
+			name:   "resource exists, with custom annotations preserved",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectDifferentAnnotations, nil
 			},
 			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
 				if obj.GetAnnotations()[checks.IgnoreStepsAnnotationList] != "step1" {
@@ -90,14 +132,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name: "create error",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
+			name:   "create error",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return nil, k8sErrs.NewNotFound(schema.GroupResource{}, id.Name)
 			},
 			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
 				return nil, errors.New("create error")
@@ -106,17 +144,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: errors.New("create error"),
 		},
 		{
-			name: "update error",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
-			},
-			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
-				return nil, k8sErrs.NewAlreadyExists(schema.GroupResource{}, obj.GetName())
+			name:   "update error",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectDifferentAnnotations, nil
 			},
 			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
 				return nil, errors.New("update error")
@@ -124,17 +155,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: errors.New("update error"),
 		},
 		{
-			name: "shutting down error",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
-			},
-			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
-				return nil, k8sErrs.NewAlreadyExists(schema.GroupResource{}, obj.GetName())
+			name:   "shutting down error",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectDifferentAnnotations, nil
 			},
 			updateFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
 				return nil, errors.New("apiserver is shutting down")
@@ -142,14 +166,10 @@ func TestCheckTypesRegisterer_Run(t *testing.T) {
 			expectedErr: nil,
 		},
 		{
-			name: "custom namespace",
-			checks: []checks.Check{
-				&mockCheck{
-					id: "check1",
-					steps: []checks.Step{
-						&mockStep{id: "step1", title: "Step 1", description: "Description 1"},
-					},
-				},
+			name:   "custom namespace",
+			checks: []checks.Check{newMockCheck},
+			getFunc: func(ctx context.Context, id resource.Identifier) (resource.Object, error) {
+				return existingObjectDifferentAnnotations, nil
 			},
 			createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
 				if obj.GetNamespace() != "custom-namespace" {
@@ -262,13 +282,19 @@ func (m *mockClient) Get(ctx context.Context, id resource.Identifier) (resource.
 	if m.getFunc != nil {
 		return m.getFunc(ctx, id)
 	}
-	return advisorv0alpha1.CheckTypeKind().ZeroValue(), nil
+	return nil, errors.New("not implemented")
 }
 
 func (m *mockClient) Create(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) {
-	return m.createFunc(ctx, id, obj, opts)
+	if m.createFunc != nil {
+		return m.createFunc(ctx, id, obj, opts)
+	}
+	return nil, errors.New("not implemented")
 }
 
 func (m *mockClient) Update(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.UpdateOptions) (resource.Object, error) {
-	return m.updateFunc(ctx, id, obj, opts)
+	if m.updateFunc != nil {
+		return m.updateFunc(ctx, id, obj, opts)
+	}
+	return nil, errors.New("not implemented")
 }