🐛 bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31 #10983

chrischdi · 2024-08-01T13:17:57Z

What this PR does / why we need it:

This adds a new script for useExperimentalRetryJoin for kubernetes >= v1.31 due to the following changes:

control-plane update-status phase got removed
new phases got added for the ControlPlaneKubeletLocalMode feature gate

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 #

chrischdi · 2024-08-01T13:21:38Z

/test help

k8s-ci-robot · 2024-08-01T13:21:41Z

@chrischdi: The specified target(s) for /test were not found.
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-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-30-1-31-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 help

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.

chrischdi · 2024-08-01T13:21:49Z

/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

neolit123 · 2024-08-01T14:08:46Z

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script-pre-k8s-1-31.sh

@@ -0,0 +1,137 @@
+#!/bin/bash
+# Copyright 2020 The Kubernetes Authors.


2024
(because it's a new file, even if we are just copying code to it)

github is misdisplaying it a bit. This is the old file, and just the same :-)

But thanks for the pointer, have to bump the year on the other file!

github is misdisplaying it a bit.

hm, not sure how so.

seems like bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script-pre-k8s-1-31.sh is a new file that has the old contents. it should be with the 2024 year. old file can have the new changes but be with the old year.

It's basically a git mv bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script-pre-k8s-1-31.sh 🤔

FWIW, it ends up as a new file in the tree, so the correct action is to apply 2024.
i don't think the license police will mind, so up to you.

There is a license police? :)

test/e2e/data/infrastructure-docker/main/clusterclass-quick-start.yaml

neolit123 · 2024-08-01T14:13:26Z

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

+try-or-die-command kubeadm join phase etcd-join
+# {{ end }}
+# Note: below phase is a no-op if ControlPlaneKubeletLocalMode feature gate is false.
+retry-command kubelet-wait-boostrap


IIRC, phase order was relevant to the FG enablement, do we need to consider it here?
(non trivial i would assume)

Phase order is correct here, this is the file for >= v1.31

there is a typo boostrap.

this is the 1.31 CLI output

preflight Run join pre-flight checks control-plane-prepare Prepare the machine for serving a control plane /download-certs Download certificates shared among control-plane nodes from the kubeadm-certs Secret /certs Generate the certificates for the new control plane components /kubeconfig Generate the kubeconfig for the new control plane components /control-plane Generate the manifests for the new control plane components kubelet-start Write kubelet settings, certificates and (re)start the kubelet control-plane-join Join a machine as a control plane instance /etcd Add a new local etcd member /mark-control-plane Mark a node as a control-plane wait-control-plane EXPERIMENTAL: Wait for the control plane to start

but these etcd-join and kubelet-wait-bootstrap phases are hidden for now:
https://github.com/kubernetes/kubernetes/blob/f8d5b2074c9b3a1629ef085b9e17a5370f393732/cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go#L93

https://github.com/kubernetes/kubernetes/blob/f8d5b2074c9b3a1629ef085b9e17a5370f393732/cmd/kubeadm/app/cmd/phases/join/kubelet.go#L103

so there has to be another iteration here. or you can use the fact that calling an unknown or hidden kubeadm join phase non-existent will return exit code 0, which is a bit of a mistake on the kubeadm side. just found it out by doing kubeadm join phase foo > /dev/null && echo ok and prepopulate these hidden phases in this PR.

hmm, looks like I even cannot run these hidden phases manually :-(

chrischdi · 2024-08-01T16:00:44Z

/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

chrischdi · 2024-08-01T16:01:46Z

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

+if [[ "${CONTROLPLANE_KUBELET_LOCAL_MODE}" == "true" ]]; then
+  # Run kubeadm join and skip all already executed phases
+  # kubeadm join relies on hidden phases when CONTROLPLANE_KUBELET_LOCAL_MODE. These cannot be run
+  # externally.


We can even think about not doing this if/else.

We would only loose that we don't retry the kubeadm join phase control-plane-join mark-control-plane.

chrischdi · 2024-08-01T16:03:27Z

/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

neolit123 · 2024-08-01T16:12:34Z

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

+  try-or-die-command kubeadm join phase control-plane-join etcd
+  retry-command kubeadm join phase control-plane-join mark-control-plane
+  # {{ end }}
+fi


NOTE: there is another phase at the end:

wait-control-plane EXPERIMENTAL: Wait for the control plane to start

as i listed here:
#10983 (comment)

https://github.com/kubernetes/kubernetes/blob/f8d5b2074c9b3a1629ef085b9e17a5370f393732/cmd/kubeadm/app/cmd/phases/join/waitcontrolplane.go#L63
but it comes from another alpha FG that can be skipped in the CAPI exp-join-retry IMO.

but i guess it depends on the timeline of exp-retry-join vs features.WaitForAllControlPlaneComponents graudation.
i would assume that features.WaitForAllControlPlaneComponents will graduate faster than the removal of exp-retry-join, so might be worth adding it here after line 148 without retries.

it's a no-op if features.WaitForAllControlPlaneComponents is disabled.

jep. I simplified it for now to just run kubeadm join for all leftover phases, so no retries on mark-control-plane, but with the Pro of running all hidden and potentially enabled and new phases.

neolit123

/approve

the change SGTM. like you pointed out we get it similar to the way it was at 80% so i think that's fine and give the feature is deprecated.

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

chrischdi · 2024-08-01T16:40:58Z

/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

chrischdi · 2024-08-01T16:50:42Z

/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

chrischdi · 2024-08-01T16:56:12Z

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

+
+# Run kubeadm join and skip all already executed phases.
+try-or-die-command kubeadm join --skip-phases preflight,control-plane-prepare,kubelet-start


We have two options here:

Current implementation

Just run kubeadm join without any retries at this point.
The disadvantage is that we do not run control-plane-join mark-control-plane with retries anymore.
The advantage here (and requirement when ControlPlaneKubeletLocalMode is enabled): we also take advantage of hidden and new phases to be run when they are enabled.

The alternative would be:

Keep the current implementation here for worker nodes.

For ControlPlane nodes: use the /etc/kubernetes/admin.conf to check if the ControlPlaneKubeletLocalMode is enabled

Only if it is not enabled: run the control-plane-join mark-control-plane with retries as before

I think the advantages of the current implementation are better then doing the alternative here.

i think given all recent versions of kubeadm in support by CAPI have integrated retries. breaking down the join in phases and/or retrying any of them is pointless.

so i'm +1 to make experimental-retry-join be just a regular single kubeadm cmd join too.

Agreed.
At this point we need to fix this script for 1.31 as a stop gap before removal of the entire experimentalRetryMachines, but it does not add any value on top of retry/timeout management embedded in recent versions of kubeadm

g-gaston

lgtm

This adds a new script for useExperimentalRetryJoin for kubernetes >= v1.31 due to the following changes: * control-plane update-status phase got removed * new phases got added for the ControlPlaneKubeletLocalMode feature gate

chrischdi

Rebased and squashed. Also tested it locally for:

CP + workers
experimentalretryjoin on and off
v1.30 and v1.31

neolit123

thanks for the updates.
after yesterday's discussion i understood that we want to still keep the external retries and phase breakdown when the retry feature is enabled.

so
/lgtm
but best to have another reviewer.
/hold

k8s-ci-robot · 2024-08-02T06:33:00Z

LGTM label has been added.

Git tree hash: 27adcc190a4324df413d24bb5d77e2e9c747f047

chrischdi · 2024-08-02T06:49:30Z

/retest

Github flake

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh

sbueringer · 2024-08-02T11:30:46Z

One question #10983 (comment) (but should be fine based on what you tested)

/lgtm

Deferring to @fabriziopandini
for final approval

Let's get this merged before the weekend

chrischdi · 2024-08-02T12:03:21Z

/cherry-pick release-1.8

k8s-infra-cherrypick-robot · 2024-08-02T12:03:23Z

@chrischdi: 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.

fabriziopandini · 2024-08-02T13:03:35Z

thanks @chrischdi for taking care of this fix

looking forward to when we can get finally get rid of useExperimentalRetryJoin

/lgtm
/approve

/hold cancel

k8s-ci-robot · 2024-08-02T13:03:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [fabriziopandini,neolit123]

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

k8s-infra-cherrypick-robot · 2024-08-02T13:38:37Z

@chrischdi: new pull request created: #11000

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.

chrischdi added the area/provider/bootstrap-kubeadm Issues or PRs related to CAPBK label Aug 1, 2024

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

k8s-ci-robot requested review from g-gaston and JoelSpeed August 1, 2024 13:18

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

chrischdi force-pushed the pr-v1-31-fix-experimental-retry-join branch from 18bb31c to 755db50 Compare August 1, 2024 13:18

neolit123 reviewed Aug 1, 2024

View reviewed changes

chrischdi commented Aug 1, 2024

View reviewed changes

neolit123 reviewed Aug 1, 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 Aug 1, 2024

neolit123 reviewed Aug 1, 2024

View reviewed changes

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh Outdated Show resolved Hide resolved

chrischdi commented Aug 1, 2024

View reviewed changes

chrischdi mentioned this pull request Aug 1, 2024

Remove experimentalRetryJoin #6942

Open

g-gaston reviewed Aug 1, 2024

View reviewed changes

chrischdi force-pushed the pr-v1-31-fix-experimental-retry-join branch from 52ec37e to 6f1543f Compare August 2, 2024 06:13

chrischdi changed the title ~~🐛 [WIP] bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31~~ 🐛 bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31 Aug 2, 2024

chrischdi commented Aug 2, 2024

View reviewed changes

neolit123 reviewed Aug 2, 2024

View reviewed changes

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

k8s-ci-robot assigned neolit123 Aug 2, 2024

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

sbueringer reviewed Aug 2, 2024

View reviewed changes

bootstrap/kubeadm/internal/cloudinit/kubeadm-bootstrap-script.sh Show resolved Hide resolved

k8s-ci-robot assigned sbueringer Aug 2, 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 Aug 2, 2024

k8s-ci-robot assigned fabriziopandini Aug 2, 2024

k8s-ci-robot merged commit a96a990 into kubernetes-sigs:main Aug 2, 2024
25 checks passed

k8s-ci-robot added this to the v1.9 milestone Aug 2, 2024

k8s-infra-cherrypick-robot mentioned this pull request Aug 2, 2024

[release-1.8] 🐛 bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31 #11000

Merged

		@@ -0,0 +1,137 @@
		#!/bin/bash
		# Copyright 2020 The Kubernetes Authors.


		# Run kubeadm join and skip all already executed phases.
		try-or-die-command kubeadm join --skip-phases preflight,control-plane-prepare,kubelet-start

🐛 bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31 #10983

🐛 bootstrap: fix useExperimentalRetryJoin for kubernetes v1.31 #10983

Conversation

chrischdi commented Aug 1, 2024

chrischdi commented Aug 1, 2024

k8s-ci-robot commented Aug 1, 2024

chrischdi commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrischdi commented Aug 1, 2024

Choose a reason for hiding this comment

chrischdi commented Aug 1, 2024

neolit123 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

neolit123 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

chrischdi commented Aug 1, 2024

chrischdi commented Aug 1, 2024

Choose a reason for hiding this comment

neolit123 Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-gaston left a comment

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

neolit123 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 2, 2024

chrischdi commented Aug 2, 2024

sbueringer commented Aug 2, 2024 • edited Loading

chrischdi commented Aug 2, 2024

k8s-infra-cherrypick-robot commented Aug 2, 2024

fabriziopandini commented Aug 2, 2024

k8s-ci-robot commented Aug 2, 2024

k8s-infra-cherrypick-robot commented Aug 2, 2024

neolit123 Aug 1, 2024 •

edited

Loading

neolit123 Aug 1, 2024 •

edited

Loading

neolit123 Aug 1, 2024 •

edited

Loading

neolit123 Aug 1, 2024 •

edited

Loading

sbueringer commented Aug 2, 2024 •

edited

Loading