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

Minikube guide updates #16346

Merged
merged 2 commits into from Jun 3, 2021
Merged

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented May 28, 2021

Add a note about --cni=cilium flag that can automatically install Cilium. However, it installs v1.8 by default so add a note regarding this.

Cilium init script mounts the BPF fs if not already mounted so the manual step to mount BPF fs isn't required. Let's double check -

$ minikube start --network-plugin=cni
$ cilium install
$ minikube ssh -- mount | grep bpf
none on /sys/fs/bpf type bpf (rw,relatime)

ks logs cilium-zp2st -c ebpf-mount
Mounting eBPF filesystem...

ebpf-mount is the init container that mounts the BPF fs. Here is the relevant code in cilium/cli - https://github.com/cilium/cilium-cli/blob/master/install/install.go#L685

Fixes: #16324

@aditighag aditighag added the release-note/misc This PR makes changes that have no direct user impact. label May 28, 2021
@aditighag aditighag requested a review from a team as a code owner May 28, 2021 01:27
@aditighag aditighag requested a review from qmonnet May 28, 2021 01:27
@maintainer-s-little-helper
Copy link

Commit 7a3445db6e92da4ceec28c0e3523c351968878c8 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 28, 2021
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from c1a0f07 to aa6eb96 Compare May 28, 2021 01:31
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 28, 2021
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from aa6eb96 to e0fbc5a Compare May 28, 2021 01:33
@aditighag aditighag requested a review from aanm May 28, 2021 01:36
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from e0fbc5a to cca09b8 Compare May 28, 2021 02:25
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from cca09b8 to 6b1f8e3 Compare May 28, 2021 04:08
@maintainer-s-little-helper
Copy link

Commit 12f089d757c683d701f730c4e568653ca09a440f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 28, 2021
@aditighag aditighag changed the title Add note about --network-plugin=cilium flag in minikube guide Minikube guide updates May 28, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Cilium init script mounts the BPF fs if not already mounted so the manual step to mount BPF fs isn't required.

(Note that it could be worth having the above in the commit log of the revert patch. Not blocking, though.)

Looks good, thank you! Don't forget to sign-off :)

This reverts commit f3c90b8.

Cilium init container mounts the BPF fs if it's not already mounted so the 
manual step to mount BPF fs isn't required. For details, check GH cilium#16346.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from 6b1f8e3 to 7584e4f Compare June 1, 2021 15:01
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 1, 2021
@aditighag
Copy link
Member Author

@qmonnet Updated the commit description for the reverted commit. Thanks!

@aditighag aditighag requested a review from aanm June 1, 2021 15:03
@qmonnet qmonnet mentioned this pull request Jun 1, 2021
23 tasks
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

👍

I'm ambivalent about documenting the minikube --cni=cilium ... flag or not, seems like we have legitimate reasons not to recommend the user to use it so it'd be simpler to omit the instruction. That said it's not a big deal to also just let the user know like this. However I think if we do so, we should move the note inside the group tab for minikube users rather than instruct all users about this, even if they've clicked one of the other group tabs (why would cloud or kind users need to be told about a minikube feature?).

Documentation/gettingstarted/k8s-install-default.rst Outdated Show resolved Hide resolved
From minikube v1.12.1+, the flag passed to `minikube start` command
will deploy quick-install.yaml automatically. However, it currently installs
v1.8 by default so add a note regarding this.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/fix-minikube-guide branch from 7584e4f to 4c33568 Compare June 1, 2021 16:22
@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2021
@aanm aanm merged commit a33b703 into cilium:master Jun 3, 2021
aanm pushed a commit that referenced this pull request Jun 3, 2021
This reverts commit f3c90b8.

Cilium init container mounts the BPF fs if it's not already mounted so the 
manual step to mount BPF fs isn't required. For details, check GH #16346.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants