From a3e85d831995014bdc43c0e2058fdd982147eed4 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Thu, 12 Jun 2025 17:37:07 +0200 Subject: [PATCH] Advisor: Fix issues (#106612) --- apps/advisor/pkg/app/app.go | 4 +- .../pkg/app/checkscheduler/checkscheduler.go | 27 ++++++- .../app/checkscheduler/checkscheduler_test.go | 71 +++++++++++++------ 3 files changed, 76 insertions(+), 26 deletions(-) diff --git a/apps/advisor/pkg/app/app.go b/apps/advisor/pkg/app/app.go index b647e86a964..ba84a14f7f7 100644 --- a/apps/advisor/pkg/app/app.go +++ b/apps/advisor/pkg/app/app.go @@ -71,7 +71,7 @@ func New(cfg app.Config) (app.App, error) { logger.Error("Error getting requester", "error", err) return } - ctx = identity.WithRequester(context.Background(), requester) + ctx = identity.WithServiceIdentityContext(context.WithoutCancel(ctx), requester.GetOrgID()) err = processCheck(ctx, logger, client, typesClient, req.Object, check) if err != nil { logger.Error("Error processing check", "error", err) @@ -87,7 +87,7 @@ func New(cfg app.Config) (app.App, error) { logger.Error("Error getting requester", "error", err) return } - ctx = identity.WithRequester(context.Background(), requester) + ctx = identity.WithServiceIdentityContext(context.WithoutCancel(ctx), requester.GetOrgID()) err = processCheckRetry(ctx, logger, client, typesClient, req.Object, check) if err != nil { logger.Error("Error processing check retry", "error", err) diff --git a/apps/advisor/pkg/app/checkscheduler/checkscheduler.go b/apps/advisor/pkg/app/checkscheduler/checkscheduler.go index b8bf81299bf..adf0cc1569d 100644 --- a/apps/advisor/pkg/app/checkscheduler/checkscheduler.go +++ b/apps/advisor/pkg/app/checkscheduler/checkscheduler.go @@ -22,10 +22,16 @@ import ( const defaultEvaluationInterval = 7 * 24 * time.Hour // 7 days const defaultMaxHistory = 10 +var ( + waitInterval = 5 * time.Second + waitMaxRetries = 3 +) + // 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 // the only way existing at the moment to expose the check types. type Runner struct { + checkRegistry checkregistry.CheckService client resource.Client typesClient resource.Client evaluationInterval time.Duration @@ -41,6 +47,7 @@ func New(cfg app.Config, log logging.Logger) (app.Runnable, error) { if !ok { return nil, fmt.Errorf("invalid config type") } + checkRegistry := specificConfig.CheckRegistry evalInterval, err := getEvaluationInterval(specificConfig.PluginConfig) if err != nil { return nil, err @@ -66,6 +73,7 @@ func New(cfg app.Config, log logging.Logger) (app.Runnable, error) { } return &Runner{ + checkRegistry: checkRegistry, client: client, typesClient: typesClient, evaluationInterval: evalInterval, @@ -85,7 +93,7 @@ func (r *Runner) Run(ctx context.Context) error { } else { // do an initial creation if necessary if lastCreated.IsZero() { - err = r.createChecks(ctx) + err = r.createChecks(ctx, logger) if err != nil { logger.Error("Error creating new check reports", "error", err) } else { @@ -101,7 +109,7 @@ func (r *Runner) Run(ctx context.Context) error { for { select { case <-ticker.C: - err = r.createChecks(ctx) + err = r.createChecks(ctx, logger) if err != nil { logger.Error("Error creating new check reports", "error", err) } @@ -150,12 +158,25 @@ func (r *Runner) checkLastCreated(ctx context.Context, log logging.Logger) (time } // createChecks creates a new check for each check type in the registry. -func (r *Runner) createChecks(ctx context.Context) error { +func (r *Runner) createChecks(ctx context.Context, logger logging.Logger) error { // List existing CheckType objects list, err := r.typesClient.List(ctx, r.namespace, resource.ListOptions{}) if err != nil { return fmt.Errorf("error listing check types: %w", err) } + // This may be run before the check types are registered, so we need to wait for them to be registered. + allChecksRegistered := len(list.GetItems()) == len(r.checkRegistry.Checks()) + retryCount := 0 + for !allChecksRegistered && retryCount < waitMaxRetries { + logger.Error("Waiting for all check types to be registered", "retryCount", retryCount, "waitInterval", waitInterval) + time.Sleep(waitInterval) + list, err = r.typesClient.List(ctx, r.namespace, resource.ListOptions{}) + if err != nil { + return fmt.Errorf("error listing check types: %w", err) + } + allChecksRegistered = len(list.GetItems()) == len(r.checkRegistry.Checks()) + retryCount++ + } // Create checks for each CheckType for _, item := range list.GetItems() { diff --git a/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go b/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go index b699b50e7b7..69b4e809b4c 100644 --- a/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go +++ b/apps/advisor/pkg/app/checkscheduler/checkscheduler_test.go @@ -36,7 +36,7 @@ func TestRunner_Run(t *testing.T) { runner := &Runner{ client: mockClient, typesClient: mockTypesClient, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, evaluationInterval: 1 * time.Hour, } @@ -56,10 +56,10 @@ func TestRunner_checkLastCreated_ErrorOnList(t *testing.T) { runner := &Runner{ client: mockClient, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - lastCreated, err := runner.checkLastCreated(context.Background(), logging.DefaultLogger) + lastCreated, err := runner.checkLastCreated(context.Background(), &logging.NoOpLogger{}) assert.Error(t, err) assert.True(t, lastCreated.IsZero()) } @@ -89,10 +89,10 @@ func TestRunner_checkLastCreated_UnprocessedCheck(t *testing.T) { runner := &Runner{ client: mockClient, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - lastCreated, err := runner.checkLastCreated(context.Background(), logging.DefaultLogger) + lastCreated, err := runner.checkLastCreated(context.Background(), &logging.NoOpLogger{}) assert.NoError(t, err) assert.True(t, lastCreated.IsZero()) assert.Equal(t, "check-1", identifier.Name) @@ -104,6 +104,8 @@ func TestRunner_checkLastCreated_UnprocessedCheck(t *testing.T) { } func TestRunner_createChecks_ErrorOnCreate(t *testing.T) { + mockCheckService := &MockCheckService{checks: []checks.Check{&mockCheck{id: "check-1"}}} + mockClient := &MockClient{ createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) { return nil, errors.New("create error") @@ -121,16 +123,19 @@ func TestRunner_createChecks_ErrorOnCreate(t *testing.T) { } runner := &Runner{ - client: mockClient, - typesClient: mockTypesClient, - log: logging.DefaultLogger, + checkRegistry: mockCheckService, + client: mockClient, + typesClient: mockTypesClient, + log: &logging.NoOpLogger{}, } - err := runner.createChecks(context.Background()) + err := runner.createChecks(context.Background(), &logging.NoOpLogger{}) assert.Error(t, err) } func TestRunner_createChecks_Success(t *testing.T) { + mockCheckService := &MockCheckService{checks: []checks.Check{&mockCheck{id: "check-1"}}} + mockClient := &MockClient{ createFunc: func(ctx context.Context, id resource.Identifier, obj resource.Object, opts resource.CreateOptions) (resource.Object, error) { return &advisorv0alpha1.Check{}, nil @@ -148,12 +153,13 @@ func TestRunner_createChecks_Success(t *testing.T) { } runner := &Runner{ - client: mockClient, - typesClient: mockTypesClient, - log: logging.DefaultLogger, + checkRegistry: mockCheckService, + client: mockClient, + typesClient: mockTypesClient, + log: &logging.NoOpLogger{}, } - err := runner.createChecks(context.Background()) + err := runner.createChecks(context.Background(), &logging.NoOpLogger{}) assert.NoError(t, err) } @@ -166,10 +172,10 @@ func TestRunner_cleanupChecks_ErrorOnList(t *testing.T) { runner := &Runner{ client: mockClient, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - err := runner.cleanupChecks(context.Background(), logging.DefaultLogger) + err := runner.cleanupChecks(context.Background(), &logging.NoOpLogger{}) assert.Error(t, err) } @@ -187,10 +193,10 @@ func TestRunner_cleanupChecks_WithinMax(t *testing.T) { runner := &Runner{ client: mockClient, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - err := runner.cleanupChecks(context.Background(), logging.DefaultLogger) + err := runner.cleanupChecks(context.Background(), &logging.NoOpLogger{}) assert.NoError(t, err) } @@ -217,9 +223,9 @@ func TestRunner_cleanupChecks_ErrorOnDelete(t *testing.T) { runner := &Runner{ client: mockClient, maxHistory: defaultMaxHistory, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - err := runner.cleanupChecks(context.Background(), logging.DefaultLogger) + err := runner.cleanupChecks(context.Background(), &logging.NoOpLogger{}) assert.ErrorContains(t, err, "delete error") } @@ -253,9 +259,9 @@ func TestRunner_cleanupChecks_Success(t *testing.T) { runner := &Runner{ client: mockClient, maxHistory: defaultMaxHistory, - log: logging.DefaultLogger, + log: &logging.NoOpLogger{}, } - err := runner.cleanupChecks(context.Background(), logging.DefaultLogger) + err := runner.cleanupChecks(context.Background(), &logging.NoOpLogger{}) assert.NoError(t, err) assert.Equal(t, []string{"check-0"}, itemsDeleted) } @@ -334,3 +340,26 @@ func (m *MockClient) Delete(ctx context.Context, identifier resource.Identifier, 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 MockCheckService struct { + checks []checks.Check +} + +func (m *MockCheckService) Checks() []checks.Check { + return m.checks +} + +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 +}