kubeadm: Add configurable control plane up timeout #70480

rosti · 2018-10-31T12:53:27Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Until now the control plane timeout was fixed to 4 minutes and users did not have the ability to change it. This commit allows that timeout to be configured via the new timeoutForControlPlane option in the API server config (itself a member of the ClusterConfiguration).

The default timeout is still 4 minutes.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Refs kubernetes/kubeadm#911

Special notes for your reviewer:

This PR depends on:

Please, review only the last commit here.

This PR does not modify the self hosting timeout code, which is going to be deleted in another PR:

Fixes /kubernetes/kubeadm/issues/1072 Cleanup of selfhosting logic #69878

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/assign @fabriziopandini
/assign @timothysc

Does this PR introduce a user-facing change?:

kubeadm: timeoutForControlPlane is introduced as part of the API Server config, that controls the timeout for the wait for control plane to be up. Default value is 4 minutes.

neolit123

thanks @rosti

neolit123 · 2018-10-31T13:08:43Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/doc.go

should we use the default here 4m?

I don't think it's necessary.

I agree with @neolit123, please change to 4 b/c that's the default used all over.

timothysc

/approve

Generally looks like what we talked about, thx for the patch.

timothysc · 2018-11-01T21:35:59Z

cmd/kubeadm/app/apis/kubeadm/v1beta1/doc.go

I agree with @neolit123, please change to 4 b/c that's the default used all over.

fabriziopandini

Thanks @rosti!
/approve
Waiting for dependency to merge before final lgtm

rosti · 2018-11-02T16:09:50Z

Hold because of unmerged dependency PR.

/hold

Until now the control plane timeout was fixed to 4 minutes and users did not have the ability to change it. This commit allows that timeout to be configured via the new `timeoutForControlPlane` option in the API server config (itself a member of the ClusterConfiguration). The default timeout is still 4 minutes. Signed-off-by: Rostislav M. Georgiev <[email protected]>

rosti · 2018-11-05T10:47:30Z

/hold cancel

neolit123 · 2018-11-05T12:36:04Z

@rosti @fabriziopandini

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/apiclient/wait.go#L159
^ it runs a couple of go routines concurrently an whichever fails first returns an error. so if f() there is for instance WaitForAPI() (for init / wait-control-plane phase) the control plane timeout would be respected but the other target is always WaitForHealthyKubelet() which has a timeout of total 195 seconds. so even if the user sets DefaultControlPlaneTimeout to 10 minutes it will always fail in no more than 195 seconds.

fabriziopandini · 2018-11-05T12:38:54Z

@neolit123 @rosti
In my opionin it makes sense

if kubelet doesn't start, fail fast after 195 sec (does not make sense wait for the control plane)
– if kubelet starts, wait for control plane max 4 minutes

neolit123 · 2018-11-05T12:39:48Z

i think what has to be done here is not use exponential backoff in WaitForHealthyKubelet, but rather allow us to make the function match a timeout the user has specified - e..g. DefaultKubeletTimeout

dixudx · 2018-11-05T13:07:45Z

cmd/kubeadm/app/cmd/init.go

+
 	// TODO: NewControlPlaneWaiter should be converted to private after the self-hosting phase is removed.
-	waiter, err := phases.NewControlPlaneWaiter(i.dryRun, client, i.outputWriter)
+	timeout := i.cfg.ClusterConfiguration.APIServer.TimeoutForControlPlane.Duration


Why declaring this var, which is used only once?

That's because the expression to take the timeout value is huge and best viewed on a single line. Also the call to NewControlPlaneWaiter on the following line continues to fit on screen.

rosti · 2018-11-05T13:21:28Z

@neolit123 I agree with you, that the waiter code can be simplified a bit (to say the least), but perhaps this is a job for another PR?

fabriziopandini

@rosti Thanks for this PR!
/approve
/lgtm

@neolit123 @dixudx we can eventually iterate on details, but I'm moving on with this PR to get all the API changes in before code slush

k8s-ci-robot · 2018-11-05T13:27:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rosti, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini,timothysc]

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 assigned fabriziopandini and timothysc Oct 31, 2018

rosti force-pushed the controlplane-timeout branch from 2aa3aca to 7eb0e8c Compare October 31, 2018 12:55

neolit123 approved these changes Oct 31, 2018

View reviewed changes

neolit123 mentioned this pull request Oct 31, 2018

Add timeout value to configure how long kubeadm init takes to timeout kubernetes/kubeadm#1168

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2018

rosti force-pushed the controlplane-timeout branch from 7eb0e8c to dd32858 Compare November 1, 2018 11:07

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 1, 2018

timothysc added the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Nov 1, 2018

timothysc approved these changes Nov 1, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2018

fabriziopandini approved these changes Nov 2, 2018

View reviewed changes

rosti force-pushed the controlplane-timeout branch from dd32858 to 486c8f3 Compare November 2, 2018 16:08

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 2, 2018

rosti force-pushed the controlplane-timeout branch from 486c8f3 to eb6f7b1 Compare November 5, 2018 10:37

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 5, 2018

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 5, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2018

dixudx approved these changes Nov 5, 2018

View reviewed changes

fabriziopandini approved these changes Nov 5, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2018

k8s-ci-robot merged commit b3d81e5 into kubernetes:master Nov 5, 2018

rosti deleted the controlplane-timeout branch November 5, 2018 16:59

kubeadm: Add configurable control plane up timeout #70480

kubeadm: Add configurable control plane up timeout #70480

Uh oh!

Conversation

rosti commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neolit123 left a comment

Choose a reason for hiding this comment

Uh oh!

neolit123 Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

rosti Oct 31, 2018

Choose a reason for hiding this comment

Uh oh!

timothysc Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

timothysc left a comment

Choose a reason for hiding this comment

Uh oh!

timothysc Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

fabriziopandini left a comment

Choose a reason for hiding this comment

Uh oh!

rosti commented Nov 2, 2018

Uh oh!

rosti commented Nov 5, 2018

Uh oh!

neolit123 commented Nov 5, 2018

Uh oh!

fabriziopandini commented Nov 5, 2018

Uh oh!

neolit123 commented Nov 5, 2018

Uh oh!

dixudx Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

rosti Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

rosti commented Nov 5, 2018

Uh oh!

fabriziopandini left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rosti commented Oct 31, 2018 •

edited

Loading