Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a driver check for existing machines #523

Closed
wants to merge 1 commit into from

Conversation

r2d4
Copy link
Contributor

@r2d4 r2d4 commented Aug 25, 2016

Not really sure if this the right way to fix this, but I thought I'd start a discussion.

This prevents minikube from silently falling back to the previously
configured driver instead of the one you've specified.

Right now, minikube start will silently fall back to an existing machine on subsequent calls. For example, if you had previously run a minikube start --vm-driver=virtualbox then a minikube start --vm-driver=kvm would ignore the flag and restart the virtualbox based minikube.

On the flip side, this could be annoying if you run minikube start --vm-driver=kvm and then minikube start would give you an error text thinking that you meant to use the default (virtualbox). Of course, this is slightly alleviated now that you can use a config or env var to specify your vm-driver.

nit: I used an os.Exit instead of a glog.Errorf because we run this function in a retry loop and it would print this message 3 times otherwise.

ref #519 not the underlying issue, but might have helped alert the user something was up

@codecov-io
Copy link

codecov-io commented Aug 25, 2016

Current coverage is 23.59% (diff: 100%)

Merging #523 into master will decrease coverage by 8.76%

@@             master       #523   diff @@
==========================================
  Files            44         31    -13   
  Lines          1879       1153   -726   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits            608        272   -336   
+ Misses         1147        840   -307   
+ Partials        124         41    -83   

Powered by Codecov. Last update f58c9ca...03e6da8

if driverSpecified != "" && driverSpecified != currentDriver {
fmt.Printf(
"Error: specified vm-driver '%s' but there is already a host configured with driver '%s'. Please try running a [minikube delete] first if you would like to run minikube with driver '%s'.\n", driverSpecified, currentDriver, driverSpecified)
os.Exit(0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exit 1, assuming this is an error state?

@jimmidyson
Copy link
Member

How about offering the user the option to automatically recreate the vm with the changed driver? Forking delete, & then system execing start with new driver flag?

// This is the name of the driver that is currently used with minikube
currentDriver := h.Driver.DriverName()

if driverSpecified != "" && driverSpecified != currentDriver {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. Could we turn this into a general validation here, stuff like CPUs, memory, etc. are also confusing in that they can't be changed with another start invocation.

I guess you could turn this into a loop over a list of flags that require a restart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we change the resources? Future enhancement perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we could try. Virtualbox definitely requires a restart, which I guess we could do here but it might be surprising.

This prevents minikube from silently falling back to the previously
configured driver instead of the one you've specified.
@aaron-prindle
Copy link
Contributor

@minikube-bot retest this please

@r2d4 r2d4 closed this Oct 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants