CNTRLPLANE-3632: Predictable NodePool rollout control - #8698
Conversation
|
@csrwng: This pull request references OCPSTRAT-3298 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 feature to target either version "5.0." or "openshift-5.0.", but it targets "openshift-5.1" instead. 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. |
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughThis PR separates full configuration hashes from rollout-specific hashes. Rollout hashes exclude management-side HAProxy content and platform-derived defaults. NodePool reconciliation stores the rollout hash in a dedicated annotation and reports management-side drift separately. MachineDeployment and MachineSet updates compare rollout hashes and versions, then record rollout completion conditionally. Token and Karpenter reconciliation track both hash types. Unit and E2E tests cover stable management changes, annotation seeding, and spec-driven rollouts. Sequence Diagram(s)sequenceDiagram
participant NodePool
participant Controller
participant ConfigGenerator
participant MachineDeployment
participant MachineSet
ConfigGenerator->>Controller: return full and rollout hashes
Controller->>NodePool: seed or read rollout annotation
Controller->>MachineDeployment: compare version and rollout hash
Controller->>MachineSet: compare version and rollout hash
MachineDeployment-->>Controller: report rollout completion
MachineSet-->>Controller: report rollout completion
Controller->>NodePool: persist current rollout state
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_test.go`:
- Around line 742-794: Replace the manual annotation writes and plain equality
checks in TestRolloutHashAnnotationSeeding with calls into the real production
seeding helper (the function that computes/returns the rollout hash and a seeded
bool) and assert the boolean return; specifically, when
nodePoolAnnotationCurrentRolloutConfig is absent call the seeding helper and
expect seeded==true and the returned hash to be stored on nodePool.Annotations,
and when the annotation already exists call the helper and expect seeded==false;
also update TestGenerateMCORawConfig to consume and assert on rolloutConfigsRaw
(rather than discarding it) so changes to the generated rollout-only config
(e.g., reintroducing HAProxy) will fail tests.
In `@hypershift-operator/controllers/nodepool/config.go`:
- Around line 155-192: generateMCORawConfig currently only strips inline
cg.haproxyRawConfig via parseWithoutHaproxy, but when cg.haproxyRawConfig == ""
getCoreConfigs still returns the legacy HAProxy core ConfigMap which then ends
up in rolloutConfigsRaw; change generateMCORawConfig so that before calling
parseWithoutHaproxy it builds a filtered configs slice that excludes the legacy
HAProxy core ConfigMap (the same ConfigMap returned by getCoreConfigs for
HAProxy — detect it by the same unique identifier used in getCoreConfigs, e.g.,
its ConfigMap name/label or by comparing its data to cg.haproxyRawConfig), then
pass that filtered slice to parseWithoutHaproxy while still passing the full
configs slice to parse so fullConfig is unchanged (use functions/fields:
generateMCORawConfig, getCoreConfigs, getUserConfigs, getNTOGeneratedConfig,
cg.haproxyRawConfig, parse, parseWithoutHaproxy).
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Around line 402-409: The current seeding writes
nodePoolAnnotationCurrentRolloutConfig using token.RolloutHashWithoutVersion()
unconditionally, which can suppress user-driven rollouts; change the logic in
the reconcile path that sets nodePoolAnnotationCurrentRolloutConfig so it
prefers the last-applied/legacy value (check any legacy annotation like the
prior "lastApplied" key or bootstrap state) and only auto-seed from
token.RolloutHashWithoutVersion() when you can prove no rollout is pending
(e.g., verify isUpdatingConfig() is false or compare current vs target rollout
hashes and ensure they differ/are stable). Update the code that currently
checks/creates nodePool.Annotations and assigns
nodePoolAnnotationCurrentRolloutConfig to first try restoring from
legacy/last-applied state, and fallback to auto-seed from
token.RolloutHashWithoutVersion() only when no pending rollout is detected.
In `@test/e2e/v2/tests/nodepool_rollout_control_test.go`:
- Around line 67-174: The test ManagementImageChangeNoRolloutTest is mutating
the shared default NodePool via getDefaultNodePool; instead create an isolated
NodePool using buildTestNodePool (mirroring SpecDrivenChangeTriggersRolloutTest)
and register cleanup with DeferCleanup to call cleanupNodePool(...) so the test
operates on its own NodePool; update the teardown logic to only remove the
HAProxy annotation on the test NodePool (no IsNotFound branch) and use the test
NodePool object in all subsequent references instead of
defaultNP/getDefaultNodePool.
- Around line 287-396: The test OperatorUpgradeNoRolloutTest mutates the shared
default NodePool via getDefaultNodePool; change it to create and use a dedicated
test NodePool like SpecDrivenChangeTriggersRolloutTest does by calling
buildTestNodePool (or the same factory used there), wait for the NodePool to
become ready (so annotations are seeded), then remove the rollout annotation on
that test NodePool to simulate pre-upgrade state, and register cleanup with
DeferCleanup to call cleanupNodePool (or the same cleanup helper) to delete the
test NodePool and restore state; ensure all references to np and baseline node
lists use the newly created NodePool instead of getDefaultNodePool.
🪄 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: ad3269e8-032f-403d-8aef-f75206c60cb9
📒 Files selected for processing (7)
hypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/config.gohypershift-operator/controllers/nodepool/config_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gotest/e2e/v2/tests/nodepool_lifecycle_test.gotest/e2e/v2/tests/nodepool_rollout_control_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8698 +/- ##
==========================================
+ Coverage 47.11% 47.16% +0.05%
==========================================
Files 786 786
Lines 99220 99334 +114
==========================================
+ Hits 46744 46850 +106
- Misses 49317 49320 +3
- Partials 3159 3164 +5
... 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:
|
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
|
This confirms the root cause. The PR added new tests in Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryTwo unit tests fail because the PR changed Root CauseThe PR introduces a new "rollout config" concept to separate management-side config changes (e.g., HAProxy image bumps) from spec-driven config changes that require node replacement. This involved:
The two tests were not updated:
The PR added correct new tests in Recommendations
Evidence
|
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
|
/remove-lifecycle rotten |
|
@csrwng: This pull request references CNTRLPLANE-3632 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.0.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. |
8d9fd1a to
b2abede
Compare
|
[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 |
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 `@api/hypershift/v1beta1/nodepool_conditions.go`:
- Around line 91-95: The comment for NodePoolConfigUpdatePendingConditionType
inaccurately describes scale-up behavior. Update it to state that new or
replaced nodes may receive the latest management-side configuration while
existing nodes retain the previous configuration until the next spec-driven
rollout, without changing payload hashing or secret naming.
🪄 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: ba91d631-99b7-469c-a822-dfb15d50706c
📒 Files selected for processing (1)
api/hypershift/v1beta1/nodepool_conditions.go
e360cca to
b1e14e5
Compare
35be501 to
b137169
Compare
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
b137169 to
958a749
Compare
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
958a749 to
2376ab7
Compare
2376ab7 to
1bfc25a
Compare
…ig changes Introduce RolloutHash and RolloutHashWithoutVersion that hash only spec-driven inputs (user MachineConfigs, release version, pull secret, trust bundle, global config), excluding management-side content like HAProxy image digests. Rollout decisions in both Replace and InPlace paths now compare rollout hashes against a new nodePoolCurrentRolloutConfig annotation instead of comparing data secret names. This prevents automated HAProxy image digest bumps from triggering full worker node replacement while ensuring new nodes still receive the latest payload when they ARE replaced for spec-driven reasons. On first reconcile after operator upgrade the annotation is seeded without triggering a rollout, and isUpdatingConfig returns false when the annotation is absent to prevent condition flip-flop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1bfc25a to
c010b61
Compare
|
/test unit |
|
/lgtm |
|
Scheduling tests matching the |
|
Retrying flakes:
/test e2e-v2-aws
/test e2e-aws
/test e2e-aks-5-0 |
|
@csrwng: 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. |
|
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. |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
What this PR does / why we need it:
Decouples NodePool rollout triggering from management-side configuration changes (e.g. HAProxy image digest bumps) so that only user-driven spec changes cause worker node replacement.
Today the NodePool controller uses a single hash over the entire rendered ignition config — including management-side image references — to drive rollout decisions. Any change to this hash triggers a full Replace/InPlace rollout. This means automated HAProxy image updates (which happen on every HyperShift operator upgrade) cause unnecessary node churn.
This PR introduces:
RolloutHash()/RolloutHashWithoutVersion(): New hash methods that include only spec-driven inputs (user MachineConfigs, release version, pull secret, trust bundle, global config), excluding management-side content like HAProxynodePoolCurrentRolloutConfigannotation: Tracks the current rollout config hash, used for rollout decisions instead of comparing data secret namesisUpdatingConfig()safety: Returns false when the annotation is absent to prevent condition flip-flop during upgradeThe existing
Hash()and payload secret naming are unchanged — new nodes always get the latest payload including management-side content when they ARE replaced for spec-driven reasons.Which issue(s) this PR fixes:
Fixes OCPSTRAT-3298
Special notes for your reviewer:
RolloutHashWithoutVersion()intentionally includesglobalConfigunlike the existingHashWithoutVersion()which omits it. Proxy/image config changes should trigger rollouts.parse()→doParse()refactor is necessary because haproxy content is prepended insideparse()and cannot be stripped from the final string after the fact.secret_janitor_test.gobuild error is pre-existing and unrelated to this PR.Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes