Add output to give user awareness of how long timeouts are expected to be #65164

xlgao-zju · 2018-06-16T14:18:18Z

What this PR does / why we need it:
Add output to give user awareness of how long manifest upgrade timeout is expected to be.

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

Special notes for your reviewer:

Release note:

kubeadm: notify the user of manifest upgrade timeouts

xlgao-zju · 2018-06-16T14:19:11Z

/assign @detiber

xlgao-zju · 2018-06-16T14:19:31Z

/assign @neolit123

xlgao-zju · 2018-06-16T14:20:14Z

ref #64988

neolit123

thanks for having a look a this @xlgao-zju !
i've added a couple of comments.

please, also rename the commit title to be shorter in the lines of:
kubeadm-upgrade: notify the user of manifest upgrade timeouts

if you want to add more information add it in the commit message body.

neolit123 · 2018-06-16T15:19:28Z

cmd/kubeadm/app/cmd/upgrade/apply.go

@@ -194,6 +194,7 @@ func RunApply(flags *applyFlags) error {
 	// Use a prepuller implementation based on creating DaemonSets
 	// and block until all DaemonSets are ready; then we know for sure that all control plane images are cached locally
 	glog.V(1).Infof("[upgrade/apply] creating prepuller")
+	upgrade.UpgradeManifestTimeout = upgradeManifestTimeout


cmd/upgrade imports phases/upgrade as a backend.
instead of modifying a package global variable in upgrade, why not move the constant to the upgrade package?

also make sure you add a comment on top of global variables / constants.

@neolit123 Since I tried to modify the origin code as less as I could... But this thought seems wrong... 😅

neolit123 · 2018-06-16T15:23:31Z

cmd/kubeadm/app/phases/upgrade/staticpods.go

@@ -228,6 +231,7 @@ func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticP

 	if waitForComponentRestart {
 		fmt.Println("[upgrade/staticpods] Waiting for the kubelet to restart the component")
+		fmt.Println("[upgrade/staticpods] This operation will timeout in ", UpgradeManifestTimeout)


we should use Printf():
fmt.Printf("[upgrade/staticpods] This operation will timeout in %v\n", UpgradeManifestTimeout)

ok, but this will print the same thing as what I added...

yes, definitely!
the kubeadm codebase uses Printf() in all places where an argument is printed and Println() if no arguments are printed.

Got you!
So kind you are. Thank you for your help!

neolit123 · 2018-06-16T15:50:49Z

@xlgao-zju
adding a release note is probably a good idea too:
kubeadm: notify the user of manifest upgrade timeouts

you can modify it in the first message in this PR (under Release note:)

xlgao-zju · 2018-06-16T16:05:08Z

@xlgao-zju
adding a release note is probably a good idea too:
kubeadm: notify the user of manifest upgrade timeouts

Added

neolit123 · 2018-06-16T19:46:26Z

thanks @xlgao-zju !
@detiber wdyt?

kad · 2018-06-18T10:55:08Z

cmd/kubeadm/app/phases/upgrade/staticpods.go

@@ -228,6 +233,7 @@ func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticP

 	if waitForComponentRestart {
 		fmt.Println("[upgrade/staticpods] Waiting for the kubelet to restart the component")
+		fmt.Printf("[upgrade/staticpods] This operation will timeout in %v\n", UpgradeManifestTimeout)


why not just alter message above to add timeout value ?

@kad updated

xlgao-zju · 2018-06-18T15:06:37Z

@kad updated

detiber

@xlgao-zju This looks great, thanks. I would like to keep the original linked issue open, since there are still other timeouts that we do not provide user output for as well, though.

xlgao-zju · 2018-06-19T03:08:45Z

@detiber Yes, I think we can merge this PR. And then we should figure out where we should add the output. Before we call the function in waiter or somewhere else?

detiber · 2018-06-19T12:40:15Z

@xlgao-zju yes, adding some type of output in waiter was my thought as well.

/lgtm

xlgao-zju · 2018-06-19T15:20:23Z

@detiber Yes, we can add a TODO list in the original issue to track all the output we should add.

@timothysc need your approve...

rosti · 2018-06-19T15:28:33Z

Should we place UpgradeManifestTimeout in consts.go?

neolit123 · 2018-06-19T17:22:02Z

@rosti
i think consts.go is a good option too. possibly depends if a constant is used more globally.
i would leave for others to comment on that.

xlgao-zju · 2018-06-20T02:29:18Z

@neolit123 +1 for possibly depends if a constant is used more globally.

xlgao-zju · 2018-06-23T08:58:19Z

/assign @fabriziopandini
/assign @timothysc
PTAL

fabriziopandini

@xlgao-zju thanks for this PR!
I'm fine why your proposal, with a simple request of improvement on the message for bettet consistency with the overall kubeadm UX.
@neolit123 @detiber feel free to improve the message

xlgao-zju · 2018-06-25T06:58:02Z

@fabriziopandini Hi, I am not quiet understand your requirement... Cloud you make it more clearer, os I can improve this PR. Or cloud you explain the overall kubeadm UX for me, I'd like to learn this. 😆

xlgao-zju · 2018-06-25T15:46:59Z

with a simple request of improvement on the message for bettet consistency with the overall kubeadm UX.

@neolit123 Cloud you take another look at this? How could I improve this PR?

neolit123 · 2018-06-25T15:51:17Z

@fabriziopandini can you please elaborate on how to improve the message? :)

fabriziopandini · 2018-06-25T15:51:49Z

cmd/kubeadm/app/phases/upgrade/staticpods.go

@@ -227,7 +232,7 @@ func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticP
 	fmt.Printf("[upgrade/staticpods] Moved new manifest to %q and backed up old manifest to %q\n", currentManifestPath, backupManifestPath)

 	if waitForComponentRestart {
-		fmt.Println("[upgrade/staticpods] Waiting for the kubelet to restart the component")
+		fmt.Printf("[upgrade/staticpods] Waiting for the kubelet to restart the component. This operation will timeout in %v\n", UpgradeManifestTimeout)



@xlgao-zju sorry I forgot to clck add comment 😶
Please make this more consistent with other kubeadm Wait messages e.g.

fmt.Printf("[upgrade/staticpods] Waiting for the kubelet to restart the component") fmt.Printf("[upgrade/staticpods] This might take a minute or longer depending on the component/version gap (timeout %v\n", UpgradeManifestTimeout)

This includes a short reassuring description about what Kubeadm is doing and why the user should wait (in this case I suggest a generic note)

Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>

xlgao-zju · 2018-06-25T16:11:26Z

@fabriziopandini updated.

@neolit123 @detiber As I said, we can add a TODO list in kubernetes/kubeadm/#914. In that TODO list, We can add all the places where we should print the similar log. And we can use this PR as an example.
wdyt?

detiber · 2018-06-25T16:27:09Z

/lgtm

xlgao-zju · 2018-06-25T16:30:15Z

@fabriziopandini need your approve

timothysc

/approve

k8s-ci-robot · 2018-06-25T20:17:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, timothysc, xlgao-zju

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:

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

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

k8s-github-robot · 2018-06-25T21:44:03Z

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

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 16, 2018

k8s-ci-robot requested review from kad and xiangpengzhao June 16, 2018 14:18

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jun 16, 2018

k8s-ci-robot assigned detiber Jun 16, 2018

k8s-ci-robot assigned neolit123 Jun 16, 2018

xlgao-zju force-pushed the add-log-for-timeout branch from 6900f45 to fda81d7 Compare June 16, 2018 15:07

neolit123 reviewed Jun 16, 2018

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 16, 2018

xlgao-zju force-pushed the add-log-for-timeout branch from fda81d7 to 69be0c9 Compare June 16, 2018 15:59

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 16, 2018

kad reviewed Jun 18, 2018

View reviewed changes

xlgao-zju force-pushed the add-log-for-timeout branch from 69be0c9 to 4b8b73f Compare June 18, 2018 15:05

detiber approved these changes Jun 18, 2018

View reviewed changes

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

k8s-ci-robot assigned fabriziopandini and timothysc Jun 23, 2018

fabriziopandini suggested changes Jun 25, 2018

View reviewed changes

fabriziopandini reviewed Jun 25, 2018

View reviewed changes

xlgao-zju force-pushed the add-log-for-timeout branch from 4b8b73f to ad5abab Compare June 25, 2018 16:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2018

kubeadm-upgrade: notify the user of manifest upgrade timeouts

b309ace

Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>

xlgao-zju force-pushed the add-log-for-timeout branch from ad5abab to b309ace Compare June 25, 2018 16:03

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

timothysc approved these changes Jun 25, 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 Jun 25, 2018

k8s-github-robot merged commit db80cdf into kubernetes:master Jun 25, 2018

xlgao-zju deleted the add-log-for-timeout branch June 26, 2018 01:50

stealthybox mentioned this pull request Jul 14, 2018

Add output to give user awareness of how long timeouts are expected to be kubernetes/kubeadm#914

Closed

Add output to give user awareness of how long timeouts are expected to be #65164

Add output to give user awareness of how long timeouts are expected to be #65164

Conversation

xlgao-zju commented Jun 16, 2018 • edited Loading

xlgao-zju commented Jun 16, 2018

xlgao-zju commented Jun 16, 2018

xlgao-zju commented Jun 16, 2018

neolit123 left a 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 commented Jun 16, 2018

xlgao-zju commented Jun 16, 2018

neolit123 commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlgao-zju commented Jun 18, 2018

detiber left a comment

Choose a reason for hiding this comment

xlgao-zju commented Jun 19, 2018 • edited Loading

detiber commented Jun 19, 2018

xlgao-zju commented Jun 19, 2018

rosti commented Jun 19, 2018

neolit123 commented Jun 19, 2018

xlgao-zju commented Jun 20, 2018

xlgao-zju commented Jun 23, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

xlgao-zju commented Jun 25, 2018

xlgao-zju commented Jun 25, 2018

neolit123 commented Jun 25, 2018

Choose a reason for hiding this comment

xlgao-zju commented Jun 25, 2018

detiber commented Jun 25, 2018

xlgao-zju commented Jun 25, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 25, 2018

k8s-github-robot commented Jun 25, 2018

xlgao-zju commented Jun 16, 2018 •

edited

Loading

xlgao-zju commented Jun 19, 2018 •

edited

Loading