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

docs: disable host dns resolver for minikube installation guide #25569

Merged
merged 1 commit into from May 24, 2023

Conversation

zhouhaibing089
Copy link
Contributor

@zhouhaibing089 zhouhaibing089 commented May 20, 2023

When virtualbox provider is used as minikube driver, all the VM instances are going to have 10.0.2.3 as dns server (inspected by looking at /etc/resolv.conf). This seems to have collapsed with cilium cni cidr. This is going to fail the cilium connectivity test because image pulling are going to fail due to dns resolution failure to image registry.

The fix here is to specify --host-dns-resolver=false so that such problem doesn't occur. The new dns resolver will be 192.168.0.1 (all with default configurations).

I also thought about customizing cilium cni cidr, but that may involve more changes to the doc. It might not be worthwhile in this quick installation guide.

docs: Disable host DNS resolver with Virtualbox for Minikube quick installation guide

@zhouhaibing089 zhouhaibing089 requested a review from a team as a code owner May 20, 2023 19:49
@maintainer-s-little-helper
Copy link

Commit ca33df8889a66f2bab2279ec76b992a2fc122d68 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 dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 20, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 20, 2023
@zhouhaibing089
Copy link
Contributor Author

The following change also works:

$ cilium install --helm-set ipam.operator.clusterPoolIPv4PodCIDR=172.16.0.0/12

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.

Looks good to me, thanks. I have some minor nits on capitalisation (see below), but it's not blocking.

Documentation/gettingstarted/k8s-install-default.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels May 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 22, 2023
@qmonnet
Copy link
Member

qmonnet commented May 22, 2023

Can you please sign off your commit, as explained by the bot?

@maintainer-s-little-helper
Copy link

Commit ca33df8889a66f2bab2279ec76b992a2fc122d68 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

@qmonnet
Copy link
Member

qmonnet commented May 23, 2023

Thanks for the update!

I see your second commit is signed off, but not the first. Can you please do the following:

  • Squash your two commits together
  • Make sure the resulting commit contains your Signed-off-by tag
  • Either drop the Co-authored-by tag (what I'd recommend), or at least use my work email (quentin@isovalent.com) instead of personal one that GitHub picked automatically

I can help if necessary.

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.

it appears that github bot requested a bunch of reviewers as a result

No worries, it's not the first time this kind of things happens, nor will it be the last :). Thanks for fixing the PR and updating your commit!

Documentation change only, and related tests passed, so I'm marking as ready-to-merge.

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 24, 2023
@squeed squeed merged commit 71fb437 into cilium:main May 24, 2023
36 checks passed
@zhouhaibing089 zhouhaibing089 deleted the minikube-installation-doc branch May 26, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/community-contribution This was a contribution made by a community member. 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

4 participants