Skip to content
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

WIP: Add warn about old driver version (hyperkit, kvm) #4681

Closed
wants to merge 1 commit into from

Conversation

RA489
Copy link

@RA489 RA489 commented Jul 4, 2019

Add warn about old driver version (hyperkit, kvm)
fixes #4658

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RA489

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 4, 2019
"OLD_HYPERKIT_VERSION": {
Regexp: re(`Too old hyperkit version found`),
Advice: "too old hyperkit version. Jump forward to new valid hyperkit version as a workaround.",
Issues: []int{4658},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add URL to documentation on how to upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@@ -119,6 +124,11 @@ var vmProblems = map[string]match{
URL: "http://mikko.repolainen.fi/documents/virtualization-with-kvm",
Issues: []int{2991},
},
"OLD_KVM_VERSION": {
Regexp: re(`Too old KVM version found`),
Advice: "too old KVM version. Jump forward to new valid KVM version as a workaround.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add URL to documentation on how to upgrade.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josedonizetti
Copy link
Member

josedonizetti commented Jul 4, 2019

@RA489 I added two comments, but I don't understand how this fixes the issue. For me, the problem package would display a message based on an existing error that matches the regex. But I never saw those drivers message before Too old KVM/Hyperkit version found if they already exist is great and I can close the PR I opened to check it #4676. Thanks :)

@tstromberg tstromberg changed the title Add warn about old driver version (hyperkit, kvm) WIP: Add warn about old driver version (hyperkit, kvm) Jul 11, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019
@@ -36,6 +36,11 @@ var vmProblems = map[string]match{
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#hyperkit-driver",
Issues: []int{1926, 4206},
},
"OLD_HYPERKIT_VERSION": {
Regexp: re(`Too old hyperkit version found`),
Copy link
Contributor

@tstromberg tstromberg Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure this regexp matches the exit error thrown at head - #4676 introduced some changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@josedonizetti josedonizetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RA489 I still fail to see which minikube messages are those regexp matching. If you can show in the code, or maybe the output of minikube would help a lot clarify it. Thank you!

Because considering the latest changes to minikube, I think those regexes should match the message here: https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/start.go#L945

"OLD_HYPERKIT_VERSION": {
Regexp: re(`Add warn about old driver version (hyperkit, kvm)`),
Advice: "Too old hyperkit version. Jump forward to new valid hyperkit version as a workaround.",
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md"
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#hyperkit-upgrade",

There's a specific link for hyperkit upgrade. Also missing a comma at the end.

"OLD_KVM_VERSION": {
Regexp: re(`Too old KVM version found`),
Advice: "too old KVM version. Jump forward to new valid KVM version as a workaround.",
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md"
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#kvm2-upgrade",

There's a specific link for kvm upgrade. Also missing a comma at the end.

@@ -36,6 +36,12 @@ var vmProblems = map[string]match{
URL: "https://github.com/kubernetes/minikube/blob/master/docs/drivers.md#hyperkit-driver",
Issues: []int{1926, 4206},
},
"OLD_HYPERKIT_VERSION": {
Regexp: re(`Add warn about old driver version (hyperkit, kvm)`),
Copy link
Member

@josedonizetti josedonizetti Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which message is this regexp matching? Do you have an example of where this message is displayed to the user?

@@ -119,6 +125,12 @@ var vmProblems = map[string]match{
URL: "http://mikko.repolainen.fi/documents/virtualization-with-kvm",
Issues: []int{2991},
},
"OLD_KVM_VERSION": {
Regexp: re(`Too old KVM version found`),
Copy link
Member

@josedonizetti josedonizetti Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which message is this regexp matching? Do you have an example of where this message is displayed to the user?

@tstromberg
Copy link
Contributor

Closing in preference to #4691

@tstromberg tstromberg closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warn about old driver version (hyperkit, kvm)
4 participants