WIP: OCPBUGS-80958: Kube apiserver node replace#2178
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@bhperry: This pull request references Jira Issue OCPBUGS-80958, which is invalid:
Comment 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. |
|
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:
WalkthroughAdds Node UID tracking to static pod node reconciliation, updates helpers and tests for NodeUID propagation, and redirects two OpenShift modules in ChangesStatic Pod Node UID Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bhperry 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 |
![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
🧹 Nitpick comments (3)
pkg/operator/staticpod/controller/node/node_controller_test.go (3)
740-740: Remove unusedexpectedNodesfield.
expectedNodes [][2]stringis declared in the table struct but never populated or read by any of the four scenarios. Drop it to avoid confusion about what the test actually asserts.♻️ Proposed change
tests := []struct { name string startNodes []runtime.Object startNodeStatus []operatorv1.NodeStatus - expectedNodes [][2]string evaluateNodeStatus func([]operatorv1.NodeStatus) error }{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/staticpod/controller/node/node_controller_test.go` at line 740, The test table struct contains an unused field expectedNodes [][2]string; remove this field from the table type declaration in node_controller_test.go and from any table literal entries so the scenarios only include fields actually used by the tests (ensure you update the table struct definition that declares expectedNodes and all test case literals that reference it, e.g., in the testCases slice for the Node controller tests).
786-807: Consider asserting the replacement event and JSONPatch, not just the finalNodeStatuses.The "node replaced mismatched uid in status" scenario is the most important new behavior this PR introduces, but the test only checks the final
NodeStatuses. Nothing verifies that:
- The controller emits
MasterNodeReplaced(with old+new UIDs) via the recorder — seenode_controller.goline 106.- A
jsonPatchis created to remove the stale entry at the expected index before the apply — seenode_controller.goline 117.Both are observable via
kubeClient.Fake.Actions()(event create) andfakeStaticPodOperatorClient.GetPatchedOperatorStatus(), as already done inTestNodeControllerDegradedConditionTypescenarios 5 and 12. Adding these assertions would lock in the behavior the PR description calls out ("ensure the node status is reset by comparing the node UID"). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/staticpod/controller/node/node_controller_test.go` around lines 786 - 807, The test currently only verifies final NodeStatuses; update the "node replaced mismatched uid in status" case to also assert that the controller emitted a MasterNodeReplaced event and that a JSONPatch removing the stale status entry was created: after running the controller, inspect kubeClient.Fake.Actions() for an event create matching MasterNodeReplaced (including old and new UIDs) and call fakeStaticPodOperatorClient.GetPatchedOperatorStatus() to retrieve the patch and assert it contains the expected JSONPatch operation that removes the stale index (as produced by the jsonPatch logic in node_controller.go). Ensure you reference the test case by name and check the exact event reason/message and the remove op in the patch, failing the test if either is missing.
808-829: Scenario name is misleading.The "node removed" scenario actually exercises the case where the previously tracked node name is gone and a different node has appeared (
test-node-1→test-node-2), i.e. a combined remove-and-add, not a pure removal. Consider renaming to something like"node removed and replaced by different name"to match the existing"single-node-removed"case inTestNewNodeControllerwhich tests the pure-removal behavior. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/staticpod/controller/node/node_controller_test.go` around lines 808 - 829, The test case in the table uses a misleading name "node removed" but actually exercises a remove-and-add scenario (previous NodeStatus test-node-1 replaced by startNodes test-node-2); update the test case's name field in node_controller_test.go to something clearer (for example "node removed and replaced by different name") so it accurately reflects the scenario and aligns with the existing "single-node-removed" naming in TestNewNodeController; locate the table entry by its name field in the test case slice and change only that string (no behavior changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 5-7: The go.mod contains forked-replace directives (replace
github.com/openshift/api => github.com/bhperry/openshift-api
v0.0.0-20260422220337-85d77b44a5a5 and replace github.com/openshift/client-go =>
github.com/bhperry/openshift-client-go v0.0.0-20260424144941-de7d2d9bafad) that
must be removed from the shared module manifest; delete these two replace lines
and either move the local overrides into a developer-only go.work (or similar
local config) or update the require lines to point to the approved upstream
module revisions before committing. Ensure no other project-wide go.mod contains
the same forked replace entries and run go mod tidy to verify dependency graph
is clean.
---
Nitpick comments:
In `@pkg/operator/staticpod/controller/node/node_controller_test.go`:
- Line 740: The test table struct contains an unused field expectedNodes
[][2]string; remove this field from the table type declaration in
node_controller_test.go and from any table literal entries so the scenarios only
include fields actually used by the tests (ensure you update the table struct
definition that declares expectedNodes and all test case literals that reference
it, e.g., in the testCases slice for the Node controller tests).
- Around line 786-807: The test currently only verifies final NodeStatuses;
update the "node replaced mismatched uid in status" case to also assert that the
controller emitted a MasterNodeReplaced event and that a JSONPatch removing the
stale status entry was created: after running the controller, inspect
kubeClient.Fake.Actions() for an event create matching MasterNodeReplaced
(including old and new UIDs) and call
fakeStaticPodOperatorClient.GetPatchedOperatorStatus() to retrieve the patch and
assert it contains the expected JSONPatch operation that removes the stale index
(as produced by the jsonPatch logic in node_controller.go). Ensure you reference
the test case by name and check the exact event reason/message and the remove op
in the patch, failing the test if either is missing.
- Around line 808-829: The test case in the table uses a misleading name "node
removed" but actually exercises a remove-and-add scenario (previous NodeStatus
test-node-1 replaced by startNodes test-node-2); update the test case's name
field in node_controller_test.go to something clearer (for example "node removed
and replaced by different name") so it accurately reflects the scenario and
aligns with the existing "single-node-removed" naming in TestNewNodeController;
locate the table entry by its name field in the test case slice and change only
that string (no behavior changes).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0131c169-b1c2-4ef4-ad19-3d6cd3ae6310
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/operator/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/operator/staticpod/controller/node/node_controller.gopkg/operator/staticpod/controller/node/node_controller_test.gopkg/operator/v1helpers/test_helpers.go
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bhperry: This pull request references Jira Issue OCPBUGS-80958, 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. |
|
/retest |
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
from
69d77b8 to
9f5d47f
Compare
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)
pkg/operator/staticpod/controller/node/node_controller.go (1)
110-118:⚠️ Potential issue | 🟠 MajorTighten the remove patch precondition to the UID-aware match.
The JSONPatch delete still only guards on
nodeName. If the status list has already been rewritten for the same name, this can delete the wrong entry. The removal path should validate the UID as well, or re-fetch before patching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/staticpod/controller/node/node_controller.go` around lines 110 - 118, The remove operation currently only tests /status/nodeStatuses/%d/nodeName via jsonPatch.WithRemove and jsonpatch.NewTestCondition using nodeState.NodeName, which can remove the wrong entry if names were reused; update the precondition to also validate the node UID (e.g., add a test for /status/nodeStatuses/%d/uid using nodeState.UID or otherwise combine name+uid checks) before calling jsonPatch.WithRemove (referencing jsonPatch.WithRemove, jsonpatch.NewTestCondition, nodeState.NodeName, nodeState.UID, and removedNodeStatusesCounter), or alternatively re-fetch the status list and re-compute removeAtIndex immediately prior to patching to ensure the exact entry is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/staticpod/controller/node/node_controller.go`:
- Around line 98-103: The current condition treats an empty nodeState.NodeUID as
a match which preserves legacy statuses and can keep stale state after a node is
deleted/recreated; update the matching logic so you only consider it a match
when nodeState.NodeUID exactly equals the currentUID (i.e., remove the "||
nodeState.NodeUID == \"\"" branch) in the block building newTargetNodeState (the
code that calls
applyoperatorv1.NodeStatus().WithNodeName(...).WithNodeUID(currentUID) and
appends to newTargetNodeStates), and apply the same strict-equality change to
the analogous logic around lines 126-136 so legacy empty UIDs are not treated as
matches and will be reset instead of preserved.
---
Outside diff comments:
In `@pkg/operator/staticpod/controller/node/node_controller.go`:
- Around line 110-118: The remove operation currently only tests
/status/nodeStatuses/%d/nodeName via jsonPatch.WithRemove and
jsonpatch.NewTestCondition using nodeState.NodeName, which can remove the wrong
entry if names were reused; update the precondition to also validate the node
UID (e.g., add a test for /status/nodeStatuses/%d/uid using nodeState.UID or
otherwise combine name+uid checks) before calling jsonPatch.WithRemove
(referencing jsonPatch.WithRemove, jsonpatch.NewTestCondition,
nodeState.NodeName, nodeState.UID, and removedNodeStatusesCounter), or
alternatively re-fetch the status list and re-compute removeAtIndex immediately
prior to patching to ensure the exact entry is removed.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e0840c87-5aa4-4d00-bb60-525c3ca309aa
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/operator/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/client-go/operator/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/operator/applyconfigurations/operator/v1/nodestatus.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/operator/staticpod/controller/node/node_controller.gopkg/operator/staticpod/controller/node/node_controller_test.gopkg/operator/v1helpers/test_helpers.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/v1helpers/test_helpers.go
- pkg/operator/staticpod/controller/node/node_controller_test.go
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
from
9f5d47f to
849d9a8
Compare
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
from
849d9a8 to
21ab80d
Compare
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
8 times, most recently
from
e522e0d to
a3d906c
Compare
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
3 times, most recently
from
aaca0d3 to
e96b42c
Compare
|
@bhperry: 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. |
bhperry
force-pushed
the
kube-apiserver-node-replace
branch
from
e96b42c to
82ef33f
Compare
3 participants
When master node is deleted and recreated with the same name, ensure the node status is reset by comparing UID.
Related PRs:
openshift/api#2821
openshift/client-go#371
openshift/cluster-kube-apiserver-operator#2199
TODO:
Summary by CodeRabbit