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.

Sign up for GitHub

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

Jump to bottom

Support HostAlias for HostNetwork Pods #50646

Merged
merged 3 commits into from
Aug 24, 2017
Merged

Support HostAlias for HostNetwork Pods #50646

merged 3 commits into from
Aug 24, 2017

Conversation

evie404
Copy link
Contributor

@evie404 evie404 commented Aug 14, 2017

What this PR does / why we need it: Currently, HostAlias does not support HostNetwork pods because historically, kubelet only manages hosts file for non-HostNetwork pods. With the recent change in #49140, kubelet now manages hosts file for all Pods, which enables HostAlias support also.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #48398

Special notes for your reviewer: might be easier to review commit-by-commit

Release note:

HostAlias is now supported for both non-HostNetwork Pods and HostNetwork Pods.

@yujuhong @hongchaodeng @thockin

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 14, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @rickypai. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 14, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 14, 2017
@cmluciano
Copy link

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2017
@evie404
Copy link
Contributor Author

evie404 commented Aug 15, 2017

/retest

hongchaodeng
hongchaodeng reviewed Aug 17, 2017
View reviewed changes
Copy link
Contributor

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

@rickypai Great work! Have some comments please take a look.

One question: I saw that ensureHostsFile() will overwrite the host /etc/hosts file. Is it an assumption that after kubelet starts no one will modify /etc/hosts ?

pkg/kubelet/kubelet_pods_test.go
@@ -201,6 +213,70 @@ fe00::2 ip6-allrouters
},
{
"hosts_test_file2",
[]v1.HostAlias{},
`# another hosts file for testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to this test? It's already tested in above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests different file path just to be exhaustive

pkg/kubelet/kubelet_pods_test.go
@@ -184,11 +184,23 @@ func TestMakeMounts(t *testing.T) {

func TestNodeHostsFileContent(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems to be only testing nodeHostsFileContent() ?
Any test to verify that hostNetwork=true hostAlias ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in ensureHostsFile, nodeHostsFileContent is only called when hostNetwork=true. so this is already testing the case.

@evie404
Copy link
Contributor Author

evie404 commented Aug 17, 2017

In the case /etc/hosts is modified after a hostNetwork pod is scheduled, the pod will not be able to get the new content. This is not ideal, but it is consistent with the old behavior when docker was responsible for /etc/hosts.

@hongchaodeng
Copy link
Contributor

Yeah. I understand that it's the same as non-hostNetwork case.

Just to make sure that it's assumed that /etc/hosts is managed by docker and kubelet only, and further assumed that docker won't modify it after kubelet takes control. Isn't it?

@evie404
Copy link
Contributor Author

evie404 commented Aug 17, 2017

the original intention for kubelet-managed hosts file is to prevent docker from changing it. this was already the case for non-hostNetwork pods, so I think the assumption still applies for hostNetwork pods.

more background: https://kubernetes.io/docs/concepts/services-networking/add-entries-to-pod-etc-hosts-with-host-aliases/#why-does-kubelet-manage-the-hosts-file

@hongchaodeng
Copy link
Contributor

👍

@evie404
Copy link
Contributor Author

evie404 commented Aug 18, 2017

/assign @thockin

@thockin
Copy link
Member

thockin commented Aug 22, 2017

/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 Aug 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rickypai, thockin

Associated issue: 48398

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2017
@evie404
Copy link
Contributor Author

evie404 commented Aug 22, 2017

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ef1b835 into kubernetes:master Aug 24, 2017
@evie404 evie404 deleted the rpai/hostalias_hostnetwork branch August 24, 2017 05:37
@evie404 evie404 mentioned this pull request Sep 26, 2017
Merged
This was referenced Dec 20, 2023
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongchaodeng hongchaodeng hongchaodeng left review comments

Assignees

@smarterclayton smarterclayton

@thockin thockin

@brendandburns brendandburns

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HostAlias to Support HostNetwork Pods
8 participants
@evie404 @k8s-ci-robot @cmluciano @hongchaodeng @thockin @k8s-github-robot @smarterclayton @brendandburns