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: surface external etcd preflight validation errors #60585

Merged
merged 1 commit into from Apr 12, 2018

Conversation

@alexbrand
Member

alexbrand commented Feb 28, 2018

What this PR does / why we need it:
This PR fixes a bug where errors that could occur while running preflight against an external etcd cluster were not being surfaced to the user.

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

Release note:

kubeadm: surface external etcd preflight validation errors
@@ -715,7 +715,10 @@ type etcdVersionResponse struct {
// ExternalEtcdVersionCheck checks if version of external etcd meets the demand of kubeadm
type ExternalEtcdVersionCheck struct {
Etcd kubeadmapi.Etcd
Etcd kubeadmapi.Etcd
requestTimeout time.Duration

This comment has been minimized.

@dixudx

dixudx Mar 27, 2018

Member

I don't think we need to add three new parameters.

L930 still uses old configuration. That does not make any change.

This comment has been minimized.

@alexbrand

alexbrand Mar 27, 2018

Member

Thanks for the review @dixudx. These were added to avoid doing retries in unit tests. Otherwise, the tests run for ~ 45 seconds. With that said, it is true that it is unclear in the tests that we are exploiting the zero value of these fields. Instead, we should probably set them so it is clear that these fields are used. I am also open to other suggestions if there's a better way to do this.

}
stopRetry, err = func() (stopRetry bool, err error) {
r, err := client.Get(url)
if err != nil {
loopCount--
return false, nil
return false, err

This comment has been minimized.

@dixudx

dixudx Mar 27, 2018

Member

Right. L835 and L841 are the only two places we should change.

This comment has been minimized.

@alexbrand

alexbrand Apr 9, 2018

Member

Meaning the PR should only touch these two lines? It is currently a bit more involved due to the added unit tests, and trying to avoid the tests running for longer than they need by being able to specify the retry parameters.

Is there a better approach to this? Thanks!

cc @timothysc

@timothysc

This comment has been minimized.

Member

timothysc commented Apr 5, 2018

@k8s-ci-robot k8s-ci-robot requested a review from timothysc Apr 5, 2018

@timothysc

/approve

please address @dixudx 's comments for lgtm

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 10, 2018

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@alexbrand

Important: This pull request was missing labels required for the v1.11 milestone for more than 3 days:

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help
@alexbrand

This comment has been minimized.

Member

alexbrand commented Apr 11, 2018

Synced up with @timothysc. The tests I added are not very useful because we are faking the etcd response using a fake http server. Will update PR.

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

This comment has been minimized.

@timothysc

timothysc Apr 11, 2018

Member

Given that you are just surfacing simple errors, and we generally discourage stub'd test servers vs. real in memory servers in the code, you can trim this down to just the changes that at @dixudx had requested.

kubeadm: surface external etcd preflight validation errors
Signed-off-by: Alexander Brand <alexbrand09@gmail.com>
@timothysc

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexbrand, timothysc

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

@timothysc

This comment has been minimized.

Member

timothysc commented Apr 11, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 12, 2018

Automatic merge from submit-queue (batch tested with PRs 60585, 62398, 62258, 62042). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 8d1a4ca into kubernetes:master Apr 12, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation alexbrand authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce 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
@luxas

This comment has been minimized.

Member

luxas commented May 16, 2018

@alexbrand can you backport this to v1.10 please? Seems like a small enough and useful change to get into v1.10.x

@CelsoSantos

This comment has been minimized.

CelsoSantos commented May 17, 2018

I back that request..

No issues on Vagrant but on the servers themselves I'm having a lot of problems, and I'm not entirely sure why, but I do get the same exact error.

@alexbrand

This comment has been minimized.

Member

alexbrand commented May 18, 2018

@luxas created cherry pick PR

#64048

k8s-merge-robot added a commit that referenced this pull request May 23, 2018

Merge pull request #64048 from alexbrand/automated-cherry-pick-of-#60585
-origin-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #60585: kubeadm: surface external etcd preflight validation errors

Cherry pick of #60585 on release-1.10.

#60585: kubeadm: surface external etcd preflight validation errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment