Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm : fix-kubeadm-upgrade-12-13-14 #75956

Conversation

@fabriziopandini
Copy link
Contributor

commented Apr 1, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
As reported by users, kubedm upgrade fails when upgrading cluster created with kubeadm v1.12, then upgraded to v1.13, and finally in the process to upgrade to v1.14.

This happened because the upgrade v1.12 to v1.13, didn't change the etcd manifest because the etcd version didn't change

Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1471

Special notes for your reviewer:
This fix should apply to v1.14 branch only and aims to get rid of etcd manifests with listening address localhost only when upgrading from v1.13 to v1.14

Does this PR introduce a user-facing change?:

kubeadm: fixes error when upgrading from v1.13 to v1.14 clusters created with kubeadm v1.12. Please note that it is required to upgrade etcd during the final v1.13 to v1.14 upgrade.

/sig cluster-lifecycle
/priority critical-urgent
/milestone v1.14
/assign @neolit123 @rosti @mauilion @timothysc
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

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

@neolit123
Copy link
Member

left a comment

thanks @fabriziopandini
added some comments.

Show resolved Hide resolved cmd/kubeadm/app/cmd/upgrade/apply.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/certs/renewal/renewal.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go
Show resolved Hide resolved cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go
@rosti
Copy link
Member

left a comment

Thanks @fabriziopandini !
Looks OK at first glance, although I am a bit concerned that we add a couple of funcs, that don't have any tests.
I'll try to test it manually.

Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go Outdated
@rosti
Copy link
Member

left a comment

Have a few more things in the second pass.
Tested it on my setup and it works like a charm.

Show resolved Hide resolved cmd/kubeadm/app/cmd/upgrade/apply.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go Outdated

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:fix-kubeadm-upgrade-12-13-14 branch from 6e2d826 to 5ccb3e2 Apr 1, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@neolit123 @rosti @SataQiu thanks for the feedback! everything should be addressed now
@rosti thank you for taking time to test this in your setup

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@mauilion
Copy link
Contributor

left a comment

a couple of nits. Looks good.

I think we should state in the kubeadm output that this means that the etcd node will be bound to an exeternal ip address and that the user should take this into account.

Show resolved Hide resolved cmd/kubeadm/app/phases/upgrade/staticpods.go
Show resolved Hide resolved cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
@runam0K

This comment has been minimized.

Copy link

commented Apr 2, 2019

I'm trying to update my cluster right now.
When will the bug in the debian packages on http://apt.kubernetes.io/ be fixed?

Thank you for your feedback.

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@runam0K
the fix (this PR) will hopefully be available in 1.14.1.

@runam0K

This comment has been minimized.

Copy link

commented Apr 2, 2019

@runam0K
the fix (this PR) will hopefully be available in 1.14.1.

Thanks for the information. Is there already a planned date?

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Thanks for the information. Is there already a planned date?

possibly later this week or next week.

@fabriziopandini fabriziopandini force-pushed the fabriziopandini:fix-kubeadm-upgrade-12-13-14 branch from 5ccb3e2 to 211bc61 Apr 2, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 2, 2019

@fabriziopandini

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@rosti @neolit123 @mauilion
now, in case of etcd listening on local host only, the version check in performEtcdStaticPodUpgrade doesn't care if the etcd member url is localhost:2379 or 127.0.0.1:2379

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 2, 2019

@neolit123
Copy link
Member

left a comment

/lgtm

@mauilion

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

/lgtm

@yagonobre
Copy link
Member

left a comment

Thanks @fabriziopandini
Found a small nit non blocking
/lgtm

Show resolved Hide resolved cmd/kubeadm/app/cmd/upgrade/apply.go
@Klaven
Copy link
Contributor

left a comment

looks good to me, my question is more for my understanding! Thank you.

Show resolved Hide resolved cmd/kubeadm/app/util/etcd/etcd.go
@runam0K

This comment has been minimized.

Copy link

commented Apr 4, 2019

After I set the IP from the Master-Node manually in the manifest:

I get an other error: error syncing endpoints with etc: context deadline exceeded
Same problem like here: kubernetes/kubeadm#1469 (comment)

Does this hotfix solve this problem also?

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@tpepper hi, this is the most urgent fix, compared to the other "cherry picks to" 1.14 that were just approved.

@fabriziopandini

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@runam0K your hotfix is not enough for solving the problem; please refer to the issue where a possible manual hotfix is described; if you want to propose/discuss an alternative approach, it would be great if you move the discussion there for better discoverability

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

added some non blocking for merge. comments.

@k8s-ci-robot k8s-ci-robot merged commit e169638 into kubernetes:release-1.14 Apr 4, 2019

17 checks passed

cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@fabriziopandini fabriziopandini deleted the fabriziopandini:fix-kubeadm-upgrade-12-13-14 branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.