refactor: replace error-based validation with structured ValidationResult - #6057
Open
David Eads (deads2k) wants to merge 2 commits into
Open
David Eads (deads2k) wants to merge 2 commits into
David Eads (deads2k) wants to merge 2 commits into
Conversation
…sult Validation controllers now return *ValidationResult (Passed/Failed/Unknown) instead of error, giving each outcome explicit retry timing, user-facing messages, and service-provider diagnostics. Key changes: - Validate() returns *ValidationResult with Outcome, Failed/Unknown details, and EarliestRetryAfter - SettableCooldownChecker enforces per-key retry timing inside SyncOnce - AfterEnqueuer exposes workqueue AddAfter for explicit requeue scheduling - Unknown results preserve the previous stored condition for up to 10 consecutive attempts before overwriting - DeepEqual guard skips Cosmos Replace when the condition hasn't changed - DefaultResult and BuildCondition extracted as shared helpers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
requested review from
Jan-Hendrik Boll (janboll) and
Matthew Barnes (mbarnes)
July 13, 2026 19:50
| _, err := uaisClient.Get(ctx, resourceID.ResourceGroupName, resourceID.Name, nil) | ||
| if azureclient.IsResourceNotFoundErr(err) { | ||
| notFoundMIsStrs = append(notFoundMIsStrs, resourceID.String()) | ||
| continue |
Collaborator
Author
There was a problem hiding this comment.
existing bug?
| Reason: "ClientError", | ||
| ServiceProviderMessage: fmt.Sprintf("failed to get user assigned identities client: %s", err), | ||
| UserMessage: "Failed to check managed identity existence.", | ||
| ReportingPolicy: ReportingPolicyTypeError, |
Collaborator
Author
There was a problem hiding this comment.
I have no preference, you?
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors validation controllers to return a structured *ValidationResult (Passed/Failed/Unknown) instead of error, enabling explicit retry scheduling (EarliestRetryAfter), more precise condition reporting, and reduced write churn via DeepEqual guards.
Changes:
- Introduces
ValidationResult/OutcomeTypeplus shared helpers (DefaultResult,BuildCondition) and updates validation interfaces to return structured outcomes. - Adds explicit requeue scheduling via
AfterEnqueuer(workqueue.AddAfter) and per-key retry cooldown enforcement withSettableCooldownChecker. - Updates cluster/nodepool validation controllers and tests to use the new result model, including “preserve previous condition on Unknown for N attempts” behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controllerutils/cooldown.go | Adds a settable per-key cooldown gate and “time until ready” helper used for retry enforcement. |
| backend/pkg/controllers/validationcontrollers/validations/nodepool_validation.go | Updates nodepool validation interface to return *ValidationResult. |
| backend/pkg/controllers/validationcontrollers/validations/cluster_validation.go | Defines ValidationResult model and shared helpers to default/convert results into metav1.Condition. |
| backend/pkg/controllers/validationcontrollers/validations/azure_rp_registration_validation.go | Migrates RP registration validation to structured validation outcomes and retry timing. |
| backend/pkg/controllers/validationcontrollers/validations/azure_cluster_resource_group_existence_validation.go | Migrates RG existence validation to structured outcomes and retry timing. |
| backend/pkg/controllers/validationcontrollers/validations/azure_cluster_mis_existence_validation.go | Migrates managed identity existence validation to structured outcomes and retry timing. |
| backend/pkg/controllers/validationcontrollers/validations/always_success_validation.go | Updates always-success validation to return a Passed result. |
| backend/pkg/controllers/validationcontrollers/nodepool_validation_controller.go | Implements result-based condition writing, explicit retry scheduling, and unknown-preservation logic for nodepools. |
| backend/pkg/controllers/validationcontrollers/nodepool_validation_controller_test.go | Updates/extends tests for structured outcomes and retry scheduling timing. |
| backend/pkg/controllers/validationcontrollers/cluster_validation_controller.go | Implements result-based condition writing, explicit retry scheduling, and unknown-preservation logic for clusters. |
| backend/pkg/controllers/controllerutils/generic_watching_controller.go | Adds AfterEnqueuer API backed by workqueue.AddAfter. |
| .gitignore | Ignores /.claude/ directory. |
Comment on lines
+80
to
+84
| if enqueuer, ok := controller.(controllerutils.AfterEnqueuer); ok { | ||
| syncer.enqueueAfter = enqueuer | ||
| } else { | ||
| panic("ClusterValidationController must implement AfterEnqueuer") | ||
| } |
Comment on lines
27
to
+29
| 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(ctx context.Context, cluster *api.HCPOpenShiftCluster, nodePoolSubscription *arm.Subscription, nodePool *api.HCPOpenShiftClusterNodePool) *ValidationResult |
Comment on lines
+96
to
+113
| func DefaultResult(result *ValidationResult) *ValidationResult { | ||
| if result == nil { | ||
| return &ValidationResult{ | ||
| Outcome: OutcomeTypeUnknown, | ||
| Unknown: &UnknownResult{ | ||
| Reason: "NilResult", | ||
| ServiceProviderMessage: "Validation returned nil result.", | ||
| UserMessage: "Validation status is unknown.", | ||
| ReportingPolicy: ReportingPolicyTypeError, | ||
| }, | ||
| EarliestRetryAfter: ptr.To(60 * time.Second), | ||
| } | ||
| } | ||
| if result.EarliestRetryAfter == nil { | ||
| result.EarliestRetryAfter = ptr.To(60 * time.Second) | ||
| } | ||
| return result | ||
| } |
Comment on lines
+89
to
+90
| // ReportingPolicyTypeError will return an error for rapid retry, but the EarliestRetryAfter will prevent rapid retry and queue (without error) for a time after that. | ||
| ReportingPolicyTypeError ReportingPolicyType = "ReportError" |
Comment on lines
+87
to
+93
| func (c *genericWatchingController[T]) EnqueueAfter(keyObj any, duration time.Duration) { | ||
| key, ok := keyObj.(T) | ||
| if !ok { | ||
| return | ||
| } | ||
| c.queue.AddAfter(key, duration) | ||
| } |
Comment on lines
+118
to
+139
| 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 | ||
| } |
Contributor
|
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. |
14 tasks
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.
Validation controllers now return *ValidationResult (Passed/Failed/Unknown)
instead of error, giving each outcome explicit retry timing, user-facing
messages, and service-provider diagnostics.
Key changes:
and EarliestRetryAfter
consecutive attempts before overwriting