Bring KVM driver in-tree #1828

r2d4 · 2017-08-16T18:20:52Z

No description provided.

codecov-io · 2017-08-16T22:48:32Z

Codecov Report

Merging #1828 into master will increase coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1828      +/-   ##
==========================================
+ Coverage   32.23%   32.26%   +0.02%     
==========================================
  Files          69       69              
  Lines        4104     4104              
==========================================
+ Hits         1323     1324       +1     
+ Misses       2608     2607       -1     
  Partials      173      173

Impacted Files	Coverage Δ
pkg/minikube/cluster/cluster.go	`40.73% <0%> (ø)`	⬆️
cmd/minikube/cmd/start.go	`10.3% <0%> (+0.2%)`	⬆️
pkg/util/utils.go	`42.96% <0%> (-2.12%)`	⬇️
pkg/util/kubeconfig/config.go	`47.61% <0%> (+0.68%)`	⬆️

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 3c9603a...531e811. Read the comment docs.

r2d4 · 2017-08-17T20:39:38Z

pkg/drivers/kvm/domain.go

@@ -0,0 +1,124 @@
+/*


@dlorenc @aaron-prindle Where should the in-tree drivers live? I just used the location that @dlorenc had in his hyperkit PR. We have the none driver in pkg/minikube/machine/drivers/none

r2d4 · 2017-08-22T23:29:18Z

I think this is ready to go @dlorenc. I think theres a little overlap in the hack/ changes between our driver PRs, so if you want to wait until you merge yours, I'll fix this up afterwards.

dlorenc

Looks pretty good. Made a first pass.

dlorenc · 2017-08-23T16:45:06Z

pkg/drivers/kvm/domain.go

@@ -0,0 +1,124 @@
+/*


dlorenc · 2017-08-23T16:45:28Z

cmd/drivers/kvm/main.go

@@ -0,0 +1,26 @@
+/*


Could you add a build tag here to restrict to linux?

dlorenc · 2017-08-23T16:50:17Z

pkg/drivers/kvm/domain.go

+func getConnection() (*libvirt.Connect, error) {
+	conn, err := libvirt.NewConnect(qemusystem)
+	if err != nil {
+		return nil, errors.Wrap(err, "Error connecting to libvirt socket")


We've had a bunch of bugs about users not understanding this or a similar error. Could we make this a bit more helpful, with some suggestions about how to resolve it?

Added our KVM driver doc to the help text and a link back to our docs.

dlorenc · 2017-08-23T16:50:54Z

pkg/drivers/kvm/domain.go

+func (d *Driver) createDomain() (*libvirt.Domain, error) {
+	tmpl := template.Must(template.New("domain").Parse(domainTmpl))
+	var domainXml bytes.Buffer
+	err := tmpl.Execute(&domainXml, d)


Collapse if statement?

dlorenc · 2017-08-23T16:51:34Z

Makefile

@@ -257,3 +259,16 @@ $(ISO_BUILD_IMAGE): deploy/iso/minikube-iso/Dockerfile
 release-iso: minikube_iso checksum
 	gsutil cp out/minikube.iso gs://$(ISO_BUCKET)/minikube-$(ISO_VERSION).iso
 	gsutil cp out/minikube.iso.sha256 gs://$(ISO_BUCKET)/minikube-$(ISO_VERSION).iso.sha256
+
+out/docker-machine-driver-kvm: $(KVM_DRIVER_FILES)


Maybe give this a different name so users can swap between the two for testing? kvm2?

I think renaming might be a lot of work. We'd need to add it to the supported driver list in minikube, so it would show up for all users. I was planning to drop official support for the docker-machine-driver-kvm when we start bundling the in-tree one. There really should be no functionality difference between the two.

From the meeting it seemed like renaming to 'minikube-kvm' or 'kvm2' is preferred to not break things.

dlorenc · 2017-08-23T16:52:08Z

pkg/drivers/kvm/kvm.go

+	}
+}
+
+//Not implemented yet


I think you can just delete these?

I don't think so. They are part of the drivers.Driver interface

dlorenc · 2017-08-23T16:52:38Z

pkg/drivers/kvm/kvm.go

+		return "", nil
+	}
+
+	for {


Will this retry forever?

I dont think we actually need this block. Removed it.

dlorenc · 2017-08-23T16:53:11Z

pkg/drivers/kvm/kvm.go

+		return state.None, errors.Wrap(err, "getting domain state")
+	}
+
+	stateMap := map[libvirt.DomainState]state.State{


Make this a var initialized outside of here?

Or a switch statement, which would let you avoid the ok check as well.

changed it to a switch statement

dlorenc · 2017-08-23T16:54:08Z

pkg/drivers/kvm/kvm.go

+	}
+
+	log.Info("Waiting to get IP...")
+	time.Sleep(5 * time.Second)


Why do we sleep here?

no reason. probably a debug artifact. removed.

dlorenc · 2017-08-23T16:54:48Z

pkg/drivers/kvm/network.go

+
+	var statusEntries []StatusEntry
+
+	err = json.Unmarshal(statuses, &statusEntries)


err is ignored?

checking it now. done

r2d4 · 2017-08-24T21:40:12Z

pkg/drivers/kvm/kvm.go

+		return state.None, errors.Wrap(err, "getting domain state")
+	}
+
+	switch libvirtState {


multiple cases per line

Add boilerplate Review feedback

r2d4 · 2017-08-28T20:56:39Z

@dlorenc @aaron-prindle rebased and addressed comments, renaming to kvm2. PTAL

aaron-prindle · 2017-08-30T17:46:01Z

You might want to update:
https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#kvm-driver
with some information about this driver.

BrendanBall · 2017-09-09T07:57:05Z

can someone please explain what this means in terms of the docker machine kvm driver?

r2d4 · 2017-09-09T16:32:08Z

It just means that you can build a custom kvm driver make install-kvm from this repo, for use with minikube using --vm-driver=kvm2

warmchang · 2017-09-11T09:42:28Z

Is this means that minikube no need the docker-machine-kvm (https://github.com/dhiltgen/docker-machine-kvm) anymore, and it can use kvm directly now?

r2d4 · 2017-09-11T16:10:45Z

No, unfortunately the new driver is also a standalone binary. The in-tree just refers to the fact that we're going to be developing and releasing it from this two rather than keep development somewhere else. It does allow us to easily share a lot of the overlap between the hyperkit and kvm drivers though #1941

warmchang · 2017-09-12T06:21:21Z

So it looks like we need to add some descriptions in the kvm-driver preparation (https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#kvm-driver):

1.Install the KVM binary as follows (e.g. CentOS):
$ sudo curl -L https://github.com/dhiltgen/docker-machine-kvm/releases/download/v0.10.0/docker-machine-driver-kvm-centos7 -o /usr/local/bin/docker-machine-driver-kvm

2. Make the binary executable:
$ sudo chmod +x /usr/local/bin/docker-machine-driver-kvm

For more information, see the documentation of the link: [docker machine KVM driver](https://github.com/dhiltgen/docker-machine-kvm#quick-start-instructions)

r2d4 · 2017-09-12T16:36:12Z

Yeah, I'll add some documentation today

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 16, 2017

r2d4 force-pushed the in-tree-kvm branch from 287b47a to 96a9146 Compare August 16, 2017 20:28

r2d4 commented Aug 17, 2017

View reviewed changes

r2d4 force-pushed the in-tree-kvm branch 7 times, most recently from 2f4eddf to 76edd14 Compare August 22, 2017 22:32

r2d4 requested a review from dlorenc August 22, 2017 23:28

r2d4 added the ready-for-review label Aug 23, 2017

r2d4 force-pushed the in-tree-kvm branch from 76edd14 to 819a81e Compare August 23, 2017 15:46

dlorenc reviewed Aug 23, 2017

View reviewed changes

r2d4 force-pushed the in-tree-kvm branch from 1b716db to 425b086 Compare August 23, 2017 20:05

r2d4 commented Aug 24, 2017

View reviewed changes

Move CalculateDiskSizeInMB to utils to use in kvm

1052dda

r2d4 force-pushed the in-tree-kvm branch from 425b086 to 69ca647 Compare August 28, 2017 18:40

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 28, 2017

r2d4 added 2 commits August 28, 2017 11:52

Bring KVM driver in-tree

48b1658

Add boilerplate Review feedback

Rename kvm driver to kvm2

531e811

r2d4 force-pushed the in-tree-kvm branch from 69ca647 to 531e811 Compare August 28, 2017 18:52

aaron-prindle approved these changes Aug 30, 2017

View reviewed changes

r2d4 merged commit 81f12ca into kubernetes:master Aug 30, 2017

r2d4 deleted the in-tree-kvm branch August 30, 2017 17:46

r2d4 mentioned this pull request Sep 12, 2017

Add documentation for this repo's kvm driver #1954

Closed

wallrj mentioned this pull request Jan 11, 2018

Use the minikube in-tree kvm2 driver jetstack/test-infra#101

Merged


		var statusEntries []StatusEntry

		err = json.Unmarshal(statuses, &statusEntries)

Bring KVM driver in-tree #1828

Bring KVM driver in-tree #1828

Conversation

r2d4 commented Aug 16, 2017

codecov-io commented Aug 16, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 commented Aug 22, 2017

dlorenc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-prindle Aug 24, 2017 • 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

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

r2d4 commented Aug 28, 2017

aaron-prindle commented Aug 30, 2017

BrendanBall commented Sep 9, 2017 • edited Loading

r2d4 commented Sep 9, 2017

warmchang commented Sep 11, 2017

r2d4 commented Sep 11, 2017

warmchang commented Sep 12, 2017

r2d4 commented Sep 12, 2017

codecov-io commented Aug 16, 2017 •

edited

Loading

aaron-prindle Aug 24, 2017 •

edited

Loading

BrendanBall commented Sep 9, 2017 •

edited

Loading