Repository navigation
CNTRLPLANE-3691: Decouple NodePool condition handling from CAPI reconciliation - #9875
redhat-chai-bot wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@redhat-chai-bot: This pull request references CNTRLPLANE-3691 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 task 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. |
|
Pipeline controller notification This PR uses the pipeline controller for second-stage tests. Selection and triggering follow the repository configuration. Use |
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: moving NodePool condition handling out of CAPI reconciliation. |
| 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 | |
All added subtest titles use literal, stable names. The ignition cases use two literal table names, and the status value remains in the test body. No changed test title uses generated identifiers, tim… |
| Test Structure And Quality | |
The prior assertion-message concern is addressed. Changed assertions in capi_test.go and nodepool_controller_test.go include diagnostic messages for reconciliation errors, results, conditions, res… |
| Topology-Aware Scheduling Compatibility | |
PASS: The pull request changes condition propagation and reconciliation return values. The changed-file inventory contains no deployment manifests. Added production lines in capi.go and `nodepool_co… |
| Ipv6 And Disconnected Network Test Compatibility | |
No new Ginkgo e2e test is added in the reviewed diff. The changed tests are standard Go unit tests, and the allowlist test only parses existing Ginkgo source. The diff introduces no e2e network connec… |
| No-Weak-Crypto | |
The pull request changes NodePool reconciliation, condition conversion, tests, and an allowlist. The added and modified lines contain no MD5, SHA-1, DES, RC4, 3DES, Blowfish, ECB, crypto API, custom c… |
| Container-Privileges | |
The pull request changes only Go source and test files. No container or Kubernetes manifest files changed. Added-line inspection found no privileged: true, hostPID, hostNetwork, hostIPC, `SYS_… |
| No-Sensitive-Data-In-Logs | |
No sensitive-data logging was introduced. The changed capi.go lines only rename the existing result variable to operationResult and preserve the same reconciliation log messages and fields. The … |
Full details: Stable And Deterministic Test Names
-
Explanation
All added subtest titles use literal, stable names. The ignition cases use two literal table names, and the status value remains in the test body. No changed test title uses generated identifiers, timestamps, namespaces, node names, IP addresses, UUIDs, or legacy metadata tokens. Prior failure addressed: capi_test.go now defines literal False and Unknown case names at lines 2007-2012 and passes the status only through the test body; the prior formatted ignitionStatus title is absent.
Full details: Test Structure And Quality
-
Explanation
The prior assertion-message concern is addressed. Changed assertions in
capi_test.goandnodepool_controller_test.goinclude diagnostic messages for reconciliation errors, results, conditions, resource counts, and persisted fields. The new allowlist test uses contextualFatalfandErrorfmessages. The diff adds no GinkgoIt,Eventually, orConsistentlyblocks, and it uses fake clients rather than cluster resources requiring cleanup. Prior failure addressed: Changed test assertions now include specific messages throughout capi_test.go and nodepool_controller_test.go, including condition persistence and error-path checks. The previously cited bare assertions are remediated.
Full details: Topology-Aware Scheduling Compatibility
-
Explanation
PASS: The pull request changes condition propagation and reconciliation return values. The changed-file inventory contains no deployment manifests. Added production lines in
capi.goandnodepool_controller.gointroduce no affinity, topology spread, toleration, node selector, replica-count, arbiter, or scheduling constraints. TheMaxUnavailablereferences remain existing rollout configuration and are not new scheduling constraints.
Full details: Ipv6 And Disconnected Network Test Compatibility
-
Explanation
No new Ginkgo e2e test is added in the reviewed diff. The changed tests are standard Go unit tests, and the allowlist test only parses existing Ginkgo source. The diff introduces no e2e network connection or IPv4-only behavior requiring this check.
Full details: No-Weak-Crypto
-
Explanation
The pull request changes NodePool reconciliation, condition conversion, tests, and an allowlist. The added and modified lines contain no MD5, SHA-1, DES, RC4, 3DES, Blowfish, ECB, crypto API, custom crypto implementation, or secret/token comparison. The added equality checks compare conditions and allowlist symbols only.
Full details: Container-Privileges
-
Explanation
The pull request changes only Go source and test files. No container or Kubernetes manifest files changed. Added-line inspection found no
privileged: true,hostPID,hostNetwork,hostIPC,SYS_ADMIN,allowPrivilegeEscalation: true, or root execution setting. TheCPOCapabilitiestest field is unrelated to container privileges.
Full details: No-Sensitive-Data-In-Logs
-
Explanation
No sensitive-data logging was introduced. The changed
capi.golines only rename the existingresultvariable tooperationResultand preserve the same reconciliation log messages and fields. The new log value is a controller-runtime operation result enum. No added logs contain passwords, tokens, API keys, PII, session IDs, hostnames, or customer data.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
openshift-ci
Bot
added
the
area/hypershift-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9875 +/- ##
==========================================
+ Coverage 52.31% 52.43% +0.11%
==========================================
Files 826 826
Lines 105380 105368 -12
==========================================
+ Hits 55130 55249 +119
+ Misses 46728 46606 -122
+ Partials 3522 3513 -9
... and 4 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:
|
| reason = c.Reason | ||
| if condition.Reason != "" { | ||
| reason = condition.Reason | ||
| } |
There was a problem hiding this comment.
Could you extract the MachinesReadyCondition to NodePoolReadyCondition conversion into one unexported helper and use it from both reconcileMachineDeploymentStatus and reconcileMachineSet? Both paths currently repeat the status/message mapping, empty-reason fallback to AsExpectedReason, and observed-generation assignment. A shared helper would keep those semantics aligned while preserving the current behavior, including returning no condition when MachinesReadyCondition is absent.
This is a nonblocking cleanup suggestion; the duplication predates this PR.
There was a problem hiding this comment.
Addressed in b4df383. Extracted the unexported nodePoolReadyConditions helper and used it from both reconcileMachineDeploymentStatus and reconcileMachineSet. It preserves the absent-condition nil return, status/message mapping, empty-reason fallback to AsExpectedReason, and NodePool observed generation, while leaving transition-time handling with the caller.
Added focused conversion tests and callsite coverage, including repeated MachineSet reconciliation and absent readiness. Full make pre-commit and recursive race-enabled NodePool tests pass on the pushed revision. Final-revision CI and live cloud E2E results remain unverified.
AI-generated. Review for accuracy.
openshift-ci
Bot
added
the
area/ci-tooling
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Addressed the test-name and assertion-diagnostic feedback in #9875 (comment).
Published revision: b4df383. Complete AI-generated. Review for accuracy. |
Return reconcile-local condition results from CAPI so resource reconciliation no longer mutates NodePool conditions directly. Apply accumulated results in the NodePool controller before error handling. Preserve condition order and status updates across early returns and failures. Assisted-by: LLM Signed-off-by: Chai Bot <chai-bot@redhat.com>
b4df383 to
1efae11
Compare
|
@redhat-chai-bot: all tests passed! 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. |
| r.reachedIgnitionEndpointCondition, | ||
| r.machineAndNodeConditions, | ||
| r.validPlatformConfigCondition, | ||
| // TODO(alberto): consider moving here: |
There was a problem hiding this comment.
This removes only the TODO comment, not either condition. The cleanup accompanied moving condition application into the NodePool controller: CAPI still computes NodePoolUpdatingPlatformMachineTemplateConditionType and NodePoolAutorepairEnabledConditionType, returns them, and the controller applies them immediately after capi.Reconcile, before handling its error.
However, the TODO specifically suggested moving them into this signalConditions list. This PR does not do that; it preserves their later reconciliation order. Removing the comment therefore overstates how much of that TODO is addressed. The location-specific TODO can remain for a separate reorganization.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
Ok so are you putting the todo comment back?
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
The spec review found a readiness rollback regression on failed workload writes. The standards review found an unrelated test addition. Both findings are inline.
Validation: focused NodePool tests and the added linter test passed with race detection. A controller-level regression check passed on base 364a78f and failed on this head. Live E2E tests were not run. The spec review used this PR and #9032 because Jira was inaccessible.
| } | ||
| } | ||
|
|
||
| func TestAllowlistOSImageStreamUpgradeScopes(t *testing.T) { |
There was a problem hiding this comment.
[P3][Standards] Move the unrelated OSImageStream test to a separate PR.
TestAllowlistOSImageStreamUpgradeScopes has no dependency on CAPI condition collection or application. CONTRIBUTING.md, under "Prior to Submitting a Pull Request", requires contributors to "Scope commits to one thing" and "save additional improvements for separate PRs." Please separate this test so the controller refactor does not also include unrelated linter-test maintenance.
| return fmt.Errorf("failed to reconcile MachineDeployment %q: %w", | ||
| return reconcileErr | ||
| }) | ||
| result.Conditions = append(result.Conditions, conditions...) |
There was a problem hiding this comment.
[P2][Spec] Preserve the Ready rollback when workload persistence fails.
reconcileWorkload restores the previous NodePool snapshot when CreateOrPatch fails. Previously, that also rolled back the Ready update computed inside the mutation. This append returns that condition despite the rollback, and the controller then persists it through its error path. The MachineSet branch at line 155 has the same issue.
I reproduced this through the outer NodePool reconciler by requesting five replicas from an existing three-replica MachineDeployment and injecting a workload patch failure. The stored MachineDeployment still requested three replicas, but this revision persisted Ready=True with ObservedGeneration=17. The same regression check passed on the base revision, which preserved the previous readiness state.
This contradicts #9032's promise that "the same conditions are set under the same circumstances" and this PR's requirement to preserve conditions across later errors. Please append workload conditions only after reconcileWorkload succeeds. Conditions computed before that operation should still survive its failure, and successful workload conditions should still survive later MHC errors. Update the new failure tests, which currently assert the changed behavior.
Summary
Tests
Live E2E execution and final-revision CI results remain to be verified on this PR.
AI-generated. Review for accuracy.
@bryan-cox requested from Slack
Summary by CodeRabbit