Kubelet manage hosts file for HostNetwork Pods instead of Docker #49140

evie404 · 2017-07-18T18:13:46Z

What this PR does / why we need it: Currently, Docker manages the hosts file for containers inside Pods using hostNetwork. It creates discrepancy between how we treat hostNetwork and non-hostNetwork Pods. Kubelet should manage the file regardless of the network setup.

Which issue this PR fixes: fixes #48397 more context in #43632 (comment)

Special notes for your reviewer: Because the new logic relies on reading the node filesystem, I'm not sure how to write a proper unit test. I was thinking about using a node e2e test to cover the case, but suggestions are greatly welcomed.

Release note:

Kubelet now manages `/etc/hosts` file for both hostNetwork Pods and non-hostNetwork Pods.

/kind feature
/sig node

@yujuhong @hongchaodeng @thockin
@kubernetes/sig-network-feature-requests @kubernetes/sig-node-feature-requests

k8s-ci-robot · 2017-07-18T18:13:53Z

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 · 2017-07-18T18:13:55Z

@rickypai: Reiterating the mentions to trigger a notification:
@kubernetes/sig-network-feature-requests, @kubernetes/sig-node-feature-requests.

In response to this:

What this PR does / why we need it: Currently, Docker manages the hosts file for containers inside Pods using hostNetwork. It creates discrepancy between how we treat hostNetwork and non-hostNetwork Pods. Kubelet should manage the file regardless of the network setup.

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

Special notes for your reviewer: Because the new logic relies on reading the node filesystem, I'm not sure how to write a proper unit test. I was thinking about using a node e2e test to cover the case, but suggestions are greatly welcomed.

Release note:
Kubelet now manages `/etc/hosts` file for both hostNetwork Pods and non-hostNetwork Pods.
/kind feature
/sig node

@yujuhong @hongchaodeng @thockin
@kubernetes/sig-network-feature-requests @kubernetes/sig-node-feature-requests

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.

hongchaodeng · 2017-07-18T18:39:28Z

@yujuhong @thockin

This is very useful and important feature. Can we get this in 1.8 ?

yujuhong · 2017-07-18T19:00:44Z

I'd like to see this happen since it improves consistency, eliminating the burden for integration. Marking this 1.8 tentatively.
/cc @mrunalp @Random-Liu as this could affect the CRI shim implementation.

feiskyer · 2017-07-19T02:09:44Z

pkg/kubelet/kubelet_pods.go

+
+	if useHostNetwork {
+		// if Pod is using host network, read hosts file from the node's filesystem.
+		hostsFileContent, err = nodeHostsFileContent()


Also add hostAliases for host-networked containers?

i plan to do this as a follow-up. this is to keep the change and relevant discussions focused

the work is covered in: #48398

feiskyer · 2017-07-19T02:09:58Z

/ok-to-test

evie404 · 2017-07-19T06:14:20Z

/test pull-kubernetes-e2e-gce-etcd3

evie404 · 2017-07-25T00:05:32Z

can i get more reviews on this? thanks a lot

evie404 · 2017-07-27T20:49:02Z

@yujuhong @feiskyer can i get reviews on this? i have some follow-up PRs to enable HostAlias for HostNetwork pods also

thockin · 2017-07-28T04:10:58Z

For testing we should probably have a param to define which file to read. You can pass "/etc/hosts" in real code and you can write a bogus file in test code.

/approve

feiskyer · 2017-07-28T10:20:18Z

pkg/kubelet/kubelet_pods_test.go

@@ -264,8 +264,8 @@ fe00::2	ip6-allrouters
 	}

 	for _, testCase := range testCases {
-		actualContent := string(hostsFileContent(testCase.hostIP, testCase.hostName, testCase.hostDomainName, testCase.hostAliases))
-		assert.Equal(t, testCase.expectedContent, actualContent, "hosts file content not expected")
+		actualContent, _ := managedHostsFileContent(testCase.hostIP, testCase.hostName, testCase.hostDomainName, testCase.hostAliases)


Also adds a test case for nodeHostsFileContent?

added tests

yujuhong

Test is needed as @thockin and @feiskyer suggested.

I think this should not cause any kubelet in-place upgrade issue, but would be nice to confirm that.

yujuhong · 2017-07-28T18:17:12Z

pkg/kubelet/kubelet_pods.go

-	mountEtcHostsFile := !pod.Spec.HostNetwork && len(podIP) > 0 && runtime.GOOS != "windows"
+	// - OS is not Windows
+	// Kubernetes will not mount /etc/hosts if:
+	// - when the pause container is being created, its IP is still unknown. Hence, PodIP will not have been set.


Change the comment to reflect the current status since you are touching it:

s/when the pause container/when the pod sandbox

didn't know the pause container was changed. will update the comment

updated comment

yujuhong · 2017-07-28T18:24:52Z

pkg/kubelet/kubelet_pods.go

-func hostsFileContent(hostIP, hostName, hostDomainName string, hostAliases []v1.HostAlias) []byte {
+// managedHostsFileContent generates the content of the managed etc hosts based on Pod IP and other
+// information.
+func managedHostsFileContent(hostIP, hostName, hostDomainName string, hostAliases []v1.HostAlias) ([]byte, error) {


Don't think returning the error is necessary since it's always nil. I think it's ok to not add it.

i added it so it'd have the same return as nodeHostsFileContent. i can remove it

removed extra error

evie404 · 2017-07-29T07:06:31Z

/test pull-kubernetes-bazel

evie404 · 2017-07-29T07:31:08Z

/test pull-kubernetes-bazel

cmluciano

can you squash some of these commits?

evie404 · 2017-07-31T18:44:58Z

squashed all commits

evie404 · 2017-07-31T20:03:02Z

/retest

…

On Mon, Jul 31, 2017 at 12:50 PM k8s-ci-robot ***@***.***> wrote: @rickypai <https://github.com/rickypai>: The following tests *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-unit a5fb7e0 <a5fb7e0> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/49140/pull-kubernetes-unit/43868/> /test pull-kubernetes-unit pull-kubernetes-e2e-gce-etcd3 a5fb7e0 <a5fb7e0> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/49140/pull-kubernetes-e2e-gce-etcd3/44129/> /test pull-kubernetes-e2e-gce-etcd3 pull-kubernetes-e2e-kops-aws a5fb7e0 <a5fb7e0> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/49140/pull-kubernetes-e2e-kops-aws/38339/> /test pull-kubernetes-e2e-kops-aws Full PR test history <https://k8s-gubernator.appspot.com/pr/49140>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/rickypai>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49140 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZsi-ZXEwgZ1KslvOykR-u885hgBMB5ks5sTjAKgaJpZM4ObuaT> .

evie404 · 2017-07-31T21:31:47Z

/retest

evie404 · 2017-08-04T00:01:28Z

can i get another review? thanks a lot!

yujuhong · 2017-08-04T00:04:33Z

pkg/kubelet/kubelet_pods.go

+
+// nodeHostsFileContent reads the content of node's hosts file.
+func nodeHostsFileContent(hostsFilePath string) ([]byte, error) {
+	// `etcHostsPath` references the location of the hosts file on the node.


Move the comment to where the variable is used (line 234)

yujuhong · 2017-08-04T00:14:45Z

pkg/kubelet/hosts_test_file1

@@ -0,0 +1,8 @@
+# hosts file for testing.


Can we not add test files in the pkg/kubelet? The test itself should write a temporary file and clean up after itself.

i think it's brittle to try to write files during a test. what if the test run fails unexpectedly? then the file stays there and you end up potentially polluting the testing infrastructure :/

Better to create the test file dynamically when running tests, e.g. under /tmp dir. The test could be useful when hostAliases is introduced.

Yes, it should be written to a temp directory, or you can mock the filesystem call. I don't have a strong opinion since many of existing unit tests write to disk already. I don't want test data checked in the non-testing directory though

evie404 · 2017-08-09T18:30:49Z

@yujuhong can you take a look again? thanks a lot

yujuhong · 2017-08-09T18:35:41Z

pkg/kubelet/kubelet_pods_test.go

+	}
+
+	for _, testCase := range testCases {
+		hostsFilePath := path.Join("/", "tmp", testCase.hostsFileName)


Suggestion: use ioutil.TempDir and ioutil.WriteFile to simplify the code and write to an OS specific temp directory.
Example: https://github.com/kubernetes/kubernetes/blob/v1.8.0-alpha.2/pkg/kubelet/dockershim/helpers_test.go#L161

evie404 · 2017-08-10T20:35:22Z

/retest

thockin · 2017-08-11T05:30:04Z

Is this going in for 1.8?

evie404 · 2017-08-11T18:34:32Z

@thockin it should be. i make an effort to respond to code review comments within 24hrs. is there a deadline i'm working against? i presume sept 1st?

evie404 · 2017-08-11T19:02:53Z

@yujuhong can you take a look again? thanks a lot

yujuhong · 2017-08-14T17:24:44Z

Is this going in for 1.8?

This PR alone doesn't (or shouldn't) have user-facing impact and is not really a feature, so I am treating it as a consistency cleanup for the code.
I didn't have any specific target milestone in mind, but it should get in once it's ready.

/lgtm

k8s-github-robot · 2017-08-14T17:24:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 48397

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

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

~~pkg/kubelet/OWNERS~~ [thockin,yujuhong]

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 · 2017-08-14T19:15:03Z

Automatic merge from submit-queue (batch tested with PRs 50094, 48966, 49478, 50593, 49140)

@yujuhong

Automatic merge from submit-queue Support HostAlias for HostNetwork Pods **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**: ```release-note HostAlias is now supported for both non-HostNetwork Pods and HostNetwork Pods. ``` @yujuhong @hongchaodeng @thockin

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/network Categorizes an issue or PR as relevant to SIG Network. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 18, 2017

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 Jul 18, 2017

k8s-github-robot assigned dims and dchen1107 Jul 18, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 18, 2017

yujuhong assigned thockin and yujuhong and unassigned dims and dchen1107 Jul 18, 2017

feiskyer reviewed Jul 19, 2017

View reviewed changes

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 Jul 19, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

feiskyer reviewed Jul 28, 2017

View reviewed changes

yujuhong reviewed Jul 28, 2017

View reviewed changes

evie404 force-pushed the rpai/hostnetwork_etc_hosts branch from 2e863c9 to 7bf79ea Compare July 31, 2017 18:10

cmluciano reviewed Jul 31, 2017

View reviewed changes

evie404 force-pushed the rpai/hostnetwork_etc_hosts branch from 7bf79ea to a5fb7e0 Compare July 31, 2017 18:44

thockin removed their assignment Aug 1, 2017

yujuhong suggested changes Aug 4, 2017

View reviewed changes

evie404 force-pushed the rpai/hostnetwork_etc_hosts branch from a5fb7e0 to f0e2ad1 Compare August 4, 2017 18:20

yujuhong reviewed Aug 9, 2017

View reviewed changes

Kubelet manage hosts file for HostNetwork Pods instead of Docker

fb4bff0

evie404 force-pushed the rpai/hostnetwork_etc_hosts branch from f0e2ad1 to fb4bff0 Compare August 10, 2017 18:41

yujuhong approved these changes Aug 14, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2017

k8s-github-robot merged commit b161831 into kubernetes:master Aug 14, 2017

evie404 mentioned this pull request Aug 14, 2017

Support HostAlias for HostNetwork Pods #50646

Merged

evie404 deleted the rpai/hostnetwork_etc_hosts branch September 28, 2017 20:20

perithompson mentioned this pull request Jul 2, 2021

Explicitly skip host file mounting for Windows when HostProcess pod #103434

Merged

Kubelet manage hosts file for HostNetwork Pods instead of Docker #49140

Kubelet manage hosts file for HostNetwork Pods instead of Docker #49140

Conversation

evie404 commented Jul 18, 2017 • edited Loading

k8s-ci-robot commented Jul 18, 2017

k8s-ci-robot commented Jul 18, 2017

hongchaodeng commented Jul 18, 2017

yujuhong commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Jul 19, 2017

evie404 commented Jul 19, 2017

evie404 commented Jul 25, 2017

evie404 commented Jul 27, 2017

thockin commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evie404 commented Jul 29, 2017

evie404 commented Jul 29, 2017

cmluciano left a comment

Choose a reason for hiding this comment

evie404 commented Jul 31, 2017

evie404 commented Jul 31, 2017 via email

evie404 commented Jul 31, 2017

evie404 commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evie404 commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evie404 commented Aug 10, 2017

thockin commented Aug 11, 2017

evie404 commented Aug 11, 2017

evie404 commented Aug 11, 2017

yujuhong commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

k8s-github-robot commented Aug 14, 2017

evie404 commented Jul 18, 2017 •

edited

Loading

feiskyer Aug 4, 2017 •

edited

Loading