From 3cb74e5abf46b2e62d360d6819bede36b9edd71c Mon Sep 17 00:00:00 2001 From: Suraj Patil Date: Thu, 23 Jul 2026 11:56:55 +0530 Subject: [PATCH 1/2] refactor: return structured ValidationResult from ClusterValidation/NodePoolValidation instead of error Signed-off-by: Suraj Patil --- backend/pkg/app/backend.go | 5 +- .../cluster_validation_controller.go | 171 +++++- .../cluster_validation_controller_test.go | 571 ++++++++++++++++++ .../validation/mock_cluster_validation.go | 80 +++ .../validation/mock_nodepool_validation.go | 80 +++ .../nodepool_validation_controller.go | 171 +++++- .../nodepool_validation_controller_test.go | 434 +++++++++++-- .../generic_watching_controller.go | 15 + .../always_success_validation.go | 4 +- .../azure_cluster_mis_existence_validation.go | 24 +- ...ter_resource_group_existence_validation.go | 29 +- ...e_nodepool_ephemeral_os_disk_validation.go | 38 +- ...epool_ephemeral_os_disk_validation_test.go | 44 +- .../azure_nodepool_vm_quota_validation.go | 76 ++- ...azure_nodepool_vm_quota_validation_test.go | 59 +- .../azure_rp_registration_validation.go | 25 +- .../validationutils/cluster_validation.go | 4 +- .../validationutils/nodepool_validation.go | 4 +- .../validationutils/validation_result.go | 355 +++++++++++ .../validationutils/validation_result_test.go | 323 ++++++++++ internal/controllerutils/cooldown.go | 49 ++ internal/controllerutils/cooldown_test.go | 87 +++ 22 files changed, 2442 insertions(+), 206 deletions(-) create mode 100644 backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go create mode 100644 backend/pkg/controllers/cluster/validation/mock_cluster_validation.go create mode 100644 backend/pkg/controllers/nodepool/validation/mock_nodepool_validation.go create mode 100644 backend/pkg/utils/validationutils/validation_result.go create mode 100644 backend/pkg/utils/validationutils/validation_result_test.go diff --git a/backend/pkg/app/backend.go b/backend/pkg/app/backend.go index b6d65c7b960..830edaae9aa 100644 --- a/backend/pkg/app/backend.go +++ b/backend/pkg/app/backend.go @@ -551,6 +551,7 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, http.DefaultClient, activeOperationInformer, ) + clusterServiceMatchingClusterController := mismatch.NewClusterServiceClusterMatchingController(b.options.ResourcesDBClient, subscriptionLister, b.options.ClustersServiceClient) alwaysSuccessClusterValidationController := clustervalidation.NewClusterValidationController( validationutils.NewAlwaysSuccessValidation(), @@ -715,12 +716,14 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, serviceProviderClusterLister, backendInformers, ) + azureClusterResourceGroupExistenceValidationController := clustervalidation.NewClusterValidationController( validationutils.NewAzureClusterResourceGroupExistenceValidation(b.options.FPAClientBuilder), b.options.ResourcesDBClient, serviceProviderClusterLister, backendInformers, ) + azureClusterManagedIdentitiesExistenceValidationController := clustervalidation.NewClusterValidationController( validationutils.NewAzureClusterManagedIdentitiesExistenceValidation(b.options.SMIClientBuilder), b.options.ResourcesDBClient, @@ -729,7 +732,6 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, ) azureVMSizeSupportsEphemeralOSDiskValidationController := nodepoolvalidation.NewNodePoolValidationController( validationutils.NewAzureVMSizeSupportsEphemeralOSDiskValidation(virtualMachineResourceSKUsCachedReaderController), - activeOperationLister, b.options.ResourcesDBClient, serviceProviderNodePoolLister, backendInformers, @@ -737,7 +739,6 @@ func (b *Backend) runBackendControllersUnderLeaderElection(ctx context.Context, ) azureNodePoolVMQuotaValidationController := nodepoolvalidation.NewNodePoolValidationController( validationutils.NewAzureNodePoolVMQuotaValidation(virtualMachineResourceSKUsCachedReaderController, b.options.FPAClientBuilder), - activeOperationLister, b.options.ResourcesDBClient, serviceProviderNodePoolLister, backendInformers, diff --git a/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go b/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go index 4e8252a820e..290f453ce23 100644 --- a/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go +++ b/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go @@ -19,12 +19,15 @@ import ( "fmt" "time" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/lru" "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" "github.com/Azure/ARO-HCP/internal/api" + controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" "github.com/Azure/ARO-HCP/internal/database/informers/coreinformers" @@ -32,20 +35,44 @@ import ( "github.com/Azure/ARO-HCP/internal/utils" ) +const ( + // consecutiveUnknownCountsCacheCapacity bounds the size of the consecutiveUnknownCounts LRU cache. + consecutiveUnknownCountsCacheCapacity = 50000 + + // maxConsecutiveUnknownsBeforeWrite bounds how many consecutive Unknown validation results are + // suppressed (i.e. the previously stored condition is kept as-is) before an Unknown condition is + // allowed to overwrite it. This avoids flapping a cluster's validation status to Unknown on a + // transient blip while still surfacing a persistent Unknown once it has been observed repeatedly. + maxConsecutiveUnknownsBeforeWrite = 10 +) + // clusterValidationSyncer is a Cluster syncer that performs a Cluster // validation. type clusterValidationSyncer struct { - resourcesDBClient corecosmosstorage.ResourcesDBClient + resourcesDBClient corecosmosstorage.ResourcesDBClient + // retryCooldownChecker gates re-execution of a key(HCPCluster) that recently had a + // retry scheduled. Prevents redundant validation runs while the cooldown + // from a previous EarliestRetryAfter is still active. + retryCooldownChecker *controllerutil.SettableCooldownChecker + // enqueueAfter allows the syncer to schedule a delayed re-processing of a + // key(HCPCluster), bypassing the workqueue's default rate limiter. + enqueueAfter controllerutils.AfterEnqueuer + serviceProviderClusterLister corelisters.ServiceProviderClusterLister // validation is the validation to perform on the cluster. validation validationutils.ClusterValidation + + // consecutiveUnknownCounts tracks, per HCPClusterKey, how many consecutive Unknown validation + // results have been observed since the last non-Unknown result. It backs the suppression + // policy in trackConsecutiveUnknowns, which avoids flapping a cluster's validation status + // to Unknown on a transient blip. + consecutiveUnknownCounts *lru.Cache } var _ controllerutils.ClusterSyncer = (*clusterValidationSyncer)(nil) -// NewClusterValidationController creates a new controller that -// executes the provided Cluster validation on each cluster. +// NewClusterValidationController creates a new controller that executes the provided Cluster validation on each cluster. func NewClusterValidationController( validation validationutils.ClusterValidation, resourcesDBClient corecosmosstorage.ResourcesDBClient, @@ -54,9 +81,11 @@ func NewClusterValidationController( ) controllerutils.Controller { syncer := &clusterValidationSyncer{ + retryCooldownChecker: controllerutil.NewSettableCooldownChecker(), resourcesDBClient: resourcesDBClient, serviceProviderClusterLister: serviceProviderClusterLister, validation: validation, + consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity), } controller := controllerutils.NewClusterWatchingController( @@ -68,10 +97,30 @@ func NewClusterValidationController( syncer, ) + // Assert that genericWatchingController implements AfterEnqueuer, which lets the syncer explicitly schedule retries via EnqueueAfter rather than + // relying on error-based rate-limited requeue. Panics at startup if the interface is not satisfied. + if enqueuer, ok := controller.(controllerutils.AfterEnqueuer); ok { + syncer.enqueueAfter = enqueuer + } else { + panic("ClusterValidationController must implement AfterEnqueuer") + } + return controller } func (c *clusterValidationSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPClusterKey) error { + logger := utils.LoggerFromContext(ctx) + + // Skip processing if the key is still within its cooldown window from a previous validation. All outcomes can schedule a cooldown via + // EarliestRetryAfter so validations run continuously without racing. Re-enqueue so the item is revisited once the cooldown expires. + if !c.retryCooldownChecker.CanSync(ctx, key) { + if c.enqueueAfter != nil { + // Add a one-second buffer so the requeue lands strictly after the cooldown expires, avoiding a race where the item fires just before CanSync flips to true. + c.enqueueAfter.EnqueueAfter(key, c.retryCooldownChecker.TimeUntilReady(key)+time.Second) + } + return nil + } + existingCluster, err := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName).Get(ctx, key.HCPClusterName) if cosmosstorageutils.IsNotFoundError(err) { return nil // cluster doesn't exist, no work to do @@ -92,8 +141,7 @@ func (c *clusterValidationSyncer) SyncOnce(ctx context.Context, key controllerut return utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster: %w", err)) } - shouldProcess := c.shouldProcess(cachedServiceProviderCluster) - if !shouldProcess { + if !c.shouldProcess(cachedServiceProviderCluster) { return nil // no work to do } existingServiceProviderCluster := cachedServiceProviderCluster.DeepCopy() @@ -102,42 +150,105 @@ func (c *clusterValidationSyncer) SyncOnce(ctx context.Context, key controllerut return utils.TrackError(fmt.Errorf("failed to get Subscription: %w", err)) } - // We store the validation error in a separate variable and we use that as the - // error to return to the caller. This allows us to perform other remaining - // tasks in the syncer even if the validation fails, and we ultimately - // drive the behavior of its controller through the outcome of the validation. - validationErr := c.validation.Validate(ctx, subscription, existingCluster) + result := c.validation.Validate(ctx, subscription, existingCluster) + if err := result.Validate(); err != nil { + return utils.TrackError(fmt.Errorf("validation %s returned invalid ValidationResult: %w", c.validation.Name(), err)) + } - validationCondition := metav1.Condition{ - Type: c.validation.Name(), + if result.Outcome.Type != validationutils.OutcomeTypePassed { + logger.Info("Validation outcome", "validation", c.validation.Name(), "result", result) } - if validationErr != nil { - validationCondition.Status = metav1.ConditionFalse - validationCondition.Reason = "Failed" - validationCondition.Message = fmt.Sprintf("Validation failed: %s", validationErr.Error()) + + replacement := existingServiceProviderCluster.DeepCopy() + + // If the validation was skipped, remove its condition so it doesn't appear in status. Otherwise, reconcile the condition with consecutive-Unknown + // suppression to avoid flapping on transient errors. + if result.Outcome.Type == validationutils.OutcomeTypeSkipped { + meta.RemoveStatusCondition(&replacement.Status.Validations, c.validation.Name()) } else { - validationCondition.Status = metav1.ConditionTrue - validationCondition.Reason = "Succeeded" - validationCondition.Message = "Validation succeeded" + previousCondition := meta.FindStatusCondition(existingServiceProviderCluster.Status.Validations, c.validation.Name()) + desiredCondition := result.ToCondition(c.validation.Name()) + + consecutiveUnknowns := c.trackConsecutiveUnknowns(key, desiredCondition) + if c.shouldWriteCondition(previousCondition, consecutiveUnknowns) { + meta.SetStatusCondition(&replacement.Status.Validations, desiredCondition) + } } - replacement := existingServiceProviderCluster.DeepCopy() - meta.SetStatusCondition(&replacement.Status.Validations, validationCondition) - serviceProviderClustersCosmosClient := c.resourcesDBClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) - _, err = serviceProviderClustersCosmosClient.Replace(ctx, replacement, nil) - if cosmosstorageutils.IsPreconditionFailedError(err) { - // if we have a conflict error, then we're guaranteed that our informer will eventually see an update and trigger us again. - return nil + if !equality.Semantic.DeepEqual(existingServiceProviderCluster, replacement) { + serviceProviderClustersCosmosClient := c.resourcesDBClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) + _, err = serviceProviderClustersCosmosClient.Replace(ctx, replacement, nil) + if cosmosstorageutils.IsPreconditionFailedError(err) { + // if we have a conflict error, then we're guaranteed that our informer will eventually see an update and trigger us again. + return nil + } + if err != nil { + return utils.TrackError(fmt.Errorf("failed to replace ServiceProviderCluster: %w", err)) + } } - if err != nil { - return utils.TrackError(fmt.Errorf("failed to replace ServiceProviderCluster: %w", err)) + + c.handleRequeue(key, result) + + // ControllerReportingPolicy governs only how this Unknown result is reported to the controller + // machinery (e.g. workqueue error metrics); it has no bearing on the requeue scheduling already + // handled above by handleRequeue based on EarliestRetryAfter. Keep this as the last step of SyncOnce. + if result.Outcome.Type == validationutils.OutcomeTypeUnknown && result.Outcome.Unknown.ControllerReportingPolicy == validationutils.ControllerReportingPolicyTypeError { + return utils.TrackError(fmt.Errorf("validation %s returned an inconclusive (Unknown) result: %s", c.validation.Name(), result.InternalMessage())) } - return validationErr + return nil +} + +// handleRequeue updates the retry cooldown and schedules a delayed re-enqueue for key based solely on result.EarliestRetryAfter. +// If EarliestRetryAfter is nil, there is no retry backoff to apply; the informer may eventually see an update and trigger again. +func (c *clusterValidationSyncer) handleRequeue(key controllerutils.HCPClusterKey, result validationutils.ValidationResult) { + if result.EarliestRetryAfter == nil { + return + } + + c.retryCooldownChecker.SetCooldown(key, *result.EarliestRetryAfter) + if c.enqueueAfter != nil { + // Add a one-second buffer so the requeue lands strictly after the cooldown expires, avoiding a race where the item fires just before CanSync flips to true. + c.enqueueAfter.EnqueueAfter(key, *result.EarliestRetryAfter+time.Second) + } } // shouldProcess returns true when the condition associated to the validation does not exist or when it exists but -// it failed to run successfully in a previous attempt. +// its status is not True. func (c *clusterValidationSyncer) shouldProcess(serviceProviderCluster *api.ServiceProviderCluster) bool { return !meta.IsStatusConditionTrue(serviceProviderCluster.Status.Validations, c.validation.Name()) } + +// shouldWriteCondition reports whether the newly computed validation condition should be written, versus +// suppressed in favor of leaving previousCondition (the condition currently stored for this validation, or +// nil if none is stored yet) untouched. +// +// The write is suppressed only while all of the following hold: +// - previousCondition is non-nil (there's something worth preserving), and +// - consecutiveUnknowns is non-zero (the newly computed condition is Unknown; trackConsecutiveUnknowns +// returns 0 for any non-Unknown result), and +// - consecutiveUnknowns has not yet exceeded maxConsecutiveUnknownsBeforeWrite. +// +// This backs a suppression policy that avoids flapping a cluster's validation status to Unknown on a +// transient blip: a persistent Unknown streak is still allowed to overwrite the stored condition once it +// exceeds maxConsecutiveUnknownsBeforeWrite, and a Passed/Failed result (consecutiveUnknowns == 0) always +// overwrites immediately, resetting the streak. +func (c *clusterValidationSyncer) shouldWriteCondition(previousCondition *metav1.Condition, consecutiveUnknowns int) bool { + return previousCondition == nil || consecutiveUnknowns == 0 || consecutiveUnknowns > maxConsecutiveUnknownsBeforeWrite +} + +// trackConsecutiveUnknowns maintains the count of consecutive Unknown validation results for the given key. When condition is Unknown it increments and returns the +// running count; otherwise it resets the counter and returns 0. +func (c *clusterValidationSyncer) trackConsecutiveUnknowns(key controllerutils.HCPClusterKey, condition metav1.Condition) int { + if condition.Status != metav1.ConditionUnknown { + c.consecutiveUnknownCounts.Remove(key) + return 0 + } + + count := 1 + if v, ok := c.consecutiveUnknownCounts.Get(key); ok { + count = v.(int) + 1 + } + c.consecutiveUnknownCounts.Add(key, count) + return count +} diff --git a/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go b/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go new file mode 100644 index 00000000000..769a6115000 --- /dev/null +++ b/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go @@ -0,0 +1,571 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/lru" + + azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + + "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" + "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" + "github.com/Azure/ARO-HCP/internal/api" + "github.com/Azure/ARO-HCP/internal/api/arm" + controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" + "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" + "github.com/Azure/ARO-HCP/internal/database/cosmosstoragetesting/corecosmosstoragetesting" + "github.com/Azure/ARO-HCP/internal/database/listertesting/corelistertesting" + "github.com/Azure/ARO-HCP/internal/utils" +) + +const ( + testSubscriptionID = "00000000-0000-0000-0000-000000000000" + testResourceGroup = "test-rg" + testClusterName = "test-cluster" + testValidationName = "TestValidation" +) + +var fixedNow = time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + +type fakeAfterEnqueuer struct { + enqueuedKeys []any + enqueuedDurations []time.Duration +} + +func (f *fakeAfterEnqueuer) EnqueueAfter(keyObj any, duration time.Duration) { + f.enqueuedKeys = append(f.enqueuedKeys, keyObj) + f.enqueuedDurations = append(f.enqueuedDurations, duration) +} + +func newTestClusterKey() controllerutils.HCPClusterKey { + return controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroup, + HCPClusterName: testClusterName, + } +} + +func newTestCluster(t *testing.T) *api.HCPOpenShiftCluster { + t.Helper() + resourceID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID + + "/resourceGroups/" + testResourceGroup + + "/providers/Microsoft.RedHatOpenShift/hcpOpenShiftClusters/" + testClusterName)) + return &api.HCPOpenShiftCluster{ + CosmosMetadata: arm.CosmosMetadata{ + ResourceID: resourceID, + PartitionKey: strings.ToLower(resourceID.SubscriptionID), + }, + TrackedResource: arm.TrackedResource{ + Resource: arm.Resource{ + ID: resourceID, + Name: testClusterName, + Type: api.ClusterResourceType.String(), + }, + Location: "eastus", + }, + } +} + +func newTestSubscription() *arm.Subscription { + subResourceID := api.Must(azcorearm.ParseResourceID( + "/subscriptions/" + testSubscriptionID)) + return &arm.Subscription{ + CosmosMetadata: api.CosmosMetadata{ + ResourceID: subResourceID, + PartitionKey: strings.ToLower(subResourceID.SubscriptionID), + }, + ResourceID: subResourceID, + State: arm.SubscriptionStateRegistered, + } +} + +func newTestSyncer(mockDB *corecosmosstoragetesting.MockResourcesDBClient, validation validationutils.ClusterValidation, fakeClock *clocktesting.FakePassiveClock) (*clusterValidationSyncer, *fakeAfterEnqueuer) { + retryCooldown := controllerutil.NewSettableCooldownChecker() + retryCooldown.SetClock(fakeClock) + enqueuer := &fakeAfterEnqueuer{} + syncer := &clusterValidationSyncer{ + retryCooldownChecker: retryCooldown, + enqueueAfter: enqueuer, + resourcesDBClient: mockDB, + serviceProviderClusterLister: &corelistertesting.DBServiceProviderClusterLister{ResourcesDBClient: mockDB}, + validation: validation, + consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity), + } + return syncer, enqueuer +} + +func TestClusterValidationSyncer_SyncOnce(t *testing.T) { + + defaultSetupDB := func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + cluster := newTestCluster(t) + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, cluster, nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + _, err = corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, mockDB, cluster.ID) + require.NoError(t, err) + } + + testCases := []struct { + name string + setupDB func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) + validation validationutils.ClusterValidation + wantErr bool + // wantCondition, if non-nil, asserts that the stored validation condition's Status/Reason/Message + // match (Type and LastTransitionTime are not compared). + wantCondition *metav1.Condition + // wantConditionAbsent asserts that no validation condition is stored at all. + wantConditionAbsent bool + wantEnqueue bool + }{ + { + name: "cluster not found -- no-op", + setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + _, err := mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + }, + validation: NewMockClusterValidation(testValidationName), + }, + { + name: "service provider cluster not found -- no-op", + setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, newTestCluster(t), nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + }, + validation: NewMockClusterValidation(testValidationName), + }, + { + name: "validation passes -- condition set to True", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithPassed(), + wantCondition: &metav1.Condition{Status: metav1.ConditionTrue, Reason: "AsExpected", Message: "As expected."}, + wantEnqueue: true, + }, + { + name: "validation fails -- condition set to False, requeue scheduled", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithFailed( + "QuotaExceeded", "quota exceeded", "Quota exceeded for this subscription.", + ), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "QuotaExceeded", Message: "Quota exceeded for this subscription."}, + wantEnqueue: true, + }, + { + // Covers the "last step of SyncOnce" reporting-policy branch that turns an Unknown result into + // an error return; see TestClusterValidationSyncer_SyncOnce's sibling case below for the + // LogOnly branch of the same decision. + name: "validation unknown with ReportError -- condition set to Unknown, requeue scheduled, error returned", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithUnknownReportError( + "InternalError", "failed to reach Azure", "Unable to verify.", + ), + wantErr: true, + wantCondition: &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "InternalError", Message: "Unable to verify."}, + wantEnqueue: true, + }, + { + // Covers handleRequeue's nil guard: EarliestRetryAfter == nil must skip cooldown/requeue + // without otherwise affecting the condition write. + name: "validation fails with nil EarliestRetryAfter -- condition still set, no cooldown or requeue scheduled", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithFailed( + "QuotaExceeded", "quota exceeded", "Quota exceeded for this subscription.", + ).WithEarliestRetryAfter(nil), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "QuotaExceeded", Message: "Quota exceeded for this subscription."}, + wantEnqueue: false, + }, + { + name: "validation unknown with LogOnly -- condition set to Unknown, requeue still scheduled, no error returned", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithUnknownLogOnly( + "TransientIssue", "temporary network blip", "Temporarily unable to verify.", + ), + wantCondition: &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "TransientIssue", Message: "Temporarily unable to verify."}, + wantEnqueue: true, + }, + { + name: "validation skipped with no prior condition -- no condition persisted", + setupDB: defaultSetupDB, + validation: NewMockClusterValidation(testValidationName).WithSkipped( + "NotApplicable", "cluster does not need this check", "Not applicable.", + ), + wantConditionAbsent: true, + wantEnqueue: true, + }, + { + name: "validation skipped with prior condition -- condition removed", + setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + defaultSetupDB(t, ctx, mockDB) + spcCRUD := mockDB.ServiceProviderClusters(testSubscriptionID, testResourceGroup, testClusterName) + spc, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + spc.Status.Validations = []metav1.Condition{ + { + Type: testValidationName, + Status: metav1.ConditionFalse, + Reason: "PreviouslyFailed", + Message: "previously failed", + }, + } + _, err = spcCRUD.Replace(ctx, spc, nil) + require.NoError(t, err) + }, + validation: NewMockClusterValidation(testValidationName).WithSkipped( + "NotApplicable", "cluster does not need this check", "Not applicable.", + ), + wantConditionAbsent: true, + wantEnqueue: true, + }, + { + name: "already-succeeded validation -- skipped", + setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + defaultSetupDB(t, ctx, mockDB) + spcCRUD := mockDB.ServiceProviderClusters(testSubscriptionID, testResourceGroup, testClusterName) + spc, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + spc.Status.Validations = []metav1.Condition{ + { + Type: testValidationName, + Status: metav1.ConditionTrue, + Reason: "AsExpected", + }, + } + _, err = spcCRUD.Replace(ctx, spc, nil) + require.NoError(t, err) + }, + validation: NewMockClusterValidation(testValidationName).WithFailed( + "ShouldNotBeCalled", "should not be called", "should not be called", + ), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mockDB := corecosmosstoragetesting.NewMockResourcesDBClient() + if tc.setupDB != nil { + tc.setupDB(t, ctx, mockDB) + } + + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, enqueuer := newTestSyncer(mockDB, tc.validation, fakeClock) + + err := syncer.SyncOnce(ctx, newTestClusterKey()) + if tc.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + if tc.wantEnqueue { + require.NotEmpty(t, enqueuer.enqueuedKeys, "expected a requeue to be scheduled") + } else { + require.Empty(t, enqueuer.enqueuedKeys, "expected no requeue to be scheduled") + } + + if tc.wantConditionAbsent { + spc, spcErr := mockDB.ServiceProviderClusters( + testSubscriptionID, testResourceGroup, testClusterName, + ).Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, spcErr) + + cond := meta.FindStatusCondition(spc.Status.Validations, testValidationName) + assert.Nil(t, cond, "expected validation condition to be absent") + } + + if tc.wantCondition != nil { + spc, spcErr := mockDB.ServiceProviderClusters( + testSubscriptionID, testResourceGroup, testClusterName, + ).Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, spcErr) + + cond := meta.FindStatusCondition(spc.Status.Validations, testValidationName) + require.NotNil(t, cond, "expected validation condition to be set") + assert.Equal(t, tc.wantCondition.Status, cond.Status) + assert.Equal(t, tc.wantCondition.Reason, cond.Reason) + assert.Equal(t, tc.wantCondition.Message, cond.Message) + } + }) + } +} + +// TestClusterValidationSyncer_ShouldWriteCondition unit-tests the suppression decision in isolation from +// Cosmos/DB plumbing, covering the boundary cases around maxConsecutiveUnknownsBeforeWrite. +func TestClusterValidationSyncer_ShouldWriteCondition(t *testing.T) { + syncer := &clusterValidationSyncer{} + + t.Run("no previously stored condition -- always write, even mid-streak", func(t *testing.T) { + assert.True(t, syncer.shouldWriteCondition(nil, 5)) + }) + + // shouldWriteCondition only checks previousCondition's nilness, not its Status. Vary Status here to + // lock that contract: suppression depends solely on consecutiveUnknowns. A prior passed condition is + // unreachable via SyncOnce (shouldProcess skips it), but the helper must still behave consistently. + previousConditionFixtures := []struct { + name string + condition *metav1.Condition + }{ + { + name: "Unknown", + condition: &metav1.Condition{Type: testValidationName, Status: metav1.ConditionUnknown}, + }, + { + name: "Failed", + condition: &metav1.Condition{Type: testValidationName, Status: metav1.ConditionFalse, Reason: "PreviouslyFailed"}, + }, + { + name: "Passed", + condition: &metav1.Condition{Type: testValidationName, Status: metav1.ConditionTrue, Reason: "AsExpected"}, + }, + } + + scenarios := []struct { + name string + consecutiveUnknowns int + want bool + }{ + { + name: "non-Unknown result (streak reset to 0) -- write", + consecutiveUnknowns: 0, + want: true, + }, + { + name: "first Unknown in streak -- suppress", + consecutiveUnknowns: 1, + want: false, + }, + { + name: "streak exactly at threshold -- suppress (boundary)", + consecutiveUnknowns: maxConsecutiveUnknownsBeforeWrite, + want: false, + }, + { + name: "streak one past threshold -- write (boundary)", + consecutiveUnknowns: maxConsecutiveUnknownsBeforeWrite + 1, + want: true, + }, + } + + for _, fixture := range previousConditionFixtures { + for _, scenario := range scenarios { + t.Run(fixture.name+" prior, "+scenario.name, func(t *testing.T) { + assert.Equal(t, scenario.want, syncer.shouldWriteCondition(fixture.condition, scenario.consecutiveUnknowns)) + }) + } + } +} + +// TestClusterValidationSyncer_TrackConsecutiveUnknowns unit-tests the per-key streak bookkeeping in +// isolation: incrementing across consecutive Unknown results, resetting on a non-Unknown result, and +// tracking each HCPClusterKey independently. Each test case is a sequence of steps run against a single +// fresh syncer, asserting the returned count after every step. +func TestClusterValidationSyncer_TrackConsecutiveUnknowns(t *testing.T) { + keyA := newTestClusterKey() + keyB := controllerutils.HCPClusterKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroup, + HCPClusterName: "other-cluster", + } + + type step struct { + key controllerutils.HCPClusterKey + status metav1.ConditionStatus + want int + } + + testCases := []struct { + name string + steps []step + }{ + { + name: "increments on consecutive Unknown results", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionUnknown, want: 3}, + }, + }, + { + name: "non-Unknown result resets the streak to 0", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionFalse, want: 0}, + }, + }, + { + name: "streak restarts at 1 after a reset, not continuing the pre-reset count", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionTrue, want: 0}, + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + }, + }, + { + name: "non-Unknown result with no prior streak stays at 0", + steps: []step{ + {key: keyA, status: metav1.ConditionFalse, want: 0}, + }, + }, + { + name: "keys are tracked independently", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyB, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 3}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + syncer := &clusterValidationSyncer{consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity)} + for i, s := range tc.steps { + condition := metav1.Condition{Type: testValidationName, Status: s.status} + got := syncer.trackConsecutiveUnknowns(s.key, condition) + assert.Equalf(t, s.want, got, "step %d: key=%s, status=%s", i, s.key.HCPClusterName, s.status) + } + }) + } +} + +// TestClusterValidationSyncer_ConsecutiveUnknownSuppression exercises the consecutive-Unknown suppression +// policy end-to-end across repeated SyncOnce calls: a previously stored Failed condition should survive the +// first maxConsecutiveUnknownsBeforeWrite consecutive Unknown results untouched (and skip the Cosmos write +// each time, per the equality.Semantic.DeepEqual guard), then get overwritten with Unknown once the streak +// persists past the threshold. +func TestClusterValidationSyncer_ConsecutiveUnknownSuppression(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mockDB := corecosmosstoragetesting.NewMockResourcesDBClient() + cluster := newTestCluster(t) + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, cluster, nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + _, err = corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, mockDB, cluster.ID) + require.NoError(t, err) + + spcCRUD := mockDB.ServiceProviderClusters(testSubscriptionID, testResourceGroup, testClusterName) + spc, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + spc.Status.Validations = []metav1.Condition{ + { + Type: testValidationName, + Status: metav1.ConditionFalse, + Reason: "PreviouslyFailed", + Message: "previously failed", + }, + } + _, err = spcCRUD.Replace(ctx, spc, nil) + require.NoError(t, err) + + validation := NewMockClusterValidation(testValidationName).WithUnknownLogOnly( + "InternalError", "failed to reach Azure", "Unable to verify.", + ) + + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, _ := newTestSyncer(mockDB, validation, fakeClock) + + for i := 1; i <= maxConsecutiveUnknownsBeforeWrite; i++ { + fakeClock.SetTime(fakeClock.Now().Add(time.Hour)) + + before, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + + require.NoError(t, syncer.SyncOnce(ctx, newTestClusterKey())) + + after, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + + cond := meta.FindStatusCondition(after.Status.Validations, testValidationName) + require.NotNil(t, cond) + assert.Equalf(t, metav1.ConditionFalse, cond.Status, "attempt %d: previous condition should be preserved", i) + assert.Equalf(t, "PreviouslyFailed", cond.Reason, "attempt %d: previous condition should be preserved", i) + assert.Equalf(t, before.CosmosETag, after.CosmosETag, "attempt %d: Cosmos write should have been skipped", i) + } + + // The next attempt exceeds the threshold, so the Unknown condition finally overwrites the stored one. + fakeClock.SetTime(fakeClock.Now().Add(time.Hour)) + + before, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + + require.NoError(t, syncer.SyncOnce(ctx, newTestClusterKey())) + + after, err := spcCRUD.Get(ctx, api.ServiceProviderClusterResourceName) + require.NoError(t, err) + + cond := meta.FindStatusCondition(after.Status.Validations, testValidationName) + require.NotNil(t, cond) + assert.Equal(t, metav1.ConditionUnknown, cond.Status) + assert.Equal(t, "InternalError", cond.Reason) + assert.NotEqual(t, before.CosmosETag, after.CosmosETag, "expected a Cosmos write once the suppression threshold was exceeded") +} + +// TestClusterValidationSyncer_CooldownSuppression verifies that when the +// retryCooldownChecker's cooldown is active for a key, SyncOnce returns +// immediately without performing validation, and schedules a re-enqueue. +func TestClusterValidationSyncer_CooldownSuppression(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mockDB := corecosmosstoragetesting.NewMockResourcesDBClient() + cluster := newTestCluster(t) + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, cluster, nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + _, err = corecosmosstorage.GetOrCreateServiceProviderCluster(ctx, mockDB, cluster.ID) + require.NoError(t, err) + + validation := NewMockClusterValidation(testValidationName).WithFailed( + "ShouldNotRun", "should not run", "should not run", + ) + + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, enqueuer := newTestSyncer(mockDB, validation, fakeClock) + + key := newTestClusterKey() + syncer.retryCooldownChecker.SetCooldown(key, 60*time.Second) + + err = syncer.SyncOnce(ctx, key) + require.NoError(t, err, "SyncOnce should return nil when cooldown is active") + + require.NotEmpty(t, enqueuer.enqueuedKeys, "should have re-enqueued after cooldown skip") + assert.Greater(t, enqueuer.enqueuedDurations[0], time.Duration(0), "enqueue duration should be positive") +} diff --git a/backend/pkg/controllers/cluster/validation/mock_cluster_validation.go b/backend/pkg/controllers/cluster/validation/mock_cluster_validation.go new file mode 100644 index 00000000000..574aed1ce6b --- /dev/null +++ b/backend/pkg/controllers/cluster/validation/mock_cluster_validation.go @@ -0,0 +1,80 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "context" + "time" + + "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" + "github.com/Azure/ARO-HCP/internal/api" + "github.com/Azure/ARO-HCP/internal/api/arm" +) + +// MockClusterValidation is a ClusterValidation implementation for controller tests. +type MockClusterValidation struct { + validationName string + result validationutils.ValidationResult +} + +var _ validationutils.ClusterValidation = (*MockClusterValidation)(nil) + +// NewMockClusterValidation creates a mock validation with the given name and no result configured. +func NewMockClusterValidation(name string) *MockClusterValidation { + return &MockClusterValidation{validationName: name} +} + +// WithPassed configures the mock to return a passed validation result. +func (m *MockClusterValidation) WithPassed() *MockClusterValidation { + m.result = validationutils.PassedValidation(api.ControllerConditionReasonAsExpected, "As expected.", "") + return m +} + +// WithFailed configures the mock to return a failed validation result. +func (m *MockClusterValidation) WithFailed(reason, internalMessage, userMessage string) *MockClusterValidation { + m.result = validationutils.FailedValidation(reason, userMessage, internalMessage) + return m +} + +// WithUnknownLogOnly configures the mock to return an unknown validation result with log-only reporting. +func (m *MockClusterValidation) WithUnknownLogOnly(reason, internalMessage, userMessage string) *MockClusterValidation { + m.result = validationutils.UnknownValidation(reason, userMessage, internalMessage, validationutils.ControllerReportingPolicyTypeLogOnly) + return m +} + +// WithUnknownReportError configures the mock to return an unknown validation result that reports as an error. +func (m *MockClusterValidation) WithUnknownReportError(reason, internalMessage, userMessage string) *MockClusterValidation { + m.result = validationutils.UnknownValidation(reason, userMessage, internalMessage, validationutils.ControllerReportingPolicyTypeError) + return m +} + +// WithSkipped configures the mock to return a skipped validation result. +func (m *MockClusterValidation) WithSkipped(reason, internalMessage, userMessage string) *MockClusterValidation { + m.result = validationutils.SkippedValidation(reason, userMessage, internalMessage) + return m +} + +// WithEarliestRetryAfter overrides the currently configured result's EarliestRetryAfter, e.g. to nil to +// exercise the "no retry backoff" path. +func (m *MockClusterValidation) WithEarliestRetryAfter(d *time.Duration) *MockClusterValidation { + m.result.EarliestRetryAfter = d + return m +} + +func (m *MockClusterValidation) Name() string { return m.validationName } + +func (m *MockClusterValidation) Validate(_ context.Context, _ *arm.Subscription, _ *api.HCPOpenShiftCluster) validationutils.ValidationResult { + return m.result +} diff --git a/backend/pkg/controllers/nodepool/validation/mock_nodepool_validation.go b/backend/pkg/controllers/nodepool/validation/mock_nodepool_validation.go new file mode 100644 index 00000000000..3867ae94e7a --- /dev/null +++ b/backend/pkg/controllers/nodepool/validation/mock_nodepool_validation.go @@ -0,0 +1,80 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "context" + "time" + + "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" + "github.com/Azure/ARO-HCP/internal/api" + "github.com/Azure/ARO-HCP/internal/api/arm" +) + +// MockNodePoolValidation is a NodePoolValidation implementation for controller tests. +type MockNodePoolValidation struct { + validationName string + result validationutils.ValidationResult +} + +var _ validationutils.NodePoolValidation = (*MockNodePoolValidation)(nil) + +// NewMockNodePoolValidation creates a mock validation with the given name and no result configured. +func NewMockNodePoolValidation(name string) *MockNodePoolValidation { + return &MockNodePoolValidation{validationName: name} +} + +// WithPassed configures the mock to return a passed validation result. +func (m *MockNodePoolValidation) WithPassed() *MockNodePoolValidation { + m.result = validationutils.PassedValidation(api.ControllerConditionReasonAsExpected, "As expected.", "") + return m +} + +// WithFailed configures the mock to return a failed validation result. +func (m *MockNodePoolValidation) WithFailed(reason, internalMessage, userMessage string) *MockNodePoolValidation { + m.result = validationutils.FailedValidation(reason, userMessage, internalMessage) + return m +} + +// WithUnknownLogOnly configures the mock to return an unknown validation result with log-only reporting. +func (m *MockNodePoolValidation) WithUnknownLogOnly(reason, internalMessage, userMessage string) *MockNodePoolValidation { + m.result = validationutils.UnknownValidation(reason, userMessage, internalMessage, validationutils.ControllerReportingPolicyTypeLogOnly) + return m +} + +// WithUnknownReportError configures the mock to return an unknown validation result that reports as an error. +func (m *MockNodePoolValidation) WithUnknownReportError(reason, internalMessage, userMessage string) *MockNodePoolValidation { + m.result = validationutils.UnknownValidation(reason, userMessage, internalMessage, validationutils.ControllerReportingPolicyTypeError) + return m +} + +// WithSkipped configures the mock to return a skipped validation result. +func (m *MockNodePoolValidation) WithSkipped(reason, internalMessage, userMessage string) *MockNodePoolValidation { + m.result = validationutils.SkippedValidation(reason, userMessage, internalMessage) + return m +} + +// WithEarliestRetryAfter overrides the currently configured result's EarliestRetryAfter, e.g. to nil to +// exercise the "no retry backoff" path. +func (m *MockNodePoolValidation) WithEarliestRetryAfter(d *time.Duration) *MockNodePoolValidation { + m.result.EarliestRetryAfter = d + return m +} + +func (m *MockNodePoolValidation) Name() string { return m.validationName } + +func (m *MockNodePoolValidation) Validate(_ context.Context, _ *api.HCPOpenShiftCluster, _ *arm.Subscription, _ *api.HCPOpenShiftClusterNodePool) validationutils.ValidationResult { + return m.result +} diff --git a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go index 5ccc962dfcc..44870b58393 100644 --- a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go +++ b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go @@ -19,12 +19,15 @@ import ( "fmt" "time" + "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/lru" "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" "github.com/Azure/ARO-HCP/internal/api" + controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" "github.com/Azure/ARO-HCP/internal/database/informers/coreinformers" @@ -33,23 +36,46 @@ import ( "github.com/Azure/ARO-HCP/internal/utils" ) +const ( + // consecutiveUnknownCountsCacheCapacity bounds the size of the consecutiveUnknownCounts LRU cache. + consecutiveUnknownCountsCacheCapacity = 50000 + + // maxConsecutiveUnknownsBeforeWrite bounds how many consecutive Unknown validation results are + // suppressed (i.e. the previously stored condition is kept as-is) before an Unknown condition is + // allowed to overwrite it. This avoids flapping a node pool's validation status to Unknown on a + // transient blip while still surfacing a persistent Unknown once it has been observed repeatedly. + maxConsecutiveUnknownsBeforeWrite = 10 +) + // nodePoolValidationSyncer is a NodePool syncer that performs a NodePool // validation. type nodePoolValidationSyncer struct { - resourcesDBClient corecosmosstorage.ResourcesDBClient + resourcesDBClient corecosmosstorage.ResourcesDBClient + // retryCooldownChecker gates re-execution of a key(HCPNodePool) that recently had a + // retry scheduled. Prevents redundant validation runs while the cooldown + // from a previous EarliestRetryAfter is still active. + retryCooldownChecker *controllerutil.SettableCooldownChecker + // enqueueAfter allows the syncer to schedule a delayed re-processing of a + // key(HCPNodePool), bypassing the workqueue's default rate limiter. + enqueueAfter controllerutils.AfterEnqueuer + serviceProviderNodePoolLister corelisters.ServiceProviderNodePoolLister // validation is the validation to perform on the node pool. validation validationutils.NodePoolValidation + + // consecutiveUnknownCounts tracks, per HCPNodePoolKey, how many consecutive Unknown validation + // results have been observed since the last non-Unknown result. It backs the suppression + // policy in trackConsecutiveUnknowns, which avoids flapping a node pool's validation status + // to Unknown on a transient blip. + consecutiveUnknownCounts *lru.Cache } var _ controllerutils.NodePoolSyncer = (*nodePoolValidationSyncer)(nil) -// NewNodePoolValidationController creates a new controller that -// executes the provided NodePool validation on each node pool. +// NewNodePoolValidationController creates a new controller that executes the provided NodePool validation on each node pool. func NewNodePoolValidationController( validation validationutils.NodePoolValidation, - activeOperationLister corelisters.ActiveOperationLister, resourcesDBClient corecosmosstorage.ResourcesDBClient, serviceProviderNodePoolLister corelisters.ServiceProviderNodePoolLister, informers coreinformers.BackendInformers, @@ -57,9 +83,11 @@ func NewNodePoolValidationController( ) controllerutils.Controller { syncer := &nodePoolValidationSyncer{ + retryCooldownChecker: controllerutil.NewSettableCooldownChecker(), resourcesDBClient: resourcesDBClient, serviceProviderNodePoolLister: serviceProviderNodePoolLister, validation: validation, + consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity), } controller := controllerutils.NewNodePoolWatchingController( @@ -71,10 +99,30 @@ func NewNodePoolValidationController( syncer, ) + // Assert that genericWatchingController implements AfterEnqueuer, which lets the syncer explicitly schedule retries via EnqueueAfter rather than + // relying on error-based rate-limited requeue. Panics at startup if the interface is not satisfied. + if enqueuer, ok := controller.(controllerutils.AfterEnqueuer); ok { + syncer.enqueueAfter = enqueuer + } else { + panic("NodePoolValidationController must implement AfterEnqueuer") + } + return controller } func (c *nodePoolValidationSyncer) SyncOnce(ctx context.Context, key controllerutils.HCPNodePoolKey) error { + logger := utils.LoggerFromContext(ctx) + + // Skip processing if the key is still within its cooldown window from a previous validation. All outcomes can schedule a cooldown via + // EarliestRetryAfter so validations run continuously without racing. Re-enqueue so the item is revisited once the cooldown expires. + if !c.retryCooldownChecker.CanSync(ctx, key) { + if c.enqueueAfter != nil { + // Add a one-second buffer so the requeue lands strictly after the cooldown expires, avoiding a race where the item fires just before CanSync flips to true. + c.enqueueAfter.EnqueueAfter(key, c.retryCooldownChecker.TimeUntilReady(key)+time.Second) + } + return nil + } + existingCluster, err := c.resourcesDBClient.HCPClusters(key.SubscriptionID, key.ResourceGroupName).Get(ctx, key.HCPClusterName) if cosmosstorageutils.IsNotFoundError(err) { return nil // cluster doesn't exist, no work to do @@ -106,8 +154,7 @@ func (c *nodePoolValidationSyncer) SyncOnce(ctx context.Context, key controlleru return utils.TrackError(fmt.Errorf("failed to get ServiceProviderNodePool: %w", err)) } - shouldProcess := c.shouldProcess(cachedServiceProviderNodePool) - if !shouldProcess { + if !c.shouldProcess(cachedServiceProviderNodePool) { return nil // no work to do } existingServiceProviderNodePool := cachedServiceProviderNodePool.DeepCopy() @@ -116,41 +163,105 @@ func (c *nodePoolValidationSyncer) SyncOnce(ctx context.Context, key controlleru return utils.TrackError(fmt.Errorf("failed to get Subscription: %w", err)) } - // We store the validation error in a separate variable and we use that as the - // error to return to the caller. This allows us to perform other remaining - // tasks in the syncer even if the validation fails, and we ultimately - // drive the behavior of its controller through the outcome of the validation. - validationErr := c.validation.Validate(ctx, existingCluster, subscription, existingNodePool) + result := c.validation.Validate(ctx, existingCluster, subscription, existingNodePool) + if err := result.Validate(); err != nil { + return utils.TrackError(fmt.Errorf("validation %s returned invalid ValidationResult: %w", c.validation.Name(), err)) + } - validationCondition := metav1.Condition{ - Type: c.validation.Name(), + if result.Outcome.Type != validationutils.OutcomeTypePassed { + logger.Info("Validation outcome", "validation", c.validation.Name(), "result", result) } - if validationErr != nil { - validationCondition.Status = metav1.ConditionFalse - validationCondition.Reason = "Failed" - validationCondition.Message = fmt.Sprintf("Validation failed: %s", validationErr.Error()) + + replacement := existingServiceProviderNodePool.DeepCopy() + + // If the validation was skipped, remove its condition so it doesn't appear in status. Otherwise, reconcile the condition with consecutive-Unknown + // suppression to avoid flapping on transient errors. + if result.Outcome.Type == validationutils.OutcomeTypeSkipped { + meta.RemoveStatusCondition(&replacement.Status.Validations, c.validation.Name()) } else { - validationCondition.Status = metav1.ConditionTrue - validationCondition.Reason = "Succeeded" - validationCondition.Message = "Validation succeeded" + previousCondition := meta.FindStatusCondition(existingServiceProviderNodePool.Status.Validations, c.validation.Name()) + desiredCondition := result.ToCondition(c.validation.Name()) + + consecutiveUnknowns := c.trackConsecutiveUnknowns(key, desiredCondition) + if c.shouldWriteCondition(previousCondition, consecutiveUnknowns) { + meta.SetStatusCondition(&replacement.Status.Validations, desiredCondition) + } } - meta.SetStatusCondition(&existingServiceProviderNodePool.Status.Validations, validationCondition) - serviceProviderNodePoolsCosmosClient := c.resourcesDBClient.ServiceProviderNodePools(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, key.HCPNodePoolName) - _, err = serviceProviderNodePoolsCosmosClient.Replace(ctx, existingServiceProviderNodePool, nil) - if cosmosstorageutils.IsPreconditionFailedError(err) { - // if we have a conflict error, then we're guaranteed that our informer will eventually see an update and trigger us again. - return nil + if !equality.Semantic.DeepEqual(existingServiceProviderNodePool, replacement) { + serviceProviderNodePoolsCosmosClient := c.resourcesDBClient.ServiceProviderNodePools(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName, key.HCPNodePoolName) + _, err = serviceProviderNodePoolsCosmosClient.Replace(ctx, replacement, nil) + if cosmosstorageutils.IsPreconditionFailedError(err) { + // if we have a conflict error, then we're guaranteed that our informer will eventually see an update and trigger us again. + return nil + } + if err != nil { + return utils.TrackError(fmt.Errorf("failed to replace ServiceProviderNodePool: %w", err)) + } } - if err != nil { - return utils.TrackError(fmt.Errorf("failed to replace ServiceProviderNodePool: %w", err)) + + c.handleRequeue(key, result) + + // ControllerReportingPolicy governs only how this Unknown result is reported to the controller + // machinery (e.g. workqueue error metrics); it has no bearing on the requeue scheduling already + // handled above by handleRequeue based on EarliestRetryAfter. Keep this as the last step of SyncOnce. + if result.Outcome.Type == validationutils.OutcomeTypeUnknown && result.Outcome.Unknown.ControllerReportingPolicy == validationutils.ControllerReportingPolicyTypeError { + return utils.TrackError(fmt.Errorf("validation %s returned an inconclusive (Unknown) result: %s", c.validation.Name(), result.InternalMessage())) } - return validationErr + return nil +} + +// handleRequeue updates the retry cooldown and schedules a delayed re-enqueue for key based solely on result.EarliestRetryAfter. +// If EarliestRetryAfter is nil, there is no retry backoff to apply; the informer may eventually see an update and trigger again. +func (c *nodePoolValidationSyncer) handleRequeue(key controllerutils.HCPNodePoolKey, result validationutils.ValidationResult) { + if result.EarliestRetryAfter == nil { + return + } + + c.retryCooldownChecker.SetCooldown(key, *result.EarliestRetryAfter) + if c.enqueueAfter != nil { + // Add a one-second buffer so the requeue lands strictly after the cooldown expires, avoiding a race where the item fires just before CanSync flips to true. + c.enqueueAfter.EnqueueAfter(key, *result.EarliestRetryAfter+time.Second) + } } // shouldProcess returns true when the condition associated to the validation does not exist or when it exists but -// it failed to run successfully in a previous attempt. +// its status is not True. func (c *nodePoolValidationSyncer) shouldProcess(serviceProviderNodePool *api.ServiceProviderNodePool) bool { return !meta.IsStatusConditionTrue(serviceProviderNodePool.Status.Validations, c.validation.Name()) } + +// shouldWriteCondition reports whether the newly computed validation condition should be written, versus +// suppressed in favor of leaving previousCondition (the condition currently stored for this validation, or +// nil if none is stored yet) untouched. +// +// The write is suppressed only while all of the following hold: +// - previousCondition is non-nil (there's something worth preserving), and +// - consecutiveUnknowns is non-zero (the newly computed condition is Unknown; trackConsecutiveUnknowns +// returns 0 for any non-Unknown result), and +// - consecutiveUnknowns has not yet exceeded maxConsecutiveUnknownsBeforeWrite. +// +// This backs a suppression policy that avoids flapping a node pool's validation status to Unknown on a +// transient blip: a persistent Unknown streak is still allowed to overwrite the stored condition once it +// exceeds maxConsecutiveUnknownsBeforeWrite, and a Passed/Failed result (consecutiveUnknowns == 0) always +// overwrites immediately, resetting the streak. +func (c *nodePoolValidationSyncer) shouldWriteCondition(previousCondition *metav1.Condition, consecutiveUnknowns int) bool { + return previousCondition == nil || consecutiveUnknowns == 0 || consecutiveUnknowns > maxConsecutiveUnknownsBeforeWrite +} + +// trackConsecutiveUnknowns maintains the count of consecutive Unknown validation results for the given key. When condition is Unknown it increments and returns the +// running count; otherwise it resets the counter and returns 0. +func (c *nodePoolValidationSyncer) trackConsecutiveUnknowns(key controllerutils.HCPNodePoolKey, condition metav1.Condition) int { + if condition.Status != metav1.ConditionUnknown { + c.consecutiveUnknownCounts.Remove(key) + return 0 + } + + count := 1 + if v, ok := c.consecutiveUnknownCounts.Get(key); ok { + count = v.(int) + 1 + } + c.consecutiveUnknownCounts.Add(key, count) + return count +} diff --git a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go index 68dea3822ec..8e2428bdcc1 100644 --- a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go +++ b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go @@ -16,9 +16,9 @@ package validation import ( "context" - "fmt" "strings" "testing" + "time" "github.com/go-logr/logr/testr" "github.com/stretchr/testify/assert" @@ -26,6 +26,8 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/lru" azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -33,6 +35,7 @@ import ( "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" + controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" "github.com/Azure/ARO-HCP/internal/database/cosmosstoragetesting/corecosmosstoragetesting" "github.com/Azure/ARO-HCP/internal/database/listertesting/corelistertesting" @@ -47,6 +50,18 @@ const ( testValidationName = "TestValidation" ) +var fixedNow = time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + +type fakeAfterEnqueuer struct { + enqueuedKeys []any + enqueuedDurations []time.Duration +} + +func (f *fakeAfterEnqueuer) EnqueueAfter(keyObj any, duration time.Duration) { + f.enqueuedKeys = append(f.enqueuedKeys, keyObj) + f.enqueuedDurations = append(f.enqueuedDurations, duration) +} + func newTestNodePoolKey() controllerutils.HCPNodePoolKey { return controllerutils.HCPNodePoolKey{ SubscriptionID: testSubscriptionID, @@ -114,18 +129,19 @@ func newTestSubscription() *arm.Subscription { } } -// mockNodePoolValidation implements validationutils.NodePoolValidation for tests. -type mockNodePoolValidation struct { - name string - validateErr error -} - -var _ validationutils.NodePoolValidation = (*mockNodePoolValidation)(nil) - -func (m *mockNodePoolValidation) Name() string { return m.name } - -func (m *mockNodePoolValidation) Validate(_ context.Context, _ *api.HCPOpenShiftCluster, _ *arm.Subscription, _ *api.HCPOpenShiftClusterNodePool) error { - return m.validateErr +func newTestSyncer(mockDB *corecosmosstoragetesting.MockResourcesDBClient, validation validationutils.NodePoolValidation, fakeClock *clocktesting.FakePassiveClock) (*nodePoolValidationSyncer, *fakeAfterEnqueuer) { + retryCooldown := controllerutil.NewSettableCooldownChecker() + retryCooldown.SetClock(fakeClock) + enqueuer := &fakeAfterEnqueuer{} + syncer := &nodePoolValidationSyncer{ + retryCooldownChecker: retryCooldown, + enqueueAfter: enqueuer, + resourcesDBClient: mockDB, + serviceProviderNodePoolLister: &corelistertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockDB}, + validation: validation, + consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity), + } + return syncer, enqueuer } func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { @@ -146,11 +162,16 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { } testCases := []struct { - name string - setupDB func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) - validation *mockNodePoolValidation - wantErr bool - wantConditionStatus *metav1.ConditionStatus + name string + setupDB func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) + validation validationutils.NodePoolValidation + wantErr bool + // wantCondition, if non-nil, asserts that the stored validation condition's Status/Reason/Message + // match (Type and LastTransitionTime are not compared). + wantCondition *metav1.Condition + // wantConditionAbsent asserts that no validation condition is stored at all. + wantConditionAbsent bool + wantEnqueue bool }{ { name: "cluster not found -- no-op", @@ -161,7 +182,7 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) require.NoError(t, err) }, - validation: &mockNodePoolValidation{name: testValidationName}, + validation: NewMockNodePoolValidation(testValidationName), }, { name: "node pool not found -- no-op", @@ -172,25 +193,90 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) require.NoError(t, err) }, - validation: &mockNodePoolValidation{name: testValidationName}, + validation: NewMockNodePoolValidation(testValidationName), + }, + { + name: "validation passes -- condition set to True", + setupDB: defaultSetupDB, + validation: NewMockNodePoolValidation(testValidationName).WithPassed(), + wantCondition: &metav1.Condition{Status: metav1.ConditionTrue, Reason: "AsExpected", Message: "As expected."}, + wantEnqueue: true, }, { - name: "validation succeeds -- condition set to True", + name: "validation fails -- condition set to False, requeue scheduled", setupDB: defaultSetupDB, - validation: &mockNodePoolValidation{ - name: testValidationName, - }, - wantConditionStatus: api.Ptr(metav1.ConditionTrue), + validation: NewMockNodePoolValidation(testValidationName).WithFailed( + "QuotaExceeded", "quota exceeded", "Quota exceeded for this subscription.", + ), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "QuotaExceeded", Message: "Quota exceeded for this subscription."}, + wantEnqueue: true, + }, + { + // Covers the "last step of SyncOnce" reporting-policy branch that turns an Unknown result into + // an error return; see TestNodePoolValidationSyncer_SyncOnce's sibling case below for the + // LogOnly branch of the same decision. + name: "validation unknown with ReportError -- condition set to Unknown, requeue scheduled, error returned", + setupDB: defaultSetupDB, + validation: NewMockNodePoolValidation(testValidationName).WithUnknownReportError( + "InternalError", "failed to reach Azure", "Unable to verify.", + ), + wantErr: true, + wantCondition: &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "InternalError", Message: "Unable to verify."}, + wantEnqueue: true, }, { - name: "validation fails -- condition set to False and error returned", + // Covers handleRequeue's nil guard: EarliestRetryAfter == nil must skip cooldown/requeue + // without otherwise affecting the condition write. + name: "validation fails with nil EarliestRetryAfter -- condition still set, no cooldown or requeue scheduled", setupDB: defaultSetupDB, - validation: &mockNodePoolValidation{ - name: testValidationName, - validateErr: fmt.Errorf("quota exceeded"), + validation: NewMockNodePoolValidation(testValidationName).WithFailed( + "QuotaExceeded", "quota exceeded", "Quota exceeded for this subscription.", + ).WithEarliestRetryAfter(nil), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "QuotaExceeded", Message: "Quota exceeded for this subscription."}, + wantEnqueue: false, + }, + { + name: "validation unknown with LogOnly -- condition set to Unknown, requeue still scheduled, no error returned", + setupDB: defaultSetupDB, + validation: NewMockNodePoolValidation(testValidationName).WithUnknownLogOnly( + "TransientIssue", "temporary network blip", "Temporarily unable to verify.", + ), + wantCondition: &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "TransientIssue", Message: "Temporarily unable to verify."}, + wantEnqueue: true, + }, + { + name: "validation skipped with no prior condition -- no condition persisted", + setupDB: defaultSetupDB, + validation: NewMockNodePoolValidation(testValidationName).WithSkipped( + "NotApplicable", "node pool does not need this check", "Not applicable.", + ), + wantConditionAbsent: true, + wantEnqueue: true, + }, + { + name: "validation skipped with prior condition -- condition removed", + setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { + t.Helper() + defaultSetupDB(t, ctx, mockDB) + spnpCRUD := mockDB.ServiceProviderNodePools(testSubscriptionID, testResourceGroup, testClusterName, testNodePoolName) + spnp, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + spnp.Status.Validations = []metav1.Condition{ + { + Type: testValidationName, + Status: metav1.ConditionFalse, + Reason: "PreviouslyFailed", + Message: "previously failed", + }, + } + _, err = spnpCRUD.Replace(ctx, spnp, nil) + require.NoError(t, err) }, - wantErr: true, - wantConditionStatus: api.Ptr(metav1.ConditionFalse), + validation: NewMockNodePoolValidation(testValidationName).WithSkipped( + "NotApplicable", "node pool does not need this check", "Not applicable.", + ), + wantConditionAbsent: true, + wantEnqueue: true, }, { name: "already-succeeded validation -- skipped", @@ -204,16 +290,15 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { { Type: testValidationName, Status: metav1.ConditionTrue, - Reason: "Succeeded", + Reason: "AsExpected", }, } _, err = spnpCRUD.Replace(ctx, spnp, nil) require.NoError(t, err) }, - validation: &mockNodePoolValidation{ - name: testValidationName, - validateErr: fmt.Errorf("should not be called"), - }, + validation: NewMockNodePoolValidation(testValidationName).WithFailed( + "ShouldNotBeCalled", "should not be called", "should not be called", + ), }, } @@ -226,11 +311,8 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { tc.setupDB(t, ctx, mockDB) } - syncer := &nodePoolValidationSyncer{ - resourcesDBClient: mockDB, - serviceProviderNodePoolLister: &corelistertesting.DBServiceProviderNodePoolLister{ResourcesDBClient: mockDB}, - validation: tc.validation, - } + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, enqueuer := newTestSyncer(mockDB, tc.validation, fakeClock) err := syncer.SyncOnce(ctx, newTestNodePoolKey()) if tc.wantErr { @@ -239,7 +321,23 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { require.NoError(t, err) } - if tc.wantConditionStatus != nil { + if tc.wantEnqueue { + require.NotEmpty(t, enqueuer.enqueuedKeys, "expected a requeue to be scheduled") + } else { + require.Empty(t, enqueuer.enqueuedKeys, "expected no requeue to be scheduled") + } + + if tc.wantConditionAbsent { + spnp, spnpErr := mockDB.ServiceProviderNodePools( + testSubscriptionID, testResourceGroup, testClusterName, testNodePoolName, + ).Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, spnpErr) + + cond := meta.FindStatusCondition(spnp.Status.Validations, testValidationName) + assert.Nil(t, cond, "expected validation condition to be absent") + } + + if tc.wantCondition != nil { spnp, spnpErr := mockDB.ServiceProviderNodePools( testSubscriptionID, testResourceGroup, testClusterName, testNodePoolName, ).Get(ctx, api.ServiceProviderNodePoolResourceName) @@ -247,15 +345,253 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { cond := meta.FindStatusCondition(spnp.Status.Validations, testValidationName) require.NotNil(t, cond, "expected validation condition to be set") - assert.Equal(t, *tc.wantConditionStatus, cond.Status) + assert.Equal(t, tc.wantCondition.Status, cond.Status) + assert.Equal(t, tc.wantCondition.Reason, cond.Reason) + assert.Equal(t, tc.wantCondition.Message, cond.Message) + } + }) + } +} - if tc.validation.validateErr != nil { - assert.Equal(t, "Failed", cond.Reason) - assert.Contains(t, cond.Message, tc.validation.validateErr.Error()) - } else { - assert.Equal(t, "Succeeded", cond.Reason) - } +// TestNodePoolValidationSyncer_ShouldWriteCondition unit-tests the suppression decision in isolation from +// Cosmos/DB plumbing, covering the boundary cases around maxConsecutiveUnknownsBeforeWrite. +func TestNodePoolValidationSyncer_ShouldWriteCondition(t *testing.T) { + // shouldWriteCondition only checks previousCondition's nilness, not its Status, so the Status value + // here is just fixture data; it could never realistically be ConditionTrue, since shouldProcess + // prevents SyncOnce from reaching this code once the stored condition is already True. + storedCondition := &metav1.Condition{Type: testValidationName, Status: metav1.ConditionUnknown} + + testCases := []struct { + name string + previousCondition *metav1.Condition + consecutiveUnknowns int + want bool + }{ + { + name: "no previously stored condition -- always write, even mid-streak", + previousCondition: nil, + consecutiveUnknowns: 5, + want: true, + }, + { + name: "previously stored condition, non-Unknown result (streak reset to 0) -- write", + previousCondition: storedCondition, + consecutiveUnknowns: 0, + want: true, + }, + { + name: "previously stored condition, first Unknown in streak -- suppress", + previousCondition: storedCondition, + consecutiveUnknowns: 1, + want: false, + }, + { + name: "previously stored condition, streak exactly at threshold -- suppress (boundary)", + previousCondition: storedCondition, + consecutiveUnknowns: maxConsecutiveUnknownsBeforeWrite, + want: false, + }, + { + name: "previously stored condition, streak one past threshold -- write (boundary)", + previousCondition: storedCondition, + consecutiveUnknowns: maxConsecutiveUnknownsBeforeWrite + 1, + want: true, + }, + } + + syncer := &nodePoolValidationSyncer{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, syncer.shouldWriteCondition(tc.previousCondition, tc.consecutiveUnknowns)) + }) + } +} + +// TestNodePoolValidationSyncer_TrackConsecutiveUnknowns unit-tests the per-key streak bookkeeping in +// isolation: incrementing across consecutive Unknown results, resetting on a non-Unknown result, and +// tracking each HCPNodePoolKey independently. Each test case is a sequence of steps run against a single +// fresh syncer, asserting the returned count after every step. +func TestNodePoolValidationSyncer_TrackConsecutiveUnknowns(t *testing.T) { + keyA := newTestNodePoolKey() + keyB := controllerutils.HCPNodePoolKey{ + SubscriptionID: testSubscriptionID, + ResourceGroupName: testResourceGroup, + HCPClusterName: testClusterName, + HCPNodePoolName: "other-nodepool", + } + + type step struct { + key controllerutils.HCPNodePoolKey + status metav1.ConditionStatus + want int + } + + testCases := []struct { + name string + steps []step + }{ + { + name: "increments on consecutive Unknown results", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionUnknown, want: 3}, + }, + }, + { + name: "non-Unknown result resets the streak to 0", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionFalse, want: 0}, + }, + }, + { + name: "streak restarts at 1 after a reset, not continuing the pre-reset count", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyA, status: metav1.ConditionTrue, want: 0}, + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + }, + }, + { + name: "non-Unknown result with no prior streak stays at 0", + steps: []step{ + {key: keyA, status: metav1.ConditionFalse, want: 0}, + }, + }, + { + name: "keys are tracked independently", + steps: []step{ + {key: keyA, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 2}, + {key: keyB, status: metav1.ConditionUnknown, want: 1}, + {key: keyA, status: metav1.ConditionUnknown, want: 3}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + syncer := &nodePoolValidationSyncer{consecutiveUnknownCounts: lru.New(consecutiveUnknownCountsCacheCapacity)} + for i, s := range tc.steps { + condition := metav1.Condition{Type: testValidationName, Status: s.status} + got := syncer.trackConsecutiveUnknowns(s.key, condition) + assert.Equalf(t, s.want, got, "step %d: key=%s, status=%s", i, s.key.HCPNodePoolName, s.status) } }) } } + +// TestNodePoolValidationSyncer_ConsecutiveUnknownSuppression exercises the consecutive-Unknown suppression +// policy end-to-end across repeated SyncOnce calls: a previously stored Failed condition should survive the +// first maxConsecutiveUnknownsBeforeWrite consecutive Unknown results untouched (and skip the Cosmos write +// each time, per the equality.Semantic.DeepEqual guard), then get overwritten with Unknown once the streak +// persists past the threshold. +func TestNodePoolValidationSyncer_ConsecutiveUnknownSuppression(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mockDB := corecosmosstoragetesting.NewMockResourcesDBClient() + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, newTestCluster(t), nil) + require.NoError(t, err) + nodePool := newTestNodePool(t) + _, err = mockDB.HCPClusters(testSubscriptionID, testResourceGroup).NodePools(testClusterName).Create(ctx, nodePool, nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + _, err = corecosmosstorage.GetOrCreateServiceProviderNodePool(ctx, mockDB, nodePool.ID) + require.NoError(t, err) + + spnpCRUD := mockDB.ServiceProviderNodePools(testSubscriptionID, testResourceGroup, testClusterName, testNodePoolName) + spnp, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + spnp.Status.Validations = []metav1.Condition{ + { + Type: testValidationName, + Status: metav1.ConditionFalse, + Reason: "PreviouslyFailed", + Message: "previously failed", + }, + } + _, err = spnpCRUD.Replace(ctx, spnp, nil) + require.NoError(t, err) + + validation := NewMockNodePoolValidation(testValidationName).WithUnknownLogOnly( + "InternalError", "failed to reach Azure", "Unable to verify.", + ) + + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, _ := newTestSyncer(mockDB, validation, fakeClock) + + for i := 1; i <= maxConsecutiveUnknownsBeforeWrite; i++ { + // Advance the clock past the previous attempt's EarliestRetryAfter deadline so SyncOnce doesn't + // short-circuit on the retryCooldownChecker suppression. + fakeClock.SetTime(fakeClock.Now().Add(time.Hour)) + + before, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + + require.NoError(t, syncer.SyncOnce(ctx, newTestNodePoolKey())) + + after, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + + cond := meta.FindStatusCondition(after.Status.Validations, testValidationName) + require.NotNil(t, cond) + assert.Equalf(t, metav1.ConditionFalse, cond.Status, "attempt %d: previous condition should be preserved", i) + assert.Equalf(t, "PreviouslyFailed", cond.Reason, "attempt %d: previous condition should be preserved", i) + assert.Equalf(t, before.CosmosETag, after.CosmosETag, "attempt %d: Cosmos write should have been skipped", i) + } + + // The next attempt exceeds the threshold, so the Unknown condition finally overwrites the stored one. + fakeClock.SetTime(fakeClock.Now().Add(time.Hour)) + + before, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + + require.NoError(t, syncer.SyncOnce(ctx, newTestNodePoolKey())) + + after, err := spnpCRUD.Get(ctx, api.ServiceProviderNodePoolResourceName) + require.NoError(t, err) + + cond := meta.FindStatusCondition(after.Status.Validations, testValidationName) + require.NotNil(t, cond) + assert.Equal(t, metav1.ConditionUnknown, cond.Status) + assert.Equal(t, "InternalError", cond.Reason) + assert.NotEqual(t, before.CosmosETag, after.CosmosETag, "expected a Cosmos write once the suppression threshold was exceeded") +} + +// TestNodePoolValidationSyncer_CooldownSuppression verifies that when the +// retryCooldownChecker's cooldown is active for a key, SyncOnce returns +// immediately without performing validation, and schedules a re-enqueue. +func TestNodePoolValidationSyncer_CooldownSuppression(t *testing.T) { + ctx := utils.ContextWithLogger(context.Background(), testr.New(t)) + + mockDB := corecosmosstoragetesting.NewMockResourcesDBClient() + _, err := mockDB.HCPClusters(testSubscriptionID, testResourceGroup).Create(ctx, newTestCluster(t), nil) + require.NoError(t, err) + nodePool := newTestNodePool(t) + _, err = mockDB.HCPClusters(testSubscriptionID, testResourceGroup).NodePools(testClusterName).Create(ctx, nodePool, nil) + require.NoError(t, err) + _, err = mockDB.Subscriptions().Create(ctx, newTestSubscription(), nil) + require.NoError(t, err) + _, err = corecosmosstorage.GetOrCreateServiceProviderNodePool(ctx, mockDB, nodePool.ID) + require.NoError(t, err) + + validation := NewMockNodePoolValidation(testValidationName).WithFailed( + "ShouldNotRun", "should not run", "should not run", + ) + + fakeClock := clocktesting.NewFakePassiveClock(fixedNow) + syncer, enqueuer := newTestSyncer(mockDB, validation, fakeClock) + + key := newTestNodePoolKey() + syncer.retryCooldownChecker.SetCooldown(key, 60*time.Second) + + err = syncer.SyncOnce(ctx, key) + require.NoError(t, err, "SyncOnce should return nil when cooldown is active") + + require.NotEmpty(t, enqueuer.enqueuedKeys, "should have re-enqueued after cooldown skip") + assert.Greater(t, enqueuer.enqueuedDurations[0], time.Duration(0), "enqueue duration should be positive") +} diff --git a/backend/pkg/utils/controllerutils/generic_watching_controller.go b/backend/pkg/utils/controllerutils/generic_watching_controller.go index b479820f389..877c396bfec 100644 --- a/backend/pkg/utils/controllerutils/generic_watching_controller.go +++ b/backend/pkg/utils/controllerutils/generic_watching_controller.go @@ -42,6 +42,13 @@ type GenericSyncer[T comparable] interface { MakeKey(resourceID *azcorearm.ResourceID) T } +// AfterEnqueuer allows scheduling a workqueue item for processing after an +// explicit delay. Validation controllers use this to implement +// EarliestRetryAfter semantics. +type AfterEnqueuer interface { + EnqueueAfter(keyObj any, duration time.Duration) +} + type Notifier interface { AddEventHandlerWithOptions(handler cache.ResourceEventHandler, options cache.HandlerOptions) (cache.ResourceEventHandlerRegistration, error) } @@ -77,6 +84,14 @@ func newGenericWatchingController[T comparable](name string, resourceType azcore return c } +func (c *genericWatchingController[T]) EnqueueAfter(keyObj any, duration time.Duration) { + key, ok := keyObj.(T) + if !ok { + return + } + c.queue.AddAfter(key, duration) +} + func (c *genericWatchingController[T]) SyncOnce(ctx context.Context, keyObj any) error { key, ok := keyObj.(T) if !ok { diff --git a/backend/pkg/utils/validationutils/always_success_validation.go b/backend/pkg/utils/validationutils/always_success_validation.go index 238af3ce750..3be71bafea9 100644 --- a/backend/pkg/utils/validationutils/always_success_validation.go +++ b/backend/pkg/utils/validationutils/always_success_validation.go @@ -30,8 +30,8 @@ func (v *AlwaysSuccessValidation) Name() string { return "AlwaysSuccessValidation" } -func (v *AlwaysSuccessValidation) Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) error { - return nil +func (v *AlwaysSuccessValidation) Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) ValidationResult { + return PassedValidation(api.ControllerConditionReasonAsExpected, "As expected", "AlwaysSuccessValidation is always successful.") } func NewAlwaysSuccessValidation() ClusterValidation { diff --git a/backend/pkg/utils/validationutils/azure_cluster_mis_existence_validation.go b/backend/pkg/utils/validationutils/azure_cluster_mis_existence_validation.go index 7603494c2da..b6890d39adf 100644 --- a/backend/pkg/utils/validationutils/azure_cluster_mis_existence_validation.go +++ b/backend/pkg/utils/validationutils/azure_cluster_mis_existence_validation.go @@ -24,7 +24,6 @@ import ( azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" - "github.com/Azure/ARO-HCP/internal/utils" ) // AzureClusterManagedIdentitiesExistenceValidation validates the existence of all managed identities defined in the cluster. @@ -45,7 +44,7 @@ func (v *AzureClusterManagedIdentitiesExistenceValidation) Name() string { return "AzureClusterManagedIdentitiesExistenceValidation" } -func (v *AzureClusterManagedIdentitiesExistenceValidation) Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) error { +func (v *AzureClusterManagedIdentitiesExistenceValidation) Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) ValidationResult { smiResourceID := cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity clusterIdentityURL := cluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL // We check the existence of the Cluster's Service Managed Identity by @@ -55,7 +54,12 @@ func (v *AzureClusterManagedIdentitiesExistenceValidation) Validate(ctx context. // service managed identity does not exist the request will fail. uaisClient, err := v.smiClientBuilder.UserAssignedIdentitiesClient(ctx, clusterIdentityURL, smiResourceID, cluster.ID.SubscriptionID) if err != nil { - return utils.TrackError(fmt.Errorf("failed to get user assigned identities client: %w", err)) + return UnknownValidation( + "InternalError", + "Unable to verify managed identities existence.", + fmt.Sprintf("failed to get user assigned identities client: %s", err), + ControllerReportingPolicyTypeError, + ) } clusterUAIsProfile := &cluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities @@ -66,18 +70,24 @@ func (v *AzureClusterManagedIdentitiesExistenceValidation) Validate(ctx context. _, err := uaisClient.Get(ctx, resourceID.ResourceGroupName, resourceID.Name, nil) if azureclient.IsResourceNotFoundErr(err) { notFoundMIsStrs = append(notFoundMIsStrs, resourceID.String()) + continue } if err != nil { - // TODO is it ok to error when one of them fails to get when the error is not a resource not found error? - return utils.TrackError(fmt.Errorf("failed to get managed identity '%s': %w", resourceID, err)) + return UnknownValidation( + "InternalError", + "Unable to verify managed identities existence.", + fmt.Sprintf("failed to get managed identity '%s': %s", resourceID, err), + ControllerReportingPolicyTypeError, + ) } } if len(notFoundMIsStrs) > 0 { - return utils.TrackError(fmt.Errorf("managed identities not found: %s", strings.Join(notFoundMIsStrs, ", "))) + internalAndUserMsg := fmt.Sprintf("Managed identities not found: %s", strings.Join(notFoundMIsStrs, ", ")) + return FailedValidation("ManagedIdentitiesNotFound", internalAndUserMsg, internalAndUserMsg) } - return nil + return PassedValidation(api.ControllerConditionReasonAsExpected, "As expected", "All managed identities exist.") } // clusterOperatorsManagedIdentities returns a list of the control and data plane identities defined in the cluster. diff --git a/backend/pkg/utils/validationutils/azure_cluster_resource_group_existence_validation.go b/backend/pkg/utils/validationutils/azure_cluster_resource_group_existence_validation.go index f06e6d01363..3801dd77f23 100644 --- a/backend/pkg/utils/validationutils/azure_cluster_resource_group_existence_validation.go +++ b/backend/pkg/utils/validationutils/azure_cluster_resource_group_existence_validation.go @@ -21,7 +21,6 @@ import ( azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" - "github.com/Azure/ARO-HCP/internal/utils" ) // AzureClusterResourceGroupExistenceValidation validates that the Azure Resource @@ -44,22 +43,40 @@ func (a *AzureClusterResourceGroupExistenceValidation) Name() string { func (a *AzureClusterResourceGroupExistenceValidation) Validate( ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster, -) error { +) ValidationResult { + // Full resource ID of the cluster's resource group. Falls back to just the name if Parent is nil. + clusterResourceGroupStr := cluster.ID.ResourceGroupName + if cluster.ID.Parent != nil { + clusterResourceGroupStr = cluster.ID.Parent.String() + } + rgClient, err := a.azureFPAClientBuilder.ResourceGroupsClient( *clusterSubscription.Properties.TenantId, cluster.ID.SubscriptionID, ) if err != nil { - return utils.TrackError(fmt.Errorf("failed to get resource groups client: %w", err)) + return UnknownValidation( + "InternalError", + "Unable to verify cluster's resource group existence.", + fmt.Sprintf("failed to get resource groups client: %s", err), + ControllerReportingPolicyTypeError, + ) } _, err = rgClient.Get(ctx, cluster.ID.ResourceGroupName, nil) if azureclient.IsResourceGroupNotFoundErr(err) { - return utils.TrackError(fmt.Errorf("resource group does not exist: %w", err)) + internalAndUserMsg := fmt.Sprintf("Resource group %q does not exist.", clusterResourceGroupStr) + return FailedValidation("ResourceGroupNotFound", internalAndUserMsg, internalAndUserMsg) } if err != nil { - return utils.TrackError(fmt.Errorf("failed to get resource group: %w", err)) + return UnknownValidation( + "InternalError", + "Unable to verify cluster's resource group existence.", + fmt.Sprintf("failed to get resource group: %s", err), + ControllerReportingPolicyTypeError, + ) } - return nil + internalAndUserMsg := fmt.Sprintf("Resource group %q exists.", clusterResourceGroupStr) + return PassedValidation(api.ControllerConditionReasonAsExpected, internalAndUserMsg, internalAndUserMsg) } diff --git a/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation.go b/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation.go index 9edd190b20e..1398308029d 100644 --- a/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation.go +++ b/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation.go @@ -21,12 +21,11 @@ import ( "github.com/Azure/ARO-HCP/backend/pkg/azure/cachedreader" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" - "github.com/Azure/ARO-HCP/internal/utils" ) // AzureVMSizeSupportsEphemeralOSDiskValidation validates that a node pool requesting // an ephemeral OS disk uses a VM size that advertises EphemeralOSDiskSupported. -// Node pools with managed OS disks are skipped. +// Node pools without ephemeral OS disks are skipped. type AzureVMSizeSupportsEphemeralOSDiskValidation struct { resourceSKUsCachedReader cachedreader.VirtualMachineResourceSKUsCachedReader } @@ -41,29 +40,50 @@ func (v *AzureVMSizeSupportsEphemeralOSDiskValidation) Name() string { return "AzureVMSizeSupportsEphemeralOSDiskValidation" } -func (v *AzureVMSizeSupportsEphemeralOSDiskValidation) Validate(ctx context.Context, _ *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) error { +func (v *AzureVMSizeSupportsEphemeralOSDiskValidation) Validate(ctx context.Context, _ *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) ValidationResult { if nodePool.Properties.Platform.OSDisk.DiskType != api.OsDiskTypeEphemeral { - return nil + return SkippedValidation( + "NotApplicable", + "Node pool does not use an ephemeral OS disk.", + "Node pool does not use an ephemeral OS disk; ephemeral OS disk validation does not apply.", + ) } if nodePoolSubscription.Properties == nil || nodePoolSubscription.Properties.TenantId == nil || *nodePoolSubscription.Properties.TenantId == "" { - return utils.TrackError(fmt.Errorf("subscription is missing tenant ID")) + return UnknownValidation( + "InternalError", + "Unable to verify VM size support for ephemeral OS disks.", + "subscription is missing tenant ID", + ControllerReportingPolicyTypeError, + ) } tenantID := *nodePoolSubscription.Properties.TenantId vmSize := nodePool.Properties.Platform.VMSize sku, err := v.resourceSKUsCachedReader.GetVirtualMachineSKU(ctx, tenantID, nodePool.ID.SubscriptionID, nodePool.Location, vmSize) if err != nil { - return utils.TrackError(fmt.Errorf("failed to get resource SKU for VM size %q: %w", vmSize, err)) + return UnknownValidation( + "InternalError", + "Unable to verify VM size support for ephemeral OS disks.", + fmt.Sprintf("failed to get resource SKU for VM size %q: %s", vmSize, err), + ControllerReportingPolicyTypeError, + ) } supported, found := isCapabilityEphemeralOSDiskSupported(sku) if !found { - return utils.TrackError(fmt.Errorf("resource SKU for VM size %q is missing %s capability", vmSize, computeResourceSKUCapabilityNameEphemeralOSDiskSupported)) + return UnknownValidation( + "InternalError", + "Unable to verify VM size support for ephemeral OS disks.", + fmt.Sprintf("resource SKU for VM size %q is missing %s capability", vmSize, computeResourceSKUCapabilityNameEphemeralOSDiskSupported), + ControllerReportingPolicyTypeError, + ) } if !supported { - return utils.TrackError(fmt.Errorf("vm size %q does not support ephemeral OS disks", vmSize)) + userMsg := fmt.Sprintf("vm size %q does not support ephemeral OS disks", vmSize) + return FailedValidation("EphemeralOSDiskNotSupported", userMsg, userMsg) } - return nil + internalMsg := fmt.Sprintf("VM size %q supports ephemeral OS disks.", vmSize) + return PassedValidation(api.ControllerConditionReasonAsExpected, internalMsg, internalMsg) } diff --git a/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation_test.go b/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation_test.go index d9bd3873a3e..abd983f1ab1 100644 --- a/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation_test.go +++ b/backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation_test.go @@ -106,15 +106,18 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { subscription *arm.Subscription nodePool *api.HCPOpenShiftClusterNodePool setupMockVMSKUCachedReader func(skuReader *cachedreader.MockVirtualMachineResourceSKUsCachedReader) - wantErr string + wantOutcome OutcomeType + wantInternalMessage string }{ { - name: "managed OS disk succeeds", - nodePool: newTestNodePool(t, api.OsDiskTypeManaged, testVMSize), + name: "managed OS disk succeeds", + nodePool: newTestNodePool(t, api.OsDiskTypeManaged, testVMSize), + wantOutcome: OutcomeTypeSkipped, }, { - name: "ephemeral OS disk succeeds when capability is True", - nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), + name: "ephemeral OS disk succeeds when capability is True", + nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), + wantOutcome: OutcomeTypePassed, setupMockVMSKUCachedReader: func(skuReader *cachedreader.MockVirtualMachineResourceSKUsCachedReader) { skuReader.EXPECT(). GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, "eastus", testVMSize). @@ -125,8 +128,9 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { }, }, { - name: "ephemeral OS disk succeeds when capability is true (case-insensitive)", - nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), + name: "ephemeral OS disk succeeds when capability is true (case-insensitive)", + nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), + wantOutcome: OutcomeTypePassed, setupMockVMSKUCachedReader: func(skuReader *cachedreader.MockVirtualMachineResourceSKUsCachedReader) { skuReader.EXPECT(). GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, "eastus", testVMSize). @@ -147,7 +151,8 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { Value: ptr.To("False"), }), nil) }, - wantErr: `vm size "Standard_D8ds_v5" does not support ephemeral OS disks`, + wantOutcome: OutcomeTypeFailed, + wantInternalMessage: `vm size "Standard_D8ds_v5" does not support ephemeral OS disks`, }, { name: "ephemeral OS disk fails when capability is missing", @@ -157,7 +162,8 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, "eastus", testVMSize). Return(makeTestVMResourceSKU(testVMSize), nil) }, - wantErr: `resource SKU for VM size "Standard_D8ds_v5" is missing EphemeralOSDiskSupported capability`, + wantOutcome: OutcomeTypeUnknown, + wantInternalMessage: `resource SKU for VM size "Standard_D8ds_v5" is missing EphemeralOSDiskSupported capability`, }, { name: "ephemeral OS disk fails when SKU lookup fails", @@ -167,15 +173,17 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, "eastus", testVMSize). Return(nil, errors.New("VM size not found")) }, - wantErr: `failed to get resource SKU for VM size "Standard_D8ds_v5": VM size not found`, + wantOutcome: OutcomeTypeUnknown, + wantInternalMessage: `failed to get resource SKU for VM size "Standard_D8ds_v5": VM size not found`, }, { name: "ephemeral OS disk fails when subscription is missing tenant ID", subscription: &arm.Subscription{ Properties: &arm.SubscriptionProperties{}, }, - nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), - wantErr: "subscription is missing tenant ID", + nodePool: newTestNodePool(t, api.OsDiskTypeEphemeral, testVMSize), + wantOutcome: OutcomeTypeUnknown, + wantInternalMessage: "subscription is missing tenant ID", }, } @@ -192,13 +200,11 @@ func TestAzureVMSizeSupportsEphemeralOSDiskValidation_Validate(t *testing.T) { sub = subscription } validation := NewAzureVMSizeSupportsEphemeralOSDiskValidation(skuReader) - err := validation.Validate(ctx, cluster, sub, tt.nodePool) - - if tt.wantErr == "" { - require.NoError(t, err) - } else { - require.Error(t, err) - assert.ErrorContains(t, err, tt.wantErr) + result := validation.Validate(ctx, cluster, sub, tt.nodePool) + require.NoError(t, result.Validate()) + assert.Equal(t, tt.wantOutcome, result.Outcome.Type) + if tt.wantInternalMessage != "" { + assert.Contains(t, result.InternalMessage(), tt.wantInternalMessage) } }) } diff --git a/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation.go b/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation.go index d17b397e005..adcf8da6801 100644 --- a/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation.go +++ b/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation.go @@ -16,7 +16,6 @@ package validationutils import ( "context" - "errors" "fmt" "strings" @@ -53,14 +52,23 @@ func (v *AzureNodePoolVMQuotaValidation) Name() string { return "AzureNodePoolVMQuotaValidation" } -func (v *AzureNodePoolVMQuotaValidation) Validate(ctx context.Context, _ *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) error { +func (v *AzureNodePoolVMQuotaValidation) Validate(ctx context.Context, _ *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) ValidationResult { instanceCount := v.requiredInstanceCount(nodePool) if instanceCount <= 0 { - return nil + return SkippedValidation( + "NotApplicable", + "Node pool has no instances to validate quota for.", + "Node pool has zero replicas and is not configured for autoscaling.", + ) } if nodePoolSubscription.Properties == nil || nodePoolSubscription.Properties.TenantId == nil || *nodePoolSubscription.Properties.TenantId == "" { - return utils.TrackError(fmt.Errorf("subscription is missing tenant ID")) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + "subscription is missing tenant ID", + ControllerReportingPolicyTypeError, + ) } tenantID := *nodePoolSubscription.Properties.TenantId @@ -69,31 +77,61 @@ func (v *AzureNodePoolVMQuotaValidation) Validate(ctx context.Context, _ *api.HC sku, err := v.resourceSKUsCachedReader.GetVirtualMachineSKU(ctx, tenantID, subscriptionID, nodePool.Location, vmSize) if err != nil { - return utils.TrackError(fmt.Errorf("failed to get resource SKU for VM size %q: %w", vmSize, err)) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + fmt.Sprintf("failed to get resource SKU for VM size %q: %s", vmSize, err), + ControllerReportingPolicyTypeError, + ) } if sku.Family == nil || *sku.Family == "" { - return utils.TrackError(fmt.Errorf("resource SKU for VM size %q is missing family", vmSize)) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + fmt.Sprintf("resource SKU for VM size %q is missing family", vmSize), + ControllerReportingPolicyTypeError, + ) } family := *sku.Family vcpusPerInstance, ok := lookupCapabilityVCPUs(sku) if !ok { - return utils.TrackError(fmt.Errorf("resource SKU for VM size %q is missing %s capability", vmSize, computeResourceSKUCapabilityNameVCPUs)) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + fmt.Sprintf("resource SKU for VM size %q is missing %s capability", vmSize, computeResourceSKUCapabilityNameVCPUs), + ControllerReportingPolicyTypeError, + ) } if vcpusPerInstance <= 0 { - return utils.TrackError(fmt.Errorf("resource SKU for VM size %q has unexpected %s capability value %d", vmSize, computeResourceSKUCapabilityNameVCPUs, vcpusPerInstance)) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + fmt.Sprintf("resource SKU for VM size %q has unexpected %s capability value %d", vmSize, computeResourceSKUCapabilityNameVCPUs, vcpusPerInstance), + ControllerReportingPolicyTypeError, + ) } requiredVCPUs := int64(instanceCount) * int64(vcpusPerInstance) usageClient, err := v.azureFPAClientBuilder.UsageClient(tenantID, subscriptionID) if err != nil { - return utils.TrackError(fmt.Errorf("failed to create usage client: %w", err)) + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + fmt.Sprintf("failed to create usage client: %s", err), + ControllerReportingPolicyTypeError, + ) } familyUsage, regionalUsage, err := v.lookupFamilyAndRegionalVCPUUsages(ctx, usageClient, nodePool.Location, family) if err != nil { - return err + return UnknownValidation( + "InternalError", + "Unable to verify VM quota.", + err.Error(), + ControllerReportingPolicyTypeError, + ) } // Limit 0 means creation is not allowed, independently of CurrentValue @@ -101,16 +139,22 @@ func (v *AzureNodePoolVMQuotaValidation) Validate(ctx context.Context, _ *api.HC familyRemaining := *familyUsage.Limit - int64(*familyUsage.CurrentValue) regionalRemaining := *regionalUsage.Limit - int64(*regionalUsage.CurrentValue) - var errs []error + var failureMessages []string if requiredVCPUs > familyRemaining { - errs = append(errs, utils.TrackError(fmt.Errorf("insufficient quota for VM size %q family %q: need %d vCPUs, have %d remaining for %q (current %d, limit %d)", - vmSize, family, requiredVCPUs, familyRemaining, localizedNameFromComputeUsage(familyUsage), *familyUsage.CurrentValue, *familyUsage.Limit))) + failureMessages = append(failureMessages, fmt.Sprintf("insufficient quota for VM size %q family %q: need %d vCPUs, have %d remaining for %q (current %d, limit %d)", + vmSize, family, requiredVCPUs, familyRemaining, localizedNameFromComputeUsage(familyUsage), *familyUsage.CurrentValue, *familyUsage.Limit)) } if requiredVCPUs > regionalRemaining { - errs = append(errs, utils.TrackError(fmt.Errorf("insufficient total regional vCPU quota for VM size %q: need %d vCPUs, have %d remaining for %q (current %d, limit %d)", - vmSize, requiredVCPUs, regionalRemaining, localizedNameFromComputeUsage(regionalUsage), *regionalUsage.CurrentValue, *regionalUsage.Limit))) + failureMessages = append(failureMessages, fmt.Sprintf("insufficient total regional vCPU quota for VM size %q: need %d vCPUs, have %d remaining for %q (current %d, limit %d)", + vmSize, requiredVCPUs, regionalRemaining, localizedNameFromComputeUsage(regionalUsage), *regionalUsage.CurrentValue, *regionalUsage.Limit)) } - return errors.Join(errs...) + if len(failureMessages) > 0 { + combined := strings.Join(failureMessages, "; ") + return FailedValidation("InsufficientVMQuota", combined, combined) + } + + internalMsg := fmt.Sprintf("Sufficient VM quota for VM size %q in location %q.", vmSize, nodePool.Location) + return PassedValidation(api.ControllerConditionReasonAsExpected, internalMsg, internalMsg) } // requiredInstanceCount returns the peak number of VMs the node pool may run. diff --git a/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation_test.go b/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation_test.go index 85a774e60da..424e969b14d 100644 --- a/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation_test.go +++ b/backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation_test.go @@ -105,23 +105,27 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { nodePool *api.HCPOpenShiftClusterNodePool setupMockVMSKUCachedReader func(skuReader *cachedreader.MockVirtualMachineResourceSKUsCachedReader) setupMockFPAUsageClient func(ctrl *gomock.Controller, fpaBuilder *azureclient.MockFirstPartyApplicationClientBuilder) - wantErrs []string + wantOutcome OutcomeType + wantInternalMessages []string }{ { - name: "zero replicas succeeds without quota checks", - nodePool: newQuotaTestNodePool(t, 0, nil), + name: "zero replicas skips without quota checks", + nodePool: newQuotaTestNodePool(t, 0, nil), + wantOutcome: OutcomeTypeSkipped, }, { name: "fails when subscription is missing tenant ID", subscription: &arm.Subscription{ Properties: &arm.SubscriptionProperties{}, }, - nodePool: newQuotaTestNodePool(t, 2, nil), - wantErrs: []string{"subscription is missing tenant ID"}, + nodePool: newQuotaTestNodePool(t, 2, nil), + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{"subscription is missing tenant ID"}, }, { - name: "fixed replicas succeeds when family and regional quota are sufficient", - nodePool: newQuotaTestNodePool(t, 3, nil), + name: "fixed replicas succeeds when family and regional quota are sufficient", + nodePool: newQuotaTestNodePool(t, 3, nil), + wantOutcome: OutcomeTypePassed, setupMockVMSKUCachedReader: func(skuReader *cachedreader.MockVirtualMachineResourceSKUsCachedReader) { skuReader.EXPECT(). GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, testLocation, testVMSize). @@ -164,7 +168,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeFailed, + wantInternalMessages: []string{ `insufficient quota for VM size "Standard_D8ds_v5" family "standardDASv4Family": need 20 vCPUs, have 15 remaining for "Standard DASv4 Family vCPUs" (current 85, limit 100)`, }, }, @@ -188,7 +193,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeFailed, + wantInternalMessages: []string{ `insufficient quota for VM size "Standard_D8ds_v5" family "standardDASv4Family": need 16 vCPUs, have 10 remaining for "Standard DASv4 Family vCPUs" (current 90, limit 100)`, }, }, @@ -212,7 +218,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeFailed, + wantInternalMessages: []string{ `insufficient total regional vCPU quota for VM size "Standard_D8ds_v5": need 16 vCPUs, have 5 remaining for "Total Regional vCPUs" (current 195, limit 200)`, }, }, @@ -236,7 +243,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeFailed, + wantInternalMessages: []string{ `insufficient quota for VM size "Standard_D8ds_v5" family "standardDASv4Family": need 16 vCPUs, have 10 remaining for "Standard DASv4 Family vCPUs" (current 90, limit 100)`, `insufficient total regional vCPU quota for VM size "Standard_D8ds_v5": need 16 vCPUs, have 5 remaining for "Total Regional vCPUs" (current 195, limit 200)`, }, @@ -249,7 +257,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, testLocation, testVMSize). Return(nil, errors.New("VM size not found")) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{ `failed to get resource SKU for VM size "Standard_D8ds_v5": VM size not found`, }, }, @@ -264,7 +273,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { Family: ptr.To(testVMFamily), }, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{ `resource SKU for VM size "Standard_D8ds_v5" is missing vCPUs capability`, }, }, @@ -276,7 +286,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { GetVirtualMachineSKU(gomock.Any(), testTenantID, testSubscriptionID, testLocation, testVMSize). Return(makeTestQuotaSKU("0"), nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{ `resource SKU for VM size "Standard_D8ds_v5" has unexpected vCPUs capability value 0`, }, }, @@ -299,7 +310,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{ `compute usage for VM family "standardDASv4Family" was not found in location "eastus"`, }, }, @@ -320,7 +332,8 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { UsageClient(testTenantID, testSubscriptionID). Return(usageClient, nil) }, - wantErrs: []string{ + wantOutcome: OutcomeTypeUnknown, + wantInternalMessages: []string{ `failed to list compute usages for location "eastus": service unavailable`, }, }, @@ -343,15 +356,11 @@ func TestAzureNodePoolVMQuotaValidation_Validate(t *testing.T) { sub = subscription } validation := NewAzureNodePoolVMQuotaValidation(skuReader, fpaBuilder) - err := validation.Validate(ctx, cluster, sub, tt.nodePool) - - if len(tt.wantErrs) == 0 { - require.NoError(t, err) - } else { - require.Error(t, err) - for _, wantErr := range tt.wantErrs { - assert.ErrorContains(t, err, wantErr) - } + result := validation.Validate(ctx, cluster, sub, tt.nodePool) + require.NoError(t, result.Validate()) + assert.Equal(t, tt.wantOutcome, result.Outcome.Type) + for _, wantInternalMessage := range tt.wantInternalMessages { + assert.Contains(t, result.InternalMessage(), wantInternalMessage) } }) } diff --git a/backend/pkg/utils/validationutils/azure_rp_registration_validation.go b/backend/pkg/utils/validationutils/azure_rp_registration_validation.go index 432bf039368..f9c9c0ee632 100644 --- a/backend/pkg/utils/validationutils/azure_rp_registration_validation.go +++ b/backend/pkg/utils/validationutils/azure_rp_registration_validation.go @@ -22,7 +22,6 @@ import ( azureclient "github.com/Azure/ARO-HCP/backend/pkg/azure/client" "github.com/Azure/ARO-HCP/internal/api" "github.com/Azure/ARO-HCP/internal/api/arm" - "github.com/Azure/ARO-HCP/internal/utils" ) // The RpRegistrationValidation struct validates the states of several @@ -45,7 +44,7 @@ func (v *AzureResourceProvidersRegistrationValidation) Name() string { func (v *AzureResourceProvidersRegistrationValidation) Validate( ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster, -) error { +) ValidationResult { resourceProvidersToCheck := []string{ "Microsoft.Authorization", "Microsoft.Compute", @@ -60,13 +59,23 @@ func (v *AzureResourceProvidersRegistrationValidation) Validate( cluster.ID.SubscriptionID, ) if err != nil { - return utils.TrackError(fmt.Errorf("failed to get resource providers client: %w", err)) + return UnknownValidation( + "InternalError", + "Unable to verify resource provider registration.", + fmt.Sprintf("failed to get resource providers client: %s", err), + ControllerReportingPolicyTypeError, + ) } for _, rp := range resourceProvidersToCheck { providerResp, err := rpClient.Get(ctx, rp, nil) if err != nil { - return err + return UnknownValidation( + "InternalError", + "Unable to verify resource provider registration.", + fmt.Sprintf("failed to get resource provider %s: %s", rp, err), + ControllerReportingPolicyTypeError, + ) } if providerResp.RegistrationState == nil || *providerResp.RegistrationState != "Registered" { @@ -75,9 +84,11 @@ func (v *AzureResourceProvidersRegistrationValidation) Validate( } if len(missingResourcesProviders) > 0 { - return utils.TrackError(fmt.Errorf("%v of the resource providers are not registered, or their state is empty: %s", - len(missingResourcesProviders), strings.Join(missingResourcesProviders, ", "))) + internalAndUserMsg := fmt.Sprintf("%d of the resource providers are not registered, or their state is empty: %s", + len(missingResourcesProviders), strings.Join(missingResourcesProviders, ", ")) + return FailedValidation("ResourceProvidersNotRegistered", internalAndUserMsg, internalAndUserMsg) } - return nil + internalMsg := fmt.Sprintf("All resource providers are registered: %s", strings.Join(resourceProvidersToCheck, ", ")) + return PassedValidation(api.ControllerConditionReasonAsExpected, "All resource providers are registered.", internalMsg) } diff --git a/backend/pkg/utils/validationutils/cluster_validation.go b/backend/pkg/utils/validationutils/cluster_validation.go index 4840dacdbc4..a64ee021770 100644 --- a/backend/pkg/utils/validationutils/cluster_validation.go +++ b/backend/pkg/utils/validationutils/cluster_validation.go @@ -25,6 +25,6 @@ import ( type ClusterValidation interface { // Name returns the name of the validation. Name() string - // Validate validates the Cluster. It returns nil if the validation succeeds and an error otherwise. - Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) error + // Validate validates the Cluster and returns a ValidationResult describing the outcome. + Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) ValidationResult } diff --git a/backend/pkg/utils/validationutils/nodepool_validation.go b/backend/pkg/utils/validationutils/nodepool_validation.go index fda52ba35f1..b33a8612f51 100644 --- a/backend/pkg/utils/validationutils/nodepool_validation.go +++ b/backend/pkg/utils/validationutils/nodepool_validation.go @@ -25,6 +25,6 @@ import ( type NodePoolValidation interface { // Name returns the name of the validation. Name() string - // Validate validates the NodePool. It returns nil if the validation succeeds and an error otherwise. - Validate(ctx context.Context, cluster *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) error + // Validate validates the NodePool and returns a ValidationResult describing the outcome. + Validate(ctx context.Context, cluster *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) ValidationResult } diff --git a/backend/pkg/utils/validationutils/validation_result.go b/backend/pkg/utils/validationutils/validation_result.go new file mode 100644 index 00000000000..73c4e0fa022 --- /dev/null +++ b/backend/pkg/utils/validationutils/validation_result.go @@ -0,0 +1,355 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validationutils + +import ( + "fmt" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/utils/ptr" +) + +const ( + // nonpassedRetryBase is the base retry delay for non-passed outcomes (Failed, Unknown, Skipped). + nonpassedRetryBase = 60 * time.Second + // passedRetryBase is the base retry delay for passed outcomes. + passedRetryBase = 12 * time.Hour + // jitterFactor is the maxFactor passed to wait.Jitter; 0.5 means up to +50% of the base. + jitterFactor = 0.5 +) + +// OutcomeType discriminates which validation outcome payload is populated. +type OutcomeType string + +const ( + // OutcomeTypePassed becomes a .status.validation.status=True, .reason=Passed.Reason, .message=Passed.UserMessage. + OutcomeTypePassed OutcomeType = "Passed" + // OutcomeTypeFailed becomes a .status.validation.status=False, .reason=Failed.Reason, .message=Failed.UserMessage. + OutcomeTypeFailed OutcomeType = "Failed" + // OutcomeTypeUnknown becomes a .status.validation.status=Unknown, .reason=Unknown.Reason, .message=Unknown.UserMessage. + OutcomeTypeUnknown OutcomeType = "Unknown" + // OutcomeTypeSkipped becomes a .status.validation.status=Unknown, .reason=Skipped.Reason, .message=Skipped.UserMessage. + OutcomeTypeSkipped OutcomeType = "Skipped" +) + +// ValidationResult is the result of a single validation check. Controllers map it to a status +// condition via ToCondition. Requeue scheduling is handled explicitly by the controller using +// SettableCooldownChecker and AfterEnqueuer based on EarliestRetryAfter. +// +// Do not instantiate this directly. Build one with FailedValidation, PassedValidation, +// SkippedValidation, or UnknownValidation. Those helpers set sensible defaults: +// FailedValidation, UnknownValidation, and SkippedValidation set EarliestRetryAfter to +// nonPassedRetryBase + jitter; PassedValidation sets it to passedRetryBase + jitter. +// If further customization is needed, EarliestRetryAfter can be overridden to a different value. +// +// For example: +// +// result := validationutils.FailedValidation("ValidationFailed", "The validation failed.", "The validation failed.") +// result.EarliestRetryAfter = ptr.To(60 * time.Second) +// +// return result +type ValidationResult struct { + // Outcome is exactly one of Passed, Failed, Unknown, or Skipped, as indicated by its Type. + Outcome outcome + // EarliestRetryAfter controls whether, and how soon, the controller explicitly requeues this + // resource after handling the result (see handleRequeue in each controller): it sets both the + // retry cooldown, via SettableCooldownChecker, and the delay before AfterEnqueuer re-adds the key + // to the workqueue. + // - nil means no explicit requeue is scheduled at all; the resource is only revisited whenever + // the informer next sees an update (e.g. a periodic resync or an external change), which may + // take an arbitrary amount of time. + // - 0 means requeue as soon as possible, with no artificial backoff. + // - A positive duration delays the requeue by that amount, e.g. to back off after a Failed or + // Unknown result instead of immediately re-running a validation likely to fail again. + EarliestRetryAfter *time.Duration +} + +// Reason returns the machine-readable reason string for the outcome. +func (r ValidationResult) Reason() string { + switch r.Outcome.Type { + case OutcomeTypeFailed: + return r.Outcome.Failed.Reason + case OutcomeTypeUnknown: + return r.Outcome.Unknown.Reason + case OutcomeTypeSkipped: + return r.Outcome.Skipped.Reason + case OutcomeTypePassed: + return r.Outcome.Passed.Reason + } + return "" +} + +// InternalMessage returns the human-readable internal message for the outcome, +// intended for logs and diagnostics (not surfaced to the user). +func (r ValidationResult) InternalMessage() string { + switch r.Outcome.Type { + case OutcomeTypeFailed: + return r.Outcome.Failed.InternalMessage + case OutcomeTypeUnknown: + return r.Outcome.Unknown.InternalMessage + case OutcomeTypeSkipped: + return r.Outcome.Skipped.InternalMessage + case OutcomeTypePassed: + return r.Outcome.Passed.InternalMessage + } + return "" +} + +// Validate checks that the outcome is well-formed: exactly one payload is set and it matches the declared Type. Controllers should call this before acting +// on the result to fail fast on programmer errors. +func (r ValidationResult) Validate() error { + count := 0 + if r.Outcome.Passed != nil { + count++ + } + if r.Outcome.Failed != nil { + count++ + } + if r.Outcome.Unknown != nil { + count++ + } + if r.Outcome.Skipped != nil { + count++ + } + if count != 1 { + return fmt.Errorf("expected exactly one outcome to be set, got %d", count) + } + + switch r.Outcome.Type { + case OutcomeTypePassed: + if r.Outcome.Passed == nil { + return fmt.Errorf("outcome Type is %q but Passed payload is nil", r.Outcome.Type) + } + if r.Outcome.Passed.Reason == "" { + return fmt.Errorf("passed outcome has empty Reason") + } + case OutcomeTypeFailed: + if r.Outcome.Failed == nil { + return fmt.Errorf("outcome Type is %q but Failed payload is nil", r.Outcome.Type) + } + if r.Outcome.Failed.Reason == "" { + return fmt.Errorf("failed outcome has empty Reason") + } + case OutcomeTypeUnknown: + if r.Outcome.Unknown == nil { + return fmt.Errorf("outcome Type is %q but Unknown payload is nil", r.Outcome.Type) + } + if r.Outcome.Unknown.Reason == "" { + return fmt.Errorf("unknown outcome has empty Reason") + } + case OutcomeTypeSkipped: + if r.Outcome.Skipped == nil { + return fmt.Errorf("outcome Type is %q but Skipped payload is nil", r.Outcome.Type) + } + if r.Outcome.Skipped.Reason == "" { + return fmt.Errorf("skipped outcome has empty Reason") + } + default: + return fmt.Errorf("unrecognized outcome Type: %q", r.Outcome.Type) + } + + if r.EarliestRetryAfter != nil && *r.EarliestRetryAfter < 0 { + return fmt.Errorf("EarliestRetryAfter must be >= 0, got %s", *r.EarliestRetryAfter) + } + return nil +} + +// ToCondition maps the validationResult to a metav1.Condition with the given condition type (typically the validation name). +func (r ValidationResult) ToCondition(conditionType string) metav1.Condition { + cond := metav1.Condition{ + Type: conditionType, + } + switch r.Outcome.Type { + case OutcomeTypePassed: + cond.Status = metav1.ConditionTrue + cond.Reason = r.Outcome.Passed.Reason + cond.Message = r.Outcome.Passed.UserMessage + case OutcomeTypeFailed: + cond.Status = metav1.ConditionFalse + cond.Reason = r.Outcome.Failed.Reason + cond.Message = r.Outcome.Failed.UserMessage + case OutcomeTypeSkipped: + cond.Status = metav1.ConditionUnknown + cond.Reason = r.Outcome.Skipped.Reason + cond.Message = r.Outcome.Skipped.UserMessage + case OutcomeTypeUnknown: + unknown := r.Outcome.Unknown + cond.Status = metav1.ConditionUnknown + cond.Reason = unknown.Reason + cond.Message = unknown.UserMessage + } + return cond +} + +// outcome is the outcome of a validation: exactly one of Passed, Failed, Unknown, or Skipped, as +// indicated by Type. Construct one via the FailedValidation, PassedValidation, SkippedValidation, or +// UnknownValidation helpers — never build an outcome literal directly. Because outcome itself is +// unexported, those helpers (and the validationResult they return) are the only way for callers +// outside this package to produce one, which guarantees Type can never disagree with the populated +// payload. +type outcome struct { + // Type discriminates which payload field is populated. Exactly one of Passed, Failed, Unknown, or Skipped will be non-nil, matching this value. + Type OutcomeType + + // Failed is set when the validation deterministically failed (e.g. quota exceeded). + Failed *failedOutcome + // Unknown is set when the validation could not reach a conclusive result (e.g. transient Azure error). + Unknown *unknownOutcome + // Passed is set when the validation succeeded. + Passed *passedOutcome + // Skipped is set when the validation was intentionally not evaluated. + Skipped *skippedOutcome +} + +// failedOutcome indicates the validation determinately failed. It becomes a +// .status.validation.status=False, .reason=Reason, .message=UserMessage. +type failedOutcome struct { + // machine readable, must not be sensitive + Reason string + // human readable, for internal use (e.g. logs); not surfaced to the user + InternalMessage string + // human readable for user + UserMessage string +} + +// FailedValidation returns a validationResult indicating the validation failed. +// EarliestRetryAfter is set to nonPassedRetryBase + jitter. +func FailedValidation(reason string, userMessage string, internalMessage string) ValidationResult { + result := ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeFailed, + Failed: &failedOutcome{ + Reason: reason, + InternalMessage: internalMessage, + UserMessage: userMessage, + }}, + } + // Jitter avoids retry storms: wait.Jitter(base, 0.5) returns a value in [base, base*1.5]. + retryWithJitter := wait.Jitter(nonpassedRetryBase, jitterFactor) + result.EarliestRetryAfter = ptr.To(retryWithJitter) + return result +} + +// passedOutcome indicates the validation succeeded. It becomes a +// .status.validation.status=True, .reason=Reason, .message=UserMessage. +type passedOutcome struct { + // machine readable, must not be sensitive + Reason string + // human readable, for internal use (e.g. logs); not surfaced to the user + InternalMessage string + // human readable for user + UserMessage string +} + +// PassedValidation returns a validationResult indicating the validation passed. +// EarliestRetryAfter is set to passedRetryBase + jitter. +func PassedValidation(reason string, userMessage string, internalMessage string) ValidationResult { + result := ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypePassed, + Passed: &passedOutcome{ + Reason: reason, + InternalMessage: internalMessage, + UserMessage: userMessage, + }, + }, + } + // Jitter avoids retry storms: wait.Jitter(base, 0.5) returns a value in [base, base*1.5]. + retryWithJitter := wait.Jitter(passedRetryBase, jitterFactor) + result.EarliestRetryAfter = ptr.To(retryWithJitter) + return result +} + +// skippedOutcome indicates the validation was not evaluated. It becomes a +// .status.validation.status=Unknown, .reason=Reason, .message=UserMessage. +type skippedOutcome struct { + // machine readable, must not be sensitive + Reason string + // human readable, for internal use (e.g. logs); not surfaced to the user + InternalMessage string + // human readable for user + UserMessage string +} + +// SkippedValidation returns a validationResult indicating the validation was not evaluated. +// EarliestRetryAfter is set to nonPassedRetryBase + jitter. +func SkippedValidation(reason string, userMessage string, internalMessage string) ValidationResult { + result := ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeSkipped, + Skipped: &skippedOutcome{ + Reason: reason, + InternalMessage: internalMessage, + UserMessage: userMessage, + }, + }, + } + // Jitter avoids retry storms: wait.Jitter(base, 0.5) returns a value in [base, base*1.5]. + retryWithJitter := wait.Jitter(nonpassedRetryBase, jitterFactor) + result.EarliestRetryAfter = ptr.To(retryWithJitter) + return result +} + +// unknownOutcome indicates the validation could not be conclusively evaluated. It becomes a +// .status.validation.status=Unknown, .reason=Reason, .message=UserMessage. +type unknownOutcome struct { + // machine readable, must not be sensitive + Reason string + // human readable, for internal use (e.g. logs); not surfaced to the user + InternalMessage string + // human readable for user + UserMessage string + // ControllerReportingPolicy controls how this Unknown result is surfaced to the controller machinery + // (see controllerReportingPolicyType); it has no effect on retry/requeue scheduling. + ControllerReportingPolicy controllerReportingPolicyType +} + +// UnknownValidation returns a validationResult indicating the validation could not be conclusively evaluated. +// EarliestRetryAfter is set to nonPassedRetryBase + jitter. reportingPolicy only controls whether the +// controller's SyncOnce returns nil or an error for this result (see controllerReportingPolicyType); it +// does not affect requeue scheduling, which is driven entirely by EarliestRetryAfter. +func UnknownValidation(reason string, userMessage string, internalMessage string, reportingPolicy controllerReportingPolicyType) ValidationResult { + result := ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeUnknown, + Unknown: &unknownOutcome{ + Reason: reason, + InternalMessage: internalMessage, + UserMessage: userMessage, + ControllerReportingPolicy: reportingPolicy, + }, + }, + } + // Jitter avoids retry storms: wait.Jitter(base, 0.5) returns a value in [base, base*1.5]. + retryWithJitter := wait.Jitter(nonpassedRetryBase, jitterFactor) + result.EarliestRetryAfter = ptr.To(retryWithJitter) + return result +} + +// controllerReportingPolicyType governs how a controller's SyncOnce reports an validation outcome back to the generic controller machinery, +// by selecting whether SyncOnce returns nil or a non-nil error for that sync. +// It is deliberately independent of retry/requeue scheduling: EarliestRetryAfter alone controls whether and how soon, the resource is requeued. +// Do not use controllerReportingPolicyType to try to suppress or influence requeue behavior — use EarliestRetryAfter for that instead. +type controllerReportingPolicyType string + +var ( + // ControllerReportingPolicyTypeLogOnly means SyncOnce returns nil, so it is only logged and does not count as a controller error (e.g. in workqueue error metrics). Useful for + // certain types of failures that are expected/benign and shouldn't be alerted on. + ControllerReportingPolicyTypeLogOnly controllerReportingPolicyType = "LogOnly" + // ControllerReportingPolicyTypeError means SyncOnce returns a non-nil error, so it is tracked as a controller error for reporting/metrics purposes. + ControllerReportingPolicyTypeError controllerReportingPolicyType = "ReportError" +) diff --git a/backend/pkg/utils/validationutils/validation_result_test.go b/backend/pkg/utils/validationutils/validation_result_test.go new file mode 100644 index 00000000000..1fbb06fcffc --- /dev/null +++ b/backend/pkg/utils/validationutils/validation_result_test.go @@ -0,0 +1,323 @@ +// Copyright 2026 Microsoft Corporation +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validationutils + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" +) + +func TestValidationResult_Validate(t *testing.T) { + testCases := []struct { + name string + result ValidationResult + wantErr string // empty means Validate() must return nil + }{ + { + name: "passed outcome from helper is valid", + result: PassedValidation("R", "u", "i"), + }, + { + name: "failed outcome from helper is valid", + result: FailedValidation("R", "u", "i"), + }, + { + name: "skipped outcome from helper is valid", + result: SkippedValidation("R", "u", "i"), + }, + { + name: "unknown outcome from helper is valid", + result: UnknownValidation("R", "u", "i", ControllerReportingPolicyTypeError), + }, + { + name: "zero value has no outcome set", + result: ValidationResult{}, + wantErr: "expected exactly one outcome to be set, got 0", + }, + { + name: "two outcomes set", + result: ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypePassed, + Passed: &passedOutcome{Reason: "R"}, + Failed: &failedOutcome{Reason: "R"}, + }, + }, + wantErr: "expected exactly one outcome to be set, got 2", + }, + { + name: "Type is Passed but Passed payload is nil", + result: ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypePassed, + Failed: &failedOutcome{Reason: "R"}, + }, + }, + wantErr: `outcome Type is "Passed" but Passed payload is nil`, + }, + { + name: "Type is Failed but Failed payload is nil", + result: ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeFailed, + Passed: &passedOutcome{Reason: "R"}, + }, + }, + wantErr: `outcome Type is "Failed" but Failed payload is nil`, + }, + { + name: "Type is Unknown but Unknown payload is nil", + result: ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeUnknown, + Passed: &passedOutcome{Reason: "R"}, + }, + }, + wantErr: `outcome Type is "Unknown" but Unknown payload is nil`, + }, + { + name: "Type is Skipped but Skipped payload is nil", + result: ValidationResult{ + Outcome: outcome{ + Type: OutcomeTypeSkipped, + Passed: &passedOutcome{Reason: "R"}, + }, + }, + wantErr: `outcome Type is "Skipped" but Skipped payload is nil`, + }, + { + name: "passed outcome has empty Reason", + result: ValidationResult{ + Outcome: outcome{Type: OutcomeTypePassed, Passed: &passedOutcome{Reason: ""}}, + }, + wantErr: "passed outcome has empty Reason", + }, + { + name: "failed outcome has empty Reason", + result: ValidationResult{ + Outcome: outcome{Type: OutcomeTypeFailed, Failed: &failedOutcome{Reason: ""}}, + }, + wantErr: "failed outcome has empty Reason", + }, + { + name: "unknown outcome has empty Reason", + result: ValidationResult{ + Outcome: outcome{Type: OutcomeTypeUnknown, Unknown: &unknownOutcome{Reason: ""}}, + }, + wantErr: "unknown outcome has empty Reason", + }, + { + name: "skipped outcome has empty Reason", + result: ValidationResult{ + Outcome: outcome{Type: OutcomeTypeSkipped, Skipped: &skippedOutcome{Reason: ""}}, + }, + wantErr: "skipped outcome has empty Reason", + }, + { + name: "unrecognized outcome Type", + result: ValidationResult{ + Outcome: outcome{Type: OutcomeType("Bogus"), Passed: &passedOutcome{Reason: "R"}}, + }, + wantErr: `unrecognized outcome Type: "Bogus"`, + }, + { + name: "nil EarliestRetryAfter is valid", + result: func() ValidationResult { + result := FailedValidation("R", "u", "i") + result.EarliestRetryAfter = nil + return result + }(), + }, + { + name: "zero EarliestRetryAfter is valid", + result: func() ValidationResult { + result := FailedValidation("R", "u", "i") + result.EarliestRetryAfter = ptr.To(time.Duration(0)) + return result + }(), + }, + { + name: "negative EarliestRetryAfter is invalid", + result: func() ValidationResult { + result := FailedValidation("R", "u", "i") + result.EarliestRetryAfter = ptr.To(-time.Second) + return result + }(), + wantErr: "EarliestRetryAfter must be >= 0", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := tc.result.Validate() + if tc.wantErr == "" { + assert.NoError(t, err) + return + } + assert.ErrorContains(t, err, tc.wantErr) + }) + } +} + +// TestValidationConstructors covers FailedValidation, PassedValidation, SkippedValidation, and +// UnknownValidation: the outcome Type they set, the values Reason()/InternalMessage()/ToCondition() +// surface, and that EarliestRetryAfter is populated. +func TestValidationConstructors(t *testing.T) { + testCases := []struct { + name string + construct func() ValidationResult + wantType OutcomeType + wantReason string + wantInternalMsg string + wantUserMsg string + wantStatus metav1.ConditionStatus + // extra optionally asserts on fields specific to one constructor's outcome (e.g. Unknown's + // ControllerReportingPolicy). + extra func(t *testing.T, result ValidationResult) + }{ + { + name: "FailedValidation", + construct: func() ValidationResult { return FailedValidation("FailReason", "user message", "internal message") }, + wantType: OutcomeTypeFailed, + wantReason: "FailReason", + wantInternalMsg: "internal message", + wantUserMsg: "user message", + wantStatus: metav1.ConditionFalse, + }, + { + name: "PassedValidation", + construct: func() ValidationResult { return PassedValidation("PassReason", "user message", "internal message") }, + wantType: OutcomeTypePassed, + wantReason: "PassReason", + wantInternalMsg: "internal message", + wantUserMsg: "user message", + wantStatus: metav1.ConditionTrue, + }, + { + name: "SkippedValidation", + construct: func() ValidationResult { return SkippedValidation("SkipReason", "user message", "internal message") }, + wantType: OutcomeTypeSkipped, + wantReason: "SkipReason", + wantInternalMsg: "internal message", + wantUserMsg: "user message", + wantStatus: metav1.ConditionUnknown, + }, + { + name: "UnknownValidation", + construct: func() ValidationResult { + return UnknownValidation("UnknownReason", "user message", "internal message", ControllerReportingPolicyTypeError) + }, + wantType: OutcomeTypeUnknown, + wantReason: "UnknownReason", + wantInternalMsg: "internal message", + wantUserMsg: "user message", + wantStatus: metav1.ConditionUnknown, + extra: func(t *testing.T, result ValidationResult) { + require.NotNil(t, result.Outcome.Unknown, "Unknown payload was nil") + assert.Equal(t, ControllerReportingPolicyTypeError, result.Outcome.Unknown.ControllerReportingPolicy) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := tc.construct() + + assert.NoError(t, result.Validate()) + assert.Equal(t, tc.wantType, result.Outcome.Type) + assert.Equal(t, tc.wantReason, result.Reason()) + assert.Equal(t, tc.wantInternalMsg, result.InternalMessage()) + + cond := result.ToCondition("SomeCondition") + assert.Equal(t, tc.wantStatus, cond.Status) + assert.Equal(t, tc.wantReason, cond.Reason) + assert.Equal(t, tc.wantUserMsg, cond.Message) + + assert.NotNil(t, result.EarliestRetryAfter, "EarliestRetryAfter was nil") + + if tc.extra != nil { + tc.extra(t, result) + } + }) + } +} + +func TestValidationResult_ToCondition(t *testing.T) { + testCases := []struct { + name string + result ValidationResult + conditionType string + wantStatus metav1.ConditionStatus + wantReason string + wantMessage string + }{ + { + name: "passed outcome maps to status True", + result: PassedValidation("PassReason", "user message", "internal message"), + conditionType: "AzureRPRegistration", + wantStatus: metav1.ConditionTrue, + wantReason: "PassReason", + wantMessage: "user message", + }, + { + name: "failed outcome maps to status False", + result: FailedValidation("FailReason", "user message", "internal message"), + conditionType: "AzureRPRegistration", + wantStatus: metav1.ConditionFalse, + wantReason: "FailReason", + wantMessage: "user message", + }, + { + name: "skipped outcome maps to status Unknown", + result: SkippedValidation("SkipReason", "user message", "internal message"), + conditionType: "AzureRPRegistration", + wantStatus: metav1.ConditionUnknown, + wantReason: "SkipReason", + wantMessage: "user message", + }, + { + name: "unknown outcome maps to status Unknown", + result: UnknownValidation("UnknownReason", "user message", "internal message", ControllerReportingPolicyTypeLogOnly), + conditionType: "AzureRPRegistration", + wantStatus: metav1.ConditionUnknown, + wantReason: "UnknownReason", + wantMessage: "user message", + }, + { + name: "conditionType argument is passed through verbatim", + result: PassedValidation("PassReason", "user message", "internal message"), + conditionType: "SomeOtherConditionType", + wantStatus: metav1.ConditionTrue, + wantReason: "PassReason", + wantMessage: "user message", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cond := tc.result.ToCondition(tc.conditionType) + assert.Equal(t, tc.conditionType, cond.Type) + assert.Equal(t, tc.wantStatus, cond.Status) + assert.Equal(t, tc.wantReason, cond.Reason) + assert.Equal(t, tc.wantMessage, cond.Message) + }) + } +} diff --git a/internal/controllerutils/cooldown.go b/internal/controllerutils/cooldown.go index 364ef7747c4..e9be41989d0 100644 --- a/internal/controllerutils/cooldown.go +++ b/internal/controllerutils/cooldown.go @@ -88,3 +88,52 @@ func (c *TimeBasedCooldownChecker) CanSync(_ context.Context, key any) bool { } return false } + +// SettableCooldownChecker is a cooldown gate where the per-key cooldown +// duration is set explicitly by the caller via SetCooldown, rather than +// being fixed at construction time. A key with no cooldown set is always +// allowed. +type SettableCooldownChecker struct { + clock utilsclock.PassiveClock + nextExecTime *lru.Cache +} + +func NewSettableCooldownChecker() *SettableCooldownChecker { + return &SettableCooldownChecker{ + clock: utilsclock.RealClock{}, + nextExecTime: lru.New(1000000), + } +} + +func (c *SettableCooldownChecker) SetClock(clock utilsclock.PassiveClock) { + c.clock = clock +} + +// SetCooldown records that the given key should not be re-synced until +// now+duration has elapsed. +func (c *SettableCooldownChecker) SetCooldown(key any, duration time.Duration) { + c.nextExecTime.Add(key, c.clock.Now().Add(duration)) +} + +func (c *SettableCooldownChecker) CanSync(_ context.Context, key any) bool { + now := c.clock.Now() + nextExecTime, ok := c.nextExecTime.Get(key) + if !ok { + return true + } + return now.After(nextExecTime.(time.Time)) +} + +// TimeUntilReady returns the duration until the key's cooldown expires. +// Returns 0 if the key has no cooldown set or the cooldown has already expired. +func (c *SettableCooldownChecker) TimeUntilReady(key any) time.Duration { + nextExecTime, ok := c.nextExecTime.Get(key) + if !ok { + return 0 + } + d := nextExecTime.(time.Time).Sub(c.clock.Now()) + if d < 0 { + return 0 + } + return d +} diff --git a/internal/controllerutils/cooldown_test.go b/internal/controllerutils/cooldown_test.go index deb16da007d..43bf8708928 100644 --- a/internal/controllerutils/cooldown_test.go +++ b/internal/controllerutils/cooldown_test.go @@ -51,3 +51,90 @@ func TestTimeBasedCooldownChecker_RepeatedFalseDoesNotPreventTrue(t *testing.T) t.Fatal("expected CanSync to return true after cooldown expired") } } + +func TestSettableCooldownChecker_NoKeyAlwaysAllowed(t *testing.T) { + checker := NewSettableCooldownChecker() + ctx := context.Background() + + if !checker.CanSync(ctx, "unknown-key") { + t.Fatal("expected CanSync to return true for key with no cooldown set") + } +} + +func TestSettableCooldownChecker_SetCooldownBlocksThenExpires(t *testing.T) { + startTime := time.Now() + fakeClock := clocktesting.NewFakePassiveClock(startTime) + checker := NewSettableCooldownChecker() + checker.SetClock(fakeClock) + + ctx := context.Background() + key := "test-key" + + checker.SetCooldown(key, 30*time.Second) + + if checker.CanSync(ctx, key) { + t.Fatal("expected CanSync to return false within cooldown window") + } + + fakeClock.SetTime(startTime.Add(29 * time.Second)) + if checker.CanSync(ctx, key) { + t.Fatal("expected CanSync to return false just before cooldown expires") + } + + fakeClock.SetTime(startTime.Add(31 * time.Second)) + if !checker.CanSync(ctx, key) { + t.Fatal("expected CanSync to return true after cooldown expired") + } +} + +func TestSettableCooldownChecker_TimeUntilReady(t *testing.T) { + startTime := time.Now() + fakeClock := clocktesting.NewFakePassiveClock(startTime) + checker := NewSettableCooldownChecker() + checker.SetClock(fakeClock) + + key := "test-key" + + if d := checker.TimeUntilReady(key); d != 0 { + t.Fatalf("expected 0 for key with no cooldown, got %v", d) + } + + checker.SetCooldown(key, 60*time.Second) + + if d := checker.TimeUntilReady(key); d != 60*time.Second { + t.Fatalf("expected 60s, got %v", d) + } + + fakeClock.SetTime(startTime.Add(45 * time.Second)) + if d := checker.TimeUntilReady(key); d != 15*time.Second { + t.Fatalf("expected 15s, got %v", d) + } + + fakeClock.SetTime(startTime.Add(61 * time.Second)) + if d := checker.TimeUntilReady(key); d != 0 { + t.Fatalf("expected 0 after expiry, got %v", d) + } +} + +func TestSettableCooldownChecker_OverwriteCooldown(t *testing.T) { + startTime := time.Now() + fakeClock := clocktesting.NewFakePassiveClock(startTime) + checker := NewSettableCooldownChecker() + checker.SetClock(fakeClock) + + ctx := context.Background() + key := "test-key" + + checker.SetCooldown(key, 10*time.Second) + checker.SetCooldown(key, 60*time.Second) + + fakeClock.SetTime(startTime.Add(11 * time.Second)) + if checker.CanSync(ctx, key) { + t.Fatal("expected CanSync to return false; second SetCooldown should overwrite the first") + } + + fakeClock.SetTime(startTime.Add(61 * time.Second)) + if !checker.CanSync(ctx, key) { + t.Fatal("expected CanSync to return true after the overwritten cooldown expires") + } +} From 2c698835ee7464fd76f2d31b1d232ff25fd95140 Mon Sep 17 00:00:00 2001 From: Suraj Patil Date: Wed, 5 Aug 2026 22:49:28 +0530 Subject: [PATCH 2/2] enable constant validation for clusters and node pools Signed-off-by: Suraj Patil --- .../validation/cluster_validation_controller.go | 10 ---------- .../validation/cluster_validation_controller_test.go | 9 +++++---- .../validation/nodepool_validation_controller.go | 10 ---------- .../validation/nodepool_validation_controller_test.go | 9 +++++---- docs/cosmos-data-flow.md | 10 +++++----- 5 files changed, 15 insertions(+), 33 deletions(-) diff --git a/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go b/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go index 290f453ce23..181b4dc63e5 100644 --- a/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go +++ b/backend/pkg/controllers/cluster/validation/cluster_validation_controller.go @@ -26,7 +26,6 @@ import ( "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" - "github.com/Azure/ARO-HCP/internal/api" controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" @@ -141,9 +140,6 @@ func (c *clusterValidationSyncer) SyncOnce(ctx context.Context, key controllerut return utils.TrackError(fmt.Errorf("failed to get ServiceProviderCluster: %w", err)) } - if !c.shouldProcess(cachedServiceProviderCluster) { - return nil // no work to do - } existingServiceProviderCluster := cachedServiceProviderCluster.DeepCopy() subscription, err := c.resourcesDBClient.Subscriptions().Get(ctx, existingCluster.ID.SubscriptionID) if err != nil { @@ -213,12 +209,6 @@ func (c *clusterValidationSyncer) handleRequeue(key controllerutils.HCPClusterKe } } -// shouldProcess returns true when the condition associated to the validation does not exist or when it exists but -// its status is not True. -func (c *clusterValidationSyncer) shouldProcess(serviceProviderCluster *api.ServiceProviderCluster) bool { - return !meta.IsStatusConditionTrue(serviceProviderCluster.Status.Validations, c.validation.Name()) -} - // shouldWriteCondition reports whether the newly computed validation condition should be written, versus // suppressed in favor of leaving previousCondition (the condition currently stored for this validation, or // nil if none is stored yet) untouched. diff --git a/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go b/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go index 769a6115000..6249bbbf598 100644 --- a/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go +++ b/backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go @@ -248,7 +248,7 @@ func TestClusterValidationSyncer_SyncOnce(t *testing.T) { wantEnqueue: true, }, { - name: "already-succeeded validation -- skipped", + name: "already-succeeded validation -- still re-run", setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { t.Helper() defaultSetupDB(t, ctx, mockDB) @@ -266,8 +266,10 @@ func TestClusterValidationSyncer_SyncOnce(t *testing.T) { require.NoError(t, err) }, validation: NewMockClusterValidation(testValidationName).WithFailed( - "ShouldNotBeCalled", "should not be called", "should not be called", + "NoLongerValid", "no longer valid", "No longer valid.", ), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "NoLongerValid", Message: "No longer valid."}, + wantEnqueue: true, }, } @@ -332,8 +334,7 @@ func TestClusterValidationSyncer_ShouldWriteCondition(t *testing.T) { }) // shouldWriteCondition only checks previousCondition's nilness, not its Status. Vary Status here to - // lock that contract: suppression depends solely on consecutiveUnknowns. A prior passed condition is - // unreachable via SyncOnce (shouldProcess skips it), but the helper must still behave consistently. + // lock that contract: suppression depends solely on consecutiveUnknowns. previousConditionFixtures := []struct { name string condition *metav1.Condition diff --git a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go index 44870b58393..73a66a62208 100644 --- a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go +++ b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go @@ -26,7 +26,6 @@ import ( "github.com/Azure/ARO-HCP/backend/pkg/utils/controllerutils" "github.com/Azure/ARO-HCP/backend/pkg/utils/validationutils" - "github.com/Azure/ARO-HCP/internal/api" controllerutil "github.com/Azure/ARO-HCP/internal/controllerutils" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/corecosmosstorage" "github.com/Azure/ARO-HCP/internal/database/cosmosstorage/cosmosstorageutils" @@ -154,9 +153,6 @@ func (c *nodePoolValidationSyncer) SyncOnce(ctx context.Context, key controlleru return utils.TrackError(fmt.Errorf("failed to get ServiceProviderNodePool: %w", err)) } - if !c.shouldProcess(cachedServiceProviderNodePool) { - return nil // no work to do - } existingServiceProviderNodePool := cachedServiceProviderNodePool.DeepCopy() subscription, err := c.resourcesDBClient.Subscriptions().Get(ctx, existingNodePool.ID.SubscriptionID) if err != nil { @@ -226,12 +222,6 @@ func (c *nodePoolValidationSyncer) handleRequeue(key controllerutils.HCPNodePool } } -// shouldProcess returns true when the condition associated to the validation does not exist or when it exists but -// its status is not True. -func (c *nodePoolValidationSyncer) shouldProcess(serviceProviderNodePool *api.ServiceProviderNodePool) bool { - return !meta.IsStatusConditionTrue(serviceProviderNodePool.Status.Validations, c.validation.Name()) -} - // shouldWriteCondition reports whether the newly computed validation condition should be written, versus // suppressed in favor of leaving previousCondition (the condition currently stored for this validation, or // nil if none is stored yet) untouched. diff --git a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go index 8e2428bdcc1..60329079587 100644 --- a/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go +++ b/backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go @@ -279,7 +279,7 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { wantEnqueue: true, }, { - name: "already-succeeded validation -- skipped", + name: "already-succeeded validation -- still re-run", setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) { t.Helper() defaultSetupDB(t, ctx, mockDB) @@ -297,8 +297,10 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { require.NoError(t, err) }, validation: NewMockNodePoolValidation(testValidationName).WithFailed( - "ShouldNotBeCalled", "should not be called", "should not be called", + "NoLongerValid", "no longer valid", "No longer valid.", ), + wantCondition: &metav1.Condition{Status: metav1.ConditionFalse, Reason: "NoLongerValid", Message: "No longer valid."}, + wantEnqueue: true, }, } @@ -357,8 +359,7 @@ func TestNodePoolValidationSyncer_SyncOnce(t *testing.T) { // Cosmos/DB plumbing, covering the boundary cases around maxConsecutiveUnknownsBeforeWrite. func TestNodePoolValidationSyncer_ShouldWriteCondition(t *testing.T) { // shouldWriteCondition only checks previousCondition's nilness, not its Status, so the Status value - // here is just fixture data; it could never realistically be ConditionTrue, since shouldProcess - // prevents SyncOnce from reaching this code once the stored condition is already True. + // here is just fixture data. storedCondition := &metav1.Condition{Type: testValidationName, Status: metav1.ConditionUnknown} testCases := []struct { diff --git a/docs/cosmos-data-flow.md b/docs/cosmos-data-flow.md index 7b687b3a267..10da20a71ed 100644 --- a/docs/cosmos-data-flow.md +++ b/docs/cosmos-data-flow.md @@ -1010,14 +1010,14 @@ No Cosmos writes. Posts `NodePoolUpgradePolicy` to Cluster Service. **File:** [cluster_validation_controller.go](../backend/pkg/controllers/cluster/validation/cluster_validation_controller.go), [nodepool_validation_controller.go](../backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go) **Trigger:** Cluster/NodePool informer, 1-minute resync -**Gate (shouldProcess on ServiceProviderCluster/ServiceProviderNodePool):** -- `!meta.IsStatusConditionTrue(ServiceProviderCluster.Status.Validations, validation.Name())` (condition must not yet be True) -- SyncOnce also checks `DeletionTimestamp == nil` on the resource +**Gate:** +- SyncOnce checks `DeletionTimestamp == nil` on the resource +- The validation always re-runs regardless of the previously stored condition | | Object | Fields | |---|--------|--------| -| Read | `ServiceProviderCluster` |
  • `Status.Validations[]` (shouldProcess: condition must not be True)
| -| Read | `ServiceProviderNodePool` |
  • `Status.Validations[]` (shouldProcess: condition must not be True)
| +| Read | `ServiceProviderCluster` |
  • `Status.Validations[]` (used to compute consecutive-Unknown suppression, not to gate whether validation runs)
| +| Read | `ServiceProviderNodePool` |
  • `Status.Validations[]` (used to compute consecutive-Unknown suppression, not to gate whether validation runs)
| | Read | `HCPOpenShiftCluster` |
  • `ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)
| | Read | `HCPOpenShiftClusterNodePool` |
  • `ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)
| | **Write** | **`ServiceProviderCluster`** |
  • **`Status.Validations[]`** = condition (True/False)
|