Add registry to discover driver in different platforms #2526

anfernee · 2018-02-09T01:11:34Z

Different platform has differnet list of supported drivers. The
registry contains the correct list of drivers that are supported. In
future we could add commands like minikube list-drivers

minikube-bot · 2018-02-09T01:12:05Z

Can one of the admins verify this patch?

anfernee · 2018-02-10T01:15:03Z

I found the machine package has very similar stuff. I think it's possible to merge them. I will update this PR. but @r2d4 feel free to review this.

afbjorklund · 2018-02-10T14:26:32Z

Probably related to #2527 ?

anfernee · 2018-02-12T19:03:11Z

@afbjorklund kind of, but not just that. the intention is to remove the hardcoded switch statement in different packages to manage hardcoded driver names, and use a centralized component to deal with drivers (marshal/unmarshal, initialization, etc). Another thing is drivers are platform dependent, but those switches are not.

gbraad · 2018-02-12T22:30:27Z

@praveenkumar WDYT?

dlorenc · 2018-02-15T16:01:10Z

@minikube-bot ok to test

dlorenc · 2018-02-15T16:01:27Z

Nice. I like this pattern. Do you have any immediate use-cases in mind?

k8s-ci-robot · 2018-02-15T19:28:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @aaron-prindle 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

anfernee · 2018-02-15T19:39:09Z

Guys, I moved all driver logic together and register them in registry, so it's easier to add new driver support in minikube (potentally minishift and other tools). simply register with a name, and couple of functions.

Specifically:

what platform it supports?
It's built-in or not
a function to translate from minikube config to driver config
a function to create a driver (only for builtin).

@dlorenc for use cases, the top of my mind is #2527 all supported drivers were hardcoded. With this change, we only need to call registry.ListDrivers(). It will tell you all supported drivers on your platform, whether it's builtin or not. It can potentially tell you where to get the plugin, and even help download and install the plugin for you. Just adding those to DriverDef and it's all possible.

gbraad · 2018-02-15T19:53:23Z

@minishift/minishift-dev @praveenkumar @LalatenduMohanty Please, have a look

praveenkumar · 2018-02-16T04:21:38Z

With this PR we are adding now more and more stuff to /pkg/minikube/driver which we directly can't use as part of minishift until we put minikube as dependency but then it means we have whole kuberenetes and other parts also comes to vendor which doesn't look good tradeoff in terms of consuming those bits. It would be great if all that driver part can be abstracted and maintained in a different repo and then minishift/minikube or any other can consume it.

A lot of great work is done in this PR which actually help us some of the plans we thought before like installing driver plugin as part of pre-start.

anfernee · 2018-02-17T00:31:32Z

@praveenkumar I think you can import individual packages into your vendor folder without importing the whole repo. There are tools that can do that.

You probably need those packages:

k8s.io/minikube/pkg/minikube/registry
k8s.io/minikube/pkg/minikube/config
k8s.io/minikube/pkg/minikube/drivers/...

The config package might not make sense for minishift, because you probably have different MachineConfig. Anyway, I agree the minikube in package is strange. I think it would be better if it is more neutral.

praveenkumar · 2018-02-19T11:22:10Z

I think you can import individual packages into your vendor folder without importing the whole repo. There are tools that can do that.

@anfernee we use dep for our package management (previously were using glide), do you think dep support that kind of scenarios? if yes can you provide some pointer?

The config package might not make sense for minishift, because you probably have different MachineConfig.

Yes, as of now we do have some more datatype in that structure for some validations.

anfernee · 2018-02-19T18:33:21Z

@praveenkumar Just try doing this using dep:

$ cat main.go 
package main

import (
	"fmt"

	"k8s.io/kubernetes/pkg/util/slice"
)

func main() {
	fmt.Println(slice.SortStrings([]string{"foo", "bar"}))
}
$ dep init 
  Using ^1.9.3 as constraint for direct dep k8s.io/kubernetes
  Locking in v1.9.3 (d283541) for direct dep k8s.io/kubernetes
  Locking in master (cced8e6) for transitive dep k8s.io/apimachinery

It will not copy everything into vendor.

In terms of minishift's machine config, I think there are several ways to make it more generic. One of them is to take a viper like interface, instead of confg.MachineConfig, so that use GetInt, GetBool to retrieve machine's config information.

praveenkumar · 2018-02-26T08:35:05Z

It will not copy everything into vendor.

@anfernee Looks like it does most of the things.

$ cat Gopkg.lock 
# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'.
[[projects]]
  branch = "master"
  name = "k8s.io/apimachinery"
  packages = ["pkg/util/rand"]
  revision = "cced8e64b6ca92a8b6afcbfea3353ca016694a45"

[[projects]]
  name = "k8s.io/kubernetes"
  packages = ["pkg/util/slice"]
  revision = "d2835416544f298c919e2ead3be3d0864b52323b"
  version = "v1.9.3"

[solve-meta]
  analyzer-name = "dep"
  analyzer-version = 1
  inputs-digest = "690bb1977ae5ee1ddd17d183f087cb11f8d54154bda0d0012f76babae7fc4d66"
  solver-name = "gps-cdcl"
  solver-version = 1

$ ls vendor/k8s.io/
apimachinery  kubernetes

$ ls vendor/k8s.io/kubernetes/
BUILD.bazel       CONTRIBUTING.md  Makefile                  OWNERS_ALIASES  Vagrantfile  build    code-of-conduct.md  hack         pkg      test
CHANGELOG-1.9.md  Godeps           Makefile.generated_files  README.md       WORKSPACE    cluster  docs                labels.yaml  plugin   third_party
CHANGELOG.md      LICENSE          OWNERS                    SUPPORT.md      api          cmd      examples            logo         staging  translations

$ du -sh vendor/
114M	vendor/

anfernee · 2018-02-28T23:56:24Z

@dlorenc @r2d4 anything issues with this PR? Looking forward to hearing your feedback.

frapposelli · 2018-03-05T14:34:44Z

@gbraad this would tie in nicely with the improved vmware driver that @anfernee just moved to github.com/machine-drivers.

Can you guys let us know what are the next steps to move this forward?

/cc: @dlorenc @r2d4

dlorenc · 2018-03-05T19:18:52Z

Looks like there's a conflict in kubeadm.go. Woul you mind rebasing to kick off a final test run?

Different platform has differnet list of supported drivers. The registry contains the correct list of drivers that are supported. In future we could add commands like `minikube list-drivers`

Use ListDrivers() method to get the list of VM drivers. SupportedVMDrivers become the whole list of VM drivers on all platforms, which is used in gendocs only.

anfernee · 2018-03-07T21:48:34Z

The OSX test looks suspicious. I would go grab a mac and test it. I assume it would be either red herring or small change. The rest of this PR should be ready for review. @dlorenc @r2d4 if you guys can take a look at the basic structure of this PR and give me some feedback, it would be great. thanks!

anfernee · 2018-03-08T06:49:05Z

I tried to run this PR on OSX, it works well. Still not sure why test fails though.

frapposelli · 2018-03-09T18:14:26Z

maybe a fluke? does

/retest

work here?

dlorenc · 2018-03-09T18:19:09Z

I think the mac slave needed a reboot. I'll kick it off again after the machine comes back up.

dlorenc

Two small nits, otherwise LGTM. The test passed after the reboot.

dlorenc · 2018-03-09T18:40:31Z

pkg/minikube/cluster/cluster.go

@@ -308,6 +296,7 @@ func MountHost(api libmachine.API, ip net.IP, path, port, mountVersion string, u
 }

 // GetVMHostIP gets the ip address to be used for mapping host -> VM and VM -> host
+// TODO: remove this (why not just using driver.GetIP?)


This is kind of the opposite - driver.GetIP is the IP address of the VM from the host, this is the IP of the host from the VM.

OK. I see. I am removing this comment. but i still want to see if it's possible to remove the switch for driver names sometime in future.

dlorenc · 2018-03-09T18:42:02Z

pkg/minikube/cluster/cluster_test.go

@@ -67,15 +68,17 @@ func TestCreateHost(t *testing.T) {
 	}

 	found := false
-	for _, driver := range constants.SupportedVMDrivers {
-		if h.DriverName == driver {
+	drivers := []string{}


If you add a "String()" method to DriverDef that returns the .Name field, I think you can simplify this a bit and avoid collecting the drivers slice.

- Add registry to register all supported drivers in different platforms. - Add DriverDef to define driver's metadata. - All driver support logic moved to pkg/minikube/drivers, removed all driver name switches scattered in different packages.

anfernee · 2018-03-09T21:58:54Z

done, thanks @dlorenc

afbjorklund · 2018-03-11T17:19:07Z

Is there any quick guide on how to convert a driver to this new regime ?

praveenkumar · 2018-03-12T03:57:40Z

@anfernee Can you also send a PR to minishift side? I think it would be good if you send instead we copy stuff and then paste.

anfernee · 2018-03-13T00:28:35Z

thanks @dlorenc

@afbjorklund Good question. Not yet. but I am going to write one maybe in the drivers folder. @praveenkumar will do. but I would need to get myself familiar to minishift source. I don't mind if you guys want to copy&paste it.

praveenkumar · 2018-03-13T04:08:37Z

will do. but I would need to get myself familiar to minishift source.

@anfernee Thanks, no hurry from our side (Take your time)

I don't mind if you guys want to copy&paste it.

Right but I really like you to put that code instead we copy&paste :)

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

anfernee force-pushed the discover branch 2 times, most recently from 5c0c9a8 to 30ceaa5 Compare February 9, 2018 17:32

anfernee force-pushed the discover branch from ba24d3c to 1bd2d16 Compare February 15, 2018 19:28

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2018

anfernee force-pushed the discover branch from 1bd2d16 to e1ebfcf Compare February 15, 2018 21:40

anfernee force-pushed the discover branch from e1ebfcf to 79cb056 Compare February 17, 2018 00:18

anfernee mentioned this pull request Feb 28, 2018

Make this driver consumable to minikube machine-drivers/docker-machine-driver-vmware#1

Closed

Yongkun Anfernee Gui added 2 commits March 7, 2018 10:44

Add registry to discover driver in different platforms

c4b4135

Different platform has differnet list of supported drivers. The registry contains the correct list of drivers that are supported. In future we could add commands like `minikube list-drivers`

Remove OS dependent SupportedVMDrivers

a95f254

Use ListDrivers() method to get the list of VM drivers. SupportedVMDrivers become the whole list of VM drivers on all platforms, which is used in gendocs only.

anfernee force-pushed the discover branch from 79cb056 to 1cb45ec Compare March 7, 2018 18:51

dlorenc reviewed Mar 9, 2018

View reviewed changes

Reorganize the minikube drivers

2e461e1

- Add registry to register all supported drivers in different platforms. - Add DriverDef to define driver's metadata. - All driver support logic moved to pkg/minikube/drivers, removed all driver name switches scattered in different packages.

anfernee force-pushed the discover branch from 1cb45ec to 2e461e1 Compare March 9, 2018 21:58

dlorenc approved these changes Mar 9, 2018

View reviewed changes

dlorenc approved these changes Mar 11, 2018

View reviewed changes

dlorenc merged commit 0fa64b3 into kubernetes:master Mar 11, 2018

anfernee deleted the discover branch March 13, 2018 00:28

frapposelli mentioned this pull request Mar 15, 2018

Add new VMware unified driver to supported list #2606

Closed

anfernee mentioned this pull request Mar 22, 2018

Add document for adding new driver #2633

Merged

frapposelli mentioned this pull request Jan 16, 2019

Add new VMware unified driver to supported list #2 #3534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registry to discover driver in different platforms #2526

Add registry to discover driver in different platforms #2526

anfernee commented Feb 9, 2018

minikube-bot commented Feb 9, 2018

anfernee commented Feb 10, 2018

afbjorklund commented Feb 10, 2018

anfernee commented Feb 12, 2018

gbraad commented Feb 12, 2018

dlorenc commented Feb 15, 2018

dlorenc commented Feb 15, 2018

k8s-ci-robot commented Feb 15, 2018

anfernee commented Feb 15, 2018

gbraad commented Feb 15, 2018 •

edited

Loading

praveenkumar commented Feb 16, 2018

anfernee commented Feb 17, 2018

praveenkumar commented Feb 19, 2018

anfernee commented Feb 19, 2018

praveenkumar commented Feb 26, 2018

anfernee commented Feb 28, 2018

frapposelli commented Mar 5, 2018

dlorenc commented Mar 5, 2018

anfernee commented Mar 7, 2018

anfernee commented Mar 8, 2018

frapposelli commented Mar 9, 2018

dlorenc commented Mar 9, 2018

dlorenc left a comment

dlorenc Mar 9, 2018

anfernee Mar 9, 2018

dlorenc Mar 9, 2018

anfernee commented Mar 9, 2018

afbjorklund commented Mar 11, 2018

praveenkumar commented Mar 12, 2018

anfernee commented Mar 13, 2018

praveenkumar commented Mar 13, 2018

Add registry to discover driver in different platforms #2526

Add registry to discover driver in different platforms #2526

Conversation

anfernee commented Feb 9, 2018

minikube-bot commented Feb 9, 2018

anfernee commented Feb 10, 2018

afbjorklund commented Feb 10, 2018

anfernee commented Feb 12, 2018

gbraad commented Feb 12, 2018

dlorenc commented Feb 15, 2018

dlorenc commented Feb 15, 2018

k8s-ci-robot commented Feb 15, 2018

anfernee commented Feb 15, 2018

gbraad commented Feb 15, 2018 • edited Loading

praveenkumar commented Feb 16, 2018

anfernee commented Feb 17, 2018

praveenkumar commented Feb 19, 2018

anfernee commented Feb 19, 2018

praveenkumar commented Feb 26, 2018

anfernee commented Feb 28, 2018

frapposelli commented Mar 5, 2018

dlorenc commented Mar 5, 2018

anfernee commented Mar 7, 2018

anfernee commented Mar 8, 2018

frapposelli commented Mar 9, 2018

dlorenc commented Mar 9, 2018

dlorenc left a comment

Choose a reason for hiding this comment

dlorenc Mar 9, 2018

Choose a reason for hiding this comment

anfernee Mar 9, 2018

Choose a reason for hiding this comment

dlorenc Mar 9, 2018

Choose a reason for hiding this comment

anfernee commented Mar 9, 2018

afbjorklund commented Mar 11, 2018

praveenkumar commented Mar 12, 2018

anfernee commented Mar 13, 2018

praveenkumar commented Mar 13, 2018

gbraad commented Feb 15, 2018 •

edited

Loading