chore(controllers): Do not run controlplane specific controllers on workers#1738
Conversation
fd02061 to
705a40d
Compare
705a40d to
531079c
Compare
|
@HomayoonAlimohammadi should we really backport this? I understand that this is not really a bugfix but just a cleanup. |
|
@HomayoonAlimohammadi let's bring this PR over the finish line, rebase and merge it. |
531079c to
1a19f8e
Compare
|
@bschimke95 I've rebased on top of main (had to account for the new DNSRebalancer controller). |
…stinct structs for clarity Signed-off-by: Homayoon (Hue) Alimohammadi <homayoon.alimohammadi@canonical.com>
1a19f8e to
5491a5e
Compare
There was a problem hiding this comment.
Pull request overview
This PR prevents control plane specific controllers (upgrade, CSR signing, DNS rebalancer) from running on worker nodes by adding an early return in the coordinator's Run method when the node is a worker. The coordinator is refactored to use structured option types for better organization of controller configuration.
Changes:
- Added worker node detection in the coordinator to skip controller setup on workers
- Refactored controller options into dedicated structs (
UpgradeControllerOptions,CSRSigningControllerOptions,DNSRebalancerControllerOptions) - Moved log statement for node failure domain reconciliation inside the control plane check in node_label controller
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/k8s/pkg/k8sd/controllers/coordinator.go | Adds worker detection and structured controller option types; skips controller setup on workers |
| src/k8s/pkg/k8sd/app/app.go | Updates coordinator initialization to use new structured option types |
| src/k8s/pkg/k8sd/app/hooks_start.go | Adds nil check for controllerCoordinator before starting |
| src/k8s/pkg/k8sd/controllers/node_label.go | Moves log message inside control plane check to avoid logging on workers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bschimke95
left a comment
There was a problem hiding this comment.
LGTM, if CI is happy.
Signed-off-by: Homayoon (Hue) Alimohammadi <homayoon.alimohammadi@canonical.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.32
git worktree add -d .worktree/backport-1738-to-release-1.32 origin/release-1.32
cd .worktree/backport-1738-to-release-1.32
git switch --create backport-1738-to-release-1.32
git cherry-pick -x 5491a5e28b96eca96bb319f8721cba443a24d06b 863546278795e17a279c81e7f1479d68ee752dd1 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.33
git worktree add -d .worktree/backport-1738-to-release-1.33 origin/release-1.33
cd .worktree/backport-1738-to-release-1.33
git switch --create backport-1738-to-release-1.33
git cherry-pick -x 5491a5e28b96eca96bb319f8721cba443a24d06b 863546278795e17a279c81e7f1479d68ee752dd1 |
|
Doesn't need backporting. The labels were added by mistake a long time ago. Luckily the backport PRs failed to get created. |
…orkers (canonical#1738) Signed-off-by: Homayoon (Hue) Alimohammadi <homayoon.alimohammadi@canonical.com>
…stinct structs for clarity Copied over from canonical/k8s-snap#1738 since it got merged after k8sd was moved out of the k8s-snap.
…stinct structs for clarity (#8) Copied over from canonical/k8s-snap#1738 since it got merged after k8sd was moved out of the k8s-snap.
…stinct structs for clarity (#8) Copied over from canonical/k8s-snap#1738 since it got merged after k8sd was moved out of the k8s-snap.
Overview
This PR prevents control plane specific controllers from running on workers. The coordinator is refactored a bit to better separate controller options as well.