Set --wait=false to default but still wait for apiserver #5757

priyawadhwa · 2019-10-28T17:17:12Z

This PR changes --wait=false to be the default, but to still check for the apiserver in this case. If wait=true, we still wait for all pods to be up and running. This change should speed up minikube start, since we won't have to wait for other pods which can take a longer time to start up.

Ref #5681

Output now looks like:

$ ./out/minikube start 
😄  minikube v1.5.0-beta.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.1 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"


$ ./out/minikube delete
🔥  Deleting "minikube" in hyperkit ...
💔  The "minikube" cluster has been deleted.
🔥  Successfully deleted profile "minikube"


$ ./out/minikube start --wait true
😄  minikube v1.5.0-beta.0 on Darwin 10.14.6
💡  Tip: Use 'minikube start -p <name>' to create a new cluster, or 'minikube delete' to delete this one.
🔄  Starting existing hyperkit VM for "minikube" ...
⌛  Waiting for the host to be provisioned ...
🐳  Preparing Kubernetes v1.16.1 on Docker 18.09.9 ...
🔄  Relaunching Kubernetes using kubeadm ... 
⌛  Waiting for: apiserver proxy etcd scheduler controller dns
🏄  Done! kubectl is now configured to use "minikube"

$ ./out/minikube stop
✋  Stopping "minikube" in hyperkit ...
🛑  "minikube" stopped.

$ ./out/minikube start
😄  minikube v1.5.0-beta.0 on Darwin 10.14.6
💡  Tip: Use 'minikube start -p <name>' to create a new cluster, or 'minikube delete' to delete this one.
🔄  Starting existing hyperkit VM for "minikube" ...
⌛  Waiting for the host to be provisioned ...
🐳  Preparing Kubernetes v1.16.1 on Docker 18.09.9 ...
🔄  Relaunching Kubernetes using kubeadm ... 
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"

TODO:

look into writing unit tests
look into how this effects integration tests

k8s-ci-robot · 2019-10-28T17:17:20Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

codecov-io · 2019-10-28T17:24:11Z

Codecov Report

Merging #5757 into master will increase coverage by 0.02%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##           master    #5757      +/-   ##
==========================================
+ Coverage   37.83%   37.86%   +0.02%     
==========================================
  Files         106      106              
  Lines        7773     7796      +23     
==========================================
+ Hits         2941     2952      +11     
- Misses       4452     4464      +12     
  Partials      380      380

Impacted Files	Coverage Δ
pkg/minikube/bootstrapper/bootstrapper.go	`0% <ø> (ø)`	⬆️
cmd/minikube/cmd/start.go	`20.39% <16.66%> (-0.07%)`	⬇️
pkg/minikube/bootstrapper/kubeadm/kubeadm.go	`21.44% <42.3%> (+1.59%)`	⬆️

This PR changes --wait=false to be the default, but to still check for the apiserver in this case. If wait=true, we still wait for all pods to be up and running. This change should speed up `minikube start`, since we won't have to wait for other pods which can take a longer time to start up. Ref kubernetes#5681

priyawadhwa · 2019-10-28T17:49:29Z

Running mkcmp on this branch vs master on my Mac with hyperkit show significant speedup:

Logs

$ ./out/mkcmp ./out/master.minikube ./out/wait_false.minikube 
2019/10/28 10:34:13 Executing run 0...
2019/10/28 10:34:13 Running: [./out/master.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver proxy etcd scheduler controller dns
🏄  Done! kubectl is now configured to use "minikube"
2019/10/28 10:36:50 Running: [./out/wait_false.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"
2019/10/28 10:38:04 Executing run 1...
2019/10/28 10:38:04 Running: [./out/master.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver proxy etcd scheduler controller dns
🏄  Done! kubectl is now configured to use "minikube"
2019/10/28 10:40:42 Running: [./out/wait_false.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"
2019/10/28 10:41:59 Executing run 2...
2019/10/28 10:41:59 Running: [./out/master.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver proxy etcd scheduler controller dns
🏄  Done! kubectl is now configured to use "minikube"
2019/10/28 10:44:32 Running: [./out/wait_false.minikube start]...
😄  minikube v1.5.0 on Darwin 10.14.6
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker 18.09.9 ...
🚜  Pulling images ...
🚀  Launching Kubernetes ... 
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"
Old binary: [151.380704891 152.648160101 147.704785009]
New binary: [69.619480977 71.68093154 66.499298735]
Average Old: 150.577883
Average New: 69.266570

Old binary: [151.380704891 152.648160101 147.704785009]
New binary: [69.619480977 71.68093154 66.499298735]
Average Old: 150.577883
Average New: 69.266570

medyagh · 2019-10-28T17:58:32Z

wow what a big difference, I am curious which one of the integration tests would need to pass --wait=false

sharifelgamal · 2019-10-28T19:53:16Z

/ok-to-test

tstromberg · 2019-10-28T23:29:47Z

Do you mind confirming that kubectl get po -A returns non-zero content with this PR?

minikube start && kubectl get po -A should at least return the apiserver.

Please add an integration test that asserts here right after startup here:

minikube/test/integration/functional_test.go

Line 62 in 7569f77

    
           {"KubeContext", validateKubeContext},       // Racy: must come immediately after "minikube start"

I suggest using validateKubeContext as a template to get started.

tstromberg · 2019-10-28T23:35:11Z

Current integration tests appear to be unrelated flakes. Great work! =)

priyawadhwa · 2019-10-28T23:39:06Z

@tstromberg I check if we can get the apiserver pod here without error, do you think that's enough?

(It shouldn't return err==nil until the pod is up, but I don't check if the pod is Running)

to make sure apiserver is up and running and that the pod can be accessed via kubectl.

tstromberg

This PR is awesome, and completely overdue.

cmd/minikube/cmd/start.go

pkg/minikube/bootstrapper/kubeadm/kubeadm.go

tstromberg · 2019-10-28T23:52:57Z

@tstromberg I check if we can get the apiserver pod here without error, do you think that's enough?

(It shouldn't return err==nil until the pod is up, but I don't check if the pod is Running)

Your PR is doing the right thing. The integration test is so that future contributors do not unknowingly change this behavior.

when waiting for the cluster to start up.

priyawadhwa · 2019-10-29T01:04:24Z

@tstromberg sounds good! I removed updating the docs from this PR, it was getting large/complicated and it'll be easier to review it in a separate PR.

tstromberg

/lgtm

k8s-ci-robot · 2019-10-29T04:51:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: priyawadhwa, tstromberg

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:

~~OWNERS~~ [tstromberg]

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 added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2019

k8s-ci-robot requested review from blueelvis and sharifelgamal October 28, 2019 17:17

Priya Wadhwa added 3 commits October 28, 2019 10:45

Remove extra log

cf62ff6

Rebased on master

f92b308

priyawadhwa force-pushed the wait_false branch from e5aecf2 to f92b308 Compare October 28, 2019 17:46

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 28, 2019

medyagh added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 28, 2019

update start flags

67901b4

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 28, 2019

make sure we can get api server pod

993967b

Add integration test

c3dd5bc

to make sure apiserver is up and running and that the pod can be accessed via kubectl.

tstromberg suggested changes Oct 28, 2019

View reviewed changes

cmd/minikube/cmd/start.go Outdated Show resolved Hide resolved

pkg/minikube/bootstrapper/kubeadm/kubeadm.go Show resolved Hide resolved

Priya Wadhwa added 2 commits October 28, 2019 17:12

Update docs and add error message to generate-docs

5f12e9a

Update integration test name

5113270

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2019

Priya Wadhwa added 2 commits October 28, 2019 17:34

Clean up bootstrapper interface to accept list of pods to wait for

f3b1a09

when waiting for the cluster to start up.

Remove additional docs

c41eb8a

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 29, 2019

improve comments

bd5840a

priyawadhwa force-pushed the wait_false branch from 59978e1 to bd5840a Compare October 29, 2019 01:02

tstromberg approved these changes Oct 29, 2019

View reviewed changes

k8s-ci-robot assigned tstromberg Oct 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2019

tstromberg merged commit b4ce29d into kubernetes:master Oct 29, 2019

tstromberg mentioned this pull request Nov 12, 2019

Make --wait=false non-blocking, --wait=true blocks on system pods #5894

Merged

priyawadhwa mentioned this pull request Dec 16, 2019

make WaitCluster smarter. #5125

Closed

priyawadhwa deleted the wait_false branch January 14, 2020 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set --wait=false to default but still wait for apiserver #5757

Set --wait=false to default but still wait for apiserver #5757

priyawadhwa commented Oct 28, 2019 •

edited

Loading

k8s-ci-robot commented Oct 28, 2019

codecov-io commented Oct 28, 2019 •

edited

Loading

priyawadhwa commented Oct 28, 2019

medyagh commented Oct 28, 2019

sharifelgamal commented Oct 28, 2019

tstromberg commented Oct 28, 2019 •

edited

Loading

tstromberg commented Oct 28, 2019

priyawadhwa commented Oct 28, 2019 •

edited

Loading

tstromberg left a comment

tstromberg commented Oct 28, 2019 •

edited

Loading

priyawadhwa commented Oct 29, 2019

tstromberg left a comment

k8s-ci-robot commented Oct 29, 2019

Set --wait=false to default but still wait for apiserver #5757

Set --wait=false to default but still wait for apiserver #5757

Conversation

priyawadhwa commented Oct 28, 2019 • edited Loading

k8s-ci-robot commented Oct 28, 2019

codecov-io commented Oct 28, 2019 • edited Loading

Codecov Report

priyawadhwa commented Oct 28, 2019

medyagh commented Oct 28, 2019

sharifelgamal commented Oct 28, 2019

tstromberg commented Oct 28, 2019 • edited Loading

tstromberg commented Oct 28, 2019

priyawadhwa commented Oct 28, 2019 • edited Loading

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg commented Oct 28, 2019 • edited Loading

priyawadhwa commented Oct 29, 2019

tstromberg left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 29, 2019

priyawadhwa commented Oct 28, 2019 •

edited

Loading

codecov-io commented Oct 28, 2019 •

edited

Loading

tstromberg commented Oct 28, 2019 •

edited

Loading

priyawadhwa commented Oct 28, 2019 •

edited

Loading

tstromberg commented Oct 28, 2019 •

edited

Loading