Fix `minikube start` in order to be able to start VM even if machine does not exist #5730

govargo · 2019-10-25T16:02:46Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When minikube machine does not exist, the Virtual Machine fails starting.
If the host config exists but machine does not exist, we should start recreate VM automatically.
This PR fixes out of control error.

Which issue(s) this PR fixes:

Fixes #5509

Does this PR introduce a user-facing change?

Yes.
This PR fixes bug in order to minikube start command work successfully even if machine does not exist.

Before this PR, minikube start command fails, when machine does not exist

1. minikube start (and stop command by Ctrl + C after this)
minikube start
😄  minikube v1.4.0 on Darwin 10.14.6
🔥  Creating virtualbox VM (CPUs=2, Memory=6500MB, Disk=20000MB) ...
^C

# Restart Minikube
2. minikube start
😄  minikube v1.4.0 on Darwin 10.14.6
💡  Tip: Use 'minikube start -p <name>' to create a new cluster, or 'minikube delete' to delete this one.
🔄  Retriable failure: Error getting state for host: machine does not exist

After this PR, minikube start command success, even if machine does not exist

1. minikube start (and stop command by Ctrl + C after this)
$ out/minikube start
😄  minikube v1.5.0-beta.0 on Darwin 10.14.6
🔥  Creating virtualbox VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
^C

# Restart Minikube
2. out/minikube start
😄  minikube v1.5.0-beta.0 on Darwin 10.14.5
💡  Tip: Use 'minikube start -p <name>' to create a new cluster, or 'minikube delete' to delete this one.
🙄  "machine minikube" does not exist. Proceeding ahead with recreating VM.
🔥  Creating virtualbox VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2019-10-25T16:02:54Z

Hi @govargo. 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.

minikube-bot · 2019-10-25T16:03:02Z

Can one of the admins verify this patch?

k8s-ci-robot · 2019-10-25T16:03:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: govargo
To complete the pull request process, please assign sharifelgamal
You can assign the PR to them by writing /assign @sharifelgamal 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

govargo · 2019-10-25T16:04:21Z

/assign @sharifelgamal

pkg/minikube/cluster/cluster.go

medyagh · 2019-10-25T17:21:20Z

@minikube-bot OK to test

medyagh · 2019-10-25T17:26:18Z

@govargo that is a good improvement ! I was always annoyed by ctrl+c destroying things. thank you for working on this

codecov-io · 2019-10-26T06:46:11Z

Codecov Report

Merging #5730 into master will decrease coverage by 0.07%.
The diff coverage is 20.63%.

@@            Coverage Diff             @@
##           master    #5730      +/-   ##
==========================================
- Coverage    38.1%   38.02%   -0.08%     
==========================================
  Files         137      137              
  Lines        8690     8751      +61     
==========================================
+ Hits         3311     3328      +17     
- Misses       4963     5007      +44     
  Partials      416      416

Impacted Files	Coverage Δ
cmd/minikube/cmd/start.go	`22.53% <0%> (-0.07%)`	⬇️
pkg/minikube/cluster/fix.go	`32.33% <21.31%> (-8.21%)`	⬇️
pkg/minikube/cluster/status.go	`42.42% <0%> (+6.06%)`	⬆️
pkg/minikube/cluster/delete.go	`45.83% <0%> (+8.33%)`	⬆️

medyagh · 2019-10-26T19:09:53Z

/retest this please

RA489 · 2019-10-30T13:49:08Z

/ok-to-test

RA489 · 2019-10-30T13:52:06Z

/ok-to-test

minikube-bot · 2019-11-04T23:56:14Z

Old binary: [187.551600129 178.817823191 186.470780925]
New binary: [190.090872852 187.572592764 184.947368562]
Average Old: 184.280068
Average New: 187.536945

medyagh · 2019-11-06T01:03:16Z

pkg/minikube/cluster/cluster.go

@@ -112,6 +113,18 @@ func StartHost(api libmachine.API, config cfg.MachineConfig) (*host.Host, error)
 	s, err := h.Driver.GetState()
 	glog.Infoln("Machine state: ", s)
 	if err != nil {
+		// If VirtualBox Machine does not exist due to user interrupt cancel(i.e. Ctrl + C), recreate VirtualBox Machine
+		if driver.BareMetal(config.VMDriver) || config.VMDriver == driver.VirtualBox {


could you please remind me why we need this check for none driver ?
driver.BareMetal(config.VMDriver) ?

Thanks for your point out.
I thought that Mock driver judgement is needed and driver.BareMetal is its method.
But I misunderstood.

I modified this driver.BareMetal → config.VMDriver == driver.Mock.

medyagh · 2019-11-06T01:05:25Z

the tests failures are not related to this PR

medyagh · 2019-11-06T01:06:04Z

@govargo sorry for the delay in the PR review, I left one comment.

govargo · 2019-11-06T07:23:18Z

@medyagh

Thanks for your review.

I merged v1.5.2 codebase into this branch.
And I noticed that v1.5.2 contains many changes in the codebase.

In v1.5.2, func startMachine has preExists flag.
If preExists flag is true, the bootstrapper skips pulling necessary images.
If machine does not exist, of course the VM does not have kubeadm config files but skips pulling.
So, minikube fails to starting kubernetes in VM due to absence of kubeadm config files.

minikube/cmd/minikube/cmd/start.go

Line 337 in a583cbe

mRunner, preExists, machineAPI, host := startMachine(&config)

minikube/cmd/minikube/cmd/start.go

Lines 1181 to 1186 in a583cbe

    
           if isUpgrade || !preexisting { 
        
           	out.T(out.Pulling, "Pulling images ...") 
        
           	if err := bs.PullImages(kc); err != nil { 
        
           		out.T(out.FailureType, "Unable to pull images, which may be OK: {{.error}}", out.V{"error": err}) 
        
           	} 
        
           }

I changed the following code to avoid skipping pulling necessary images.

https://github.com/kubernetes/minikube/pull/5730/files#diff-221fdad1b39f75fb2d0214c25944dcb3R116-R134

https://github.com/kubernetes/minikube/pull/5730/files#diff-ed78cd81495af3964aba5fbc898c33a2R984-R991

minikube-bot · 2019-11-06T07:33:35Z

Old binary: [192.644036866 200.378705655 178.628237978]
New binary: [176.892592363 175.28189630399999 193.400321535]
Average Old: 190.550327
Average New: 181.858270

govargo · 2019-11-12T15:55:39Z

Updated code to resolve conflict(latest codebase edited the same file)

minikube-bot · 2019-11-12T15:58:23Z

Error: building minikube at head: updating minikube master branch: running [git pull origin master] in /home/performance-monitor/minikube:
From https://github.com/kubernetes/minikube

branch master -> FETCH_HEAD
error: Your local changes to the following files would be overwritten by merge:
go.mod
Please commit your changes or stash them before you merge.
Aborting
Updating 1d0ca6c..e611b38
: exit status 1

govargo · 2019-11-13T10:13:29Z

Umm.. ↑ Is there anything I can do?

Updating 1d0ca6c..e611b38

It looks master commit range...

medyagh · 2019-12-21T05:18:35Z

/retest this please

govargo · 2019-12-22T13:24:51Z

I rebased for the latest codebase.

medyagh

this is truely a great PR ! I just have one request change, to make an Error type called ErrMachineNotFound which is kndown to all drivers and all drivers return that if machine doesnt exist, so our test case code doesnt have to rely on virtuablox

pkg/minikube/cluster/cluster_test.go

pkg/minikube/tests/driver_mock.go

pkg/minikube/cluster/cluster_test.go

pkg/minikube/cluster/cluster.go

govargo · 2019-12-23T07:54:30Z

@medyagh
Thank you for the review.
As you mentioned, the ErrorType will be better for all drivers.
I'll check it and work on later.

govargo · 2020-01-11T09:39:02Z

we could make our own ErrorType for not found and then make all drivers return that so we can use it as a generic machine not found across all drivers

I investigated that where the drivers(& registries) return machine status.
And I found it returned in disjointed places(e.g. docker/machine, minikube repository).

I think that I will make check function which check MachineDoesNotExist in somewhere file.
And I will make ErrorType there.

In the check function, it will absorb the difference of all drivers error(hyperkit, kvm2, hyperv...).

I think I have to check the all driver's MachineDoesNotExist Error case.
So, it takes more time to check and test...
I'm sorry to keep you waiting. Please wait🙂

govargo · 2020-01-22T05:36:09Z

@medyagh
I updated following 2 point.
・Add new Error ErrMachineNotExist
・Check the all driver's error case - func isMachineExists

I use ErrMachineNotExist instead of ErrMachineNotFound because virtualbox & parallels use this ErrMachineNotExist.
I thought that the err name should be unified.
However, if you have different thought, I would like to hear that(any other review too).

medyagh · 2020-01-22T18:20:54Z

pkg/minikube/cluster/cluster.go

+
+// isMachineExists checks if virtual machine does not exist
+// if the virtual machine exists, return true
+func isMachineExists(vmDriver string, s state.State, err error) (bool, error) {


english nit: change this to, MachineExists , or DoesMachineExists. (shorter better)

renamed to machineExists func

medyagh · 2020-01-22T18:22:03Z

pkg/minikube/cluster/cluster.go

+			return false, ErrorMachineNotExist
+		}
+		return true, err
+	case driver.Mock:


add a case dor kic driver (Docker)

Added kic(docker) case

Added other case for kic.
I noticed kic return other behavior depending on whether the kic image is pulled or not.

https://github.com/kubernetes/minikube/pull/5730/files#diff-68ced156bfe1892e288da479d66535f2R244

minikube-pr-bot · 2020-01-25T00:11:53Z

All Times minikube: [ 99.347414 94.832747 98.741138]
All Times Minikube (PR 5730): [ 96.986774 93.176963 95.375147]

Average minikube: 97.640433
Average Minikube (PR 5730): 95.179628

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5730) |
+----------------------+-----------+--------------------+
| minikube v           |  0.284080 |           0.295561 |
| Creating kvm2        | 20.196651 |          19.998007 |
| Preparing Kubernetes | 50.157458 |          49.385994 |
| Pulling images       |  4.305111 |           3.794645 |
| Launching Kubernetes | 19.640035 |          18.824602 |
| Waiting for cluster  |  2.550031 |           2.381059 |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-02-03T15:05:36Z

All Times minikube: [ 96.461382 97.864359 96.592991]
All Times Minikube (PR 5730): [ 98.894388 95.593440 98.070681]

Average minikube: 96.972911
Average Minikube (PR 5730): 97.519503

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5730) |
+----------------------+-----------+--------------------+
| minikube v           |  0.290601 |           0.286606 |
| Creating kvm2        | 20.353344 |          19.812911 |
| Preparing Kubernetes | 49.022815 |          49.775150 |
| Pulling images       |  4.472485 |           3.421344 |
| Launching Kubernetes | 19.698978 |          19.782117 |
| Waiting for cluster  |  1.062089 |           2.551448 |
+----------------------+-----------+--------------------+

tstromberg · 2020-02-05T19:38:01Z

Do you mind fixing the merge conflict? Otherwise this PR appears to be ready to merge.

govargo · 2020-02-06T11:32:29Z

@tstromberg
I fixed merge conflict!
I adjusted to #6484.

minikube-pr-bot · 2020-02-06T11:42:09Z

All Times minikube: [ 98.037351 95.849441 94.035101]
All Times Minikube (PR 5730): [ 95.144312 96.140510 96.734421]

Average minikube: 95.973965
Average Minikube (PR 5730): 96.006414

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5730) |
+----------------------+-----------+--------------------+
| minikube v           |  0.122333 |           0.242949 |
| Creating kvm2        | 19.962555 |          20.168191 |
| Preparing Kubernetes | 50.250112 |          49.114025 |
| Pulling images       |  3.233085 |           3.974378 |
| Launching Kubernetes | 20.495308 |          19.654502 |
| Waiting for cluster  |  0.391611 |           1.223197 |
+----------------------+-----------+--------------------+

…t exist

…Name()

…t cycle

medyagh · 2020-02-10T18:37:04Z

the integreation test failures are all time out waiting for a pod doesnt seem to be related to this PR
/

medyagh · 2020-02-10T18:38:12Z

@govargo thank you very much for this PR ! and your persistence to fix all the code review comments.

…does not exist (kubernetes#5730) * fix startHost in order to be able to start VM even if machine does not exist * Update func StartHost() and its unit test no to use config.GetMachineName() * Add handle case when the kic image isn't pulled

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 25, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 25, 2019

k8s-ci-robot requested review from blueelvis and RA489 October 25, 2019 16:03

k8s-ci-robot assigned sharifelgamal Oct 25, 2019

medyagh reviewed Oct 25, 2019

View reviewed changes

pkg/minikube/cluster/cluster.go Outdated Show resolved Hide resolved

medyagh added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 26, 2019

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 30, 2019

medyagh requested changes Nov 6, 2019

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 21, 2019

medyagh requested changes Dec 22, 2019

View reviewed changes

pkg/minikube/cluster/cluster_test.go Show resolved Hide resolved

pkg/minikube/tests/driver_mock.go Show resolved Hide resolved

pkg/minikube/cluster/cluster_test.go Outdated Show resolved Hide resolved

pkg/minikube/cluster/cluster.go Outdated Show resolved Hide resolved

medyagh requested changes Jan 22, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2020

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2020

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 5, 2020

govargo added 4 commits February 10, 2020 22:12

fix startHost in order to be able to start VM even if machine does no…

2216604

…t exist

Update func StartHost() and its unit test no to use config.GetMachine…

82c8ccd

…Name()

Add comment why don't use cluster.ErrorMachineNotExist to avoid impor…

c9f58bc

…t cycle

Add handle case when the kic image isn't pulled

81c037c

medyagh merged commit e18e4d0 into kubernetes:master Feb 10, 2020

fmotrifork mentioned this pull request Feb 24, 2020

minikube 1.7.3 fishworks/fish-food#585

Merged

govargo deleted the fix-outof-vmcontrol branch March 6, 2020 10:15

Fix minikube start in order to be able to start VM even if machine does not exist #5730

Fix minikube start in order to be able to start VM even if machine does not exist #5730

Conversation

govargo commented Oct 25, 2019

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 25, 2019

minikube-bot commented Oct 25, 2019

k8s-ci-robot commented Oct 25, 2019

govargo commented Oct 25, 2019

medyagh commented Oct 25, 2019

medyagh commented Oct 25, 2019 • edited Loading

codecov-io commented Oct 26, 2019 • edited Loading

Codecov Report

medyagh commented Oct 26, 2019

RA489 commented Oct 30, 2019

RA489 commented Oct 30, 2019

minikube-bot commented Nov 4, 2019

medyagh Nov 6, 2019

Choose a reason for hiding this comment

govargo Nov 6, 2019

Choose a reason for hiding this comment

medyagh commented Nov 6, 2019

medyagh commented Nov 6, 2019

govargo commented Nov 6, 2019 • edited Loading

minikube-bot commented Nov 6, 2019

govargo commented Nov 12, 2019

minikube-bot commented Nov 12, 2019

govargo commented Nov 13, 2019

medyagh commented Dec 21, 2019

govargo commented Dec 22, 2019 • edited Loading

medyagh left a comment

Choose a reason for hiding this comment

govargo commented Dec 23, 2019

govargo commented Jan 11, 2020

govargo commented Jan 22, 2020

medyagh Jan 22, 2020

Choose a reason for hiding this comment

govargo Feb 3, 2020

Choose a reason for hiding this comment

medyagh Jan 22, 2020

Choose a reason for hiding this comment

govargo Feb 3, 2020

Choose a reason for hiding this comment

govargo Feb 6, 2020

Choose a reason for hiding this comment

minikube-pr-bot commented Jan 25, 2020

minikube-pr-bot commented Feb 3, 2020

tstromberg commented Feb 5, 2020

govargo commented Feb 6, 2020

minikube-pr-bot commented Feb 6, 2020

medyagh commented Feb 10, 2020

medyagh commented Feb 10, 2020

Fix `minikube start` in order to be able to start VM even if machine does not exist #5730

Fix `minikube start` in order to be able to start VM even if machine does not exist #5730

medyagh commented Oct 25, 2019 •

edited

Loading

codecov-io commented Oct 26, 2019 •

edited

Loading

govargo commented Nov 6, 2019 •

edited

Loading

govargo commented Dec 22, 2019 •

edited

Loading