Advisor: Clean up old checks (#100375)

apps/advisor/pkg/app/checkscheduler/checkscheduler.go

@@ -3,6 +3,7 @@ package checkscheduler
 import (
 	"context"
 	"fmt"
+	"sort"
 	"time"
 
 	"github.com/grafana/grafana-app-sdk/app"
@@ -16,6 +17,7 @@ import (
 )
 
 const evaluateChecksInterval = 24 * time.Hour
+const maxChecks = 10
 
 // Runner is a "runnable" app used to be able to expose and API endpoint
 // with the existing checks types. This does not need to be a CRUD resource, but it is
@@ -78,6 +80,11 @@ func (r *Runner) Run(ctx context.Context) error {
 				klog.Error("Error creating new check reports", "error", err)
 			}
 
+			err = r.cleanupChecks(ctx)
+			if err != nil {
+				klog.Error("Error cleaning up old check reports", "error", err)
+			}
+
 			if nextSendInterval != evaluateChecksInterval {
 				nextSendInterval = evaluateChecksInterval
 			}
@@ -127,3 +134,45 @@ func (r *Runner) createChecks(ctx context.Context) error {
 	}
 	return nil
 }
+
+// cleanupChecks deletes the olders checks if the number of checks exceeds the limit.
+func (r *Runner) cleanupChecks(ctx context.Context) error {
+	list, err := r.client.List(ctx, metav1.NamespaceDefault, resource.ListOptions{Limit: -1})
+	if err != nil {
+		return err
+	}
+
+	// organize checks by type
+	checksByType := map[string][]resource.Object{}
+	for _, check := range list.GetItems() {
+		labels := check.GetLabels()
+		checkType, ok := labels[checks.TypeLabel]
+		if !ok {
+			klog.Error("Check type not found in labels", "check", check)
+			continue
+		}
+		checksByType[checkType] = append(checksByType[checkType], check)
+	}
+
+	for _, checks := range checksByType {
+		if len(checks) > maxChecks {
+			// Sort checks by creation time
+			sort.Slice(checks, func(i, j int) bool {
+				ti := checks[i].GetCreationTimestamp().Time
+				tj := checks[j].GetCreationTimestamp().Time
+				return ti.Before(tj)
+			})
+			// Delete the oldest checks
+			for i := 0; i < len(checks)-maxChecks; i++ {
+				check := checks[i]
+				id := check.GetStaticMetadata().Identifier()
+				err := r.client.Delete(ctx, id, resource.DeleteOptions{})
+				if err != nil {
+					return fmt.Errorf("error deleting check: %w", err)
+				}
+			}
+		}
+	}
+
+	return nil
+}

apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go

@@ -3,51 +3,18 @@ package checkscheduler
 import (
 	"context"
 	"errors"
+	"fmt"
+	"math/rand/v2"
 	"testing"
+	"time"
 
 	"github.com/grafana/grafana-app-sdk/resource"
 	advisorv0alpha1 "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1"
 	"github.com/grafana/grafana/apps/advisor/pkg/app/checks"
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-type MockCheckService struct {
-	checks []checks.Check
-}
-
-func (m *MockCheckService) Checks() []checks.Check {
-	return m.checks
-}
-
-type MockClient struct {
-	resource.Client
-	listFunc   func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error)
-	createFunc func(ctx context.Context, identifier resource.Identifier, obj resource.Object, options resource.CreateOptions) (resource.Object, error)
-}
-
-func (m *MockClient) List(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
-	return m.listFunc(ctx, namespace, options)
-}
-
-func (m *MockClient) Create(ctx context.Context, identifier resource.Identifier, obj resource.Object, options resource.CreateOptions) (resource.Object, error) {
-	return m.createFunc(ctx, identifier, obj, options)
-}
-
-type mockCheck struct {
-	checks.Check
-
-	id    string
-	steps []checks.Step
-}
-
-func (m *mockCheck) ID() string {
-	return m.id
-}
-
-func (m *mockCheck) Steps() []checks.Step {
-	return m.steps
-}
-
 func TestRunner_Run_ErrorOnList(t *testing.T) {
 	mockCheckService := &MockCheckService{}
 	mockClient := &MockClient{
@@ -129,3 +96,142 @@ func TestRunner_createChecks_Success(t *testing.T) {
 	err := runner.createChecks(context.Background())
 	assert.NoError(t, err)
 }
+
+func TestRunner_cleanupChecks_ErrorOnList(t *testing.T) {
+	mockClient := &MockClient{
+		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+			return nil, errors.New("list error")
+		},
+	}
+
+	runner := &Runner{
+		client: mockClient,
+	}
+
+	err := runner.cleanupChecks(context.Background())
+	assert.Error(t, err)
+}
+
+func TestRunner_cleanupChecks_WithinMax(t *testing.T) {
+	mockClient := &MockClient{
+		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+			return &advisorv0alpha1.CheckList{
+				Items: []advisorv0alpha1.Check{{}, {}},
+			}, nil
+		},
+		deleteFunc: func(ctx context.Context, identifier resource.Identifier, options resource.DeleteOptions) error {
+			return fmt.Errorf("shouldn't be called")
+		},
+	}
+
+	runner := &Runner{
+		client: mockClient,
+	}
+
+	err := runner.cleanupChecks(context.Background())
+	assert.NoError(t, err)
+}
+
+func TestRunner_cleanupChecks_ErrorOnDelete(t *testing.T) {
+	mockClient := &MockClient{
+		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+			items := make([]advisorv0alpha1.Check, 0, maxChecks+1)
+			for i := 0; i < maxChecks+1; i++ {
+				item := advisorv0alpha1.Check{}
+				item.ObjectMeta.SetLabels(map[string]string{
+					checks.TypeLabel: "mock",
+				})
+				items = append(items, item)
+			}
+			return &advisorv0alpha1.CheckList{
+				Items: items,
+			}, nil
+		},
+		deleteFunc: func(ctx context.Context, identifier resource.Identifier, options resource.DeleteOptions) error {
+			return errors.New("delete error")
+		},
+	}
+
+	runner := &Runner{
+		client: mockClient,
+	}
+	err := runner.cleanupChecks(context.Background())
+	assert.ErrorContains(t, err, "delete error")
+}
+
+func TestRunner_cleanupChecks_Success(t *testing.T) {
+	itemsDeleted := []string{}
+	items := make([]advisorv0alpha1.Check, 0, maxChecks+1)
+	for i := 0; i < maxChecks+1; i++ {
+		item := advisorv0alpha1.Check{}
+		item.ObjectMeta.SetName(fmt.Sprintf("check-%d", i))
+		item.ObjectMeta.SetLabels(map[string]string{
+			checks.TypeLabel: "mock",
+		})
+		item.ObjectMeta.SetCreationTimestamp(metav1.NewTime(time.Time{}.Add(time.Duration(i) * time.Hour)))
+		items = append(items, item)
+	}
+	// shuffle the items to ensure the oldest are deleted
+	rand.Shuffle(len(items), func(i, j int) { items[i], items[j] = items[j], items[i] })
+
+	mockClient := &MockClient{
+		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+			return &advisorv0alpha1.CheckList{
+				Items: items,
+			}, nil
+		},
+		deleteFunc: func(ctx context.Context, identifier resource.Identifier, options resource.DeleteOptions) error {
+			itemsDeleted = append(itemsDeleted, identifier.Name)
+			return nil
+		},
+	}
+
+	runner := &Runner{
+		client: mockClient,
+	}
+	err := runner.cleanupChecks(context.Background())
+	assert.NoError(t, err)
+	assert.Equal(t, []string{"check-0"}, itemsDeleted)
+}
+
+type MockCheckService struct {
+	checks []checks.Check
+}
+
+func (m *MockCheckService) Checks() []checks.Check {
+	return m.checks
+}
+
+type MockClient struct {
+	resource.Client
+	listFunc   func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error)
+	createFunc func(ctx context.Context, identifier resource.Identifier, obj resource.Object, options resource.CreateOptions) (resource.Object, error)
+	deleteFunc func(ctx context.Context, identifier resource.Identifier, options resource.DeleteOptions) error
+}
+
+func (m *MockClient) List(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+	return m.listFunc(ctx, namespace, options)
+}
+
+func (m *MockClient) Create(ctx context.Context, identifier resource.Identifier, obj resource.Object, options resource.CreateOptions) (resource.Object, error) {
+	return m.createFunc(ctx, identifier, obj, options)
+}
+
+func (m *MockClient) Delete(ctx context.Context, identifier resource.Identifier, options resource.DeleteOptions) error {
+	return m.deleteFunc(ctx, identifier, options)
+}
+
+type mockCheck struct {
+	checks.Check
+
+	id    string
+	steps []checks.Step
+}
+
+func (m *mockCheck) ID() string {
+	return m.id
+}
+
+func (m *mockCheck) Steps() []checks.Step {
+	return m.steps
+}