Fix issue 2513 #3148

mheese · 2018-09-21T17:58:05Z

This fixes issue #2513, and also effectively does the following:

fixes creation of the private network
separates activation and creation of networks
ensures to generates separate MACs for the NICs
fixed the documentation link
hardens the deletion of the network: checks if it really is not used by any other VM, and only deletes it if this instance of the minikube VM is the last one using it

It's been a while that I've written these fixes, and because of the big delay on our company's side with signing the CNCF CLA, this PR comes rather late. I'm happy it's sorted out though, and I can create the PR. Looking over the code again though, it looks like everything is in place.

@tstromberg, fyi, as you wanted to be referenced

…twork NICs

…e the network if it is in use

…twork NICs

…e the network if it is in use

…fix-issue-2513

k8s-ci-robot · 2018-09-21T17:58:12Z

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 email the CNCF helpdesk: helpdesk@rt.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.

k8s-ci-robot · 2018-09-21T17:58:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mheese
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dlorenc

If they are not already assigned, you can assign the PR to them by writing /assign @dlorenc in a comment when ready.

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

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

minikube-bot · 2018-09-21T17:59:05Z

Can one of the admins verify this patch?

mheese · 2018-09-21T18:19:26Z

I signed it

dlorenc · 2018-09-24T19:18:53Z

@minikube-bot OK to test

dlorenc

Thanks! This looks great. A few nits.

dlorenc · 2018-09-24T19:21:03Z

pkg/drivers/kvm/network.go

-		return errors.Wrapf(err, "defining network from xml: %s", networkXML.String())
+	// fail if there are 0 domains
+	if len(doms) == 0 {
+		return fmt.Errorf("list of domains is 0 lenght")


nit: typo. What's the correct fix here? It would be good to include some steps the user could take to create the domain.

yeah ... good point to think about what this error means to the user ... it's one of those statements that will/should never happen ... I think there is two things we could do to address this:

turn this into a panic()

simply remove the check: at the end of the day, it would just mean that something else deleted even our own minikube domain while we're trying to delete this network, and then there is no point of further iteration through the domains.

I'd personally opt for (2), but I let you make that decision @dlorenc. What do you think?

Let's not panic: https://github.com/golang/go/wiki/CodeReviewComments#dont-panic

I like your #2 option, since this is just at the deletion stage, but just in case it helps a future debugger, do you mind putting a log.Warningf in about the unexpected case?

Thanks again for your contribution.

dlorenc · 2018-09-24T19:21:57Z

pkg/drivers/kvm/kvm.go

-	network, err := conn.LookupNetworkByName(d.PrivateNetwork)
+	// Tear down network if it exists and is not in use by another minikube instance
+	log.Debug("Trying to delete the networks (if possible)")
+	err = d.deleteNetwork()


nit: inline the if statement here.

fair enough, inlining this statement right now

tstromberg · 2018-09-21T18:33:08Z

pkg/drivers/kvm/network.go

-		return errors.Wrapf(err, "defining network from xml: %s", networkXML.String())
+	// fail if there are 0 domains
+	if len(doms) == 0 {
+		return fmt.Errorf("list of domains is 0 lenght")


spelling of length.

tstromberg · 2018-09-26T20:29:44Z

pkg/drivers/kvm/network.go

-		return errors.Wrapf(err, "defining network from xml: %s", networkXML.String())
+	// fail if there are 0 domains
+	if len(doms) == 0 {
+		return fmt.Errorf("list of domains is 0 lenght")


Let's not panic: https://github.com/golang/go/wiki/CodeReviewComments#dont-panic

I like your #2 option, since this is just at the deletion stage, but just in case it helps a future debugger, do you mind putting a log.Warningf in about the unexpected case?

Thanks again for your contribution.

v0.30.0: Specifically call out the fixed CVE and omitted #3148

Marcus Heese added 11 commits August 14, 2018 13:43

kvm2 driver: fixed private network creation and setup

ea45197

kvm2 driver: separated activating and creating of networks

943a786

kvm2 driver: generate separate random MACs for default and private ne…

a93686b

…twork NICs

kvm2 driver: be more robust with creating of the network, don't delet…

e13bca7

…e the network if it is in use

kvm2 driver: fixed documentation link in error text

9e8af2c

kvm2 driver: fixed private network creation and setup

d1995d2

kvm2 driver: separated activating and creating of networks

c4ef7ad

kvm2 driver: generate separate random MACs for default and private ne…

b35e908

…twork NICs

kvm2 driver: be more robust with creating of the network, don't delet…

34ee591

…e the network if it is in use

kvm2 driver: fixed documentation link in error text

5e38bf4

Merge branch 'fix-issue-2513' of github.com:etherparty/minikube into …

8d75f3b

…fix-issue-2513

k8s-ci-robot requested review from aaron-prindle and balopat September 21, 2018 17:58

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2018

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 Sep 21, 2018

dlorenc reviewed Sep 24, 2018

View reviewed changes

inlining deleteNetwork into if statement

4eb2cdb

mheese mentioned this pull request Sep 26, 2018

Incorporate changes from KVM2 driver plugin and do a release machine-drivers/docker-machine-kvm#3

Closed

tstromberg suggested changes Sep 26, 2018

View reviewed changes

turning error into a warning log message

5d0e123

tstromberg added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2018

tstromberg approved these changes Sep 27, 2018

View reviewed changes

tstromberg merged commit 74f8e49 into kubernetes:master Sep 27, 2018

tstromberg added a commit to tstromberg/minikube that referenced this pull request Oct 9, 2018

Highlight the fixed CVE, add accidentally omitted kubernetes#3148

dc6a568

tstromberg added a commit that referenced this pull request Oct 19, 2018

Merge pull request #3239 from tstromberg/v0.30.0-release

6e17cb3

v0.30.0: Specifically call out the fixed CVE and omitted #3148

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 2513 #3148

Fix issue 2513 #3148

mheese commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

minikube-bot commented Sep 21, 2018

mheese commented Sep 21, 2018

dlorenc commented Sep 24, 2018

dlorenc left a comment

dlorenc Sep 24, 2018

mheese Sep 24, 2018

tstromberg Sep 26, 2018

dlorenc Sep 24, 2018

mheese Sep 24, 2018

tstromberg Sep 21, 2018

tstromberg Sep 26, 2018

Fix issue 2513 #3148

Fix issue 2513 #3148

Conversation

mheese commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

k8s-ci-robot commented Sep 21, 2018

minikube-bot commented Sep 21, 2018

mheese commented Sep 21, 2018

dlorenc commented Sep 24, 2018

dlorenc left a comment

Choose a reason for hiding this comment

dlorenc Sep 24, 2018

Choose a reason for hiding this comment

mheese Sep 24, 2018

Choose a reason for hiding this comment

tstromberg Sep 26, 2018

Choose a reason for hiding this comment

dlorenc Sep 24, 2018

Choose a reason for hiding this comment

mheese Sep 24, 2018

Choose a reason for hiding this comment

tstromberg Sep 21, 2018

Choose a reason for hiding this comment

tstromberg Sep 26, 2018

Choose a reason for hiding this comment