🐛 KCP: remove etcd member in pre-terminate hook #11137

sbueringer · 2024-09-05T06:22:34Z

What this PR does / why we need it:

Note, this fix required the introduction of a pre-terminate hook that is automatically added and managed by the KCP controller for KCP control plane Machines. If your control plane Machines are using Kubernetes 1.31, KCP will make sure that its pre-terminate hook is run last. This is done to ensure that the terminating Node has a working kubelet / Node
while other pre-terminate hooks are executed.

More details about the issue can be found in Drain not being performed for KCP machines with K8s v1.31.x .

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

sbueringer · 2024-09-05T06:22:57Z

/test ?

k8s-ci-robot · 2024-09-05T06:23:00Z

@sbueringer: The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-blocking-main
/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-blocking-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test ?

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.

sbueringer · 2024-09-05T06:23:37Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

sbueringer · 2024-09-05T07:19:19Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

sbueringer · 2024-09-05T09:23:23Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

sbueringer · 2024-09-05T09:37:22Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go

util/collections/machine_collection.go

controlplane/kubeadm/internal/controllers/remediation.go

fabriziopandini · 2024-09-05T10:02:20Z

controlplane/kubeadm/internal/controllers/controller.go

+	// Return early if there are other pre-terminate hooks for the Machine.
+	// The KCP pre-terminate hook should be the one executed last, so that kubelet
+	// is still working while other pre-terminate hooks are run.
+	if machineHasOtherPreTerminateHooks(deletingMachine) {


I think we should somehow document that KCP controlled machines are adding a hook that expects to be always run as last (to ensure kubelet is running).

This should go in the PR description first but probably in a few other places too (the latter could be part of this PR or a quick follow-up, up to you); Probably this should go:

At the end of the go comment for PreTerminateDeleteHookAnnotation

1.9 upgrade instruction

release notes for the patch release

(and we should bring this up at the office hours)

Sounds like a follow-up to me

Maybe you can take that one. Probably easier because you know better where you want to add it :)

Opened a follow-up PR (#11153) + added it to the agenda

controlplane/kubeadm/internal/controllers/controller.go

util/collections/machine_collection.go

chrischdi · 2024-09-05T11:24:02Z

/test pull-cluster-api-e2e-main

Failed on old mgmt version (flake not affected by this PR)

chrischdi · 2024-09-05T11:30:43Z

/test pull-cluster-api-e2e-mink8s-main
unrelated self-hosted flake where machine pool did not upgrade, the new machine pool machine failed because CAPD was not able to preload an image:

DockerMachine":{"name":"worker-8w8ht2","namespace":"self-hosted-9670wf"},"namespace":"self-hosted-9670wf","name":"worker-8w8ht2","reconcileID":"a593a27e-a1cd-4819-83ff-8acbc1af8d7d","err":"failed to pre-load images into the DockerMachine: failed to load image \"gcr.io/k8s-staging-cluster-api/cluster-api-controller-amd64:dev\": error creating container exec: Error response from daemon: container 7254f307428922c75bd758f33c04b8092ea8e593bbf4582f4bd72b2947b4d2aa is not running

chrischdi · 2024-09-05T11:31:17Z

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

sbueringer · 2024-09-05T13:41:17Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

controlplane/kubeadm/internal/controllers/controller.go

sbueringer · 2024-09-05T13:53:02Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

sbueringer · 2024-09-05T14:07:37Z

/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-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-31-1-32-main

fabriziopandini · 2024-09-05T14:11:08Z

Great work!
/lgtm

k8s-ci-robot · 2024-09-05T14:11:15Z

LGTM label has been added.

Git tree hash: 7846e7efa4c01e76b9d2ac602660759c05efbe46

chrischdi · 2024-09-05T14:35:52Z

/approve

k8s-ci-robot · 2024-09-05T14:35:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [chrischdi]

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

chrischdi · 2024-09-05T14:39:37Z

/hold

on your convenience

sbueringer · 2024-09-05T15:14:54Z

/hold cancel

All tests green

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Sep 5, 2024

k8s-ci-robot requested review from g-gaston and jackfrancis September 5, 2024 06:22

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2024

sbueringer force-pushed the pr-kcp-pre-terminate branch from 7b65f1a to 25b17b9 Compare September 5, 2024 07:19

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2024

This was referenced Sep 5, 2024

🌱 Improve node drain e2e test #11127

Merged

[release-1.8] 🐛 KCP: remove etcd member in pre-terminate hook #11136

Merged

sbueringer force-pushed the pr-kcp-pre-terminate branch from 25b17b9 to 5610633 Compare September 5, 2024 09:23

KCP: remove etcd member in pre-terminate hook

d5f0d74

sbueringer force-pushed the pr-kcp-pre-terminate branch from 5610633 to d5f0d74 Compare September 5, 2024 09:37

fabriziopandini added the area/provider/control-plane-kubeadm Issues or PRs related to KCP label Sep 5, 2024

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Sep 5, 2024

fabriziopandini reviewed Sep 5, 2024

View reviewed changes

chrischdi reviewed Sep 5, 2024

View reviewed changes

sbueringer added 2 commits September 5, 2024 14:17

fix review findings & add unit tests

2740c26

Add unit tests for reconcilePreTerminateHook

e1ef276

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 5, 2024

sbueringer changed the title ~~[WIP] 🐛 KCP: remove etcd member in pre-terminate hook~~ 🐛 KCP: remove etcd member in pre-terminate hook Sep 5, 2024

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 Sep 5, 2024

neolit123 reviewed Sep 5, 2024

View reviewed changes

controlplane/kubeadm/internal/controllers/controller.go Outdated Show resolved Hide resolved

controlplane/kubeadm/internal/controllers/controller.go Show resolved Hide resolved

Add unit tests for reconcilePreTerminateHook - fixups

4a3af97

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 5, 2024

fixup

8ed4065

k8s-ci-robot assigned fabriziopandini Sep 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2024

k8s-ci-robot merged commit 518fce7 into kubernetes-sigs:main Sep 5, 2024
27 checks passed

k8s-ci-robot added this to the v1.9 milestone Sep 5, 2024

sbueringer deleted the pr-kcp-pre-terminate branch September 5, 2024 15:16

This was referenced Sep 6, 2024

🌱 KCP: remove code handling Kubernetes <= v1.21 #11146

Merged

📖 Add more documentation about the KCP pre-terminate hook #11153

Merged

zioc mentioned this pull request Sep 9, 2024

Avoid deleting etcd member before node is drained rancher/cluster-api-provider-rke2#434

Closed

sbueringer mentioned this pull request Sep 10, 2024

🐛 Make KCP pre-terminate hook more robust #11161

Merged

Sunnatillo mentioned this pull request Nov 19, 2024

Tasks for v1.9 release cycle #11092

Closed

54 tasks

fabriziopandini mentioned this pull request Nov 28, 2024

kube-apiserver does not gracefully terminate when a control plane machine is scaled down #10934

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 KCP: remove etcd member in pre-terminate hook #11137

🐛 KCP: remove etcd member in pre-terminate hook #11137

sbueringer commented Sep 5, 2024 •

edited

Loading

sbueringer commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

fabriziopandini Sep 5, 2024 •

edited by sbueringer

Loading

sbueringer Sep 5, 2024

sbueringer Sep 5, 2024

sbueringer Sep 9, 2024

chrischdi commented Sep 5, 2024

chrischdi commented Sep 5, 2024

chrischdi commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

fabriziopandini commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

chrischdi commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

chrischdi commented Sep 5, 2024

sbueringer commented Sep 5, 2024

🐛 KCP: remove etcd member in pre-terminate hook #11137

🐛 KCP: remove etcd member in pre-terminate hook #11137

Conversation

sbueringer commented Sep 5, 2024 • edited Loading

sbueringer commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

fabriziopandini Sep 5, 2024 • edited by sbueringer Loading

Choose a reason for hiding this comment

sbueringer Sep 5, 2024

Choose a reason for hiding this comment

sbueringer Sep 5, 2024

Choose a reason for hiding this comment

sbueringer Sep 9, 2024

Choose a reason for hiding this comment

chrischdi commented Sep 5, 2024

chrischdi commented Sep 5, 2024

chrischdi commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024

fabriziopandini commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

chrischdi commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

chrischdi commented Sep 5, 2024

sbueringer commented Sep 5, 2024

sbueringer commented Sep 5, 2024 •

edited

Loading

fabriziopandini Sep 5, 2024 •

edited by sbueringer

Loading