CNV-83939: replace node-role.kubernetes.io/control-plane label with node-role.kubevirt.io/control-plane for HCP Clusters#4303
Conversation
kubevirt-bot
added
release-note
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a kubevirt-specific control-plane label constant and reworks node labeling for HyperShift clusters: labelNode now assigns the kubevirt control-plane label to worker nodes, removes the legacy Kubernetes control-plane label only when it equals the HyperShift value (upgrade path), and skips API patches when labels are unchanged. isWorkerNode treats nodes with the legacy label set to the HyperShift value as workers for upgrade purposes. Tests were expanded for reconcile, startup, and upgrade scenarios. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Review notesStrengths:
Areas for careful review:
End note: controller logic and the upgrade tests are the highest-value review targets; overall the change is focused and well-covered — nice work. Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 13 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
![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 (1)
controllers/nodes/nodes_controller_test.go (1)
476-491: ⚡ Quick winUnused client initialization creates confusion.
The client
clat line 477 is created but never used—the reconciler's client is reassigned tocl2beforeReconcileis called. This pattern suggests incomplete refactoring and makes the test harder to follow.♻️ Suggested simplification
- resources := []client.Object{cpNode} - cl := commontestutils.InitClient(resources) + hco := commontestutils.NewHco() + resources := []client.Object{hco, cpNode} + cl := commontestutils.InitClient(resources) r := &ReconcileNodeCounter{ Client: cl, nodeEvents: nodeEvents, HandleHyperShiftNodeLabeling: HandleHyperShiftNodeLabeling, } nodeinfo.HandleNodeChanges = func(_ context.Context, _ client.Client, _ *hcov1.HyperConverged, _ logr.Logger) (bool, error) { return false, nil } - hco := commontestutils.NewHco() - cl2 := commontestutils.InitClient([]client.Object{hco, cpNode}) - r.Client = cl2 - nodeRequest := reconcile.Request{Then update line 504 to use
clinstead ofcl2.🤖 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 `@controllers/nodes/nodes_controller_test.go` around lines 476 - 491, The test creates an unused client variable cl with commontestutils.InitClient then reassigns r.Client = cl2, which is confusing; fix by removing the unused cl and either initialize only cl2 and set r.Client to that, or stop creating cl2 and set r.Client to the original cl and update the later Reconcile call to use that client. Specifically, update the setup around ReconcileNodeCounter, the commontestutils.InitClient calls (cl and cl2), and the assignment r.Client so only one client instance (referencing InitClient, NewHco, and the ReconcileNodeCounter struct) is created and used.
🤖 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 `@controllers/nodes/nodes_controller_test.go`:
- Around line 510-550: The test "Should not patch node already having the new
label and no old label" only checks labels but not whether a Patch was executed;
before calling r.Reconcile take the existing node from the fake client (cl) and
record its ResourceVersion (e.g., origRV := workerNode.ResourceVersion or fetch
into origNode), then call r.Reconcile and refetch updatedNode and assert
updatedNode.ResourceVersion == origRV (and keep the existing label assertions).
This verifies no patch occurred; alternatively wrap or replace cl with an
interceptor that counts Patch calls and assert the count is zero, referencing
the ReconcileNodeCounter r, the client variable cl, and the r.Reconcile call.
---
Nitpick comments:
In `@controllers/nodes/nodes_controller_test.go`:
- Around line 476-491: The test creates an unused client variable cl with
commontestutils.InitClient then reassigns r.Client = cl2, which is confusing;
fix by removing the unused cl and either initialize only cl2 and set r.Client to
that, or stop creating cl2 and set r.Client to the original cl and update the
later Reconcile call to use that client. Specifically, update the setup around
ReconcileNodeCounter, the commontestutils.InitClient calls (cl and cl2), and the
assignment r.Client so only one client instance (referencing InitClient, NewHco,
and the ReconcileNodeCounter struct) is created and used.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf84c4ab-d846-43d6-af42-f284152950b9
📒 Files selected for processing (4)
controllers/nodes/nodes_controller.gocontrollers/nodes/nodes_controller_test.gopkg/internal/nodeinfo/high_availability.gopkg/nodeinfo/nodeinfo.go
orenc1
force-pushed
the
hcp_replace_cp_label
branch
from
6c9e0cf to
1920db8
Compare
Coverage Report for CI Build 27349872515Coverage increased (+0.04%) to 80.347%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
hco-e2e-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-operator-sdk-sno-aws 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 kubernetes-sigs/prow repository. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure, ci/prow/hco-e2e-upgrade-operator-sdk-aws, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure 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 kubernetes-sigs/prow repository. |
| } | ||
|
|
||
| // Upgrade path: remove the old kubernetes control-plane label if it was set by HCO | ||
| if val, exists := node.Labels[nodeinfo.LabelNodeRoleControlPlane]; exists && val == hypershiftLabelValue { |
There was a problem hiding this comment.
It's enough to check the value. if the key does not exist, it will return an empty string, which is also != hypershiftLabelValue.; e.g.
| if val, exists := node.Labels[nodeinfo.LabelNodeRoleControlPlane]; exists && val == hypershiftLabelValue { | |
| if node.Labels[nodeinfo.LabelNodeRoleControlPlane] == hypershiftLabelValue { |
There was a problem hiding this comment.
right, the exists check is redundant in this case. fixed. thanks.
orenc1
force-pushed
the
hcp_replace_cp_label
branch
from
1920db8 to
e2322f3
Compare
…ode-role.kubevirt.io/control-plane for HCP Clusters Use a kubevirt-specific label for HCP worker nodes instead of the standard Kubernetes control-plane label, in order to prevent misidentification of workers as control-plane nodes. Handle upgrade by removing the old HCO-set label when the new one is applied. Signed-off-by: Oren Cohen <ocohen@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
orenc1
force-pushed
the
hcp_replace_cp_label
branch
from
e2322f3 to
98a8b86
Compare
|
|
@orenc1: 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. |
|
hco-e2e-upgrade-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-upgrade-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
|
hco-e2e-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
5 participants
Use a kubevirt-specific label for HCP worker nodes instead of the standard Kubernetes control-plane label, in order to prevent misidentification of workers as control-plane nodes. Handle upgrade by removing the old HCO-set label when the new one is applied.
the new label of
node-role.kubevirt.io/control-planeforvirt-operatorplacement has been introduced at kubevirt/kubevirt#17763What this PR does / why we need it:
Reviewer Checklist
Jira Ticket:
Release note: