OCPBUGS-99883: fix(azure): cache VNet location validation and requeue on ARM 429 throttling - #9123
OCPBUGS-99883: fix(azure): cache VNet location validation and requeue on ARM 429 throttling#9123miquelsi wants to merge 1 commit into
Conversation
…on ARM 429 throttling Skip Azure ARM VNet/NSG/RG location checks once ValidHostedControlPlaneConfiguration is True for the current generation, and requeue with Retry-After on Azure API errors instead of hard-gating reconciliation during throttling. Signed-off-by: Miquel Simon <msimonma@redhat.com> Assisted-by: Cursor Grok 4.5 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miquelsi 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @miquelsi. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
area/control-plane-operator
📝 WalkthroughWalkthroughHosted control plane reconciliation now retains configuration validation errors for control-flow decisions. Transient Azure API errors can trigger timed requeues instead of permanent blocking, with retry intervals derived from Sequence Diagram(s)sequenceDiagram
participant HostedControlPlaneReconciler
participant AzureARM
participant ControllerQueue
HostedControlPlaneReconciler->>AzureARM: validate configuration and locations
AzureARM-->>HostedControlPlaneReconciler: validation result or ResponseError
HostedControlPlaneReconciler->>ControllerQueue: requeue after Retry-After or default interval
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
623-627: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the reconciler error variable.
Line 623 shadows the
errdeclared at Line 590. Assign the validation result toconfigValidationErrbefore branching instead.Proposed fix
- if err := r.validateConfigAndClusterCapabilities(ctx, hostedControlPlane); err != nil { - configValidationErr = err + configValidationErr = r.validateConfigAndClusterCapabilities(ctx, hostedControlPlane) + if configValidationErr != nil { condition.Status = metav1.ConditionFalse - condition.Message = err.Error() + condition.Message = configValidationErr.Error()As per coding guidelines, “Avoid variable shadowing.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 623 - 627, Update the validation handling in the reconciler to assign the result of validateConfigAndClusterCapabilities to configValidationErr before checking whether it is non-nil, rather than declaring a shadowing err inside the if initializer. Preserve the existing condition status, message, and reason updates when configValidationErr contains an error.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 3252-3258: Update requeueOnAzureAPIError to use both results from
hyperazureutil.ClassifyAzureError, returning the classifier’s non-requeueable
outcome instead of always requeueing every azcore.ResponseError. Preserve the
existing behavior for retryable responses, and add coverage for a permanent
Azure response such as http.StatusBadRequest.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 623-627: Update the validation handling in the reconciler to
assign the result of validateConfigAndClusterCapabilities to configValidationErr
before checking whether it is non-nil, rather than declaring a shadowing err
inside the if initializer. Preserve the existing condition status, message, and
reason updates when configValidationErr contains an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 22af151c-8f78-4845-8550-85c7e439ebae
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
| func requeueOnAzureAPIError(err error) (ctrl.Result, bool) { | ||
| var respErr *azcore.ResponseError | ||
| if err == nil || !errors.As(err, &respErr) { | ||
| return ctrl.Result{}, false | ||
| } | ||
| requeueAfter, _ := hyperazureutil.ClassifyAzureError(err) | ||
| return ctrl.Result{RequeueAfter: requeueAfter}, true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A35 -B5 'func\s+ClassifyAzureError\b' .
rg -n -A20 -B5 'TestRequeueOnAzureAPIError|StatusBadRequest' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.goRepository: openshift/hypershift
Length of output: 3161
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Locate relevant symbols and file sizes =="
wc -l control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go \
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go \
support/azureutil/errors.go
echo
echo "== Requeue helper and callers =="
rg -n -A25 -B8 'func requeueOnAzureAPIError|requeueOnAzureAPIError\(' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
echo
echo "== Azure test cases around requeue helpers =="
sed -n '4708,4815p' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
echo
echo "== Azure error classifier =="
sed -n '1,140p' support/azureutil/errors.go
echo
echo "== StatusBadRequest references in controller tests =="
rg -n -C4 '400|Bad Request|BadRequest|StatusBadRequest|Validation|invalid' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.goRepository: openshift/hypershift
Length of output: 10081
Requeue only retryable Azure API responses.
requeueOnAzureAPIError currently requeues every *azcore.ResponseError, including permanent validation errors and any HTTP 400 response from Azure. Return the classifier’s non-requeueable outcome in requeueOnAzureAPIError and add a coverage case for a permanent Azure status such as http.StatusBadRequest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`
around lines 3252 - 3258, Update requeueOnAzureAPIError to use both results from
hyperazureutil.ClassifyAzureError, returning the classifier’s non-requeueable
outcome instead of always requeueing every azcore.ResponseError. Preserve the
existing behavior for retryable responses, and add coverage for a permanent
Azure response such as http.StatusBadRequest.
|
/retitle OCPBUGS-99883: fix(azure): cache VNet location validation and requeue on ARM 429 throttling |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@miquelsi: This pull request references Jira Issue OCPBUGS-99883, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review from hypershift-staff-engineer agent — two-axis review (standards + spec compliance) against OCPBUGS-99883.
| func requeueOnAzureAPIError(err error) (ctrl.Result, bool) { | ||
| var respErr *azcore.ResponseError | ||
| if err == nil || !errors.As(err, &respErr) { | ||
| return ctrl.Result{}, false | ||
| } | ||
| requeueAfter, _ := hyperazureutil.ClassifyAzureError(err) | ||
| return ctrl.Result{RequeueAfter: requeueAfter}, true | ||
| } |
There was a problem hiding this comment.
The spec asks for 429 handling specifically, but this requeues on any azcore.ResponseError — including 403 (permissions misconfigured). A 403 on VNet validation is a genuine configuration error that should hard-gate reconciliation so the user sees it immediately, not silently retry for 10 minutes.
Consider filtering to only transient codes:
if respErr.StatusCode != http.StatusTooManyRequests && (respErr.StatusCode < 500 || respErr.StatusCode >= 600) {
return ctrl.Result{}, false
}| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| var configValidationErr error |
There was a problem hiding this comment.
This variable is declared here but consumed ~100 lines later at the gate check (line 714). The reconciler uses bare {} blocks to isolate scope, and this threads state across those boundaries. Could this be scoped tighter — e.g., by checking requeueOnAzureAPIError inline where the condition is set to False, or by attaching the error to the condition's Message and re-parsing it at the gate?
| // shouldSkipAzureResourceGroupLocationValidation reports whether Azure ARM calls to | ||
| // verify VNet/NSG/RG locations can be skipped because validation already succeeded for | ||
| // the current generation. Locations are immutable for a given set of Azure resource IDs, | ||
| // so re-checking on every reconcile is unnecessary and contributes to ARM throttling. | ||
| func shouldSkipAzureResourceGroupLocationValidation(hcp *hyperv1.HostedControlPlane) bool { | ||
| validConfig := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidHostedControlPlaneConfiguration)) | ||
| return validConfig != nil && | ||
| validConfig.Status == metav1.ConditionTrue && | ||
| validConfig.ObservedGeneration == hcp.Generation | ||
| } | ||
|
|
||
| // requeueOnAzureAPIError returns a RequeueAfter result when err wraps an Azure API | ||
| // ResponseError (e.g. 429), using ClassifyAzureError for differentiated backoff that | ||
| // respects the Retry-After header. Permanent validation failures that are not Azure | ||
| // API errors return false so reconciliation remains hard-gated. | ||
| func requeueOnAzureAPIError(err error) (ctrl.Result, bool) { | ||
| var respErr *azcore.ResponseError | ||
| if err == nil || !errors.As(err, &respErr) { | ||
| return ctrl.Result{}, false | ||
| } | ||
| requeueAfter, _ := hyperazureutil.ClassifyAzureError(err) | ||
| return ctrl.Result{RequeueAfter: requeueAfter}, true | ||
| } |
There was a problem hiding this comment.
Nit: per DEVELOPMENT.md ("Platform-specific logic should be isolated"), these Azure-specific helpers would fit better in support/azureutil/ alongside ClassifyAzureError. shouldSkipAzureResourceGroupLocationValidation only reads conditions and generation — it's Azure-specific by name/intent, not by implementation, so moving it keeps the ~3250-line reconciler platform-neutral.
| func shouldSkipAzureResourceGroupLocationValidation(hcp *hyperv1.HostedControlPlane) bool { | ||
| validConfig := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.ValidHostedControlPlaneConfiguration)) | ||
| return validConfig != nil && | ||
| validConfig.Status == metav1.ConditionTrue && | ||
| validConfig.ObservedGeneration == hcp.Generation | ||
| } |
There was a problem hiding this comment.
Minor: the spec suggests caching based on specific field changes (VnetID, SubnetID, NetworkSecurityGroupID), but the implementation uses ObservedGeneration == hcp.Generation. This means any unrelated spec change (e.g., authentication config) invalidates the cache and triggers another ARM call. The generation check is safer and simpler — just flagging the trade-off since the spec called out field-level granularity for the 400-cluster scenario.
3 participants
What this PR does / why we need it:
Skip Azure ARM VNet/NSG/RG location checks once ValidHostedControlPlaneConfiguration is True for the current generation, and requeue with Retry-After on Azure API errors instead of hard-gating reconciliation during throttling.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-99883
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit