Add --vm flag for users who want to autoselect only VM's #7068

vikkyomkar · 2020-03-17T07:54:46Z

k8s-ci-robot · 2020-03-17T07:54:54Z

Hi @vikkyomkar. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-03-17T07:55:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vikkyomkar
To complete the pull request process, please assign afbjorklund
You can assign the PR to them by writing /assign @afbjorklund in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

vikkyomkar · 2020-03-17T07:56:33Z

/assign @tstromberg

minikube-bot · 2020-03-17T07:57:03Z

Can one of the admins verify this patch?

codecov-io · 2020-03-17T08:25:51Z

Codecov Report

Merging #7068 into master will increase coverage by 0.01%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master    #7068      +/-   ##
==========================================
+ Coverage   36.94%   36.95%   +0.01%     
==========================================
  Files         146      146              
  Lines        9094     9102       +8     
==========================================
+ Hits         3360     3364       +4     
- Misses       5322     5325       +3     
- Partials      412      413       +1

Impacted Files	Coverage Δ
cmd/minikube/cmd/start.go	`33.91% <50%> (+0.11%)`	⬆️
pkg/minikube/driver/driver.go	`64.94% <55.55%> (-1.72%)`	⬇️

tstromberg · 2020-03-17T18:51:37Z

/ok-to-test

tstromberg · 2020-03-17T18:57:16Z

pkg/minikube/driver/driver.go

-
+	if vm {
+		for _, ds := range options {
+			if IsVM(ds.Name) {


Suggestion: Now that I see how this looks, rather than applying the filter in the Choices function, could you pass the value to registry.Available and add the filtering in that function instead?

minikube/pkg/minikube/registry/global.go

Line 62 in 07e8420

func Available() []DriverState {

This would be a slight performance improvement, as registry.Available() would not have to calldriver.Status() on non-VM drivers if the result is going to be ignored anyways.

sure I will make the suggested changes soon

minikube-pr-bot · 2020-03-17T18:58:55Z

Error: running mkcmp: exit status 1

tstromberg · 2020-03-21T17:35:13Z

Need any help with this PR?

minikube-pr-bot · 2020-03-22T14:10:25Z

All Times minikube: [ 72.150463 74.829091 74.539236]
All Times Minikube (PR 7068): [ 72.291243 70.032209 69.697311]

Average minikube: 73.839596
Average Minikube (PR 7068): 70.673588

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7068) |
+----------------------+-----------+--------------------+
| minikube v           |  0.285743 |           0.272458 |
| Creating kvm2        | 45.314773 |          44.841824 |
| Preparing Kubernetes | 25.769162 |           0.903183 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |          22.938710 |
| Waiting for cluster  |           |           0.080126 |
+----------------------+-----------+--------------------+

vikkyomkar · 2020-03-22T14:11:05Z

Need any help with this PR?

@tstromberg
I have made the changes. I had to write IsVM function separately in registry folder to avoid "import cycle error" b/w driver and registry.

tstromberg

More comments to clean things up a bit, especially since there was some code duplication required. Thank you so much for your patience!

tstromberg · 2020-03-23T15:19:09Z

pkg/minikube/registry/global.go

@@ -76,7 +110,13 @@ func Available() []DriverState {
 			priority = Unhealthy
 		}

-		sts = append(sts, DriverState{Name: d.Name, Priority: priority, State: s})
+		if vm {


With go, we try to avoid if/else statements when possible, particularly in a loop. Kind of along the lines of https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

This would be clearer:

if vm && !IsVM(d.Name) { continue }

tstromberg · 2020-03-23T15:19:20Z

pkg/minikube/registry/global.go

@@ -59,7 +93,7 @@ func Driver(name string) DriverDef {
 }

 // Available returns a list of available drivers in the global registry
-func Available() []DriverState {
+func Available(vm bool) []DriverState {


this might be clearer if it was named vmOnly

tstromberg · 2020-03-23T15:21:23Z

pkg/minikube/registry/global.go

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


I know you just copy and pasted the previous function, but because the other functions aren't used, just inline it:

if name == Docker || name == Mock || name == None || name == Podman { return true } Then you can remove IsKic and IsMock.

tstromberg · 2020-03-23T15:21:58Z

pkg/minikube/registry/global.go

+}
+
+// IsVM checks if the driver is a VM
+func IsVM(name string) bool {


rename this to isVM -- it does not need to be made public.

tstromberg · 2020-03-24T19:24:44Z

Heads up: I plan to merge this PR in it's current state today, so that v1.9.0 users can enjoy an improved experience. We can always address the final code comments in a follow-up PR.

Thank you again for contributing this.

vikkyomkar added 2 commits March 17, 2020 13:21

Add --vm flag for users who want to autoselect only VM's

e053e4f

Add --vm flag for users who want to autoselect only VM's

3216a03

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2020

k8s-ci-robot requested review from afbjorklund and blueelvis March 17, 2020 07:55

Add --vm flag for users who want to autoselect only VM's

37b23cf

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 17, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2020

tstromberg reviewed Mar 17, 2020

View reviewed changes

updated as per suggestion

6ee7e6f

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 22, 2020

tstromberg suggested changes Mar 23, 2020

View reviewed changes

tstromberg merged commit 5a316b3 into kubernetes:master Mar 25, 2020

fmotrifork mentioned this pull request Mar 27, 2020

minikube 1.9.0 fishworks/fish-food#654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --vm flag for users who want to autoselect only VM's #7068

Add --vm flag for users who want to autoselect only VM's #7068

vikkyomkar commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

vikkyomkar commented Mar 17, 2020

minikube-bot commented Mar 17, 2020

codecov-io commented Mar 17, 2020

tstromberg commented Mar 17, 2020

tstromberg Mar 17, 2020

vikkyomkar Mar 19, 2020

minikube-pr-bot commented Mar 17, 2020

tstromberg commented Mar 21, 2020

minikube-pr-bot commented Mar 22, 2020

vikkyomkar commented Mar 22, 2020

tstromberg left a comment

tstromberg Mar 23, 2020

tstromberg Mar 23, 2020

tstromberg Mar 23, 2020

tstromberg Mar 23, 2020

tstromberg commented Mar 24, 2020 •

edited

Loading

Add --vm flag for users who want to autoselect only VM's #7068

Add --vm flag for users who want to autoselect only VM's #7068

Conversation

vikkyomkar commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

k8s-ci-robot commented Mar 17, 2020

vikkyomkar commented Mar 17, 2020

minikube-bot commented Mar 17, 2020

codecov-io commented Mar 17, 2020

Codecov Report

tstromberg commented Mar 17, 2020

tstromberg Mar 17, 2020

Choose a reason for hiding this comment

vikkyomkar Mar 19, 2020

Choose a reason for hiding this comment

minikube-pr-bot commented Mar 17, 2020

tstromberg commented Mar 21, 2020

minikube-pr-bot commented Mar 22, 2020

vikkyomkar commented Mar 22, 2020

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg Mar 23, 2020

Choose a reason for hiding this comment

tstromberg Mar 23, 2020

Choose a reason for hiding this comment

tstromberg Mar 23, 2020

Choose a reason for hiding this comment

tstromberg Mar 23, 2020

Choose a reason for hiding this comment

tstromberg commented Mar 24, 2020 • edited Loading

tstromberg commented Mar 24, 2020 •

edited

Loading