CNTRLPLANE-3145: refactor(hostedcluster): segregate reconcile loop into error-collecting blocks - #7908
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThis pull request introduces a phased, modular refactoring of the HostedCluster reconciliation loop. It adds a design document analyzing reconciliation segregation, restructures the controller into nine sequential phases with independent error aggregation, and introduces multiple helper functions to isolate functionality. A signature change removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/test e2e-aws |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/hostedcluster-reconcile-segregation-analysis.md`:
- Around line 112-190: The markdown fenced block containing the ASCII diagram
(the block that begins with
"+-----------------------------------------------------+" and includes "CRITICAL
PREREQUISITES (must succeed first)") needs a language label to satisfy
markdownlint: change the opening fence from ``` to ```text so the diagram is
fenced as a text block; update the single fenced block in the file (the ASCII
diagram between the backticks) accordingly.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 1726-1728: The code currently checks and then removes the
HostedClusterRestoredFromBackupAnnotation from hcluster before writing the
durable status (ReconciliationSucceeded/HostedClusterRestoredFromBackup
condition), which can lose the trigger if the status update fails; change the
flow so you do not consume/remove HostedClusterRestoredFromBackupAnnotation
until after the status write is confirmed: first set the
HostedClusterRestoredFromBackup condition on the HostedCluster status and
perform the status update (updateStatus on hcluster), retrying on conflict as
needed, and only after the status update succeeds remove the
HostedClusterRestoredFromBackupAnnotation (or perform the annotation removal in
a separate patch/update with proper conflict handling) so the reconcile will
retry if the status write failed.
- Around line 1456-1483: If reconcileCoreHCPChain failed and hcp is nil, phase‑8
helpers will dereference hcp and panic; guard the entire phase‑8 block by
checking if hcp == nil and, if so, append a recoverable error to componentErrs
(e.g. fmt.Errorf("skipping phase 8: HostedControlPlane is nil due to earlier
error")) and skip calling reconcileKubeconfigAndPasswordSync,
reconcileOperatorDeployments, reconcileRBACAndPolicies,
reconcilePlatformSpecific, and reconcileAuxiliary; otherwise run the existing
calls as before.
- Around line 2161-2174: The status fields holding secret references
(hcluster.Status.CustomKubeconfig and hcluster.Status.KubeadminPassword) are
only being cleared in memory; after deleting the Secrets you must also persist
those changes to the API by clearing the fields on the HostedCluster status and
calling the Status().Update (or Client.Status().Update) to save them. Modify the
branch that deletes the custom kubeconfig (and the other branch mentioned around
the KubeadminPassword) to set hcluster.Status.CustomKubeconfig = nil and/or
hcluster.Status.KubeadminPassword = nil as appropriate and then call
r.Status().Update(ctx, hcluster) (handling and returning any error) so the API
no longer holds dangling secret refs; use the existing DeleteIfNeeded flow and
ensure both branches behave the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33f6ef88-9d24-48cc-8609-666d2cca5d82
📒 Files selected for processing (3)
docs/design/hostedcluster-reconcile-segregation-analysis.mdhypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
openshift-ci
Bot
added
the
area/documentation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee 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
area/hypershift-operator
|
was there jira bug we can ref reporting the scenario where this was being problematic for managed? |
openshift-merge-robot
added
the
needs-rebase
|
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. |
04ede4b to
168e67a
Compare
openshift-ci
Bot
removed
the
needs-rebase
168e67a to
15e6a0c
Compare
15e6a0c to
e7bc83c
Compare
e7bc83c to
5bc8ca5
Compare
5bc8ca5 to
922ef75
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7908 +/- ##
==========================================
+ Coverage 42.09% 42.41% +0.31%
==========================================
Files 766 767 +1
Lines 95047 95217 +170
==========================================
+ Hits 40012 40383 +371
+ Misses 52221 52032 -189
+ Partials 2814 2802 -12
... and 1 file 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:
|
ba3485a to
717ef22
Compare
openshift-ci
Bot
added
the
area/ai
717ef22 to
7e6260d
Compare
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Reviewed the behavioral changes between the legacy and new reconcile paths. Two items worth addressing before merge.
7e6260d to
484a1c1
Compare
openshift-ci
Bot
added
the
needs-rebase
484a1c1 to
6d8244d
Compare
openshift-ci
Bot
removed
the
needs-rebase
6d8244d to
d341729
Compare
…ng blocks The reconcile() method executes ~50 sequential operations where every error causes an early return, short-circuiting all remaining work. An unrelated failure (e.g., missing SSH key secret) prevents critical operations like deploying the CPO or reconciling the HCP object. This refactoring: - Extracts 12 inline blocks into named methods - Groups operations into phased error-collecting blocks - Aggregates all errors with utilerrors.NewAggregate at the end - Introduce reconcileReport struct that classifies reconcile operations as critical (blocks Phase 8) or non-critical (error-collecting, never blocks). Replace the sequential error chain where any failure short-circuits the entire loop with structured error collection and blocking rules. After this change, failures in one phase no longer block unrelated phases. For example, a missing SSH key no longer prevents CPO deployment or HCP object creation. Includes the analysis document and integration tests that verify non-blocking behavior across phases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rror-collection framework Replace the ad-hoc early-return pull-secret recovery path (PR openshift#8352) with the error-collection framework. Instead of inline HCP reconciliation when GetPullSecretBytes fails, the reconciliation now flows through the framework where PullSecretSync captures the error as critical and CoreHCPChain reconciles the HCP with full cert resolution. Key changes: - Move GetPullSecretBytes, CPO image/label resolution, and namespace reconciliation into a single report.execute("CPOImageAndNamespace") block. This prevents namespace PSA label downgrades when CPO labels are unavailable. - Make DetermineHostedClusterPayloadArch and lookupReleaseImage non-fatal so reconciliation continues to the framework. - Make cpoSupportsKASCustomKubeconfig status check unconditional — all supported CPO versions expose custom kubeconfig. - Wrap releaseImageVersion parsing in report.execute(critical) to block OperatorDeployments and RBACAndPolicies on failure instead of hard-returning. - Extract reconcileControlPlaneNamespace into its own method. - Update pull-secret-missing tests with valid fixtures (NonePlatform, Route, valid UUID) so reconciliation reaches the framework. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/lgtm |
|
Scheduling tests matching the |
Move the original reconcile method into reconcile_legacy.go as reconcileLegacy, activated by HYPERSHIFT_RECONCILE_LEGACY=1. This replaces the legacy flag in reconcileReport — the changes to the v2 reconciler are too deep for a flag to faithfully reproduce the old behavior. - Extract pre-refactor reconcile into reconcile_legacy.go - Remove legacy field from reconcileReport and simplify shouldBlock - Dispatch via ReconcileLegacy flag in the Reconcile entry point - Fix reconcileOpenShiftTrustedCAs unused bool return (unparam lint) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by unit |
|
@muraee: This PR has been marked as verified by 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. |
|
/retest-required |
|
@muraee: The following tests 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. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseTransient Docker build infrastructure failure (flaky). The
Recommendations
Evidence
|
6 participants
Summary
reconcile()in the HostedCluster controller to use categorized error handling with critical/non-critical operations instead of sequential short-circuiting. Previously, any single failure among ~50 operations would block all subsequent work — e.g., a missing SSH key secret prevented CPO deployment and HCP creation.reconcileReportstruct (reconcile_report.go) that classifies operations as critical (blocks downstream Phase 8) or nonCritical (errors collected, never blocks). When critical operations fail, Phase 8 components are automatically skipped with clear reporting of what failed and what was blocked.reconcileOperatorDeployments,reconcileRBACAndPolicies,reconcileKubeconfigAndPasswordSync,reconcileAuxiliary,reconcilePlatformSpecific) that collect errors independently.ReconciliationSucceededcondition now reflects the structured report: when critical failures exist, the condition message surfaces which operations failed and which were blocked (e.g.,critical failures: [PullSecretSync]; blocked operations: [OperatorDeployments, RBACAndPolicies, ...]).Key changes
Error categorization
Phase structure
Condition reporting
The
ReconciliationSucceededcondition now reflects the structured error report:critical failures: [PullSecretSync]; blocked operations: [KubeconfigAndPasswordSync, OperatorDeployments, RBACAndPolicies, PlatformOIDCAndCSI, MonitoringAndCLISecrets]Structured error aggregation
When critical failures exist,
aggregate()returns only critical errors with blocked operation list — non-critical errors are suppressed since the user should fix the critical issue first:When no critical failures exist, all errors are returned as-is.
reconcileReport API
Two public methods on the report:
execute(name, category, func() error)— always runs the operation and records the resultexecuteOrBlock(name, func() error)— automatically checkshasCriticalFailure()and either runs the operation or records it as blockedAnalysis
See
docs/design/hostedcluster-reconcile-segregation-analysis.mdfor the full design.Test plan
go test -count=1 -race ./hypershift-operator/controllers/hostedcluster/)make lintpasses with 0 issuesreconcileReportmethods (TestReconcileReport,TestConditionMessage,TestAggregate,TestExecuteOrBlock)TestReconcileKubeconfigAndPasswordSync_*,TestReconcileRBACAndPolicies_*)