Support HostAlias for HostNetwork Pods#50646
Support HostAlias for HostNetwork Pods#50646k8s-github-robot merged 3 commits intokubernetes:masterfrom
Conversation
|
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 I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
|
/retest |
hongchaodeng
left a comment
There was a problem hiding this comment.
@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 ?
| { | ||
| "hosts_test_file2", | ||
| []v1.HostAlias{}, | ||
| `# another hosts file for testing. |
There was a problem hiding this comment.
No need to this test? It's already tested in above?
There was a problem hiding this comment.
tests different file path just to be exhaustive
| @@ -184,11 +184,23 @@ func TestMakeMounts(t *testing.T) { | |||
|
|
|||
| func TestNodeHostsFileContent(t *testing.T) { | |||
There was a problem hiding this comment.
This test seems to be only testing nodeHostsFileContent() ?
Any test to verify that hostNetwork=true hostAlias ?
There was a problem hiding this comment.
in ensureHostsFile, nodeHostsFileContent is only called when hostNetwork=true. so this is already testing the case.
|
In the case |
|
Yeah. I understand that it's the same as non-hostNetwork case. Just to make sure that it's assumed that |
|
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 |
|
👍 |
|
/assign @thockin |
|
/lgtm |
|
[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. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test pull-kubernetes-e2e-kops-aws |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
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 #48398Special notes for your reviewer: might be easier to review commit-by-commit
Release note:
@yujuhong @hongchaodeng @thockin