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

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

Merged
merged 1 commit into from Jun 25, 2018

Conversation

@xlgao-zju
Copy link
Member

xlgao-zju commented Jun 16, 2018

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

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 16, 2018

/assign @detiber

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 16, 2018

/assign @neolit123

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 16, 2018

ref #64988

@xlgao-zju xlgao-zju force-pushed the xlgao-zju:add-log-for-timeout branch from 6900f45 to fda81d7 Jun 16, 2018

@neolit123
Copy link
Member

neolit123 left a comment

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.

@@ -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

This comment has been minimized.

@neolit123

neolit123 Jun 16, 2018

Member

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.

This comment has been minimized.

@xlgao-zju

xlgao-zju Jun 16, 2018

Author Member

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

@@ -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)

This comment has been minimized.

@neolit123

neolit123 Jun 16, 2018

Member

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

This comment has been minimized.

@xlgao-zju

xlgao-zju Jun 16, 2018

Author Member

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

This comment has been minimized.

@neolit123

neolit123 Jun 16, 2018

Member

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

This comment has been minimized.

@xlgao-zju

xlgao-zju Jun 16, 2018

Author Member

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

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jun 16, 2018

@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 xlgao-zju force-pushed the xlgao-zju:add-log-for-timeout branch from fda81d7 to 69be0c9 Jun 16, 2018

@k8s-ci-robot k8s-ci-robot added size/S and removed size/XS labels Jun 16, 2018

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 16, 2018

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

Added

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jun 16, 2018

thanks @xlgao-zju !
@detiber wdyt?

@@ -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)

This comment has been minimized.

@kad

kad Jun 18, 2018

Member

why not just alter message above to add timeout value ?

This comment has been minimized.

@xlgao-zju

xlgao-zju Jun 18, 2018

Author Member

@kad updated

@xlgao-zju xlgao-zju force-pushed the xlgao-zju:add-log-for-timeout branch from 69be0c9 to 4b8b73f Jun 18, 2018

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 18, 2018

@kad updated

@detiber
Copy link
Member

detiber left a comment

@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

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 19, 2018

@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

This comment has been minimized.

Copy link
Member

detiber commented Jun 19, 2018

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 19, 2018

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 19, 2018

@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

This comment has been minimized.

Copy link
Member

rosti commented Jun 19, 2018

Should we place UpgradeManifestTimeout in consts.go?

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Jun 19, 2018

@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

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 20, 2018

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

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 23, 2018

/assign @fabriziopandini
/assign @timothysc
PTAL

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

@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

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 25, 2018

@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

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 25, 2018

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

This comment has been minimized.

Copy link
Member

neolit123 commented Jun 25, 2018

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

@@ -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)

This comment has been minimized.

@fabriziopandini

fabriziopandini Jun 25, 2018

Contributor

@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)

@xlgao-zju xlgao-zju force-pushed the xlgao-zju:add-log-for-timeout branch from 4b8b73f to ad5abab Jun 25, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 25, 2018

kubeadm-upgrade: notify the user of manifest upgrade timeouts
Signed-off-by: Xianglin Gao <xianglin.gxl@alibaba-inc.com>

@xlgao-zju xlgao-zju force-pushed the xlgao-zju:add-log-for-timeout branch from ad5abab to b309ace Jun 25, 2018

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 25, 2018

@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

This comment has been minimized.

Copy link
Member

detiber commented Jun 25, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 25, 2018

@xlgao-zju

This comment has been minimized.

Copy link
Member Author

xlgao-zju commented Jun 25, 2018

@fabriziopandini need your approve

@timothysc
Copy link
Member

timothysc left a comment

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 25, 2018

[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:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 25, 2018

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-github-robot k8s-github-robot merged commit db80cdf into kubernetes:master Jun 25, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation xlgao-zju 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-100-performance 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-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@xlgao-zju xlgao-zju deleted the xlgao-zju:add-log-for-timeout branch Jun 26, 2018

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.