[Do Not Merge][E2E test] Enables constant validation - #6439
Open
Suraj Patil (patilsuraj767) wants to merge 2 commits into
Open
[Do Not Merge][E2E test] Enables constant validation#6439Suraj Patil (patilsuraj767) wants to merge 2 commits into
Suraj Patil (patilsuraj767) wants to merge 2 commits into
Conversation
…odePoolValidation instead of error Signed-off-by: Suraj Patil <patilsuraj767@gmail.com>
Signed-off-by: Suraj Patil <patilsuraj767@gmail.com>
openshift-ci
Bot
requested review from
Jan-Hendrik Boll (janboll) and
Matthew Barnes (mbarnes)
August 5, 2026 17:21
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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
|
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. |
|
PR needs rebase. 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. |
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.
No description provided.