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

Removed dependency on localkube to wait for the network to be up #1298

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Mar 27, 2017

Ref: #1277

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2017
@r2d4
Copy link
Contributor

r2d4 commented Mar 27, 2017

I think we have to split up the commands before we remove this dependency. I could be wrong though

@codecov-io
Copy link

codecov-io commented Mar 27, 2017

Codecov Report

Merging #1298 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1298      +/-   ##
=========================================
+ Coverage   43.06%   43.1%   +0.04%     
=========================================
  Files          48      48              
  Lines        2271    2271              
=========================================
+ Hits          978     979       +1     
+ Misses       1133    1132       -1     
  Partials      160     160
Impacted Files Coverage Δ
pkg/minikube/cluster/commands.go 61.79% <ø> (ø) ⬆️
pkg/minikube/kubeconfig/config.go 56.92% <0%> (+1.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9750a7e...a11b471. Read the comment docs.

@dlorenc
Copy link
Contributor

dlorenc commented Mar 27, 2017

I think we have to split up the commands before we remove this dependency. I could be wrong though

I don't think so. IIRC we might have to investigate some flakiness inside of localkube itself.

@dlorenc
Copy link
Contributor

dlorenc commented Mar 27, 2017

Aaron, can you run the tests like 10 times and see if there are any new flakes?

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Mar 29, 2017

test 1
1 failing test (Linux-KVM)
test 2
1 failing test (Linux-KVM)
test 3
2 failing tests (Linux-KVM and Linux-Virtualbox)
test 4
2 failing tests (Linux-KVM and OSX-Virtualbox)
test 5
2 failing tests (Linux-KVM and OSX-Virtualbox)

@aaron-prindle
Copy link
Contributor Author

@minikube-bot retest this please

1 similar comment
@aaron-prindle
Copy link
Contributor Author

@minikube-bot retest this please

@r2d4
Copy link
Contributor

r2d4 commented Mar 30, 2017

I'm a little concerned that KVM hasn't passed. I'm pretty sure his was the issue that we saw before

@aaron-prindle
Copy link
Contributor Author

We could make waiting for the network be driver specific to KVM. Or we could wait for a custom network check that avoids the delay issues mentioned in #1277

@r2d4
Copy link
Contributor

r2d4 commented Mar 30, 2017

Maybe we can do some manual testing to see how much faster this gets us

@dlorenc
Copy link
Contributor

dlorenc commented Apr 11, 2017

@minikube-bot test this please

@dlorenc
Copy link
Contributor

dlorenc commented Apr 11, 2017

@r2d4 the error looks different than before, it's actually related to SSH'ing in. I just tested this with KVM locally a bunch of times and didn't see any issues.

@dlorenc
Copy link
Contributor

dlorenc commented Apr 11, 2017

I ran some completely non-scientific benchmarks. With this change (linux/kvm):

1m7.390s
1m3.078s
0m57.739s
0m58.045s
0m58.333s
1m1.243s
0m56.404s
1m0.327s
0m58.563s
0m56.925s

Without it:

2m57.594s
3m2.195s
2m57.785s
2m57.945s
2m57.570s
2m56.606s
2m57.460s
2m57.988s
3m0.304s
2m56.295s

This looks pretty significant.

@r2d4
Copy link
Contributor

r2d4 commented Apr 11, 2017

I'm fine merging this in once we fix the KVM test. I haven't gotten a chance to try this out, but I'll look at it today and try to look at the issue.

@dlorenc
Copy link
Contributor

dlorenc commented Apr 11, 2017

@aaron-prindle could you rebase and re-run the tests?

@aaron-prindle
Copy link
Contributor Author

rebased, re-running tests

@dlorenc
Copy link
Contributor

dlorenc commented Apr 12, 2017

@minikube-bot test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants