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

--extra-config now work for kubeadm as well #3879

Merged
merged 5 commits into from
Mar 22, 2019

Conversation

marcosdiez
Copy link
Contributor

@marcosdiez marcosdiez commented Mar 14, 2019

This small patch allows us to use --extra-config also with kubeadm.

My primary motivation is to run minikube with the none vm driver in any version of docker.

This patch allows one to do so like this: sudo -E minikube start --vm-driver=none --kubernetes-version v1.11.8 --extra-config kubeadm.ignore-preflight-errors=SystemVerification

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 14, 2019
@afbjorklund
Copy link
Collaborator

This is quite a convoluted syntax for the check, and it seems that it is duplicating the preflight checks in two different places now. Can see the reason for skipping Docker version check (surely they will name the next version something weird and break it again), but supporting generic parameters to kubeadm might be hard to do. Would rather see something more specific to SystemVerification at this point in time.

@afbjorklund afbjorklund requested a review from tstromberg March 14, 2019 18:32
@marcosdiez
Copy link
Contributor Author

hey, @afbjorklund !

An alternative for my generic approach would be similar to what was implemented here: #3892

The problem with that implementation is that minikube will one day become a mess if we start exposing each and every parameter from other tools to it. Also, my approach is future compatible.

That being said, I am neutral to how this get's implemented as long as I can tell minikube to tell kubeadm not to complain about docker versions.
So if you tell me that you prefer the other way, I can implement that as well.

Please tell me if I can help you in any other way!
Marcos

@tstromberg
Copy link
Contributor

tstromberg commented Mar 21, 2019

If you add an example command-line to start_stop_delete_test.go, I'd be good to merge this.

@marcosdiez marcosdiez force-pushed the extra_config_for_kubeadm branch from 0b3e08d to 8aec728 Compare March 21, 2019 12:17
@marcosdiez
Copy link
Contributor Author

marcosdiez commented Mar 21, 2019

Hi @tstromberg !
I just added the extra line on start_stop_delete_test.go to test this.

And since travis does not do integration tests, here is my passing output:
https://gist.github.com/marcosdiez/88e029d2068cf349c1daa1fa97585ff9

This is the command I used to run the tests:
env TEST_ARGS="-minikube-start-args=--vm-driver=none -test.run TestStartStop" sudo -E make integration

(I must use --vm-driver=none because I run linux inside of a VM, so other vm drivers are not an option to me)

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcosdiez, tstromberg

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 Mar 21, 2019
@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg tstromberg merged commit cbac94a into kubernetes:master Mar 22, 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. 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.

5 participants