Add hyperkit version check whether user's hyperkit is newer #5833

govargo · 2019-11-05T14:58:12Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

(Updated 2019/11/11)
I removed point1 of the PR due to duplication of #5840.

~~#### 1. FastFail if driver error occurres.~~

~~So far, even if driver error occurres(like not installed) minikube keeps to starting VM when minikube start.~~
~~Of course this finish with failure.~~
~~This PR fix this behaviour.~~
~~If driver error occurres, minikube stop starting VM(minikube exits).~~

2. Add version check of hyperkit which inspect user's hyperkit is newer than specific version

If hyperkit's version is older than expected, minikube start(hyperkit) will fail due to version.
Related Issue #5594, #5780

This PR add suggestion about hyperkit upgrade information, if user's hyperkit version is older than expected.

Which issue(s) this PR fixes:

Fixes #5478, #5821

Does this PR introduce a user-facing change?

~~Yes. This PR add hyperkit version check to minikube start command and FastFail if driver error occurres.~~
Yes. This PR add hyperkit version check to minikube start command.

(Removed FastFail due to duplication of #5840)

**Before this PR, minikube keeps to starting VM(but keep tp failing) **

$ minikube start --vm-driver=hyperkit
😄  minikube v1.5.1 on Darwin 10.14.5

⚠️  'hyperkit' driver reported a possible issue: exec: "hyperkit": executable file not found in $PATH
💡  Suggestion: Run 'brew install hyperkit'
📘  Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/

💾  Downloading driver docker-machine-driver-hyperkit:
    > docker-machine-driver-hyperkit.sha256: 65 B / 65 B [---] 100.00% ? p/s 0s
    > docker-machine-driver-hyperkit: 10.79 MiB / 10.79 MiB  100.00% 4.57 MiB p
🔑  The 'hyperkit' driver requires elevated permissions. The following commands will be executed:

    $ sudo chown root:wheel /Users/k-iso/.minikube/bin/docker-machine-driver-hyperkit
    $ sudo chmod u+s /Users/k-iso/.minikube/bin/docker-machine-driver-hyperkit


Password:
💿  Downloading VM boot image ...
    > minikube-v1.5.1.iso.sha256: 65 B / 65 B [--------------] 100.00% ? p/s 0s
    > minikube-v1.5.1.iso: 143.76 MiB / 143.76 MiB [-] 100.00% 64.66 MiB p/s 3s
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: create: Error creating machine: Error in driver during machine creation: new-ing Hyperkit: Could not find hyperkit executable
🔥  Deleting "minikube" in hyperkit ...
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: create: Error creating machine: Error in driver during machine creation: new-ing Hyperkit: Could not find hyperkit executable
🔥  Deleting "minikube" in hyperkit ...
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: create: Error creating machine: Error in driver during machine creation: new-ing Hyperkit: Could not find hyperkit executable
🔥  Deleting "minikube" in hyperkit ...
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: create: Error creating machine: Error in driver during machine creation: new-ing Hyperkit: Could not find hyperkit executable
🔥  Deleting "minikube" in hyperkit ...

💣  Unable to start VM: create: Error creating machine: Error in driver during machine creation: new-ing Hyperkit: Could not find hyperkit executable

😿  Sorry that minikube crashed. If this was unexpected, we would love to hear from you:
👉  https://github.com/kubernetes/minikube/issues/new/choose

After this PR, minikube fail fast

$ minikube start --vm-driver=hyperkit
😄  minikube v1.5.1 on Darwin 10.14.5

⚠️  'hyperkit' driver reported a possible issue: exec: "hyperkit": executable file not found in $PATH
💡  Suggestion: Run 'brew install hyperkit'
📘  Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/

After this PR, minikube checks user's hyperkit version

If user's hyperkit current version is 0.20180621 and specific version is 0.20190201, minikube suggests hyperkit upgrade information.

$ minikube start --vm-driver=hyperkit
😄  minikube v1.5.2 on Darwin 10.14.5

⚠️   'hyperkit' driver reported a possible issue: the installed hyperkit version (0.20180621) is older than the minimum recommended version (0.20191201)
💡  Suggestion: Run 'brew upgrade hyperkit'
📘  Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/

💾  Downloading driver docker-machine-driver-hyperkit:
    > docker-machine-driver-hyperkit.sha256: 65 B / 65 B [---] 100.00% ? p/s 0s
    > docker-machine-driver-hyperkit: 10.79 MiB / 10.79 MiB  100.00% 6.34 MiB p
🔑  The 'hyperkit' driver requires elevated permissions. The following commands will be executed:

    $ sudo chown root:wheel /Users/k-iso/.minikube/bin/docker-machine-driver-hyperkit
    $ sudo chmod u+s /Users/k-iso/.minikube/bin/docker-machine-driver-hyperkit


Password:
💿  Downloading VM boot image ...
    > minikube-v1.5.1.iso.sha256: 65 B / 65 B [--------------] 100.00% ? p/s 0s
    > minikube-v1.5.1.iso: 143.76 MiB / 143.76 MiB [-] 100.00% 17.73 MiB p/s 8s
🔥  Creating hyperkit VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.16.2 on Docker '18.09.9' ...
💾  Downloading kubeadm v1.16.2
💾  Downloading kubelet v1.16.2
🚜  Pulling images ...
🚀  Launching Kubernetes ...
⌛  Waiting for: apiserver
🏄  Done! kubectl is now configured to use "minikube"

minikube-bot · 2019-11-05T14:59:04Z

Can one of the admins verify this patch?

k8s-ci-robot · 2019-11-05T14:59:11Z

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.

k8s-ci-robot · 2019-11-05T14:59:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

/assign @tstromberg

/cc @medyagh

medyagh · 2019-11-05T19:40:22Z

Makefile

@@ -24,6 +24,8 @@ ISO_VERSION ?= v$(VERSION_MAJOR).$(VERSION_MINOR).1
 # Dashes are valid in semver, but not Linux packaging. Use ~ to delimit alpha/beta
 DEB_VERSION ?= $(subst -,~,$(RAW_VERSION))
 RPM_VERSION ?= $(DEB_VERSION)
+# used by hyperkit versionCheck(whether user's hyperkit is older than this)
+HYPERKIT_VERSION ?= 0.20190201


I am not sure if this is the best place to store the mininum required hyperkit version

I think a better place for this is in the package itself as a constant

I moved this constant.
Makefile(& pkg/version/version.go) → pkg/minikube/registry/drvs/hyperkit.go

cmd/minikube/cmd/start.go

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

cmd/minikube/cmd/start.go

tstromberg · 2019-11-06T01:31:15Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+	currentVersionDate, err := time.Parse(layout, currentVersion)
+	if err != nil {
+		glog.Warningf("Unable to parse to time.Date: %v", err)
+	}


isNewerVersion should probably return the error rather than silently dropping it, so that the caller can determine what the right course of action is.

Thanks!
I changed that isNewerVersion returns error if err != nil.

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

tstromberg · 2019-11-06T01:34:16Z

pkg/version/version.go

@@ -58,6 +61,11 @@ func GetISOPath() string {
 	return isoPath
 }

+// GetHyperKitVersion returns the specific hyperKit version
+func GetHyperKitVersion() string {
+	return hyperKitVersion


Move the expected version into drvs/hyperkit - as it's only consumed there.

I moved this constant and getter method.
Makefile(& pkg/version/version.go) → pkg/minikube/registry/drvs/hyperkit.go

govargo · 2019-11-06T02:15:49Z

Thank you for review @tstromberg @medyagh!

I'll retake these.
And when that is done, I will return comments.

govargo · 2019-11-11T11:52:51Z

@tstromberg @medyagh

I'm sorry for late.
I updated my code as you reviewed.

And FastFail(if vm-driver does not installed) will be implemented by #5840, so I removed FastFail from this PR.
I just only left hyperkit version check.

I also updated this PR title & description.

Thank you.

govargo · 2019-11-12T14:00:39Z

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

tstromberg

Some minor feedback from testing this PR locally. This will definitely save some users from grief.

tstromberg · 2019-11-13T15:22:00Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+}
+
+// GetHyperKitVersion returns the specific hyperKit version
+func GetHyperKitVersion() string {


I think you can safely remove this public function. This variable is only apparently used by this file.

You are right.
I modified this to private function.

tstromberg · 2019-11-13T15:22:57Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

@@ -37,6 +39,11 @@ const (
 	docURL = "https://minikube.sigs.k8s.io/docs/reference/drivers/hyperkit/"
 )

+var (
+	// hyperkitSpecificVersion is used by hyperkit versionCheck(whether user's hyperkit is older than this)
+	hyperkitSpecificVersion = "0.20190201"


Instead of hyperkit.hyperkitSpecificVersion - can this be renamed to minimumVersion?

Thanks nice naming!
I renamed.

tstromberg · 2019-11-13T15:25:54Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+		return registry.State{Installed: true, Error: fmt.Errorf("hyperkit version check failed:\n%s", err), Doc: docURL}
+	}
+	if !isNew {
+		return registry.State{Installed: true, Error: fmt.Errorf("the mininum recommended hyperkit version is %s. your hyperkit version is 0.%s. please consider upgrading your hyperkit", GetHyperKitVersion(), currentVersion), Fix: "Run 'brew upgrade hyperkit'", Doc: docURL}


set Healthy: true, as we don't really know yet if the hyperkit driver is healthy or not even if the version is old.

This way the hyperkit driver can still be automatically selected, but the error displayed.

This way the hyperkit driver can still be automatically selected, but the error displayed.

Thanks. I added Healthy property.

tstromberg · 2019-11-13T15:30:46Z

The current text reads:

⚠️  'hyperkit' driver reported an issue: the mininum recommended hyperkit version is
0.20191201. your hyperkit version is 0.20190802. please consider upgrading you
hyperkit
💡  Suggestion: Run 'brew upgrade hyperkit'

Could the first sentence be rephrased to be shorter? Perhaps:

⚠️  'hyperkit' driver reported an issue: the installed hyperkit version (0.20191201) is
older than the minimum recommended version (0.20191201)
💡  Suggestion: Run 'brew upgrade hyperkit'

govargo · 2019-11-14T15:14:46Z

@tstromberg
Thank you for your good feedback!!

I modified as you reviewed.
And I changed warning message as you proposed.

tstromberg · 2019-11-14T15:15:35Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+	// If current hyperkit is not newer than minimumVersion, suggest upgrade information
+	isNew, err := isNewerVersion(currentVersion, specificVersion)
+	if err != nil {
+		return registry.State{Installed: true, Healthy: true, Error: fmt.Errorf("hyperkit version check failed:\n%s", err), Doc: docURL}


use %v for err, rather than %s. Don't ask me why, it's just a Go common practice. :)

Thanks. I'll remember this manner.

govargo · 2019-11-14T15:16:46Z

I updated PR description.

tstromberg · 2019-11-14T15:18:31Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+	layout := "20060102"
+	currentVersionDate, err := time.Parse(layout, currentVersion)
+	if err != nil {
+		glog.Warningf("Unable to parse to time.Date: %v", err)


In general we either: return errors, or log them. We almost never do both, as it often causes duplicate errors to be logged.

In order to get the same basic effect here, use:

return false, errors.Wrap(err, "parse date")

In general we either: return errors, or log them. We almost never do both, as it often causes duplicate errors to be logged.

Thanks.
I'll remember this!

tstromberg · 2019-11-14T15:20:14Z

pkg/minikube/registry/drvs/hyperkit/hyperkit.go

+	return version
+}
+
+// getHyperKitVersion returns the minimum hyperKit version


Get is implied for Go function names. This would normally just be called hyperkit.hyperKitVersion. I would rather just see this method removed, since it does not add anything over referring to minimumVersion directly.

Sorry, you are right.
I removed this getter func.

tstromberg

Looks great, just minor idiomatic readability improvements to suggest.

govargo · 2019-11-14T15:31:32Z

Thank you for your quick feedback.
I will work on immediately

govargo · 2019-11-14T15:46:24Z

@tstromberg
I updated.

Thanks for your suggestion.
I'll remember those.

tstromberg · 2019-11-14T22:09:27Z

Some test failures:

# k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit [k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit.test]
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:55:15: undefined: splitHyperKitVersion
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:85:15: undefined: convertVersionToDate
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:119:18: undefined: isNewerVersion

tstromberg · 2019-11-14T22:09:50Z

/ok-to-test

minikube-bot · 2019-11-14T22:27:24Z

All Times minikube: [ 123.385700 121.444881 122.374470]
All Times Minikube (PR 5833): [ 119.401989 122.342768 123.808334]

Average minikube: 122.401684
Average Minikube (PR 5833): 121.851030

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 5833) |
+----------------------+-----------+--------------------+
| minikube v           |  0.317921 |           0.310180 |
| Creating kvm2        | 38.406732 |          38.240960 |
| Preparing Kubernetes | 53.295196 |          52.552395 |
| Pulling images       |  2.903170 |           2.668731 |
| Launching Kubernetes | 19.884803 |          19.479054 |
| Waiting for cluster  |  7.547330 |           8.556659 |
+----------------------+-----------+--------------------+

govargo · 2019-11-15T02:50:50Z

Some test failures:

# k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit [k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit.test]
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:55:15: undefined: splitHyperKitVersion
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:85:15: undefined: convertVersionToDate
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:119:18: undefined: isNewerVersion

Is this test executed by master branch?
If so, the test will fail because these method don't exist yet.

https://travis-ci.org/kubernetes/minikube/jobs/611933306?utm_medium=notification&utm_source=github_status

govargo · 2019-12-17T12:46:21Z

I close this PR because the minikube v1.6 do this, unless they specify --vm-driver=hyperkit.

tstromberg · 2019-12-17T17:01:39Z

Re-opening because this PR is still quite valuable to us, for version checking.

tstromberg · 2019-12-17T17:29:22Z

Travis still failing, but the tests run correctly when diff'd against master. Do you mind merging your PR against master (git merge master) to see if it fixes the test?

# k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit [k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit.test]
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:55:15: undefined: splitHyperKitVersion
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:85:15: undefined: convertVersionToDate
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:119:18: undefined: isNewerVersion

govargo · 2019-12-18T01:35:02Z

@tstromberg
Thanks for reopening this PR.

Travis still failing, but the tests run correctly when diff'd against master. Do you mind merging your PR against master (git merge master) to see if it fixes the test?

OK, I'll rebase against latest master branch.

…eless getter func

govargo · 2019-12-18T01:52:25Z

I updated

govargo · 2019-12-18T01:52:30Z

/retest

govargo · 2019-12-18T02:44:24Z

The test result doesn't change...

# k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit [k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit.test]
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:55:15: undefined: splitHyperKitVersion
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:85:15: undefined: convertVersionToDate
pkg/minikube/registry/drvs/hyperkit/hyperkit_test.go:119:18: undefined: isNewerVersion
ok  	k8s.io/minikube/pkg/minikube/registry	0.026s	coverage: 95.1% of statements
FAIL	k8s.io/minikube/pkg/minikube/registry/drvs/hyperkit [build failed]

I doubt this test fails due to git or go mod, but I have no conviction. I have no idea...

If someone noticed this cause, would you tell me please.

tstromberg · 2019-12-19T19:44:53Z

I don't get what is going on here. I'm just going to try to merge this and see if master works.

tstromberg · 2019-12-19T19:48:36Z

PR seems to work just fine at master. ¯_(ツ)_/¯

govargo · 2019-12-20T01:23:15Z

Thank you very much!!! @tstromberg

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 5, 2019

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

k8s-ci-robot requested review from blueelvis and josedonizetti November 5, 2019 14:59

k8s-ci-robot assigned tstromberg Nov 5, 2019

k8s-ci-robot requested a review from medyagh November 5, 2019 15:00

govargo changed the title ~~Fix without hyperkit errormsg~~ Fix FastFail if driver error occured & Add hyperkit version check Nov 5, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2019

medyagh requested changes Nov 5, 2019

View reviewed changes

tstromberg suggested changes Nov 6, 2019

View reviewed changes

govargo changed the title ~~Fix FastFail if driver error occured & Add hyperkit version check~~ Add hyperkit version check whether user's hyperkit is newer Nov 11, 2019

tstromberg suggested changes Nov 13, 2019

View reviewed changes

tstromberg reviewed Nov 14, 2019

View reviewed changes

tstromberg suggested changes Nov 14, 2019

View reviewed changes

tstromberg approved these changes Nov 14, 2019

View reviewed changes

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 Nov 14, 2019

tstromberg mentioned this pull request Nov 15, 2019

Error on minikube start no config on macOS #5763

Closed

govargo mentioned this pull request Dec 17, 2019

FailFast if hyperkit is not installed. #5478

Closed

govargo closed this Dec 17, 2019

tstromberg reopened this Dec 17, 2019

govargo added 4 commits December 18, 2019 10:36

Add hyperkit version checker: whether user's hyperkit is new

fcbf4d1

Improve hyperkit specific version check

f9d4762

rename constant name and shorten warining message of version old

39f7134

refactor: remove warning log to avoid duplicate logging and remove us…

b650600

…eless getter func

tstromberg merged commit 28c4c82 into kubernetes:master Dec 19, 2019

tstromberg mentioned this pull request Dec 19, 2019

Add build tags so that the test only runs on Darwin #6127

Merged

govargo deleted the fix-without-hyperkit-errormsg branch January 23, 2020 06:25

Add hyperkit version check whether user's hyperkit is newer #5833

Add hyperkit version check whether user's hyperkit is newer #5833

Conversation

govargo commented Nov 5, 2019 • edited Loading

What type of PR is this?

What this PR does / why we need it:

2. Add version check of hyperkit which inspect user's hyperkit is newer than specific version

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

minikube-bot commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

govargo commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govargo commented Nov 6, 2019

govargo commented Nov 11, 2019

govargo commented Nov 12, 2019

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg commented Nov 13, 2019 • edited Loading

govargo commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

govargo commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg left a comment

Choose a reason for hiding this comment

govargo commented Nov 14, 2019

govargo commented Nov 14, 2019

tstromberg commented Nov 14, 2019

tstromberg commented Nov 14, 2019

minikube-bot commented Nov 14, 2019

govargo commented Nov 15, 2019

govargo commented Dec 17, 2019

tstromberg commented Dec 17, 2019

tstromberg commented Dec 17, 2019

govargo commented Dec 18, 2019

govargo commented Dec 18, 2019

govargo commented Dec 18, 2019

govargo commented Dec 18, 2019

tstromberg commented Dec 19, 2019

tstromberg commented Dec 19, 2019

govargo commented Dec 20, 2019

govargo commented Nov 5, 2019 •

edited

Loading

tstromberg commented Nov 13, 2019 •

edited

Loading