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

Don't use latest stable version for minikube #856

Merged

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Jun 21, 2019

Sometimes, minikube lags behind kubernetes which can lead
to a breakage of the test suite, since minikube will fail to start
with the latest kubernetes version.
See for example this issue.

This change defaults to leave the decision of which k8s version to use,
to minikube itself. This is defined in:

https://github.com/kubernetes/minikube/master/pkg/minikube/constants/constants.go

However, if one really desires it is still possible to pass

--kubernetes-version=X.Y.Z

to minikube initialization start command via an environment variable
before invoking the test suite:

export $MINIKUBE_ARGS="--kubernetes-version=X.Y.Z"

This allows ofcourse passing other flags to minikube also.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 21, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 21, 2019
@oz123 oz123 mentioned this pull request Jun 21, 2019
@micw523
Copy link
Contributor

micw523 commented Jun 21, 2019

If you take a look at the CI log you'll find that the behavior your changes introduced is to download k8s 1.15 first, and then minikube installs k8s 1.14 again. This is a little redundant in my opinion - I think we could simply pin the k8s version to 1.14 for now(?)

@oz123
Copy link
Contributor Author

oz123 commented Jun 21, 2019

Why would you want to introduce a future bug? Every-time Kubernetes changes behavior you risk minikube breaking. The "working" Kubernetes version for minikube is hardcoded in minikube, and I think you should just minikube choose it's own version.
If the python-client needs to test a specific k8s API there is still the possibility of overriding it with the environment variable.

The script downloads kubectl not kubernetes. kubectl in version n+1 works fine with version n.
After that the script downloads minikube in the stable version, which currently installs the version of kubernetes which is known to work.
If we change this to 1.14 now, someone will need in the future to change this to a version which is known to work with minikube, and this version will work until it won't.
My approach, to leave the decision to minikube is more sustainable.

Also, this documented in my commit message, but just in case, I am repeating this again. kubeadm in its current version installes k8s in version 1.14.3.

scripts/kube-init.sh Outdated Show resolved Hide resolved
@roycaihw
Copy link
Member

I have one nit on the link, then LGTM. We should have better mechanism to verify server-client skew, instead of tweaking minikube ad hoc

Sometimes, minikube lags behind kubernetes which can lead
to a breakage of the test suite, since minikube will fail to start
with the latest kubernetes version.
See for example [this issue](kubernetes/minikube#4371).

This change defaults to leave the decision of which k8s version to use,
to minikube itself. This is defined in:

https://github.com/kubernetes/minikube/master/pkg/minikube/constants/constants.go

However, if one really desires it is still possible to pass

`--kubernetes-version=X.Y.Z`

to minikube initialization start command via an environment variable
before invoking the test suite:

export $MINIKUBE_ARGS="--kubernetes-version=X.Y.Z"

This allows ofcourse passing other flags to minikube also.
@oz123 oz123 force-pushed the test-suite-change-k8s-version branch from 4e0332b to d7300db Compare June 25, 2019 19:22
@roycaihw
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oz123, roycaihw

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2019
@k8s-ci-robot k8s-ci-robot merged commit c4c18d9 into kubernetes-client:master Jun 25, 2019
@oz123 oz123 deleted the test-suite-change-k8s-version branch June 25, 2019 19:46
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

None yet

4 participants