Consider adding command line options to libmachine API

[nirs]

We must consider minikube command line options in Driver.Start(). The current way to make it work is adding run.CommandOptions to the CommonDriver, so the driver can access command line options later. To make this work we add run.CommandOptions to the machine.LocalClient.

When libmachine API is part of minikube, we can simplify by passing command line options Driver.Start().

Depends on #21647

[afbjorklund]

The "current" way in the libmachine API is to use the command-line flags from docker-machine, but that model didn't suit minikube very well - which is why it was re-implemented in the minikube CommonDriver.

	// GetCreateFlags returns the mcnflag.Flag slice representing the flags
	// that can be set, their descriptions and defaults.
	GetCreateFlags() []mcnflag.Flag

	// SetConfigFromFlags configures the driver with the object that was returned
	// by RegisterCreateFlags
	SetConfigFromFlags(opts DriverOptions) error

Note: RegisterCreateFlags was renamed to GetCreateFlags, but naturally not (all) the documentation was updated...

docker-archive-public/docker.machine@c8edb33

Typically this and the provisioner were the things that were changed, when the drivers were changed into minikube-specific versions. Like when going from "qemu" to "qemu2" and from "kvm" to "kvm2".

	// Do the actual provisioning piece:
	//     1. Set the hostname on the instance.
	//     2. Install Docker if it is not present.
	//     3. Configure the daemon to accept connections over TLS.
	//     4. Copy the needed certificates to the server and local config dir.
	//     5. Configure / activate swarm if applicable.
	Provision(swarmOptions swarm.Options, authOptions auth.Options, engineOptions engine.Options) error

Part of this was complicated because the libmachine API didn't know what drivers were available...

That is, the available flags had to be added dynamically. But in minikube, all drivers are hardcoded.

[afbjorklund]

Making the provisioner less docker and swarm specific is a separate topic, from this (the create flags):

• Make the provisioner interface support more container runtimes #10883

Currently the provisioner doesn't do anything*, the system is supposed to have a container runtime.

* well it does some things, like the certs and config. But it doesn't actually install containerd or cri-o

[afbjorklund]

minikube/pkg/drivers/common

Here is the current CommonDriver implementation, you can see that they are just stubs:

// CommonDriver is the common driver base class
type CommonDriver struct{}

// GetCreateFlags is not implemented yet
func (d *CommonDriver) GetCreateFlags() []mcnflag.Flag {
        return nil
}

// SetConfigFromFlags is not implemented yet
func (d *CommonDriver) SetConfigFromFlags(_ drivers.DriverOptions) error {
        return nil
}

minikube/pkg/minikube/registry

Instead the options are set (twice) in the (broken) registry, creating a tight coupling...

// Configurator emits a struct to be marshalled into JSON for Machine Driver
type Configurator func(config.ClusterConfig, config.Node) (interface{}, error)

// Loader is a function that loads a byte stream and creates a driver.
type Loader func() drivers.Driver

// DriverDef defines how to initialize and load a machine driver
type DriverDef struct {
...
        // Config is a function that emits a configured driver struct
        Config Configurator

        // Init is a function that initializes a machine driver, if built-in to the minikube binary
        Init Loader
...
}

---

It is broken because it initializes the driver structure twice, first one without any config:

• none driver can't be used with non-Docker runtimes (looks for "docker" executable) #5549

[nirs]

It is broken because it initializes the driver structure twice, first one without any config:

• none driver can't be used with non-Docker runtimes (looks for "docker" executable) #5549

The registry is not broken, but the code is complicated and inefficient.

The driver is always created with Init (expect the deprecated hyperkit driver). This creates zero value of the driver. #21683 adds command line options to the driver at this point.

When creating the profile, we call Config which creates a new driver and configure it based on the command line options. We marshal the driver to json, and then unmarshal the JSON to populate the driver created by Init.

minikube/pkg/minikube/machine/start.go

Line 124 in 5ca02f9

func createHost(api libmachine.API, cfg *config.ClusterConfig, n *config.Node) (*host.Host, error) {

	def := registry.Driver(cfg.Driver)
	if def.Empty() {
		return nil, fmt.Errorf("unsupported/missing driver: %s", cfg.Driver)
	}
	dd, err := def.Config(*cfg, *n)
	if err != nil {
		return nil, errors.Wrap(err, "config")
	}
	data, err := json.Marshal(dd)
	if err != nil {
		return nil, errors.Wrap(err, "marshal")
	}

	h, err := api.NewHost(cfg.Driver, data)
	if err != nil {
		return nil, errors.Wrap(err, "new host")
	}

minikube/pkg/minikube/machine/client.go

Line 90 in 5ca02f9

func (api *LocalClient) NewHost(drvName string, rawDriver []byte) (*host.Host, error) {

	def := registry.Driver(drvName)
	if def.Empty() {
		return nil, fmt.Errorf("driver %q does not exist", drvName)
	}
	if def.Init == nil {
		return api.legacyClient.NewHost(drvName, rawDriver)
	}
	d := def.Init()
	err := json.Unmarshal(rawDriver, d)
	if err != nil {
		return nil, errors.Wrapf(err, "Error getting driver %s", string(rawDriver))
	}

When loading an existing driver we get the JSON from the config file, and use the same flow without calling Config:

minikube/pkg/minikube/machine/client.go

Line 129 in 5ca02f9

func (api *LocalClient) Load(name string) (*host.Host, error) {

	def := registry.Driver(h.DriverName)
	if def.Empty() {
		return nil, fmt.Errorf("driver %q does not exist", h.DriverName)
	}
	if def.Init == nil {
		return api.legacyClient.Load(name)
	}
	h.Driver = def.Init()
	return h, json.Unmarshal(h.RawDriver, h.Driver)

Ideally the code to create a driver can be kept inside the registry and the drivers and avoid unneeded JSON conversion.

[afbjorklund]

The registry is not broken

I was referring to the bug where the config takes effect to late, and thus it always requires docker to run containerd...

We have worked around this by creating a symlink from docker to something else, but it still sounds like broken to me?

Then again, it is not "supported".

Using the 'containerd' runtime with the 'none' driver is an untested configuration!

I'm thinking to just deprecate it? (none driver, ssh driver)

The main usage was the interactive kubernetes tutorial*, which has now been removed...

---

Now it just uses minikube start, which means that one can use the normal drivers instead of using a custom VM.

And it was always better to set up the host environment for the user, than logging in directly to the control plane.

* https://kubernetes.io/docs/tutorials/kubernetes-basics/create-cluster/cluster-intro/

• Outdated description of minikube in upstream documentation #15651

[afbjorklund]

I did some more cleanup of the legacy machine code, so now it should be relatively clean for go1.17 and golangci-lint v1.

• Run golangci-lint on the legacy code minikube-machine/machine#16

• Cherry-pick fixes from gitlab-machine minikube-machine/machine#9

• Bump minikube-machine go version minikube-machine/machine#8

One thing about the current setup is that none of the test code is actually run, no CI in place and no tests in "vendor"

So should probably update the "vendor" import PR, to also grab the unit tests from the original machine repository:

• Internalize vendor/github.com/docker/machine #21647

drivers/hyperv/hyperv_test.go
drivers/virtualbox/network_test.go
drivers/virtualbox/vm_test.go
drivers/virtualbox/virtualbox_linux_test.go
drivers/virtualbox/vtx_test.go
drivers/virtualbox/disk_test.go
drivers/virtualbox/virtualbox_test.go
drivers/virtualbox/vbm_test.go
drivers/virtualbox/virtualbox_darwin_test.go
libmachine/log/fmt_machine_logger_test.go
libmachine/log/log_test.go
libmachine/log/history_recorder_test.go
libmachine/cert/cert_test.go
libmachine/state/state_test.go
libmachine/provision/serviceaction/service_action_test.go
libmachine/provision/provisiontest/sshcommander_test.go
libmachine/provision/os_release_test.go
libmachine/provision/utils_test.go
libmachine/provision/pkgaction/pkg_action_test.go
libmachine/host/migrate_v0_v1_test.go
libmachine/host/migrate_v1_v2_test.go
libmachine/host/host_test.go
libmachine/host/migrate_test.go
libmachine/ssh/client_test.go
libmachine/ssh/keys_test.go
libmachine/ssh/ssh_test.go
libmachine/shell/shell_windows_test.go
libmachine/shell/shell_unix_test.go
libmachine/shell/shell_test.go
libmachine/persist/filestore_test.go
libmachine/persist/persisttest/fakestore_test.go
libmachine/mcnutils/utils_test.go
libmachine/mcnutils/b2d_test.go
libmachine/check/check_test.go
libmachine/drivers/base_test.go
libmachine/drivers/rpc/server_driver_test.go
libmachine/drivers/serial_test.go
libmachine/drivers/plugin/localbinary/plugin_test.go
libmachine/versioncmp/compare_test.go
libmachine/crashreport/crash_report_test.go
libmachine/crashreport/os_windows_test.go

And to include the latest "lint" fixes, there are a bunch in third_party and in vendor/github.com/docker/machine...

169 issues:
* gocritic: 10
* gocyclo: 1
* ineffassign: 1
* revive: 52
* staticcheck: 88
* unconvert: 12
* unparam: 3
* unused: 2

Currently these are hidden on master.

Then again, it is "red" enough as it is...