Default to best-available local hypervisor rather than VirtualBox #5700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
tstromberg
requested review from
medyagh
and removed request for
josedonizetti and
afbjorklund
k8s-ci-robot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tstromberg 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 |
k8s-ci-robot
added
the
approved
Travis tests have failedHey @tstromberg, 1st Buildmake testTravisBuddy Request Identifier: 3ed33e80-f549-11e9-a1fe-396a3052531f |
|
@minikube-bot OK to test |
|
@minikube-bot OK to test |
|
Build is failing due to: |
tstromberg
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
tstromberg
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
Codecov Report
@@ Coverage Diff @@
## master #5700 +/- ##
==========================================
- Coverage 37.83% 37.75% -0.09%
==========================================
Files 102 106 +4
Lines 7601 7771 +170
==========================================
+ Hits 2876 2934 +58
- Misses 4347 4457 +110
- Partials 378 380 +2
|
k8s-ci-robot
added
size/XL
tstromberg
force-pushed
the
drv-select2
branch
from
5526b04
to
dcdac36
Compare
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
| @@ -30,6 +30,8 @@ VM_DRIVER="virtualbox" | |||
| JOB_NAME="VirtualBox_macOS" | |||
| EXTRA_ARGS="--bootstrapper=kubeadm" | |||
| PARALLEL_COUNT=3 | |||
| EXPECTED_DEFAULT_DRIVER="hyperkit" | |||
There was a problem hiding this comment.
If hyperkit is installed on the machine, this will be hyperkit as it takes precedence. I assume our VirtualBox mac also has hyperkit installed.
test/integration/a_serial_test.go
Outdated
|
|
||
| // Checking if the default driver meets expectations | ||
| if ExpectedDefaultDriver() == "" { | ||
| t.Logf("--expected-default-driver=%q, continuing", ExpectedDefaultDriver()) |
There was a problem hiding this comment.
I'm not sure I understand this log line
There was a problem hiding this comment.
Good point. I've moved this to another subtest "ExpectedDefaultDriver" so that I could have clearer behavior and messaging. PTAL.
There was a problem hiding this comment.
yeah, that makes more sense to me
sharifelgamal
approved these changes
Oct 24, 2019
β¦more appropriately
Improve VBoxManage detection on Windows Add missing install_test
tstromberg
force-pushed
the
drv-select2
branch
from
df4cd48
to
416f132
Compare
|
this is a solid PR ! I am so grateful that you are doing this ! it will make me as a user of minikube MUCH happier, output looks good I am wondering if we should have some sign in the console that it is auto detected ....it doesnt have to be big. maybe a * or (auto-detect) ? it would be nice to know when user specif icily chose the driver or we detected it for them |
medyagh
approved these changes
Oct 25, 2019
| @@ -194,7 +195,7 @@ func initKubernetesFlags() { | |||
|
|
|||
| // initDriverFlags inits the commandline flags for vm drivers | |||
| func initDriverFlags() { | |||
| startCmd.Flags().String("vm-driver", "", fmt.Sprintf("Driver is one of: %v (defaults to %s)", driver.SupportedDrivers(), driver.Default())) | |||
| startCmd.Flags().String("vm-driver", "", fmt.Sprintf("Driver is one of: %v (defaults to auto-detect)", driver.SupportedDrivers())) | |||
There was a problem hiding this comment.
just a random thought:
if auto-detect is not expensive, maybe add it to the help message. so the user doesnt have to guess what the auto-detect comes up with ?
There was a problem hiding this comment.
but thinking again, maybe not !
| @@ -32,6 +32,8 @@ JOB_NAME="HyperKit_macOS" | |||
| EXTRA_ARGS="--bootstrapper=kubeadm" | |||
| EXTRA_START_ARGS="" | |||
| PARALLEL_COUNT=3 | |||
| EXPECTED_DEFAULT_DRIVER="hyperkit" | |||
|
|
|||
| } | ||
|
|
||
| func createHypervHost(config cfg.MachineConfig) interface{} { | ||
| func configure(config cfg.MachineConfig) interface{} { |
There was a problem hiding this comment.
I would add a comment for this configure,, does what does it really do ?
and also maybe I would change the config var name to somethign shorter ?
| @@ -78,3 +85,33 @@ func createKVM2Host(mc config.MachineConfig) interface{} { | |||
| ConnectionURI: mc.KVMQemuURI, | |||
| } | |||
| } | |||
|
|
|||
| func status() registry.State { | |||
There was a problem hiding this comment.
I would add a comment for status or choose a better name ?
does it check status of hyperkit or does it check if the binary is available? or does it check if it is out of date version?
Done. |
medyagh
reviewed
Oct 25, 2019
| @@ -549,6 +549,8 @@ func selectDriver(oldConfig *cfg.Config) string { | |||
| pick, alts := driver.Choose(options) | |||
| if len(options) > 1 { | |||
| out.T(out.Sparkle, `Automatically selected the '{{.driver}}' driver (alternates: {{.alternates}})`, out.V{"driver": pick.Name, "alternates": alts}) | |||
| } else { | |||
| out.T(out.Sparkle, `Automatically selected the '{{.driver}}' driver`, out.V{"driver": pick.Name}) | |||
There was a problem hiding this comment.
Maybe put the version of Virsh or Hyperkit.... would help a lot in debugging ?
There was a problem hiding this comment.
Displaying the hypervisor version would be extremely useful, but also a fair bit of work to do across hypervisors, so I would rather not include it in this already large PR.
There was a problem hiding this comment.
This can also be done as part of hte drivers themselves, as libmachine allows to log these values... this way using the verbose/debug logs for this would collect them. This is also what we do for CRC.
medyagh
approved these changes
Oct 25, 2019
There was a problem hiding this comment.
love that it shows your potential alternatives:
(tstromberg-drv-select2)$ ./out/minikube start -p p3
π [p3] minikube v1.5.0-beta.0 on Debian rodete
β¨ Automatically selected the 'kvm2' driver (alternates: [none])
πΎ Downloading driver docker-machine-driver-kvm2:
> docker-machine-driver-kvm2.sha256: 65 B / 65 B [-------] 100.00% ? p/s 0s
> docker-machine-driver-kvm2: 13.87 MiB / 13.87 MiB 100.00% 13.49 MiB p/s
πΏ Downloading VM boot image ...
> minikube-v1.5.0-beta.0.iso.sha256: 65 B / 65 B [-------] 100.00% ? p/s 0s
> minikube-v1.5.0-beta.0.iso: 143.77 MiB / 143.77 MiB 100.00% 124.91 MiB p
π₯ Creating kvm2 VM (CPUs=2, Memory=2000MB, Disk=20000MB)
minor knit
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4614
Output change for auto-selection:
Warning-only output change for when an unhealthy driver is specified:
Output if no driver was found usable: