-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fallback to alternate drivers on failure #7389
fallback to alternate drivers on failure #7389
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sharifelgamal 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:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the surface - the PR does the right thing. I'd like to constrain what gets considered for fall-through, so that we can improve the UX for non-driver configuration errors, such as users passing in bad configuration to the apiserver.
How bad would it be to split the code up so that the driver fall-through only occurs on machine provisioning failures? That should cover ~98% of the issues that driver fall-through would fix without forcing users with bad apiserver flags to wait 20 minutes to fall through each driver they have.
cmd/minikube/cmd/start.go
Outdated
if err != nil && !specified { | ||
// Walk down the rest of the options | ||
for _, alt := range alts { | ||
out.WarningT("Startup with {{.old_driver}} driver failed, trying with {{.new_driver}}.", out.V{"old_driver": ds.Name, "new_driver": alt.Name}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start by outputting the error message. It's unlikely to be terribly long, and it should be very informative.
cmd/minikube/cmd/start.go
Outdated
// Delete the existing cluster and try again with the next driver on the list | ||
profile, err := config.LoadProfile(ClusterFlagValue()) | ||
if err != nil { | ||
out.ErrT(out.Meh, `"{{.name}}" profile does not exist, trying anyways.`, out.V{"name": ClusterFlagValue()}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be hidden in a log message.
cmd/minikube/cmd/start.go
Outdated
@@ -388,6 +423,8 @@ func runStart(cmd *cobra.Command, args []string) { | |||
if err := showKubectlInfo(kubeconfig, k8sVersion, cc.Name); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be moved out of startWithDriver
and into the main loop. It raises no error, but it also has nothing to do with drivers.
This is a super neat feature! Thank you. |
Makes sense, it's just going to require a more substantial refactor of node.Start. I'll work on it. |
This should be ready for review again. |
/ok-to-test |
Error: running mkcmp: exit status 1 |
Looks great! Thought for future development: I wonder if we should remember the default driver that finally succeeds. I guess we sort of do, because we only drop the driver if the profile is deleted. |
I tested this PR locally by:
Unfortunately, it didn't seem to fallback in this case: https://gist.github.com/tstromberg/bdd8f5c3a54ab2f4ec5ef5570fcc1d46 Any chance you could add a fallback integration test to make sure this doesn't get accidentally broken in the future? It's somewhat tricky, but so is |
Good catch, it's because part of provision are still exiting instead of returning the error. I'll fix it. I can try to add a test as well, my worry is that it will be brittle. As for making sure for fallback, I figured the list of alternate drivers that we return was a pretty safe list to iterate down, any reason to not think so? |
All Times minikube: [ 67.876060 65.834906 65.802959] Average minikube: 66.504641 Averages Time Per Log
|
Times for minikube: [68.61437737799999 63.79203957799999 65.86677142799999] Times for Minikube (PR 7389): [64.82932214 64.66789235799999 64.722607542] Averages Time Per Log
|
Times for minikube: [66.838103044 64.32249093799999 64.645972742] Times for Minikube (PR 7389): [65.491807319 66.289239556 66.402680428] Averages Time Per Log
|
Times for minikube: [66.142932129 65.477336764 64.878701535] Times for Minikube (PR 7389): [66.820549268 67.329258823 87.61991428200001] Averages Time Per Log
|
Times for minikube: [68.941013188 67.50117359000001 65.659430288] Times for Minikube (PR 7389): [66.569717708 65.78811284500001 63.950682795] Averages Time Per Log
|
Times for minikube: [65.28964660499999 65.799649482 67.564352931] Times for Minikube (PR 7389): [64.47649222899999 68.37589096399999 68.392623414] Averages Time Per Log
|
Times for minikube: [68.637234354 66.245775554 69.42458668] Times for Minikube (PR 7389): [68.379620515 65.797629348 64.993510062] Averages Time Per Log
|
Times for minikube: [67.451256079 65.720750475 65.56211417499999] Times for Minikube (PR 7389): [66.76690671099999 64.385185313 66.152168297] Averages Time Per Log
|
Times for minikube: [67.532975101 72.424078156 65.45978481700003] Times for Minikube (PR 7389): [65.428628975 68.92717355500001 68.109054576] Averages Time Per Log
|
Please merge when ready. |
Times for minikube: [67.93204453999999 65.10946705 67.430150645] Times for Minikube (PR 7389): [67.644456516 65.822593143 66.893802751] Averages Time Per Log
|
Times for minikube: [67.603174412 67.073093269 66.14986645600001] Times for Minikube (PR 7389): [67.865186992 67.294624359 66.32565942500001] Averages Time Per Log
|
Times for minikube: [70.59345408399999 67.8515118 67.24457781299999] Times for Minikube (PR 7389): [67.30720398200002 65.42459162399999 66.23458923800001] Averages Time Per Log
|
Times for minikube: [64.612109691 66.194494277 63.025563761] Times for Minikube (PR 7389): [68.96494924 64.216728601 62.42923606] Averages Time Per Log
|
Times for minikube: [71.553304557 65.434464812 64.755014035] Times for Minikube (PR 7389): [65.909565411 62.134405146999995 72.403847718] Averages Time Per Log
|
Fixes #7216
If the user doesn't explicitly supply a choice of driver to
minikube start
via thedriver
flag or config and there's a failure, we will now go down the list of alternate driver choices and retry starting the cluster with that.Specifying a driver will still fail as expected
Starting Kubernetes fails as expected as well, we only fallback on provisioning failures: