-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 dedicated network for docker driver #9294
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh 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 |
Travis tests have failedHey @medyagh, 1st Buildmake test
TravisBuddy Request Identifier: cb1d0b80-fc6b-11ea-a56a-67316b5b87cd |
/ok-to-test |
kvm2 Driver |
kvm2 Driver Times for Minikube (PR 9294): 62.8s 61.8s 64.0s Averages Time Per Log
docker Driver Times for Minikube (PR 9294): 30.5s 30.7s 29.9s Averages Time Per Log
|
Can you update the PR description to describe the problem this attempts to solve and why this PR the best way to go about it? |
kvm2 Driver Times for Minikube (PR 9294): 61.3s 59.9s 60.0s Averages Time Per Log
docker Driver Times for Minikube (PR 9294): 30.4s 30.4s 29.1s Averages Time Per Log
|
Tested this out and it seems to work on Cloud Shell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has the updated code been pushed? I see resolved comments with no change in code.
if err != ErrNetworkSubnetTaken { | ||
return nil, errors.Wrapf(err, "error creating network") | ||
} | ||
// try up to 13 times |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case my previous suggestion wasn't good enough, perhaps:
// Rather than iterate through all of the valid subnets, give up at 13 to avoid a lengthy user delay for something that is unlikely to work.
The logic I would have expected here would be an iteration of all the valid subnets within 192.168.0.0/16.
OK, it seems I was reviewing based on an outdated commit. Taking another look now. |
cab94ff
to
b84dedc
Compare
thanks @tstromberg updated the PR with addresing the review comments |
kvm2 Driver Times for Minikube (PR 9294): 61.9s 59.7s 60.1s Averages Time Per Log
docker Driver Times for Minikube (PR 9294): 29.1s 29.4s 31.3s Averages Time Per Log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see the createDockerNetwork
function refactored so that the loop is executed at least once in all cases to make sure it's part of the regular integration tests, but approved otherwise. Thanks!
kvm2 Driver Times for Minikube (PR 9294): 62.5s 63.9s 66.4s Averages Time Per Log
docker Driver Times for Minikube (PR 9294): 31.2s 33.2s 29.6s Averages Time Per Log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a few nits
kvm2 Driver Times for Minikube (PR 9294): 63.4s 62.9s 65.4s Averages Time Per Log
docker Driver Times for Minikube (PR 9294): 32.2s 31.3s 32.6s Averages Time Per Log
|
redoing this PR:
https://github.com/kubernetes/minikube/pull/9094/files
After this PR
closes docker: minikube delete should delete the network #9087
closes docker network create : Pool overlaps with other one on this address space #9069
closes docker: start fails on storage provisioner addon "x509: certificate is valid for 172.17.0.3" #8936
TODO: verify it wont break upgrade running binary test
here is the link to the binary from a PR
http://storage.googleapis.com/minikube-builds/9294/minikube-linux-amd64