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

Expose ‘—pod-network-cidr’ argument in minikube #3892

Merged
merged 2 commits into from
May 20, 2019

Conversation

11janci
Copy link
Contributor

@11janci 11janci commented Mar 18, 2019

Exposes --pod-network-cidr arg in minikube and forwards it to kubeadm so that it is not necessary to configure the cidr on multiple components separately.

Closes #3865

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 18, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@11janci
Copy link
Contributor Author

11janci commented Mar 18, 2019

/assign @balopat

@marcosdiez
Copy link
Contributor

Just for the record I had a similar problem of needing to pass a specific parameter to kubeadm and I implemented a generic approach to solve it: #3879
It has not been merged yet. I am not sure if my approach is better or worse, but I thought it would be relevant to have it mentioned here.

@11janci
Copy link
Contributor Author

11janci commented Mar 18, 2019

I'd be happy to move it under the --extra-config flag if @marcosdiez 's PR gets approved and other reviewers agree with the approach (I personally think it makes sense to move it there).

@11janci 11janci force-pushed the jjanik-expose-pod-cidr branch 2 times, most recently from d32903f to b79f98f Compare March 22, 2019 04:18
@tstromberg
Copy link
Contributor

#3879 was merged. Should this PR be closed or altered?

@11janci
Copy link
Contributor Author

11janci commented Mar 24, 2019

@tstromberg I'll be happy to alter it. Do you want to just resolve the conflicts and keep --pod-network-cidr as a top-level minikube param or do you want to move it under --extra-config?

@11janci 11janci force-pushed the jjanik-expose-pod-cidr branch 4 times, most recently from c08fb77 to cfba354 Compare March 30, 2019 06:13
@11janci
Copy link
Contributor Author

11janci commented Mar 30, 2019

@tstromberg
the existing generic solution (using --extra-conf) done within #3879 does not work for --pod-network-cidr as kubeadm does not allow this parameter to be combined with the --config param. kubeadm allows only following parameters to be used when --config is specified (see kubernetes/cmd/kubeadm/app/apis/kubeadm/validation/validation.go:ValidateMixedArguments):

kubeadmcmdoptions.CfgPath,
kubeadmcmdoptions.IgnorePreflightErrors,
kubeadmcmdoptions.DryRun,
kubeadmcmdoptions.KubeconfigPath,
kubeadmcmdoptions.NodeName,
kubeadmcmdoptions.NodeCRISocket,
kubeadmcmdoptions.KubeconfigDir,
kubeadmcmdoptions.UploadCerts,
kubeadmcmdoptions.CertificateKey,
"print-join-command"
"rootfs",
"v"

The rest of the kubeadm parameters are not allowed and thus will fail if specified in the --extra-conf param in minikube.
Here are some ideas how to fix this:

  • update kubeadm to allow all parameters with the --config option (too big of a change change so probably not feasible)
  • expose parameters which are not allowed to be used in extra-conf as top-level minikube params, i.e. like this PR does. Probably would be better to drop supporting kubeadm params in extra-conf in this case
  • add some handling in minikube that would contain a whitelist of supported kubeadm parameters. The ones that are allowed to be combined with --conf would be propagated to kubeadm as parameters, the rest could be manually added to the config file. E.g. the --pod-network-cidr would not be used as a kubeadm param, but manually put into the config file similarly as done in this PR.

If you would like to keep the --extra-conf support for kubeadm, I find the last option the best. It would fail immediately for unsupported params, is extensible in the future to add additional parameters and would not pollute minikube with parameters from kubeadm.

@tstromberg
Copy link
Contributor

@11janci - Thanks for the update. Let me give it some thought, as maintaining a whitelist sounds like something that could easily go out of date and cause issues as well. Hmm..

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2019
@tstromberg
Copy link
Contributor

@11janci - I thought about it some more, and agree that the 3rd item makes the most sense. I don't think that particular whitelist will be too much of burden to maintain.

That said, I'm flexible and will accept this PR in either direction. Thank you again for contributing it!

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

Needs merge update.

@11janci
Copy link
Contributor Author

11janci commented May 1, 2019

@tstromberg cool thx, I will look into it over the weekend and hopefully come up with a PR.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2019
@tstromberg
Copy link
Contributor

tstromberg commented May 2, 2019

Please check the failing test for TestGenerateConfig/containerd-pod-network-cidr__default - I think it might not have merged properly with the version bump.

@11janci 11janci changed the title Expose ‘—pod-network-cidr’ argument in minikube [WIP] Expose ‘—pod-network-cidr’ argument in minikube May 2, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 8, 2019
@11janci
Copy link
Contributor Author

11janci commented May 8, 2019

did you rebuilt after the merge? also did you do minikube delete before rerunning? that should work and it does on my machine:
Screen Shot 2019-05-08 at 10 45 55 PM
(fyi I rebased off latest master)

@PatrickLang
Copy link

Hmm, I must have missed a step somewhere.

Good news, this gets a working flannel setup in host-gw mode:

.\minikube-windows-amd64.exe start --vm-driver=hyperv --network-plugin=cni --enable-default-cni --extra-config=kubeadm.pod-network-cidr=10.244.0.0/16 --extra-config=controller-manager.allocate-node-cidrs=true --extra-config=controller-manager.cluster-cidr=10.244.0.0/16
kubectl apply -f https://gist.githubusercontent.com/PatrickLang/02b7c504fecdd08b498d13bdeee88186/raw/b77ab11e02dba40b597ff6614c401cd71827fa81/kube-flannel.yml

@ksubrmnn this could make the kubeadm adjustments for Windows easier since it's easy to bring up a 1-node cluster with minikube. I still haven't tried joining more nodes yet but it seems like it should work.

@PatrickLang
Copy link

/lgtm

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 11janci, PatrickLang
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: balopat

If they are not already assigned, you can assign the PR to them by writing /assign @balopat in a comment when ready.

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

@PatrickLang
Copy link

cc @neolit123 ^^

@11janci
Copy link
Contributor Author

11janci commented May 10, 2019

good to hear, thank you sir :)

@neolit123
Copy link
Member

can not mix '--config' with arguments [pod-network-cidr]

+

update kubeadm to allow all parameters with the --config option (too big of a change change so probably not feasible)

similar flag -> config overrides might work eventually in the long term (requires component config work). for now we encourage using --config. technically the kubeadm config API (e.g. v1beta1) can be used from a go template which is written to a temp directory and passed via --config.

@PatrickLang

@ksubrmnn this could make the kubeadm adjustments for Windows easier since it's easy to bring up a 1-node cluster with minikube. I still haven't tried joining more nodes yet but it seems like it should work.

👍

@hswong3i
Copy link

With my latest founding we could run minikube + flannel without kubeadm --pod-network-cidr, see

From https://github.com/coreos/flannel/blob/master/Documentation/troubleshooting.md#kubernetes-specific what we really needed are kubelet's --pod-cidr=<cidr> and controller-manager's --allocate-node-cidrs=true --cluster-cidr=<cidr>, so something like this:

minikube start \
    --extra-config=controller-manager.allocate-node-cidrs=true \
    --extra-config=controller-manager.cluster-cidr=10.233.64.0/18 \
    --extra-config=kubeadm.ignore-preflight-errors=FileContent--proc-sys-net-bridge-bridge-nf-call-iptables,SystemVerification \
    --extra-config=kubelet.cgroup-driver=systemd \
    --extra-config=kubelet.network-plugin=cni \
    --extra-config=kubelet.pod-cidr=10.233.64.0/18 \
    --extra-config=kubelet.resolv-conf=/run/systemd/resolve/resolv.conf \
    --kubernetes-version=v1.14.1 \
    --network-plugin=cni \
    --vm-driver=none

curl -skL https://raw.githubusercontent.com/coreos/flannel/master/Documentation/kube-flannel.yml > /tmp/kube-flannel.yml
sed -i 's/10.244.0.0\/16/10.233.64.0\/18/g' /tmp/kube-flannel.yml
kubectl apply -f /tmp/kube-flannel.yml

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

PR looks good, mostly just spotting some unused code that should be removed.

It's unclear from #3892 (comment) whether or not this PR behaves as expected. Is it ready to ship?

pkg/util/slice.go Outdated Show resolved Hide resolved
pkg/util/slice.go Outdated Show resolved Hide resolved
pkg/util/slice.go Outdated Show resolved Hide resolved
pkg/util/slice.go Outdated Show resolved Hide resolved
pkg/util/slice_test.go Outdated Show resolved Hide resolved
pkg/util/slice_test.go Outdated Show resolved Hide resolved
test/integration/start_stop_delete_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 17, 2019
@11janci
Copy link
Contributor Author

11janci commented May 17, 2019

Hi @tstromberg ,
thx for your review, PR updated, PTAL.
It is working as expected and is ready to ship.

@11janci
Copy link
Contributor Author

11janci commented May 17, 2019

there is a unrelated unit test failing in the build (TestNewSSHClient), it's failing in master as well. Needs to be resolved first
resolved

@11janci 11janci force-pushed the jjanik-expose-pod-cidr branch 2 times, most recently from e931416 to 1815387 Compare May 17, 2019 05:06
@tstromberg tstromberg merged commit 35dc6a2 into kubernetes:master May 20, 2019
@11janci 11janci deleted the jjanik-expose-pod-cidr branch May 20, 2019 21:51
hswong3i added a commit to pantarei/ansible-role-minikube that referenced this pull request Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose pod-network-cidr on kubeadm
9 participants