Fix multi-node cluster not working after restarting docker #2775

tnqn · 2022-05-17T16:29:31Z

In a multi-node cluster with single controlplane node, if the
controlplane node's IP changes, kube-controller-manager and
kube-scheduler would fail to connect kube-apiserver.

This patch fixes it by replacing the server address in kubeconfig files
of kube-controller-manager and kube-scheduler with the new IP and
re-generate a certificate with the new IP for kube-apiserver.

Signed-off-by: Quan Tian [email protected]

Fixes #2759

k8s-ci-robot · 2022-05-17T16:29:39Z

Hi @tnqn. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

neolit123 · 2022-05-17T16:34:41Z

images/base/files/usr/local/bin/entrypoint

-      sed -i "s#${old_ipv4}#${curr_ipv4}#" /var/lib/kubelet/kubeadm-flags.env || true
+      if [[ "${old_ipv4}" != "${curr_ipv4}" ]]; then
+        # kubernetes manifests are only present on control-plane nodes
+        sed -i "s#${old_ipv4}#${curr_ipv4}#" /etc/kubernetes/manifests/*.yaml || true


instead of sed-ing can we kubeadm init phase control-plane {foo} --config ... to generate a new manifest for component {foo}. by running it will try to get the node IP, and hopefully that will be the right one?

then again, sed-ing seems straightforward.

It could work but it would be a change irrelevant to the PR's purpose as the script has been using sed from the beginning and kubeadm.conf, kubeadm-flags.env will still need sed. It may require a few more lines to check if this is control-plane node before invoking kubeadm init phase control-plane.
Do we want to make the change in this PR?

Sed means we retain any user changes performed manually out of band except the targeted edit we're making.

thanks for commenting, then I suppose we could keep sed

we should however, do some fixes so we replace the actual literals instead of letting the IP address be mis-interpreted as a regex, though that bug is not new to this PR.

rebased the PR (because it's based on out of date version of entrypoint script) and then fixed this, we regex escape the ip addresses now (I think ...)

images/base/files/usr/local/bin/entrypoint

aojea · 2022-05-19T09:54:39Z

/ok-to-test

aojea · 2022-05-19T09:54:39Z

/ok-to-test

BenTheElder · 2022-05-19T15:38:31Z

Sorry, I am going to finish shipping a release to fix non-systemd hosts after the v0.13 changes before we ship any further potentially problematic changes #2767

BenTheElder · 2022-05-19T21:03:06Z

This is the first thing I'd like to see in v0.15.0, assigning to see this through next.

In a multi-node cluster with single controlplane node, if the controlplane node's IP changes, kube-controller-manager and kube-scheduler would fail to connect kube-apiserver. This patch fixes it by replacing the server address in kubeconfig files of kube-controller-manager and kube-scheduler with the new IP and re-generate a certificate with the new IP for kube-apiserver. Signed-off-by: Quan Tian <[email protected]>

images/base/files/usr/local/bin/entrypoint

BenTheElder · 2022-05-20T05:20:52Z

/hold
I have not tested building a node image and then rebooting a multi-node cluster with the latest changes, even though they should be "more correct" we should test that :-)

images/base/files/usr/local/bin/entrypoint

BenTheElder · 2022-05-20T05:31:16Z

dropped the base image bump while we iterate more, @tnqn handing back to you.

very excited about this PR :-)

BenTheElder · 2022-05-20T05:39:47Z

images/base/files/usr/local/bin/entrypoint

-      sed -i "s#${old_ipv4}#${curr_ipv4}#" /var/lib/kubelet/kubeadm-flags.env || true
+      if [[ "${old_ipv4}" != "${curr_ipv4}" ]]; then
+        # kubernetes manifests are only present on control-plane nodes
+        sed_ipv4_command="s#$(regex_escape_ip "${old_ipv4}")#${curr_ipv4}#g"


note that when I did the regex fix I also:

stopped computing this string repeatedly

changed it to a global replace (!) instead of just replacing the first instance

the existing sed pattern carried forward from the existing script was not good :(
it should be reasonable now.

Thanks!
Reminded by it, I also added word boundary to sed pattern to avoid mismatch like

# sed "s/1\.1\.1\.2/1.1.1.3/g" <<< "1.1.1.22" 1.1.1.32

tnqn · 2022-05-20T12:22:40Z

images/base/files/usr/local/bin/entrypoint

@@ -29,7 +29,12 @@ fi

 grep_allow_nomatch() {
  # grep exits 0 on match, 1 on no match, 2 on error
-  grep "$@" || [[ $? == 1 ]]
+  grep "$@"|| [[ $? == 1 ]]


I guess this is an unintended change and reverted it in 3rd patch, please let me know if this is on purpose

tnqn · 2022-05-20T12:24:33Z

I have tested the change by restarting IPv4, IPv6 and dualstack clusters and they worked as expected after controlplane IP changed.

aojea · 2022-05-20T14:29:55Z

/retest

error: no builder "kind-builder" found

images/base/files/usr/local/bin/entrypoint

1. Check for files and error if replacing in them fails 2. Add word boundary to sed pattern to avoid mismatch 3. Regenerate certificate only once when both IPv4 and IPv6 addresses change Signed-off-by: Quan Tian <[email protected]>

BenTheElder · 2022-05-24T00:58:25Z

Tentatively unavailable tomorrow, top of my list to revisit when I am back.

Still the first thing I intend to merge in v0.15.0, I think this is about good to go, but I want to double check everything again 😅

Thanks again for working on this 🙏

BenTheElder · 2022-05-25T23:05:28Z

it looks good, pushing images so we can soak all the CI

BenTheElder

/lgtm
/approve
/hold cancel
thank you!!! 😄

k8s-ci-robot · 2022-05-25T23:06:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, tnqn

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~~ [BenTheElder]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2022

k8s-ci-robot requested review from amwat and munnerz May 17, 2022 16:29

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 17, 2022

tnqn mentioned this pull request May 17, 2022

Support docker restart for clusters with multiple nodes (single controlplane node) #2759

Closed

neolit123 reviewed May 17, 2022

View reviewed changes

BenTheElder reviewed May 17, 2022

View reviewed changes

images/base/files/usr/local/bin/entrypoint Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 19, 2022

tnqn force-pushed the fix-docker-restart-kubeadm branch from c63323d to f0b470b Compare May 19, 2022 10:06

BenTheElder added this to the v0.15.0 milestone May 19, 2022

BenTheElder self-assigned this May 19, 2022

BenTheElder force-pushed the fix-docker-restart-kubeadm branch from f0b470b to 7f76c51 Compare May 20, 2022 04:54

fix IP address replacement

a563fe4

BenTheElder force-pushed the fix-docker-restart-kubeadm branch from 5a1120b to 5a58dbf Compare May 20, 2022 05:11

aojea reviewed May 20, 2022

View reviewed changes

images/base/files/usr/local/bin/entrypoint Show resolved Hide resolved

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

aojea reviewed May 20, 2022

View reviewed changes

images/base/files/usr/local/bin/entrypoint Show resolved Hide resolved

BenTheElder reviewed May 20, 2022

View reviewed changes

images/base/files/usr/local/bin/entrypoint Outdated Show resolved Hide resolved

BenTheElder force-pushed the fix-docker-restart-kubeadm branch from 5a58dbf to a563fe4 Compare May 20, 2022 05:30

BenTheElder reviewed May 20, 2022

View reviewed changes

tnqn commented May 20, 2022

View reviewed changes

BenTheElder reviewed May 20, 2022

View reviewed changes

images/base/files/usr/local/bin/entrypoint Outdated Show resolved Hide resolved

Improve code reliability

316e116

1. Check for files and error if replacing in them fails 2. Add word boundary to sed pattern to avoid mismatch 3. Regenerate certificate only once when both IPv4 and IPv6 addresses change Signed-off-by: Quan Tian <[email protected]>

tnqn force-pushed the fix-docker-restart-kubeadm branch from 59da8d1 to 316e116 Compare May 23, 2022 16:52

BenTheElder added 2 commits May 25, 2022 15:44

bump base image

ca64c29

bump node image

c2bc212

BenTheElder approved these changes May 25, 2022

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 25, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2022

k8s-ci-robot merged commit 9563500 into kubernetes-sigs:main May 25, 2022

BenTheElder mentioned this pull request May 25, 2022

Fix multi-node cluster not working after restarting docker #2671

Closed

tnqn deleted the fix-docker-restart-kubeadm branch May 26, 2022 03:24

BenTheElder mentioned this pull request May 2, 2023

HA clusters don't reboot properly #1689

Open

BenTheElder mentioned this pull request Oct 25, 2024

Add start and stop command to kind. #2715

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multi-node cluster not working after restarting docker #2775

Fix multi-node cluster not working after restarting docker #2775

tnqn commented May 17, 2022

k8s-ci-robot commented May 17, 2022

neolit123 May 17, 2022 •

edited

Loading

tnqn May 17, 2022

BenTheElder May 17, 2022

tnqn May 19, 2022

BenTheElder May 20, 2022

BenTheElder May 20, 2022

aojea commented May 19, 2022

aojea commented May 19, 2022

BenTheElder commented May 19, 2022

BenTheElder commented May 19, 2022

BenTheElder commented May 20, 2022

BenTheElder commented May 20, 2022

BenTheElder May 20, 2022

tnqn May 20, 2022

tnqn May 20, 2022

tnqn commented May 20, 2022 •

edited

Loading

aojea commented May 20, 2022

BenTheElder commented May 24, 2022

BenTheElder commented May 25, 2022

BenTheElder left a comment

k8s-ci-robot commented May 25, 2022

Fix multi-node cluster not working after restarting docker #2775

Fix multi-node cluster not working after restarting docker #2775

Conversation

tnqn commented May 17, 2022

k8s-ci-robot commented May 17, 2022

neolit123 May 17, 2022 • edited Loading

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

aojea commented May 19, 2022

aojea commented May 19, 2022

BenTheElder commented May 19, 2022

BenTheElder commented May 19, 2022

BenTheElder commented May 20, 2022

BenTheElder commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented May 20, 2022 • edited Loading

aojea commented May 20, 2022

BenTheElder commented May 24, 2022

BenTheElder commented May 25, 2022

BenTheElder left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 25, 2022

neolit123 May 17, 2022 •

edited

Loading

tnqn commented May 20, 2022 •

edited

Loading