fix: ARO-29372: Enables continuous validation - #6439
Open
Suraj Patil (patilsuraj767) wants to merge 1 commit into
Open
Suraj Patil (patilsuraj767) wants to merge 1 commit into
Suraj Patil (patilsuraj767) wants to merge 1 commit into
Conversation
openshift-ci
Bot
requested review from
Jan-Hendrik Boll (janboll) and
Matthew Barnes (mbarnes)
August 5, 2026 17:21
Suraj Patil (patilsuraj767)
changed the title
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
August 5, 2026 17:22
View session
Suraj Patil (patilsuraj767)
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the backend cluster/nodepool validation framework from an error-based API to a structured ValidationResult model, and updates the validation controllers to re-run validations over time with explicit retry scheduling/backoff and suppression of transient Unknown flapping. It also updates the Cosmos data-flow documentation to reflect the new “always re-run” validation behavior.
Changes:
- Introduce
validationutils.ValidationResult(Passed/Failed/Unknown/Skipped) with retry scheduling metadata and conversion to status conditions. - Update validation interfaces and several Azure validations to return
ValidationResult, and adjust unit tests accordingly. - Update cluster/nodepool validation controllers to re-run validations continuously, add explicit delayed requeue support, and add consecutive-Unknown suppression logic.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controllerutils/cooldown.go | Adds SettableCooldownChecker used to gate/schedule per-key retry cooldowns. |
| internal/controllerutils/cooldown_test.go | Adds unit tests for SettableCooldownChecker. |
| docs/cosmos-data-flow.md | Updates validation controller “Gate” documentation to reflect always re-running behavior. |
| backend/pkg/utils/validationutils/validation_result.go | Adds new ValidationResult type, constructors, validation, and mapping to metav1.Condition. |
| backend/pkg/utils/validationutils/validation_result_test.go | Adds unit tests for ValidationResult validation and condition mapping. |
| backend/pkg/utils/validationutils/nodepool_validation.go | Changes nodepool validation interface to return ValidationResult. |
| backend/pkg/utils/validationutils/cluster_validation.go | Changes cluster validation interface to return ValidationResult. |
| backend/pkg/utils/validationutils/azure_rp_registration_validation.go | Migrates RP registration validation to ValidationResult outcomes. |
| backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation.go | Migrates VM quota validation to ValidationResult outcomes (incl. skipped/not-applicable). |
| backend/pkg/utils/validationutils/azure_nodepool_vm_quota_validation_test.go | Updates quota validation tests to assert outcomes/messages instead of errors. |
| backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation.go | Migrates ephemeral OS disk validation to ValidationResult outcomes. |
| backend/pkg/utils/validationutils/azure_nodepool_ephemeral_os_disk_validation_test.go | Updates ephemeral OS disk validation tests to assert outcomes/messages. |
| backend/pkg/utils/validationutils/azure_cluster_resource_group_existence_validation.go | Migrates RG existence validation to ValidationResult outcomes. |
| backend/pkg/utils/validationutils/azure_cluster_mis_existence_validation.go | Migrates managed identity existence validation to ValidationResult outcomes. |
| backend/pkg/utils/validationutils/always_success_validation.go | Updates always-success validation to return a Passed ValidationResult. |
| backend/pkg/utils/controllerutils/generic_watching_controller.go | Adds AfterEnqueuer support (EnqueueAfter) to schedule delayed workqueue items. |
| backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go | Reworks nodepool validation controller to always rerun, write conditions based on ValidationResult, schedule delayed retries, and suppress consecutive Unknown flapping. |
| backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go | Updates controller tests for new outcomes/requeue/suppression behavior. |
| backend/pkg/controllers/nodepool/validation/mock_nodepool_validation.go | Adds mock nodepool validation for controller tests (returns ValidationResult). |
| backend/pkg/controllers/cluster/validation/mock_cluster_validation.go | Adds mock cluster validation for controller tests (returns ValidationResult). |
| backend/pkg/controllers/cluster/validation/cluster_validation_controller.go | Reworks cluster validation controller similarly (outcomes/requeue/suppression). |
| backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go | Adds/updates tests for cluster validation controller behavior. |
| backend/pkg/app/backend.go | Updates controller wiring for the updated validation controller constructors/signatures. |
Suppressed comments (2)
backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go:222
handleRequeuealways adds a 1s buffer toEarliestRetryAfter. That changes the documented semantics whereEarliestRetryAfter == 0should requeue ASAP (no artificial backoff). It also makes controller behavior diverge from the configured retry duration.
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)
}
backend/pkg/controllers/cluster/validation/cluster_validation_controller.go:209
handleRequeuealways adds a 1s buffer toEarliestRetryAfter, which breaks the documented meaning ofEarliestRetryAfter == 0(requeue ASAP) and makes the actual delay larger than requested.
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)
}
| if !ok { | ||
| return true | ||
| } | ||
| return now.After(nextExecTime.(time.Time)) |
| } | ||
|
|
||
| if r.EarliestRetryAfter != nil && *r.EarliestRetryAfter < 0 { | ||
| return fmt.Errorf("EarliestRetryAfter must be >= 0, got %s", *r.EarliestRetryAfter) |
Comment on lines
+199
to
+203
| // 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. |
| return result | ||
| } | ||
|
|
||
| // controllerReportingPolicyType governs how a controller's SyncOnce reports an validation outcome back to the generic controller machinery, |
Comment on lines
19
to
23
| "fmt" | ||
| "time" | ||
|
|
||
| "k8s.io/apimachinery/pkg/api/equality" | ||
| "k8s.io/apimachinery/pkg/api/meta" |
Comment on lines
+115
to
+119
| // 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. |
Comment on lines
+113
to
+117
| // 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. |
| |---|--------|--------| | ||
| | Read | `ServiceProviderCluster` | <ul><li>`Status.Validations[<name>]` (shouldProcess: condition must not be True)</li></ul> | | ||
| | Read | `ServiceProviderNodePool` | <ul><li>`Status.Validations[<name>]` (shouldProcess: condition must not be True)</li></ul> | | ||
| | Read | `ServiceProviderCluster` | <ul><li>`Status.Validations[<name>]` (used to compute consecutive-Unknown suppression, not to gate whether validation runs)</li></ul> | |
Collaborator
There was a problem hiding this comment.
Is this AI-generate change correct? it at the least doesn't seem very well explained
Collaborator
|
Could you switch this to a draft state? I assume you are still working on it, and don't ask for a review yet. So this way it would be easier to filter out when checking PRs for a review. |
Suraj Patil (patilsuraj767)
marked this pull request as draft
August 13, 2026 17:08
Suraj Patil (patilsuraj767)
force-pushed
the
enables-constant-validation
branch
from
September 1, 2026 11:28
2c69883 to
d3fdb7e
Compare
Suraj Patil (patilsuraj767)
marked this pull request as ready for review
September 1, 2026 11:31
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
September 1, 2026 11:31
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
docs/cosmos-data-flow.md:1061
- This section implies validations only write True/False, but the controllers can also write ConditionUnknown (and remove the condition entirely when the validation is Skipped). The doc should reflect those possible persisted states.
| Read | `ServiceProviderNodePool` | <ul><li>`Status.Validations[<name>]` (previousCondition / consecutive-Unknown suppression)</li></ul> |
| Read | `HCPOpenShiftCluster` | <ul><li>`ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)</li></ul> |
| Read | `HCPOpenShiftClusterNodePool` | <ul><li>`ServiceProviderProperties.DeletionTimestamp` (SyncOnce: must be nil)</li></ul> |
| **Write** | **`ServiceProviderCluster`** | <ul><li>**`Status.Validations[<name>]`** = condition (True/False)</li></ul> |
| **Write** | **`ServiceProviderNodePool`** | <ul><li>**`Status.Validations[<name>]`** = condition (True/False)</li></ul> |
Collaborator
Author
|
/test e2e-parallel |
Collaborator
Author
|
/test e2e-parallel |
1 similar comment
Collaborator
Author
|
/test e2e-parallel |
Suraj Patil (patilsuraj767)
changed the title
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
September 9, 2026 08:52
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces “temporary” production logging/interval changes that can create noisy logs and materially alter steady-state validation cadence without a clear, durable configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
backend/pkg/controllers/cluster/validation/cluster_validation_controller.go:160
- Logging all validation outcomes (including Passed) at Info level will likely create high-volume logs now that validations re-run continuously. Consider restoring the previous guard (or logging Passed at a higher verbosity) to keep default logs focused on non-passing outcomes.
// TODO: temporary debug log — emit all outcomes including Passed so we can confirm continuous re-runs. Remove after verification (restore the != Passed guard).
logger.Info("Validation outcome", "validation", c.validation.Name(), "outcome", result.Outcome.Type, "result", result)
backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go:173
- Logging all validation outcomes (including Passed) at Info level will likely create high-volume logs now that validations re-run continuously. Consider restoring the previous guard (or logging Passed at a higher verbosity) to keep default logs focused on non-passing outcomes.
// TODO: temporary debug log — emit all outcomes including Passed so we can confirm continuous re-runs. Remove after verification (restore the != Passed guard).
logger.Info("Validation outcome", "validation", c.validation.Name(), "outcome", result.Outcome.Type, "result", result)
docs/cosmos-data-flow.md:1052
- This section removes the old "condition must not yet be True" gate, but it also no longer mentions the new cooldown/EarliestRetryAfter gating (
retryCooldownChecker.CanSync) that now throttles continuous re-runs. Update the Gate description so the doc matches the controller behavior.
**Gate:**
- SyncOnce checks `DeletionTimestamp == nil` on the resource
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+29
to
+31
| // passedRetryBase is the base retry delay for passed outcomes. | ||
| passedRetryBase = 12 * time.Hour | ||
| // TODO: temporary — originally 12 * time.Hour. Shortened so Passed re-runs can be verified from logs without waiting overnight. Restore after verification. | ||
| passedRetryBase = 60 * time.Second |
Suraj Patil (patilsuraj767)
force-pushed
the
enables-constant-validation
branch
from
September 9, 2026 12:41
dbdce3e to
26e63c2
Compare
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
September 9, 2026 12:41
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The node pool watching controller’s new CooldownChecker delegation path is untested, which risks regressions in enqueue-time throttling behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
137
to
144
| func (c *nodePoolWatchingController) CooldownChecker() controllerutil.CooldownChecker { | ||
| if p, ok := c.syncer.(interface { | ||
| CooldownChecker() controllerutil.CooldownChecker | ||
| }); ok { | ||
| return p.CooldownChecker() | ||
| } | ||
| return nil | ||
| } |
Signed-off-by: Suraj Patil <patilsuraj767@gmail.com>
Suraj Patil (patilsuraj767)
force-pushed
the
enables-constant-validation
branch
from
September 14, 2026 00:55
26e63c2 to
787f864
Compare
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
September 14, 2026 00:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Cooldown gating, retry timing, and changed-versus-unchanged regression coverage remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
backend/pkg/controllers/cluster/validation/cluster_validation_controller.go:160
- This adds explicitly temporary debug instrumentation to the production path and logs the entire result for every Passed validation. It will create persistent per-validation log volume (especially with the current 60-second retry), so remove the TODO/debug change before merging and retain the existing conditional logging.
// TODO: temporary debug log — emit all outcomes including Passed so we can confirm continuous re-runs. Remove after verification (restore the != Passed guard).
logger.Info("Validation outcome", "validation", c.validation.Name(), "outcome", result.Outcome.Type, "result", result)
backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go:254
- This renamed case starts a fresh syncer, so
CanSyncis true and it cannot exercise the cooldown behavior that this PR is changing. The existing cooldown-suppression tests still assert the old contract, and there is no regression test proving that validation runs for a changed ETag while a cooldown is active; update the tests to cover the changed-versus-unchanged paths.
name: "already-succeeded validation -- re-runs and overwrites with Failed",
setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) {
t.Helper()
defaultSetupDB(t, ctx, mockDB)
spcCRUD := mockDB.ServiceProviderClusters(testSubscriptionID, testResourceGroup, testClusterName)
backend/pkg/controllers/nodepool/validation/nodepool_validation_controller.go:173
- This adds explicitly temporary debug instrumentation to the production path and logs the entire result for every Passed validation. It will create persistent per-validation log volume (especially with the current 60-second retry), so remove the TODO/debug change before merging and retain the existing conditional logging.
// TODO: temporary debug log — emit all outcomes including Passed so we can confirm continuous re-runs. Remove after verification (restore the != Passed guard).
logger.Info("Validation outcome", "validation", c.validation.Name(), "outcome", result.Outcome.Type, "result", result)
backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go:285
- This renamed case starts a fresh syncer, so
CanSyncis true and it cannot exercise the cooldown behavior that this PR is changing. The existing cooldown-suppression tests still assert the old contract, and there is no regression test proving that validation runs for a changed ETag while a cooldown is active; update the tests to cover the changed-versus-unchanged paths.
name: "already-succeeded validation -- re-runs and overwrites with Failed",
setupDB: func(t *testing.T, ctx context.Context, mockDB *corecosmosstoragetesting.MockResourcesDBClient) {
t.Helper()
defaultSetupDB(t, ctx, mockDB)
spnpCRUD := mockDB.ServiceProviderNodePools(testSubscriptionID, testResourceGroup, testClusterName, testNodePoolName)
docs/cosmos-data-flow.md:1098
- This updated gate description is inaccurate with the current implementation: both validation
SyncOncemethods still callretryCooldownChecker.CanSync, so cooldown remains a gate in addition toDeletionTimestamp. Either complete the source change that moves cooldown handling to the watcher, or keep that gate documented until the implementation is updated.
**Gate:**
- SyncOnce checks `DeletionTimestamp == nil` on the resource
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
118
to
122
| "validation", c.validation.Name(), | ||
| "retryAfter", c.retryCooldownChecker.TimeUntilReady(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. |
Comment on lines
+118
to
+122
| // TODO: temporary debug log to confirm continuous validation is gated (not skipped) for Passed results. Remove after verification. | ||
| logger.Info("Validation cooldown active; skipping Validate", | ||
| "validation", c.validation.Name(), | ||
| "retryAfter", c.retryCooldownChecker.TimeUntilReady(key), | ||
| ) |
Comment on lines
+30
to
+31
| // TODO: temporary — originally 12 * time.Hour. Shortened so Passed re-runs can be verified from logs without waiting overnight. Restore after verification. | ||
| passedRetryBase = 60 * time.Second |
Collaborator
Author
|
Added debug logs and verified the Kusto logs:
|
Suraj Patil (patilsuraj767)
force-pushed
the
enables-constant-validation
branch
from
September 16, 2026 12:07
787f864 to
b6b00cd
Compare
Copilot started reviewing on behalf of
Suraj Patil (patilsuraj767)
September 16, 2026 12:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The cooldown gate remains in both SyncOnce paths, watcher cooldown checkers are not wired, and tests and documentation do not reflect the intended behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
backend/pkg/controllers/cluster/validation/cluster_validation_controller_test.go:250
- This regression test only exercises removal of the
shouldProcessgate. The cluster syncer still returns beforeValidate()whenretryCooldownChecker.CanSyncis false (cluster_validation_controller.go:113-121), so an ETag-change event duringEarliestRetryAfteris still dropped—the main behavior this PR claims to fix. Please remove that worker-level short-circuit, expose the checker through the watching controller'sCooldownChecker()so unchanged resyncs remain suppressed, and update the existing cooldown test to assert the new behavior.
name: "already-succeeded validation -- re-runs and overwrites with Failed",
backend/pkg/controllers/nodepool/validation/nodepool_validation_controller_test.go:281
- This regression test only exercises removal of the
shouldProcessgate. The node-pool syncer still returns beforeValidate()whenretryCooldownChecker.CanSyncis false (nodepool_validation_controller.go:115-123), so an ETag-change event duringEarliestRetryAfteris still dropped—the main behavior this PR claims to fix. Please remove that worker-level short-circuit, expose the checker through the watching controller'sCooldownChecker()so unchanged resyncs remain suppressed, and update the existing cooldown test to assert the new behavior.
name: "already-succeeded validation -- re-runs and overwrites with Failed",
docs/cosmos-data-flow.md:1098
- This updated gate description is inconsistent with the submitted implementation: both validation
SyncOncemethods still applyretryCooldownChecker.CanSync, so the controller has a retry cooldown in addition to the deletion check, and the watching controllers do not yet own that cooldown. Complete the described move toGenericWatchingController(or document the actual gate) before updating this section.
**Gate:**
- SyncOnce checks `DeletionTimestamp == nil` on the resource
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| if !c.shouldProcess(cachedServiceProviderCluster) { | ||
| return nil // no work to do | ||
| } | ||
| existingServiceProviderCluster := cachedServiceProviderCluster.DeepCopy() |
| if !c.shouldProcess(cachedServiceProviderNodePool) { | ||
| return nil // no work to do | ||
| } | ||
| existingServiceProviderNodePool := cachedServiceProviderNodePool.DeepCopy() |
Collaborator
|
/retest |
Collaborator
Author
|
/test e2e-parallel |
Collaborator
|
/retest |
Collaborator
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miguelsorianod, patilsuraj767 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
|
Suraj Patil (@patilsuraj767): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Cluster and node pool validation controllers no longer stop after a Passed condition, and they no longer drop work in SyncOnce because of EarliestRetryAfter.
Tradeoff: any Cluster/SPC (or NodePool) ETag change retriggers Validate(), including backend-only writes. Typical extra cost is one more Validate()