🐛 Make KCP pre-terminate hook more robust #11161

sbueringer · 2024-09-10T09:05:16Z

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
Follow-up to #11137
Additional context: https://kubernetes.slack.com/archives/C8TSNPY4T/p1725952675583209

This PR ensures that KCP always removes the pre-terminate hook from Machines if KCP is deleted.

Before this PR this was only done if a Machine didn't have a deletionTimestamp.

There are edge cases where a Machine could have a deletionTimestamp before (or at the same time) as KCP:

All objects of the cluster are deleted at the same time (this is not technically supported, but some folks are running into this case)
The Machine already had the deletionTimestamp (e.g. through a scale down or remediation) when the Cluster/KCP object were deleted.

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 #

sbueringer · 2024-09-10T09:06:41Z

/cherry-pick release-1.8

k8s-infra-cherrypick-robot · 2024-09-10T09:06:44Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.8 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.8

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-10T09:06:45Z

/test ?

k8s-ci-robot · 2024-09-10T09:06:48Z

@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-10T09:07:53Z

/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-10T09:10:16Z

/assign @chrischdi @fabriziopandini @neolit123

(fyi, we have a patch release coming up this evening, we should get this fix in)

neolit123 · 2024-09-10T09:17:16Z

controlplane/kubeadm/internal/controllers/controller.go

@@ -595,11 +600,19 @@ func (r *KubeadmControlPlaneReconciler) reconcileDelete(ctx context.Context, con
 			"Failed to delete control plane Machines for cluster %s control plane: %v", klog.KObj(controlPlane.Cluster), err)
 		return ctrl.Result{}, err
 	}
+
+	log.Info("Waiting for control plane Machines to go away")


i'd replace 'to go away' with something more formal e.g. 'to be deleted'

But what does "to be deleted" mean? Setting the deeltionTimestamp? It's already on there :)

IMO, 'to be deleted' != 'to have deletionTimestamp applied'.
delete is the HTTP method and kubectl call, etc.

perhaps 'removed' will make it clearer.

I'm not sure. The most precise way to say this is something like "Waiting for control plane Machines to not exist anymore" (+ "in the apiserver"? but maybe the apiserver part would sound a bit strange :))

"Waiting for control plane Machines to not exist anymore"

+1, or anything formal.

neolit123 · 2024-09-10T09:24:44Z

/approve

Signed-off-by: Stefan Büringer [email protected]

sbueringer · 2024-09-10T10:24:49Z

/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-10T10:25:23Z

Thanks for the quick fix!
/lgtm
/approve

pending tests

k8s-ci-robot · 2024-09-10T10:25:28Z

LGTM label has been added.

Git tree hash: 8cfd0ad0f5bd121695da96d4069ce0e54c5d10f5

chrischdi

/approve

k8s-ci-robot · 2024-09-10T10:30:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, fabriziopandini, neolit123

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,fabriziopandini,neolit123]

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

sbueringer · 2024-09-10T11:18:15Z

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

(all 3 were about to fail)

sbueringer · 2024-09-10T12:26:33Z

Please dont' retest. I'll assess flakes individually and consider overrides

sbueringer · 2024-09-10T12:38:59Z

Flake assessment:

pull-cluster-api-e2e-latestk8s-main:
- this is another case of Timed out after 180.001s. waiting for cluster deletion timed out #11162
- MP stuck in deletion: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/11161/pull-cluster-api-e2e-latestk8s-main/1833465010609721344/artifacts/clusters-afterDeletionTimedOut/self-hosted-4z83gz/resources/self-hosted-8vhlow/MachinePool/self-hosted-4z83gz-mp-0-h46pb.yaml
pull-cluster-api-e2e-mink8s-main
- this is another case of Timed out after 180.001s. waiting for cluster deletion timed out #11162
- MP stuck in deletion: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/11161/pull-cluster-api-e2e-mink8s-main/1833465010479697920/artifacts/clusters-afterDeletionTimedOut/self-hosted-v05vle/resources/self-hosted-2i1e85/MachinePool/self-hosted-v05vle-mp-0-2rbh6.yaml
pull-cluster-api-e2e-main
- Job: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/11161/pull-cluster-api-e2e-main/1833465010555195392
- this is another case of what we are discussing here: flakes in clusterctl upgrade tests #11133

I'm going to retest all 3 of them to get additional data, but I would merge if the same reoccurs. Basically all 3 tests are running the same test cases (just with different Kubernetes versions). So I think we already have enough data for merge.

/retest

fabriziopandini · 2024-09-10T12:55:58Z

double checked

pull-cluster-api-e2e-latestk8s-main
"msg":"Cluster still has descendants - need to requeue",, descendants":"Machine pools: self-hosted-4z83gz-mp-0-h46pb" ...

(so it seems a well know flake)

sbueringer · 2024-09-10T13:42:03Z

Unbelievable - all green

k8s-infra-cherrypick-robot · 2024-09-10T13:42:51Z

@sbueringer: new pull request created: #11165

In response to this:

/cherry-pick release-1.8

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.

Danil-Grigorev · 2024-09-10T14:58:25Z

/cherry-pick release-1.7

For evaluation, seeing similar issue in 1.7.2 version

k8s-infra-cherrypick-robot · 2024-09-10T14:59:02Z

@Danil-Grigorev: #11161 failed to apply on top of branch "release-1.7":

Applying: Make KCP pre-terminate hook more robust
Using index info to reconstruct a base tree...
M	controlplane/kubeadm/internal/controllers/controller.go
M	controlplane/kubeadm/internal/controllers/controller_test.go
M	internal/controllers/machine/machine_controller.go
Falling back to patching base and 3-way merge...
Auto-merging internal/controllers/machine/machine_controller.go
Auto-merging controlplane/kubeadm/internal/controllers/controller_test.go
Auto-merging controlplane/kubeadm/internal/controllers/controller.go
CONFLICT (content): Merge conflict in controlplane/kubeadm/internal/controllers/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Make KCP pre-terminate hook more robust
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.7

For evaluation, seeing similar issue in 1.7.2 version

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-10T15:20:33Z

@Danil-Grigorev If you are seeing a similar effect, it should not be the same root cause. The change that triggered this issue and that we are fixing here was never backported to release-1.7

sbueringer · 2024-09-10T15:22:25Z

This is the chain of PRs and fixes that got us here. We needed it for 1.31 support, so it was never backported to 1.7:

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 10, 2024

k8s-ci-robot requested review from richardcase and vincepri September 10, 2024 09:05

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 10, 2024

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

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

k8s-ci-robot assigned chrischdi, fabriziopandini and neolit123 Sep 10, 2024

neolit123 reviewed Sep 10, 2024

View reviewed changes

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

Make KCP pre-terminate hook more robust

4c83455

Signed-off-by: Stefan Büringer [email protected]

sbueringer force-pushed the pr-make-kcp-pre-terminate-more-robust branch from db292e7 to 4c83455 Compare September 10, 2024 10:23

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

chrischdi reviewed Sep 10, 2024

View reviewed changes

sbueringer mentioned this pull request Sep 10, 2024

[release-1.8] 🐛 Make KCP pre-terminate hook more robust #11164

Merged

k8s-ci-robot merged commit 3d22daf into kubernetes-sigs:main Sep 10, 2024
25 checks passed

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

sbueringer deleted the pr-make-kcp-pre-terminate-more-robust branch September 10, 2024 13:42

k8s-infra-cherrypick-robot mentioned this pull request Sep 10, 2024

[release-1.8] 🐛 Make KCP pre-terminate hook more robust #11165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Make KCP pre-terminate hook more robust #11161

🐛 Make KCP pre-terminate hook more robust #11161

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-ci-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

neolit123 Sep 10, 2024

sbueringer Sep 10, 2024

neolit123 Sep 10, 2024 •

edited

Loading

sbueringer Sep 10, 2024 •

edited

Loading

sbueringer Sep 10, 2024

neolit123 Sep 10, 2024

neolit123 commented Sep 10, 2024

sbueringer commented Sep 10, 2024

fabriziopandini commented Sep 10, 2024

k8s-ci-robot commented Sep 10, 2024

chrischdi left a comment

k8s-ci-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

fabriziopandini commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

Danil-Grigorev commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024 •

edited

Loading

sbueringer commented Sep 10, 2024

🐛 Make KCP pre-terminate hook more robust #11161

🐛 Make KCP pre-terminate hook more robust #11161

Conversation

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-ci-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

neolit123 Sep 10, 2024

Choose a reason for hiding this comment

sbueringer Sep 10, 2024

Choose a reason for hiding this comment

neolit123 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer Sep 10, 2024

Choose a reason for hiding this comment

neolit123 Sep 10, 2024

Choose a reason for hiding this comment

neolit123 commented Sep 10, 2024

sbueringer commented Sep 10, 2024

fabriziopandini commented Sep 10, 2024

k8s-ci-robot commented Sep 10, 2024

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

sbueringer commented Sep 10, 2024

fabriziopandini commented Sep 10, 2024

sbueringer commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

Danil-Grigorev commented Sep 10, 2024

k8s-infra-cherrypick-robot commented Sep 10, 2024

sbueringer commented Sep 10, 2024 • edited Loading

sbueringer commented Sep 10, 2024

neolit123 Sep 10, 2024 •

edited

Loading

sbueringer Sep 10, 2024 •

edited

Loading

sbueringer commented Sep 10, 2024 •

edited

Loading