Advisor: Mark unprocessed checks as errored (#102888)

apps/advisor/pkg/app/checks/utils.go

@@ -1,17 +1,21 @@
 package checks
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 
 	"github.com/grafana/authlib/types"
+	"github.com/grafana/grafana-app-sdk/resource"
 	advisor "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
-	TypeLabel        = "advisor.grafana.app/type"
-	StatusAnnotation = "advisor.grafana.app/status"
+	TypeLabel                 = "advisor.grafana.app/type"
+	StatusAnnotation          = "advisor.grafana.app/status"
+	StatusAnnotationError     = "error"
+	StatusAnnotationProcessed = "processed"
 )
 
 func NewCheckReportFailure(
@@ -38,3 +42,22 @@ func GetNamespace(stackID string) (string, error) {
 	}
 	return types.CloudNamespaceFormatter(stackId), nil
 }
+
+func GetStatusAnnotation(obj resource.Object) string {
+	return obj.GetAnnotations()[StatusAnnotation]
+}
+
+func SetStatusAnnotation(ctx context.Context, client resource.Client, obj resource.Object, status string) error {
+	annotations := obj.GetAnnotations()
+	if annotations == nil {
+		annotations = map[string]string{}
+	}
+	annotations[StatusAnnotation] = status
+	return client.PatchInto(ctx, obj.GetStaticMetadata().Identifier(), resource.PatchRequest{
+		Operations: []resource.PatchOperation{{
+			Operation: resource.PatchOpAdd,
+			Path:      "/metadata/annotations",
+			Value:     annotations,
+		}},
+	}, resource.PatchOptions{}, obj)
+}

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

@@ -116,6 +116,7 @@ func (r *Runner) Run(ctx context.Context) error {
 			}
 			ticker.Reset(nextSendInterval)
 		case <-ctx.Done():
+			r.markUnprocessedChecksAsErrored(ctx)
 			return ctx.Err()
 		}
 	}
@@ -228,3 +229,21 @@ func getMaxHistory(pluginConfig map[string]string) (int, error) {
 	}
 	return maxHistory, nil
 }
+
+func (r *Runner) markUnprocessedChecksAsErrored(ctx context.Context) {
+	list, err := r.client.List(ctx, r.namespace, resource.ListOptions{})
+	if err != nil {
+		r.log.Error("Error getting checks", "error", err)
+		return
+	}
+
+	for _, check := range list.GetItems() {
+		if checks.GetStatusAnnotation(check) == "" {
+			r.log.Error("Check is unprocessed", "check", check.GetStaticMetadata().Identifier())
+			err := checks.SetStatusAnnotation(ctx, r.client, check, checks.StatusAnnotationError)
+			if err != nil {
+				r.log.Error("Error setting check status to error", "error", err)
+			}
+		}
+	}
+}

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

@@ -250,6 +250,41 @@ func Test_getMaxHistory(t *testing.T) {
 	})
 }
 
+func Test_markUnprocessedChecksAsErrored(t *testing.T) {
+	checkList := &advisorv0alpha1.CheckList{
+		Items: []advisorv0alpha1.Check{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "check-1",
+				},
+			},
+		},
+	}
+	patchOperation := resource.PatchOperation{}
+	identifier := resource.Identifier{}
+	mockClient := &MockClient{
+		listFunc: func(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
+			return checkList, nil
+		},
+		patchFunc: func(ctx context.Context, id resource.Identifier, patch resource.PatchRequest, options resource.PatchOptions, into resource.Object) error {
+			patchOperation = patch.Operations[0]
+			identifier = id
+			return nil
+		},
+	}
+	runner := &Runner{
+		client: mockClient,
+		log:    log.NewNopLogger(),
+	}
+	runner.markUnprocessedChecksAsErrored(context.Background())
+	assert.Equal(t, "check-1", identifier.Name)
+	assert.Equal(t, "/metadata/annotations", patchOperation.Path)
+	expectedAnnotations := map[string]string{
+		checks.StatusAnnotation: "error",
+	}
+	assert.Equal(t, expectedAnnotations, patchOperation.Value)
+}
+
 type MockCheckService struct {
 	checks []checks.Check
 }
@@ -263,6 +298,7 @@ type MockClient struct {
 	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
+	patchFunc  func(ctx context.Context, identifier resource.Identifier, patch resource.PatchRequest, options resource.PatchOptions, into resource.Object) error
 }
 
 func (m *MockClient) List(ctx context.Context, namespace string, options resource.ListOptions) (resource.ListObject, error) {
@@ -277,6 +313,10 @@ func (m *MockClient) Delete(ctx context.Context, identifier resource.Identifier,
 	return m.deleteFunc(ctx, identifier, options)
 }
 
+func (m *MockClient) PatchInto(ctx context.Context, identifier resource.Identifier, patch resource.PatchRequest, options resource.PatchOptions, into resource.Object) error {
+	return m.patchFunc(ctx, identifier, patch, options, into)
+}
+
 type mockCheck struct {
 	checks.Check
 

apps/advisor/pkg/app/utils.go

@@ -29,27 +29,8 @@ func getCheck(obj resource.Object, checkMap map[string]checks.Check) (checks.Che
 	return c, nil
 }
 
-func getStatusAnnotation(obj resource.Object) string {
-	return obj.GetAnnotations()[checks.StatusAnnotation]
-}
-
-func setStatusAnnotation(ctx context.Context, client resource.Client, obj resource.Object, status string) error {
-	annotations := obj.GetAnnotations()
-	if annotations == nil {
-		annotations = map[string]string{}
-	}
-	annotations[checks.StatusAnnotation] = status
-	return client.PatchInto(ctx, obj.GetStaticMetadata().Identifier(), resource.PatchRequest{
-		Operations: []resource.PatchOperation{{
-			Operation: resource.PatchOpAdd,
-			Path:      "/metadata/annotations",
-			Value:     annotations,
-		}},
-	}, resource.PatchOptions{}, obj)
-}
-
 func processCheck(ctx context.Context, client resource.Client, obj resource.Object, check checks.Check) error {
-	status := getStatusAnnotation(obj)
+	status := checks.GetStatusAnnotation(obj)
 	if status != "" {
 		// Check already processed
 		return nil
@@ -61,7 +42,7 @@ func processCheck(ctx context.Context, client resource.Client, obj resource.Obje
 	// Get the items to check
 	items, err := check.Items(ctx)
 	if err != nil {
-		setErr := setStatusAnnotation(ctx, client, obj, "error")
+		setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError)
 		if setErr != nil {
 			return setErr
 		}
@@ -71,7 +52,7 @@ func processCheck(ctx context.Context, client resource.Client, obj resource.Obje
 	steps := check.Steps()
 	failures, err := runStepsInParallel(ctx, &c.Spec, steps, items)
 	if err != nil {
-		setErr := setStatusAnnotation(ctx, client, obj, "error")
+		setErr := checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationError)
 		if setErr != nil {
 			return setErr
 		}
@@ -82,7 +63,7 @@ func processCheck(ctx context.Context, client resource.Client, obj resource.Obje
 		Failures: failures,
 		Count:    int64(len(items)),
 	}
-	err = setStatusAnnotation(ctx, client, obj, "processed")
+	err = checks.SetStatusAnnotation(ctx, client, obj, checks.StatusAnnotationProcessed)
 	if err != nil {
 		return err
 	}

apps/advisor/pkg/app/utils_test.go

@@ -48,17 +48,6 @@ func TestGetCheck_UnknownType(t *testing.T) {
 	assert.Contains(t, err.Error(), "unknown check type unknownType")
 }
 
-func TestSetStatusAnnotation(t *testing.T) {
-	obj := &advisorv0alpha1.Check{}
-	obj.SetAnnotations(map[string]string{})
-	client := &mockClient{}
-	ctx := context.TODO()
-
-	err := setStatusAnnotation(ctx, client, obj, "processed")
-	assert.NoError(t, err)
-	assert.Equal(t, "processed", obj.GetAnnotations()[checks.StatusAnnotation])
-}
-
 func TestProcessCheck(t *testing.T) {
 	obj := &advisorv0alpha1.Check{}
 	obj.SetAnnotations(map[string]string{})