Remove all minikube dependencies from drivers #4933

sharifelgamal · 2019-07-31T21:46:49Z

All the config not related to networking gets overridden in the config passed in from the client, so there's never a need to set any defaults.

k8s-ci-robot · 2019-07-31T21:47:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sharifelgamal

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~~ [sharifelgamal]

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

josedonizetti · 2019-07-31T22:21:42Z

pkg/drivers/hyperkit/driver.go

@@ -121,7 +117,7 @@ func (d *Driver) Create() error {

 // DriverName returns the name of the driver
 func (d *Driver) DriverName() string {
-	return constants.DriverHyperkit
+	return "hyperkit"


can we make it a constant inside the driver?

I disagree :-P

josedonizetti · 2019-07-31T22:21:50Z

pkg/drivers/kvm/kvm.go

@@ -224,7 +214,7 @@ func (d *Driver) GetSSHHostname() (string, error) {

 // DriverName returns the name of the driver
 func (d *Driver) DriverName() string {
-	return constants.DriverKvm2
+	return "kvm2"


can we make it a constant inside the driver?

@afbjorklund As those are the driver names I consider they good for a constant. To always have only one place to change it, even though now we don't have many. But as always, no strong feelings. and probably a too simple thing to start a big discussion, so all good to revert it to string only :)

Well, at least it's not using the constants module. But the string is only used in 1 or 2 places ?

It's the not the most important thing, just thought it was an unnecessary step of indirection.

josedonizetti · 2019-07-31T22:24:17Z

pkg/drivers/hyperkit/driver.go

 		}
-		return nil
+		if err.Error() != fmt.Sprintf(IPErrorMessage, mac) {


hey, do you think it's worth having a custom error and compare the type? comparing error strings seems ugly :/

yeah I wasn't wild about text parsing either, and the custom error type is easy to add. done.

afbjorklund · 2019-08-01T07:51:30Z

pkg/drivers/hyperkit/driver.go

@@ -52,6 +49,7 @@ const (
 	permErr         = "%s needs to run with elevated permissions. " +
 		"Please run the following command, then try again: " +
 		"sudo chown root:wheel %s && sudo chmod u+s %s"
+	driverHyperkit = "hyperkit"


or just use the string?

afbjorklund · 2019-08-01T07:51:50Z

pkg/drivers/kvm/kvm.go

@@ -91,6 +87,7 @@ const (
 	qemusystem                = "qemu:///system"
 	defaultPrivateNetworkName = "minikube-net"
 	defaultNetworkName        = "default"
+	driverKvm2 = "kvm2"


or just use the string ?

afbjorklund · 2019-08-01T07:52:00Z

pkg/drivers/kvm/kvm.go

@@ -224,7 +215,7 @@ func (d *Driver) GetSSHHostname() (string, error) {

 // DriverName returns the name of the driver
 func (d *Driver) DriverName() string {
-	return constants.DriverKvm2
+	return driverKvm2


afbjorklund · 2019-08-01T07:52:14Z

pkg/drivers/hyperkit/driver.go

@@ -121,7 +118,7 @@ func (d *Driver) Create() error {

 // DriverName returns the name of the driver
 func (d *Driver) DriverName() string {
-	return constants.DriverHyperkit
+	return driverHyperkit


afbjorklund · 2019-08-01T07:53:08Z

pkg/drivers/hyperkit/network.go

@@ -78,7 +80,7 @@ func getIPAddressFromFile(mac, path string) (string, error) {
 			return dhcpEntry.IPAddress, nil
 		}
 	}
-	return "", fmt.Errorf("could not find an IP address for %s", mac)
+	return "", fmt.Errorf(IPErrorMessage, mac)


would prefer having the printf inline here

afbjorklund · 2019-08-01T07:53:33Z

pkg/drivers/hyperkit/driver.go

@@ -121,7 +117,7 @@ func (d *Driver) Create() error {

 // DriverName returns the name of the driver
 func (d *Driver) DriverName() string {
-	return constants.DriverHyperkit
+	return "hyperkit"


I disagree :-P

afbjorklund · 2019-08-01T07:56:14Z

All the config not related to networking gets overridden in the config passed in from the client, so there's never a need to set any defaults.

I find this strange, since normally it does have some defaults. But I guess we will see, once it has been tried stand-alone (with docker-machine)

Then again, those defaults are usually 1 CPU and 1G RAM - since that is what Docker requires.

It is Kubernetes that is increasing that to 2 CPU and 2G RAM, or maybe I should say kubeadm does.

afbjorklund · 2019-08-01T13:05:18Z

This addresses the dependencies found in #4909, but seems like it doesn't build just yet.

# k8s.io/minikube/pkg/drivers/hyperkit
pkg/drivers/hyperkit/driver.go:269:12: undefined: TempError
pkg/drivers/hyperkit/driver.go:280:18: impossible type assertion:
	*tempError does not implement error (missing Error method)

sharifelgamal · 2019-08-06T22:43:19Z

@minikube-bot OK to test

sharifelgamal · 2019-08-06T23:33:26Z

This should be ready now.

afbjorklund · 2019-08-08T07:02:03Z

Why revert the libmachine version ?

Moving back to the 0.16.1 version re-introduces the AMD bug (yet again)

sharifelgamal · 2019-08-08T17:23:51Z

It was causing issues with go modules, and specifically with the go proxy:
curl https://proxy.golang.org/github.com/docker/machine/@v/v0.16.1-0.20190718054102-a555e4f7a8f5.info not found: github.com/docker/[email protected]: invalid pseudo-version: revision a555e4f7a8f5 is not a descendent of preceding tag (v0.16.0)

I didn't realize that reverting back to the release tag reintroduces a bug, I'll looking into pinning this correctly.

afbjorklund · 2019-08-08T17:40:38Z

It was causing issues with go modules, and specifically with the go proxy:

No worries, it was my fault for hacking the version. I'll go back to the weirdo v0.0.0

#4817 (comment)

Or maybe it is even worse than v0.0.0: v0.7.1-0.20190718054102-a555e4f7a8f5
Since machine doesn't tag on master, and doesn't start the tags with a silent v.

https://golang.org/cmd/go/#hdr-Pseudo_versions

So go just imagines some weird version like 0.7.1 (that never existed! it's 0.8.0),
since v0.7.0 was tagged on master - it just adds one and hopes for the best. 😠

afbjorklund · 2019-08-08T17:54:59Z

@sharifelgamal : why aren't we messing with go proxy (and Travis) in a separate PR ?

sharifelgamal · 2019-08-08T17:58:02Z

You're right, it should be a separate PR. The travis issues were causing tests to fail in this PR so I was testing solutions. I'll open a new PR.

afbjorklund · 2019-08-08T18:01:51Z

I can fix the go.mod, in order to make go proxy happy (have never tried go proxy)

	github.com/docker/machine v0.7.1-0.20190718054102-a555e4f7a8f5

sharifelgamal · 2019-08-08T18:11:54Z

#5018 is the new go proxy PR

sharifelgamal added 3 commits July 31, 2019 12:24

Remove all minikube dependencies in driver code

05da539

removing all default config

812e815

okay we need some defaults

3369ebc

sharifelgamal requested review from tstromberg and afbjorklund July 31, 2019 21:46

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 31, 2019

k8s-ci-robot requested review from medyagh and RA489 July 31, 2019 21:46

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 31, 2019

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

sharifelgamal requested review from josedonizetti and removed request for RA489 July 31, 2019 21:47

josedonizetti reviewed Jul 31, 2019

View reviewed changes

code comments

d2ab336

afbjorklund reviewed Aug 1, 2019

View reviewed changes

hyperkit builds now

5fcb676

sharifelgamal changed the title ~~Remove all minikube dependencies from drivers~~ WIP: Remove all minikube dependencies from drivers Aug 2, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2019

sharifelgamal added 3 commits August 6, 2019 11:30

sleep for an appropriate amount of time

b38e090

Merge branch 'master' of github.com:kubernetes/minikube into kvm

4a1e521

remove constant in favor of string

24fe0b4

sharifelgamal changed the title ~~WIP: Remove all minikube dependencies from drivers~~ Remove all minikube dependencies from drivers Aug 6, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2019

sharifelgamal added 2 commits August 6, 2019 17:07

try goproxy for travis

80a5cab

try goproxy for travis

2c9ab43

sharifelgamal added 5 commits August 6, 2019 17:23

try goproxy for travis

c5fb950

let's not try goproxy for now

eda4d7a

let's try goproxy once again

efb9da6

maybe use the correct url for the proxy

3264f2e

fix go mod

b970b1c

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2019

Merge branch 'master' of github.com:kubernetes/minikube into kvm

be8b237

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2019

tstromberg approved these changes Aug 12, 2019

View reviewed changes

sharifelgamal merged commit a817bff into kubernetes:master Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all minikube dependencies from drivers #4933

Remove all minikube dependencies from drivers #4933

sharifelgamal commented Jul 31, 2019 •

edited

Loading

k8s-ci-robot commented Jul 31, 2019

josedonizetti Jul 31, 2019

sharifelgamal Aug 1, 2019

afbjorklund Aug 1, 2019

josedonizetti Jul 31, 2019

sharifelgamal Aug 1, 2019

afbjorklund Aug 1, 2019

josedonizetti Aug 1, 2019

afbjorklund Aug 1, 2019

josedonizetti Jul 31, 2019 •

edited

Loading

sharifelgamal Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund Aug 1, 2019

afbjorklund commented Aug 1, 2019 •

edited

Loading

afbjorklund commented Aug 1, 2019

sharifelgamal commented Aug 6, 2019

sharifelgamal commented Aug 6, 2019

afbjorklund commented Aug 8, 2019

sharifelgamal commented Aug 8, 2019

afbjorklund commented Aug 8, 2019 •

edited

Loading

afbjorklund commented Aug 8, 2019

sharifelgamal commented Aug 8, 2019

afbjorklund commented Aug 8, 2019 •

edited

Loading

sharifelgamal commented Aug 8, 2019

Remove all minikube dependencies from drivers #4933

Remove all minikube dependencies from drivers #4933

Conversation

sharifelgamal commented Jul 31, 2019 • edited Loading

k8s-ci-robot commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josedonizetti Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afbjorklund commented Aug 1, 2019 • edited Loading

afbjorklund commented Aug 1, 2019

sharifelgamal commented Aug 6, 2019

sharifelgamal commented Aug 6, 2019

afbjorklund commented Aug 8, 2019

sharifelgamal commented Aug 8, 2019

afbjorklund commented Aug 8, 2019 • edited Loading

afbjorklund commented Aug 8, 2019

sharifelgamal commented Aug 8, 2019

afbjorklund commented Aug 8, 2019 • edited Loading

sharifelgamal commented Aug 8, 2019

sharifelgamal commented Jul 31, 2019 •

edited

Loading

josedonizetti Jul 31, 2019 •

edited

Loading

afbjorklund commented Aug 1, 2019 •

edited

Loading

afbjorklund commented Aug 8, 2019 •

edited

Loading

afbjorklund commented Aug 8, 2019 •

edited

Loading