Repository navigation
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
openshift-ci
Bot
added
needs-ok-to-test
|
Hi @alokstech. 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
area/hypershift-operator
|
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:
Walkthrough
Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Stable And Deterministic Test Names | |
The aggregate PR diff adds no Ginkgo title calls; NodePool tests use static Go Test names and literal t.Run case names, with no dynamic identifiers. |
| Test Structure And Quality | |
The changed tests use standard testing.T subtests with Gomega, not Ginkgo Describe/It suites; they use in-memory fake clients and contain no cluster waits requiring timeouts or cleanup. |
| Topology-Aware Scheduling Compatibility | |
The changed production code only updates rollout hashing, ConfigMap watching, and secret migration; no new scheduling constraints, replica derivation, selectors, tolerations, affinity, spread, or P... |
| Ipv6 And Disconnected Network Test Compatibility | |
The PR adds only standard Go unit tests under nodepool; it adds no Ginkgo e2e tests, IPv4 assumptions, or external connectivity requirements. |
| No-Weak-Crypto | |
The PR adds no MD5, SHA-1, DES, RC4, Blowfish, ECB, custom cryptography, or secret/token comparisons; it uses existing non-cryptographic FNV-1a for rollout hashes. |
| Container-Privileges | |
The PR changes no YAML, Dockerfile, or Containerfile manifests. Added-line and AST searches found no privileged, host namespace, SYS_ADMIN, root, or allowPrivilegeEscalation settings. |
| No-Sensitive-Data-In-Logs | |
Changed logs record resource names, hashes, release image, and wrapped ConfigMap errors; review found no logging of Secret data, CA-bundle content, passwords, tokens, or API keys. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: triggering NodePool rollouts when trust bundle ConfigMap content changes. |
✨ Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
feat/rfe-8751-trust-bundle-rollout
- Resolve merge conflict in branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
-
Comment @coderabbitai help to get the list of available commands.
|
I now have a complete picture. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryNo CI tests ran. The PR branch ( Root CauseThe root cause is git merge conflicts in two files, not a product bug or test failure:
Conflicting commits on
The PR branch was last committed on 2026-07-12, so it missed commit Recommendations
Evidence
|
openshift-ci
Bot
removed
the
needs-rebase
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/token.go (1)
57-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the scope of these hashes These values are written to the token Secret for change detection, not to the NodePool rollout hash.
pullSecretHashstill tracks pull-secret content separately from the rollout path, so this wording implies rollout behavior that the code does not have.🤖 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 `@hypershift-operator/controllers/nodepool/token.go` around lines 57 - 62, Update the comment above pullSecretHash, additionalTrustBundleHash, and globalConfigHash to state that these hashes are written to the token Secret for change detection, not included in the NodePool rollout config hash. Clarify that pullSecretHash tracks pull-secret content separately, and remove the inaccurate claim that these values trigger NodePool rollouts.
🤖 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 `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 137-163: Update rolloutTrustBundleHashes and configMapCABundleHash
so missing or malformed trust-bundle ConfigMaps are represented as a degraded
HostedCluster condition rather than propagated as a hard NodePool reconciliation
error. Preserve successful hash calculation, and ensure NewConfigGenerator or
its caller records the degraded state while allowing reconciliation to continue
through transient delete/recreate windows.
---
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/token.go`:
- Around line 57-62: Update the comment above pullSecretHash,
additionalTrustBundleHash, and globalConfigHash to state that these hashes are
written to the token Secret for change detection, not included in the NodePool
rollout config hash. Clarify that pullSecretHash tracks pull-secret content
separately, and remove the inaccurate claim that these values trigger NodePool
rollouts.
🪄 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: Enterprise
Run ID: b8ad4e64-1086-4660-99e7-3eb0dff885ce
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (7)
docs/content/reference/nodepool-rollouts.mdhypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/config_trustbundle_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.go
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hypershift-operator/controllers/nodepool/config_hash_migration_test.go`:
- Around line 54-128: Add a table-driven case in the config hash migration tests
using an empty additional trust bundle and a nonempty proxyTrustedCAHash, with
matching legacy annotations. Verify migration seeds both current-config
annotations with hashes incorporating the proxy trusted-CA value and confirm
UpdatingConfig remains unset.
🪄 Autofix
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: dc476609-2b7d-489a-a361-adb4a366a991
⛔ Files ignored due to path filters (1)
docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (7)
docs/content/reference/nodepool-rollouts.mdhypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config_hash_migration.gohypershift-operator/controllers/nodepool/config_hash_migration_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/secret_janitor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- hypershift-operator/controllers/nodepool/conditions.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
976-977: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle HostedCluster lookup failures in the ConfigMap mapper.
In
enqueueNodePoolsForConfig, anyr.Geterror on the HostedCluster lookup skips that NodePool, including transient cache/API failures during in-place trust-bundle updates. Also return a reconcilation error for non-NotFoundfailures here so the ConfigMap event does not get silently dropped.🤖 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 `@hypershift-operator/controllers/nodepool/nodepool_controller.go` around lines 976 - 977, Update enqueueNodePoolsForConfig so HostedCluster lookup errors are handled explicitly: continue skipping the NodePool for NotFound, but return the non-NotFound error so transient cache or API failures propagate as reconciliation errors instead of being silently dropped.Source: Path instructions
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
969-972: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the shadowing
okvariable.Line 970 shadows the
okvariable from the ConfigMap type assertion. Use a name such asseenAlready.Proposed fix
- if _, ok := seen[req.String()]; ok { + if _, seenAlready := seen[req.String()]; seenAlready {As per coding guidelines,
**/*.gosays “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 `@hypershift-operator/controllers/nodepool/nodepool_controller.go` around lines 969 - 972, Rename the `ok` result in the `seen` map lookup within the nodepool reconciliation loop to a non-shadowing name such as `seenAlready`, and update the conditional to use it while preserving the existing continue behavior.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.
Outside diff comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 976-977: Update enqueueNodePoolsForConfig so HostedCluster lookup
errors are handled explicitly: continue skipping the NodePool for NotFound, but
return the non-NotFound error so transient cache or API failures propagate as
reconciliation errors instead of being silently dropped.
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 969-972: Rename the `ok` result in the `seen` map lookup within
the nodepool reconciliation loop to a non-shadowing name such as `seenAlready`,
and update the conditional to use it while preserving the existing continue
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 61d3b347-f492-45c1-8788-ea0dcf13868a
📒 Files selected for processing (4)
hypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_hash_migration_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/token.go
🚧 Files skipped from review as they are similar to previous changes (2)
- hypershift-operator/controllers/nodepool/config_hash_migration_test.go
- hypershift-operator/controllers/nodepool/config.go
601fb0a to
b4a7c40
Compare
openshift-ci
Bot
removed
the
needs-rebase
Nirshal
left a comment
Nirshal
left a comment
There was a problem hiding this comment.
Took another look at the test coverage after the migration changes. The new pure-function tests are solid. A few areas could use additional coverage:
|
/lgtm |
|
The duplicate
Before this PR, Not blocking, but worth addressing in a follow-up. For AI agents: in |
58d1cf2 to
7c6d698
Compare
openshift-ci
Bot
added
area/control-plane-operator
|
/retest |
|
All PipelineRuns for this commit have already succeeded. Use |
7c6d698 to
25d4761
Compare
25d4761 to
75ae906
Compare
celebdor
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@alokstech: This pull request references CNTRLPLANE-4811 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
needs-rebase
75ae906 to
28f837e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng 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 |
|
/retest |
Watch additional and proxy trust-bundle ConfigMaps and version config hashes to preserve existing nodes during operator upgrades. Persist content baselines for NodePools and Karpenter so later CA edits roll out. Refresh retained bootstrap tokens, preserve ignition reachability, and validate rendered CA content before caching ignition payloads. Detect capabilities from the selected CPO and retain legitimate global-config rollouts. Add regression tests, focused e2e coverage, and documentation. Assisted-by: LLM Signed-off-by: alokstech <alok@redhat.com>
28f837e to
d4095b5
Compare
Reduce test complexity by 98 lines while maintaining coverage and clarify allowlist configuration.
|
@alokstech: 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. |
|
/retest |
|
All PipelineRuns for this commit have already succeeded. Use |
|
|
||
| func (t *Token) Reconcile(ctx context.Context) error { | ||
| log := ctrl.LoggerFrom(ctx) | ||
| if err := t.validateTrustBundleCapabilities(ctx); err != nil { |
There was a problem hiding this comment.
Blocker: for an older CPO without the rendered-trust-bundle-validation label, this returns an error whenever a trust bundle is in use. reconcile() then exits before capi.Reconcile, so the NodePool stops applying replica, release and config changes. Older payload CPOs don't have the label, so existing HostedClusters that use trust bundles break on HO upgrade. Today those clusters work with name-based rollouts.
Please fall back to v1 (name-based) hashing when the CPO lacks the capability (hashAtVersion(ConfigHashVersionV1) already exists), and add a test for it.
|
/pipeline required |
|
Scheduling tests matching the |
Summary
Implements Phase 1 of RFE-8751: roll NodePools when
ca-bundle.crtcontent changes in ConfigMaps referenced byHostedCluster.spec.additionalTrustBundleorHostedCluster.spec.configuration.proxy.trustedCA.nodePoolCurrentRolloutConfigV2and recordnodePoolConfigHashVersion=v2without replacing existing workers solely because the hash formula changed. Preserve completed payload hashes and bootstrap Secret references.In-place pull-secret edits remain outside the rollout trigger. Switching
additionalTrustBundleto another ConfigMap with identical content does not itself trigger a rollout. Forproxy.trustedCA, reference-name changes remain rollout-relevant through proxy configuration.Upgrade behavior and requirements
On the first reconcile after migration, current CA content is absorbed into the versioned rollout baseline, including CA edits made under the same ConfigMap name before upgrading HO. This does not rewrite the completed payload identity or propagate a new bootstrap Secret reference solely for migration.
Subsequent CA-content changes trigger an intended rollout. Management-only image changes do not themselves trigger a rollout; other rollout-relevant configuration or release changes still do. In-progress rollouts retain their deployed target, while a newer release request can supersede it. These guarantees concern formula-only migration, not a promise that every cluster upgrade is rollout-free.
Trust-bundle reconciliation requires a CPO/ignition-server image advertising
io.openshift.hypershift.control-plane-operator-manages.rendered-trust-bundle-validation. Updating HO alone is insufficient with an unsupported CPO. Older CPOs require the validation backport and capability label. Unsupported configurations stop trust-bundle reconciliation before bootstrap changes or old-token cleanup.See
docs/content/reference/nodepool-rollouts.mdfor the current behavior.Latest manual AWS validation — October 26, 2026
Fresh AWS HostedClusters in
ap-south-1, release 5.0.0-rc.7-x86_64, each with one two-worker NodePool andadditionalTrustBundleconfigured:clusters/rfe8751-legacy-rc7: legacy pre-CNTRLPLANE-3632: Predictable NodePool rollout control #9818 HO → PR HO.clusters-upstream/rfe8751-upstream-rc7: upstream HO at6cc7c191a4(containing CNTRLPLANE-3632: Predictable NodePool rollout control #9818) → PR HO.For each path:
UpdatingConfig=False.UpdatingConfig=False,UpdatingVersion=False, and a Completed/Available HostedCluster.Both paths passed. For the post-#9818 path, EC2 instances
i-0184a7fa94a3253f0andi-01f158ab86e735a5fsurvived migration; only the subsequent CA edit replaced them withi-04e52e3a07101cb32andi-06abfc83b30f84540.Live tests used feature source
2aad9d372b. The subsequent rebase produced28f837e92e;git range-diffconfirmed the feature patch was unchanged. Targeted NodePool, ignition-server, and Karpenter tests passed after rebasing.Compatibility caveat: these live tests used a test-only CPO adjustment omitting machine-approver's unsupported
--tls-curve-preferencesflag on rc.7, tracked by OCPBUGS-129486. Other TLS arguments were retained. This workaround is not included in this PR. The tests validate trust-bundle behavior with that adjustment, not an entirely unmodified CPO/release combination. No manual CSR approval was used to complete the replacements.Test plan and evidence scope
pull-ci-openshift-hypershift-main-e2e-aws-upgrade-hypershift-operatoron the current revision. Manual testing does not replace this presubmit.The latest two-cluster run exercised regular AWS NodePools with
additionalTrustBundle; it does not claim fresh live coverage of every proxy, Karpenter, pull-secret, or failure-recovery scenario after the implementation refactor.Historical validation — earlier implementations
The following results were recorded on earlier revisions. They are retained for context, not presented as fresh validation of the current #9818-based implementation:
rfe8751-upgnoroll, imagerollout-fix-2: migration preserved instancei-0662f531572cf1e87; a later content edit rolled toi-0609945fceacfd9f6.rfe8751-legacy-pre, imagerfe8751-squash-test: a pre-upgrade CA edit plus HO upgrade preservedi-0761141a949fe27f2; a post-migration edit rolled toi-026a47dd5cee4a78c. An in-place pull-secret edit did not roll that worker.rfe8751-e2e: the earlierNodePoolTrustBundleTestrun (labelnodepool-trust-bundle) exercised an in-place CA edit and rollout.rfe8751-test: earlier review-fix testing covered Karpenter/AutoNode migration identity preservation, regular NodePool no-roll migration, a legitimate global proxy configuration change, continued reconciliation of retained bootstrap Secret pairs, and ignition rejection of mismatched content followed by serving the new CA. The earlier Karpenter identity was0c371586; the served new CA fingerprint was07:9D:FC:B1:F6:8A:C9:64:FB:F4:4F:6A:A0:9E:84:BE:A9:76:C1:B3:80:60:44:20:84:15:C4:52:43:96:A6:21.Historical skip-log messages and annotation-rewrite descriptions apply to those earlier revisions, not the current implementation.