From 3121633ebac426350a0507cce3e0043408caddb3 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Thu, 27 Mar 2025 12:50:24 +0100 Subject: [PATCH] Advisor: Mark unprocessed checks as errored (#102888) --- apps/advisor/pkg/app/checks/utils.go | 27 ++++++++++++- .../pkg/app/checkscheduler/checkscheduler.go | 19 +++++++++ .../app/checkscheduler/checkscheduler_test.go | 40 +++++++++++++++++++ apps/advisor/pkg/app/utils.go | 27 ++----------- apps/advisor/pkg/app/utils_test.go | 11 ----- 5 files changed, 88 insertions(+), 36 deletions(-) diff --git a/apps/advisor/pkg/app/checks/utils.go b/apps/advisor/pkg/app/checks/utils.go index 145c98d0961..8b4d23705f0 100644 --- a/apps/advisor/pkg/app/checks/utils.go +++ b/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) +} diff --git a/apps/advisor/pkg/app/checkscheduler/checkscheduler.go b/apps/advisor/pkg/app/checkscheduler/checkscheduler.go index cdca40cbd77..053a964259b 100644 --- a/apps/advisor/pkg/app/checkscheduler/checkscheduler.go +++ b/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) + } + } + } +} diff --git a/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go b/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go index 23ce113df4a..e11bf77735d 100644 --- a/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go +++ b/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 diff --git a/apps/advisor/pkg/app/utils.go b/apps/advisor/pkg/app/utils.go index 86ad12a714b..d8a73935e34 100644 --- a/apps/advisor/pkg/app/utils.go +++ b/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 } diff --git a/apps/advisor/pkg/app/utils_test.go b/apps/advisor/pkg/app/utils_test.go index b3b8d36f60b..adcb2b8e28d 100644 --- a/apps/advisor/pkg/app/utils_test.go +++ b/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{})