Skip to content

🌱 KCP should handle missing control plane label#13466

Merged
k8s-ci-robot merged 3 commits intokubernetes-sigs:mainfrom
fabriziopandini:KCP-tolerate-missing-label
Mar 18, 2026
Merged

🌱 KCP should handle missing control plane label#13466
k8s-ci-robot merged 3 commits intokubernetes-sigs:mainfrom
fabriziopandini:KCP-tolerate-missing-label

Conversation

@fabriziopandini
Copy link
Copy Markdown
Member

@fabriziopandini fabriziopandini commented Mar 16, 2026 •
edited by sbueringer
Loading

What this PR does / why we need it:
With this PR tolerates missing control plane label and also surfaces etcd members stuck in learner mode

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Somewhat related to #13221

@fabriziopandini fabriziopandini added the area/provider/control-plane-kubeadm Issues or PRs related to KCP label Mar 16, 2026
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2026
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 16, 2026
@fabriziopandini
Copy link
Copy Markdown
Member Author

fabriziopandini commented Mar 16, 2026

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-latestk8s-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-main-gke
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-35-1-36-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main

@fabriziopandini fabriziopandini force-pushed the KCP-tolerate-missing-label branch from d3303bc to dd3dbb2 Compare March 17, 2026 12:33
@fabriziopandini fabriziopandini changed the title [WIP] 🌱 KCP should handle missing control plane label 🌱 KCP should handle missing control plane label Mar 17, 2026
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2026
@sbueringer
Copy link
Copy Markdown
Member

sbueringer commented Mar 17, 2026

/test pull-cluster-api-e2e-main-gke

sbueringer
sbueringer reviewed Mar 17, 2026
View reviewed changes
Comment thread api/controlplane/kubeadm/v1beta2/kubeadm_control_plane_types.go Outdated
Comment thread api/controlplane/kubeadm/v1beta2/kubeadm_control_plane_types.go Outdated
Comment thread api/controlplane/kubeadm/v1beta2/kubeadm_control_plane_types.go Outdated
Comment thread controlplane/kubeadm/internal/control_plane.go Outdated
Comment thread controlplane/kubeadm/internal/controllers/scale.go
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go Outdated
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions_test.go
Comment thread controlplane/kubeadm/internal/workload_cluster.go
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go Outdated
@fabriziopandini fabriziopandini added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 17, 2026
@fabriziopandini
Copy link
Copy Markdown
Member Author

fabriziopandini commented Mar 17, 2026

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-latestk8s-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-main-gke
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-35-1-36-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main

sbueringer
sbueringer reviewed Mar 18, 2026
View reviewed changes
Comment thread controlplane/kubeadm/internal/controllers/controller.go
Comment thread controlplane/kubeadm/internal/controllers/controller.go Outdated
Comment thread controlplane/kubeadm/internal/controllers/controller.go Outdated
Comment thread controlplane/kubeadm/internal/controllers/controller.go Outdated
Comment thread controlplane/kubeadm/internal/controllers/remediation.go Outdated
Comment thread controlplane/kubeadm/internal/controllers/scale.go Outdated
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go

// If it failed to get members, consider the check failed in case there is at least a machine already provisioned.
if members == nil {
if controlPlane.EtcdMembers == nil {
Copy link
Copy Markdown
Member

@sbueringer sbueringer Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Let's check for len instead of nil?

Copy link
Copy Markdown
Member Author

@fabriziopandini fabriziopandini Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not equivalent, because empty list is a valid case during startup

Copy link
Copy Markdown
Member

@sbueringer sbueringer Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a godoc that we are explicitly making a difference between nil and empty here. That's tricky to spot otherwise (side note: this is the only place where we are checking for nil, I'm wondering if we incorrectly check for len in other places)

Copy link
Copy Markdown
Member

@sbueringer sbueringer Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay there is a godoc above + I checked all len checks. Seems fine.

Would be maybe still good to make this more explicit instead of relying on this being nil. But it's not a regression in this PR so I don't want to block the PR.

Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go Outdated
Comment thread controlplane/kubeadm/internal/workload_cluster_conditions.go
@sbueringer
Copy link
Copy Markdown
Member

sbueringer commented Mar 18, 2026

/test pull-cluster-api-e2e-main-gke

@sbueringer
Copy link
Copy Markdown
Member

sbueringer commented Mar 18, 2026

Nice work, thx!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2026
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 18, 2026

LGTM label has been added.

DetailsGit tree hash: 357a07833d9dc90978763f04bf047cb9ba61eca7

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 18, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 18, 2026
@k8s-ci-robot k8s-ci-robot merged commit 89d30ad into kubernetes-sigs:main Mar 18, 2026
19 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Mar 18, 2026
@sbueringer sbueringer mentioned this pull request Mar 18, 2026
Open
@fabriziopandini fabriziopandini mentioned this pull request Mar 19, 2026
Merged
@fabriziopandini fabriziopandini deleted the KCP-tolerate-missing-label branch April 10, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbueringer sbueringer sbueringer left review comments

@richardcase richardcase Awaiting requested review from richardcase

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@sbueringer sbueringer

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/control-plane-kubeadm Issues or PRs related to KCP cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Milestone

v1.13

Development

Successfully merging this pull request may close these issues.

3 participants

@fabriziopandini @sbueringer @k8s-ci-robot