OCPBUGS-98213: fix router component ordering to prevent missing HAProxy backends#8971
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98213, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98213, which is valid. 3 validation(s) were run on this bug
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant Reconciler as HostedControlPlaneReconciler
participant Router as routerv2 component
participant API as Kubernetes Route API
participant HCP as HostedControlPlane
Reconciler->>Router: register component with dependencies
Router->>HCP: inspect HCP router mode and platform
Router->>API: list Routes in HCP namespace
API-->>Router: return Route objects
Router->>Router: check expected names and spec.host readiness
Router-->>Reconciler: permit or block reconciliation
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
279-285: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider
WithDependenciesinstead of positional ordering.Comment-guarded slice ordering is fragile: any future insertion/reordering in
registerComponents, or a move away from strictly sequential reconciliation ofr.components, silently reintroduces this bug. This framework already has an explicit ordering primitive for exactly this case —WithDependencies(...), which blocks a component's reconciliation until its declared dependencies are ready — used elsewhere (e.g.clusterpolicy,dnsoperator,konnectivity_agentall callWithDependencies(oapiv2.ComponentName)/kasv2.ComponentName). Declaringrouterv2.NewComponent().WithDependencies(ignitionserverv2.ComponentName, metricsproxyv2.ComponentName)(if the framework's builder API supports appending dependencies) would make the ordering contract explicit and self-enforcing rather than implicit in registration order.🤖 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 279 - 285, The router registration in registerComponents is relying on fragile slice order to wait for ignition-server and metrics-proxy, which can be broken by future reordering. Update routerv2.NewComponent to declare an explicit dependency on the components that create Route objects using the framework’s WithDependencies API, so router reconciliation is blocked until those dependencies are ready. Use the existing component names in this controller (for example ignitionserverv2.ComponentName and metricsproxyv2.ComponentName) to make the ordering contract explicit and self-enforcing.
🤖 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.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 279-285: The router registration in registerComponents is relying
on fragile slice order to wait for ignition-server and metrics-proxy, which can
be broken by future reordering. Update routerv2.NewComponent to declare an
explicit dependency on the components that create Route objects using the
framework’s WithDependencies API, so router reconciliation is blocked until
those dependencies are ready. Use the existing component names in this
controller (for example ignitionserverv2.ComponentName and
metricsproxyv2.ComponentName) to make the ordering contract explicit and
self-enforcing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8b89eabd-9f22-4e18-a641-6d02aef5df25
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8971 +/- ##
==========================================
+ Coverage 43.53% 43.74% +0.20%
==========================================
Files 771 772 +1
Lines 95798 95991 +193
==========================================
+ Hits 41707 41987 +280
+ Misses 51192 51090 -102
- Partials 2899 2914 +15
... and 16 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review
How Reconciliation Actually Works
reconcileInfrastructure()runs first (line 1084) — creates KAS routes, konnectivity routes, and OAuth routes viainfra/infra.goAdmitHCPManagedRoutes()runs next (line 1201) — sets host/status on HCP-managed routesfor _, c := range r.components(line 1263) — iterates sequentially, calling each component'sReconcile(), which callsupdate(), which applies manifests via server-side apply
Does component order matter?
Partially. The router's adaptConfig (v2/router/config.go:43) does:
cpContext.Client.List(cpContext, routeList, client.InNamespace(cpContext.HCP.Namespace))cpContext.Client is r.Client (line 1247) — the controller-runtime cached client backed by informer caches. When ignition-server's Reconcile() creates a Route via server-side apply, the Route hits etcd, but the informer cache update from the watch event is async. So ordering the router after route-creating components gives the informer more time to process the watch event, making it more likely the routes are visible — but it's not a hard guarantee.
What routes does the router care about?
The router's adaptConfig handles: KAS (3 routes), ignition, konnectivity, OAuth (3 routes), metrics_forwarder, and metrics_proxy.
- KAS, konnectivity, OAuth routes are created in
reconcileInfrastructure(), which runs before the component loop. These are not affected by component ordering at all. - Only ignition-server and metrics-proxy create routes via the component framework (
v2/assets/ignition-server/route.yamlandv2/assets/metrics-proxy/route.yaml). These are the ones affected by this PR.
The route-creating component list is complete
The PR correctly identifies ignition-server and metrics-proxy as the only two route-creating components in the framework. Confirmed by:
- Only two
route.yamlassets exist underv2/assets/ - No other components reference
routev1.Routeoutside of router itself
Test quality
The test is well-structured:
- Uses exported
ComponentNameconstants - Properly sets up the reconciler with required mocks
- Verifies position ordering in the actual component slice
- Only tests AWS platform, but the route-creating components aren't platform-conditional so this is fine
Issues
-
The PR description overstates the fix. It says this "eliminates the race window" — it doesn't. It reduces the window by giving the informer cache more time to sync, but the cache update is still async. A fully robust fix would use
WithDependencies()on the router component (the framework supports this — seemetricsproxyv2which already usesWithDependencies(endpointresolverv2.ComponentName)), or use an uncached read inadaptConfig. -
The 6-line inline comment is verbose. 2 lines would suffice:
// Router must come after route-creating components (ignition-server, metrics-proxy) // so their routes exist in the cache when adaptConfig generates the HAProxy config.
-
Not a blocker, but worth noting: the router already handles missing routes gracefully — if a route doesn't exist, it just doesn't get a backend entry. The next reconcile pass will pick it up. So this is a first-reconcile latency optimization, not a hard correctness bug.
Verdict
Conditional ACK — the ordering change is a reasonable improvement and the test is good. But the PR description should be corrected to not claim it "eliminates" the race, and the comment should be shortened.
|
/test address-review-comments |
|
Review agent triggered. View job |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-OCPBUGS-98213
branch
from
b092cf4 to
6f72168
Compare
|
Done. Addressed both items:
Good point on #3 — the router does handle missing routes gracefully via subsequent reconcile passes, so this is indeed a first-reconcile latency optimization rather than a hard correctness fix. AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98213, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
Was https://redhat.atlassian.net/browse/OCPBUGS-98213?focusedCommentId=17546910 taken into account when generating this PR? |
|
/test address-review-comments |
|
Review agent triggered. View job |
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98213, which is valid. 3 validation(s) were run on this bug
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. |
|
Addressed review feedback:
The positional ordering test is preserved as a secondary safety net — AI-assisted response via Claude Code |
|
/restructure-commits |
|
🤖 Restructuring commits: workflow run |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-OCPBUGS-98213
branch
from
20f0e73 to
eb42c61
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hypershift-jira-solve-ci[bot] 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
added
the
approved
Test Resultse2e-aws
|
9 similar comments
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
|
Test Resultse2e-aws
e2e-aks
|
1 similar comment
Test Resultse2e-aws
e2e-aks
|
|
/retest |
| "pdb.yaml", | ||
| component.AdaptPodDisruptionBudget(), | ||
| ). | ||
| WithDependencies(ignitionserverv2.ComponentName, metricsproxyv2.ComponentName). |
There was a problem hiding this comment.
Pleas let's Drop the dependency on metrics-proxy which is not guaranteed to exist, the predicate should take care of it already
There was a problem hiding this comment.
Done. Removed metricsproxyv2.ComponentName from WithDependencies — the router now only depends on ignition-server. The predicate's ensureHCPRouterRoutesExist already gates on metrics-proxy route readiness when applicable, so the hard dependency was redundant and caused deadlocks on platforms where metrics-proxy is never created.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Addressed by removing the hard dependency — the CI failure analysis confirmed the deadlock. The router predicate is sufficient to handle this.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
| message: 'Waiting for deployment router rollout to finish: 0 out of 3 new replicas | ||
| have been updated' | ||
| reason: WaitingForRolloutComplete | ||
| message: 'Waiting for Dependencies: ignition-server, metrics-proxy' |
There was a problem hiding this comment.
fix the unit tests this needs to be regenerated #8971 (comment)
There was a problem hiding this comment.
Done. Updated all 5 router fixture YAML files to reflect the actual declared dependency (ignition-server only, no metrics-proxy).
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
…dencies The metrics-proxy component is conditional (only created when MetricsForwarding.Mode=Forward), so declaring it as a hard dependency causes a deadlock on platforms where it is never instantiated (e.g. GKE). The router predicate already ensures metrics-proxy routes exist before reconciling, making the explicit dependency unnecessary. Update all 5 router fixture YAML files to match the actual declared dependency (ignition-server only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-OCPBUGS-98213
branch
from
710c679 to
c995ad0
Compare
|
/lgtm |
|
Scheduling tests matching the |
enxebre
added
tide/merge-method-squash
|
/test e2e-kubevirt-aws-ovn-reduced |
|
@hypershift-jira-solve-ci[bot]: 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. |
|
Now I have all the evidence I need. Let me compile the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe job failed in the pre phase because the Root CauseThe root cause is a CI infrastructure memory limit misconfiguration in the What happened step-by-step:
Why this is NOT related to PR #8971:
Recommendations
Evidence
|
5 participants
OCPBUGS-98213: fix router component ordering to prevent missing HAProxy backends
What this PR does / why we need it:
The router's
adaptConfigfunction lists existing Route objects to generate the HAProxy ConfigMap. When the router was registered early in the component list (position #8), components that create Routes — ignition-server (#35) and metrics-proxy (#38) — had not yet run during the first reconcile pass. This caused the initial HAProxy config to miss ignition and metrics-proxy backends, routing ignition requests to the KAS default backend and failing NodePool ignition.This PR:
metricsproxyv2(the last route-creating component) so that all Routes exist when the router config is generated.ignition-serverandmetrics-proxyas explicitWithDependencieson the router component, blocking its reconciliation until both dependencies are Available and RolloutComplete. This provides a stronger guarantee than positional ordering alone, though the informer cache update from watch events is still async.TestRouterComponentComesAfterRouteCreatingComponentsto verify the router is always registered after ignition-server and metrics-proxy, preventing future regressions from component reordering.Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98213
Special notes for your reviewer:
The fix uses two complementary mechanisms: positional ordering in
registerComponents(belt) andWithDependenciesvia the component framework (suspenders).WithDependenciesensures the router's reconciliation is skipped until ignition-server and metrics-proxy report Available + RolloutComplete, giving the informer cache time to reflect their routes.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit