Skip to content

OCPBUGS-99883: fix(azure): cache VNet location validation and requeue on ARM 429 throttling #9123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miquelsi wants to merge 1 commit into
base: main
Choose a base branch
from
+184 −0
Open

OCPBUGS-99883: fix(azure): cache VNet location validation and requeue on ARM 429 throttling #9123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,12 +614,14 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R
return ctrl.Result{}, nil
}

var configValidationErr error

@bryan-cox bryan-cox Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
condition := metav1.Condition{
Type: string(hyperv1.ValidHostedControlPlaneConfiguration),
ObservedGeneration: hostedControlPlane.Generation,
}
if err := r.validateConfigAndClusterCapabilities(ctx, hostedControlPlane); err != nil {
configValidationErr = err
condition.Status = metav1.ConditionFalse
condition.Message = err.Error()
condition.Reason = hyperv1.InsufficientClusterCapabilitiesReason
Expand Down Expand Up @@ -709,6 +711,12 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R
{
validConfig := meta.FindStatusCondition(hostedControlPlane.Status.Conditions, string(hyperv1.ValidHostedControlPlaneConfiguration))
if validConfig != nil && validConfig.Status == metav1.ConditionFalse {
// Transient Azure API failures (e.g. 429 rate limiting) should requeue
// respecting Retry-After rather than hard-blocking until an unrelated watch event.
if result, shouldRequeue := requeueOnAzureAPIError(configValidationErr); shouldRequeue {
r.Log.Info("Configuration validation hit a transient Azure API error, requeueing", "requeueAfter", result.RequeueAfter)
return result, nil
}
r.Log.Info("Configuration is invalid, reconciliation is blocked")
return reconcile.Result{}, nil
}
Expand Down Expand Up @@ -3226,8 +3234,36 @@ func (r *HostedControlPlaneReconciler) reconcileSREMetricsConfig(ctx context.Con
return nil
}

// 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
}
Comment on lines +3241 to +3246

@bryan-cox bryan-cox Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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
Comment on lines +3252 to +3258

@coderabbitai coderabbitai Bot Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.go

Repository: 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.go

Repository: 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.

}
Comment on lines +3252 to +3259

@bryan-cox bryan-cox Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Comment on lines +3237 to +3259

@bryan-cox bryan-cox Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// verifyResourceGroupLocationsMatch verifies the locations match for the VNET, network security group, and managed resource groups
func (r *HostedControlPlaneReconciler) verifyResourceGroupLocationsMatch(ctx context.Context, hcp *hyperv1.HostedControlPlane) error {
if shouldSkipAzureResourceGroupLocationValidation(hcp) {
return nil
}

var (
creds azcore.TokenCredential
found, ok bool
Expand Down
148 changes: 148 additions & 0 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ import (
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
"github.com/aws/smithy-go"

"github.com/Azure/azure-sdk-for-go/sdk/azcore"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -4620,3 +4622,149 @@ var _ util.ImageMetadataProvider = &fakeVersionImageMetadataProvider{}

// Compile-time assertion for clock interface used by tests.
var _ clock.Clock = &testingclock.FakeClock{}

func TestShouldSkipAzureResourceGroupLocationValidation(t *testing.T) {
t.Parallel()

tests := []struct {
name string
hcp *hyperv1.HostedControlPlane
expected bool
}{
{
name: "When condition is absent, it should not skip validation",
hcp: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Generation: 1},
},
expected: false,
},
{
name: "When condition is True for the current generation, it should skip validation",
hcp: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Status: hyperv1.HostedControlPlaneStatus{
Conditions: []metav1.Condition{{
Type: string(hyperv1.ValidHostedControlPlaneConfiguration),
Status: metav1.ConditionTrue,
ObservedGeneration: 2,
}},
},
},
expected: true,
},
{
name: "When condition is True for a stale generation, it should not skip validation",
hcp: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Generation: 3},
Status: hyperv1.HostedControlPlaneStatus{
Conditions: []metav1.Condition{{
Type: string(hyperv1.ValidHostedControlPlaneConfiguration),
Status: metav1.ConditionTrue,
ObservedGeneration: 2,
}},
},
},
expected: false,
},
{
name: "When condition is False, it should not skip validation",
hcp: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Generation: 1},
Status: hyperv1.HostedControlPlaneStatus{
Conditions: []metav1.Condition{{
Type: string(hyperv1.ValidHostedControlPlaneConfiguration),
Status: metav1.ConditionFalse,
ObservedGeneration: 1,
}},
},
},
expected: false,
},
{
name: "When condition is Unknown, it should not skip validation",
hcp: &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{Generation: 1},
Status: hyperv1.HostedControlPlaneStatus{
Conditions: []metav1.Condition{{
Type: string(hyperv1.ValidHostedControlPlaneConfiguration),
Status: metav1.ConditionUnknown,
ObservedGeneration: 1,
}},
},
},
expected: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
g.Expect(shouldSkipAzureResourceGroupLocationValidation(tt.hcp)).To(Equal(tt.expected))
})
}
}

func TestRequeueOnAzureAPIError(t *testing.T) {
t.Parallel()

tests := []struct {
name string
err error
expectRequeue bool
expectedRequeueFor time.Duration
}{
{
name: "When error is nil, it should not requeue",
err: nil,
expectRequeue: false,
},
{
name: "When error is a permanent validation failure, it should not requeue",
err: fmt.Errorf("the locations of the resource groups do not match"),
expectRequeue: false,
},
{
name: "When Azure returns 429 with Retry-After, it should requeue using that duration",
err: fmt.Errorf("failed to get vnet info: %w", &azcore.ResponseError{
StatusCode: http.StatusTooManyRequests,
RawResponse: &http.Response{
Header: http.Header{"Retry-After": {"120"}},
},
}),
expectRequeue: true,
expectedRequeueFor: 120 * time.Second,
},
{
name: "When Azure returns 429 without Retry-After, it should requeue after the default 5 minutes",
err: fmt.Errorf("failed to get vnet info: %w", &azcore.ResponseError{
StatusCode: http.StatusTooManyRequests,
RawResponse: &http.Response{Header: http.Header{}},
}),
expectRequeue: true,
expectedRequeueFor: 5 * time.Minute,
},
{
name: "When Azure returns 403, it should requeue after 10 minutes",
err: fmt.Errorf("failed to get vnet info: %w", &azcore.ResponseError{
StatusCode: http.StatusForbidden,
}),
expectRequeue: true,
expectedRequeueFor: 10 * time.Minute,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)
result, shouldRequeue := requeueOnAzureAPIError(tt.err)
g.Expect(shouldRequeue).To(Equal(tt.expectRequeue))
if tt.expectRequeue {
g.Expect(result.RequeueAfter).To(Equal(tt.expectedRequeueFor))
} else {
g.Expect(result).To(Equal(ctrl.Result{}))
}
})
}
}