WIP: Add minikube support for the "generic" VM driver

[afbjorklund]

(rebase of stale #4734)

This driver connects to an already existing virtual machine,
using the provided IP address and SSH user/key/port parameters.

On this machine, libmachine will provision the docker daemon
and minikube will bootstrap the kubernetes cluster (as usual).

Implements #4733

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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 [afbjorklund]

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

[afbjorklund]

Just rebased the old branch, for now. Probably need to update the driver functions.

These don't make much sense anymore: driver.BareMetal and driver.IsVM

The generic driver supports both remote physical servers and remote virtual servers...

Also to be considered is if we should fork the "generic" driver, like the "none" ?

Basically this mostly affects commands:

        // Remove a host
        Remove() error

        // Restart a host. This may just call Stop(); Start() if the provider does not
        // have any special restart behaviour.
        Restart() error

        // Start a host
        Start() error

        // Stop a host gracefully
        Stop() error

For none, these are remapped to kubelet.

[codecov-io]

Codecov Report

Merging #6873 into master will decrease coverage by 0.03%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master    #6873      +/-   ##
==========================================
- Coverage   35.50%   35.47%   -0.04%     
==========================================
  Files         148      148              
  Lines        9330     9345      +15     
==========================================
+ Hits         3313     3315       +2     
- Misses       5620     5633      +13     
  Partials      397      397              

Impacted Files	Coverage Δ
cmd/minikube/cmd/delete.go	22.13% <0.00%> (ø)
cmd/minikube/cmd/start.go	15.42% <0.00%> (ø)
pkg/minikube/cluster/ip.go	0.00% <0.00%> (ø)
pkg/minikube/driver/driver.go	60.00% <ø> (ø)
pkg/minikube/machine/stop.go	31.57% <12.50%> (+1.84%)	⬆️
pkg/minikube/machine/fix.go	37.50% <42.85%> (-1.58%)	⬇️

[medyagh]

any chance we could have integration tests for it ? even if we dont run it regulary, any script that we could run to see how many tests pass?

[afbjorklund]

Sure, not sure what would be the best way to spin up the VMs, though. For local testing I used Vagrant and VirtualBox (with Ubuntu and CentOS), but I’m not sure it would fit?

[medyagh]

Sure, not sure what would be the best way to spin up the VMs, though. For local testing I used Vagrant and VirtualBox (with Ubuntu and CentOS), but I’m not sure it would fit?

Vagrant , terraform anything is good, anything that we could run to see if be broke it.

could also add it is own jenkins job,
if you create me a hack/script like the one we have for all other drivers, I will add a jenkins job for it. maybe with Vagrant and KVM ?

[afbjorklund]

I will not have any time to work on it until later, and it probably is not for the pending release. But I still think it (“generic” driver) would be a good feature to have for the long term...

If you could help to get it tested, I could help to fix the code if needed

[tstromberg]

Can this PR be cleaned up so that it's ready for review?

[afbjorklund]

Can this PR be cleaned up so that it's ready for review?

It should be rather clean now. The main question is whether we want to fork the "generic" driver, like we did for the "none" driver earlier. To make it shutdown kubernetes, instead of the actual host.

https://github.com/docker/machine/blob/master/drivers/none/driver.go

https://github.com/kubernetes/minikube/blob/master/pkg/drivers/none/none.go

Also there is no cute function yet for the driver.IsGeneric since it is confusing already, i.e. IsVM and BareMetal might apply and it might not - depending on what kind of ssh remote you give it ?

// IsVM checks if the driver is a VM
func IsVM(name string) bool {
        if IsKIC(name) || IsMock(name) || BareMetal(name) {
                return false
        }
        return true
}

// BareMetal returns if this driver is unisolated
func BareMetal(name string) bool {
        return name == None || name == Mock
}

[tstromberg]

Can this PR be cleaned up so that it's ready for review?

It should be rather clean now. The main question is whether we want to fork the "generic" driver, like we did for the "none" driver earlier. To make it shutdown kubernetes, instead of the actual host.

In general, I prefer to start fork-less, and fork only as incompatible features are added, but what do you think?

I would also prefer that this driver should be called ssh as far as users see it. If forking generic is required to make this work, than perhaps we should do so ahead of time. Users are far more likely to be familiar with SSH than libmachine.

https://github.com/docker/machine/blob/master/drivers/none/driver.go

https://github.com/kubernetes/minikube/blob/master/pkg/drivers/none/none.go

Also there is no cute function yet for the driver.IsGeneric since it is confusing already, i.e. IsVM and BareMetal might apply and it might not - depending on what kind of ssh remote you give it ?

I think IsVM and IsBareMetal were short-sighted, and that we should have functions instead based on capabilities. driver.IsMemoryTunable / driver.SupportsPreload

// IsVM checks if the driver is a VM
func IsVM(name string) bool {
        if IsKIC(name) || IsMock(name) || BareMetal(name) {
                return false
        }
        return true
}

// BareMetal returns if this driver is unisolated
func BareMetal(name string) bool {
        return name == None || name == Mock
}

[afbjorklund]

In general, I prefer to start fork-less, and fork only as incompatible features are added, but what do you think?

The main reason for forking would be to make it similar to the other "bare-metal" driver. But it is a confusing situation, and only makes long-term sense if libmachine is to be dropped entirelly...

I would also prefer that this driver should be called ssh as far as users see it. If forking generic is required to make this work, than perhaps we should do so ahead of time. Users are far more likely to be familiar with SSH than libmachine.

We could make internal aliases for drivers, which is somewhat confusing implementation-wise but could maybe be nicer for end-users. Like call none as "local" and generic as "remote" or something.

[afbjorklund]

Basically the name "generic" originates from being the driver for any existing Cloud Provider...

https://docs.docker.com/machine/get-started-cloud/#3rd-party-driver-plugins

• docker.github.io/machine/AVAILABLE_DRIVER_PLUGINS.md

So if your cloud didn't have a built-in driver, you could provision your VM and give it to machine.

https://docs.docker.com/machine/get-started-cloud/#drivers-for-cloud-providers

• https://docs.docker.com/machine/drivers/generic/

Create machines using an existing VM/Host with SSH.

This is useful if you are using a provider that Machine does not support directly or if you would like to import an existing host to allow Docker Machine to manage.

And as we discussed earlier, "none" comes for Docker hosts that didn't have any driver.

https://docs.docker.com/machine/get-started-cloud/#add-a-host-without-a-driver

You can register an already existing docker host by passing the daemon url. With that, you can have the same workflow as on a host provisioned by docker-machine.

The terms from docker-machine can be confusing when it comes to minikube...

Since minikube assumes a local docker, and doesn't support any cloud providers.

[afbjorklund]

Will start on making a minikube fork of it... If it is not ready for next week, we can drop it from v1.10

In general, I prefer to start fork-less, and fork only as incompatible features are added, but what do you think?

The main "incompatible feature" is the need for if statements, when using the original drivers.
Since they don't implement the StartHost and StopHost commands, those must be avoided.

	// Start a host
	Start() error

	// Stop a host gracefully
	Stop() error

none

func (d *Driver) Start() error {
	return fmt.Errorf("hosts without a driver cannot be started")
}

func (d *Driver) Stop() error {
	return fmt.Errorf("hosts without a driver cannot be stopped")
}

generic

func (d *Driver) Start() error {
	return errors.New("generic driver does not support start")
}

func (d *Driver) Stop() error {
	return errors.New("generic driver does not support stop")
}

That is the reason for d3e273c, which is basically why this PR is shown as so many lines (partly also because of the indent, much smaller with diff -w)

diff --git a/pkg/minikube/machine/fix.go b/pkg/minikube/machine/fix.go
index 8aa7472..a6732dc 100644
--- a/pkg/minikube/machine/fix.go
+++ b/pkg/minikube/machine/fix.go
@@ -126,6 +133,7 @@ func recreateIfNeeded(api libmachine.API, cc config.ClusterConfig, n config.Node
                }
        }
 
+       if h.Driver.DriverName() != driver.Generic {
                if serr != constants.ErrMachineMissing {
                        glog.Warningf("unexpected machine state, will restart: %v", serr)
                }
@@ -146,6 +154,13 @@ func recreateIfNeeded(api libmachine.API, cc config.ClusterConfig, n config.Node
                if err := api.Save(h); err != nil {
                        return h, errors.Wrap(err, "save")
                }
+       } else {
+               if s == state.Running {
+                       out.T(out.Running, `Using the {{.driver_name}} "{{.cluster}}" {{.machine_type}} ...`, out.V{"driver_name": cc.Driver, "cluster": cc.Name, "machine_type": machineType})
+               } else {
+                       exit.WithCodeT(exit.Unavailable, `Unable to reach {{.driver_name}} {{.machine_type}} for "{{.cluster}}"`, out.V{"driver_name": cc.Driver, "cluster": cc.Name, "machine_type": machineType})
+               }
+       }
        return h, nil
 }
 
diff --git a/pkg/minikube/machine/stop.go b/pkg/minikube/machine/stop.go
index fafe09e..717e3fa 100644
--- a/pkg/minikube/machine/stop.go
+++ b/pkg/minikube/machine/stop.go
@@ -52,6 +52,7 @@ func stop(h *host.Host) error {
                }
        }
 
+       if h.DriverName != driver.Generic {
                if err := h.Stop(); err != nil {
                        glog.Infof("stop err: %v", err)
                        st, ok := err.(mcnerror.ErrHostAlreadyInState)
@@ -61,6 +62,7 @@ func stop(h *host.Host) error {
                        }
                        return &retry.RetriableError{Err: errors.Wrap(err, "stop")}
                }
+       }
        glog.Infof("stop complete within %s", time.Since(start))
        return nil
 }

So it will be much smaller, if we implement the minikube "variant" of those ?

[afbjorklund]

Since this is unlikely to see any proper tests set up in that time, let's move it to after v1.10

[afbjorklund]

Will drop from 1.11 as well, maybe open a new PR for 1.12 if the rewrite is ready in time

[codecov-commenter]

Codecov Report

Merging #6873 into master will decrease coverage by 0.03%.
The diff coverage is 27.77%.

@@            Coverage Diff             @@
##           master    #6873      +/-   ##
==========================================
- Coverage   34.46%   34.42%   -0.04%     
==========================================
  Files         147      147              
  Lines        9428     9443      +15     
==========================================
+ Hits         3249     3251       +2     
- Misses       5780     5793      +13     
  Partials      399      399              

Impacted Files	Coverage Δ
cmd/minikube/cmd/delete.go	21.45% <0.00%> (ø)
cmd/minikube/cmd/start.go	13.73% <0.00%> (ø)
pkg/minikube/cluster/ip.go	0.00% <0.00%> (ø)
pkg/minikube/driver/driver.go	60.00% <ø> (ø)
pkg/minikube/machine/stop.go	31.57% <12.50%> (+1.84%)	⬆️
pkg/minikube/machine/fix.go	36.80% <42.85%> (-1.51%)	⬇️