add e2e node test for Pod hostAliases feature #46385

evie404 · 2017-05-24T22:12:09Z

What this PR does / why we need it: adds node e2e test for #45148

Release note:

NONE

@yujuhong @thockin

k8s-ci-robot · 2017-05-24T22:12:17Z

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 @k8s-bot 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.

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.

0xmichalis · 2017-05-25T08:55:07Z

test/e2e_node/kubelet_test.go

+
+				rc, err := podClient.GetLogs(podName, &v1.PodLogOptions{}).Stream()
+				if err != nil {
+					return ""


Print this error?

every other example seem to return blank string when there's an error from getting logs. I'm not sure what's the rationale, but just cargo-culting.

@vishh could you help us understand why in other test cases it's acceptable to return blank string? sorry i'm not very familiar with the test framework used for the node e2e tests.

It's fine to return the empty string, logging the error is orthogonal.

mtaufen · 2017-05-26T15:58:05Z

test/e2e_node/kubelet_test.go

+
+			Eventually(func() string {
+				pod, err := podClient.Get(podName, metav1.GetOptions{})
+				Expect(err).To(BeNil(), fmt.Sprintf("Error getting Pod %v", err))


Use framework.ExpectNoError(err)

evie404 · 2017-05-30T20:55:06Z

issues have been addressed. if you can take a look at this PR again, that'd be great. @Kargakis @mtaufen

yujuhong · 2017-06-01T22:47:37Z

test/e2e_node/kubelet_test.go

+	Context("when scheduling a busybox Pod with hostAliases", func() {
+		podName := "busybox-host-aliases" + string(uuid.NewUUID())
+		expectedHostsFileContent := `# Kubernetes-managed hosts file.
+127.0.0.1	localhost


Do we really need to check the entire file? It should be sufficient if /etc/hosts contains the two lines we care about.

there's currently no e2e test checking any other content of the hosts file. so i figure i can help by checking the whole thing.

fixed to just check hostAlias-related entries

yujuhong · 2017-06-01T22:58:39Z

test/e2e_node/kubelet_test.go

+
+				expectedHostsFileContent = fmt.Sprintf(expectedHostsFileContent, podIP, podName)
+
+				rc, err := podClient.GetLogs(podName, &v1.PodLogOptions{}).Stream()


I know you copied this from other tests, but consider using framework.GetPodLogs() which returns a string directly. This saves you the need of reading the output yourself.

i haven't really figured out how to use framework.GetPodLogs(). keeping the old way for now.

Here's an example if you'd like to give it a try:
https://github.com/kubernetes/kubernetes/blob/v1.8.0-alpha.0/test/e2e_node/security_context_test.go#L87

yujuhong · 2017-06-01T23:04:26Z

test/e2e_node/kubelet_test.go

+				},
+			})
+
+			Eventually(func() string {


I'd suggest making the function returns an error, and check whether the error is nil. This way, you can generate the error you want, without thinking what should be logged (e.g., failed fetching pod logs) and whether people would notice that.

fixed to make function return error

yujuhong · 2017-06-01T23:06:15Z

@k8s-bot ok to test

fejta · 2017-06-02T05:15:39Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref: #46827

yujuhong · 2017-06-02T20:00:57Z

/lgtm

yujuhong · 2017-06-02T20:04:57Z

/cc @Random-Liu @timstclair for approval.

evie404 · 2017-06-02T20:22:03Z

thanks @yujuhong. should I backport this to release-1.7 branch? I think that'd be a good idea.

timstclair · 2017-06-02T21:04:43Z

test/e2e_node/kubelet_test.go

+						{
+							Image:   "gcr.io/google_containers/busybox:1.24",
+							Name:    podName,
+							Command: []string{"/bin/sh", "-c", "cat /etc/hosts; sleep 240"},


nit: add a longer sleep timer and just let the namespace cleanup the pod (lots of tests use 6000 (10 minutes))

timstclair · 2017-06-02T21:07:18Z

test/e2e_node/kubelet_test.go

+					return err
+				}
+				buf := new(bytes.Buffer)
+				buf.ReadFrom(rc)


If you're going to read the whole stream, just use \.DoRaw() rather than Stream()

I reverted this. this was introducing panics in the tests and the logs are quite difficult to understand: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/46385/pull-kubernetes-node-e2e/33666/nodelog?junit=junit_gci_05.xml&wrap=on

also, Stream() is used in other kubelet e2e tests, so I think it's safer to follow the pattern:

kubernetes/test/e2e_node/kubelet_test.go

Line 143 in 05bff30

rc, err := podClient.GetLogs(podName, &v1.PodLogOptions{}).Stream()

timstclair · 2017-06-02T21:09:51Z

I think 1.7 is still tracking master, so no cherrypick should be necessary.

thockin · 2017-06-06T18:46:47Z

/lgtm
/approve

k8s-github-robot · 2017-06-06T18:54:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 45148

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

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

~~test/e2e_node/OWNERS~~ [thockin]

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

evie404 · 2017-06-07T17:41:37Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-github-robot · 2017-06-07T20:30:59Z

Automatic merge from submit-queue (batch tested with PRs 43005, 46660, 46385, 46991, 47103)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 24, 2017

k8s-github-robot assigned mtaufen and euank May 24, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 24, 2017

evie404 mentioned this pull request May 24, 2017

Provide a way to add entries to /etc/hosts #43632

Closed

0xmichalis reviewed May 25, 2017

View reviewed changes

mtaufen reviewed May 26, 2017

View reviewed changes

evie404 force-pushed the rpai/host_mapping_node_e2e_test branch from c740d9d to 8ee7fbf Compare May 26, 2017 20:56

yujuhong assigned yujuhong and mtaufen and unassigned mtaufen and euank Jun 1, 2017

yujuhong reviewed Jun 1, 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 Jun 1, 2017

evie404 force-pushed the rpai/host_mapping_node_e2e_test branch 2 times, most recently from 632b460 to 15ca435 Compare June 1, 2017 23:52

evie404 force-pushed the rpai/host_mapping_node_e2e_test branch 2 times, most recently from c8b7960 to 42b7541 Compare June 2, 2017 18:42

yujuhong added this to the v1.7 milestone Jun 2, 2017

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

timstclair reviewed Jun 2, 2017

View reviewed changes

evie404 force-pushed the rpai/host_mapping_node_e2e_test branch from 42b7541 to a2692cd Compare June 2, 2017 22:33

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017

e2e node test for PodSpec HostAliases

4e7fed4

evie404 force-pushed the rpai/host_mapping_node_e2e_test branch from a2692cd to 4e7fed4 Compare June 3, 2017 00:02

thockin added approved-for-milestone approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 6, 2017

k8s-ci-robot assigned thockin Jun 6, 2017

k8s-github-robot merged commit 6ee0282 into kubernetes:master Jun 7, 2017

evie404 deleted the rpai/host_mapping_node_e2e_test branch June 7, 2017 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add e2e node test for Pod hostAliases feature #46385

add e2e node test for Pod hostAliases feature #46385

evie404 commented May 24, 2017 •

edited

Loading

k8s-ci-robot commented May 24, 2017

0xmichalis May 25, 2017

evie404 May 25, 2017

evie404 May 26, 2017

0xmichalis May 26, 2017

evie404 May 26, 2017

mtaufen May 26, 2017

evie404 May 26, 2017

evie404 commented May 30, 2017

yujuhong Jun 1, 2017

evie404 Jun 2, 2017

yujuhong Jun 1, 2017

evie404 Jun 2, 2017

yujuhong Jun 2, 2017

yujuhong Jun 1, 2017

evie404 Jun 2, 2017

yujuhong commented Jun 1, 2017

fejta commented Jun 2, 2017

yujuhong commented Jun 2, 2017

yujuhong commented Jun 2, 2017

evie404 commented Jun 2, 2017

timstclair Jun 2, 2017

evie404 Jun 2, 2017

timstclair Jun 2, 2017

evie404 Jun 2, 2017

evie404 Jun 3, 2017

timstclair commented Jun 2, 2017

thockin commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

evie404 commented Jun 7, 2017

k8s-github-robot commented Jun 7, 2017


		expectedHostsFileContent = fmt.Sprintf(expectedHostsFileContent, podIP, podName)

		rc, err := podClient.GetLogs(podName, &v1.PodLogOptions{}).Stream()

add e2e node test for Pod hostAliases feature #46385

add e2e node test for Pod hostAliases feature #46385

Conversation

evie404 commented May 24, 2017 • edited Loading

k8s-ci-robot commented May 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evie404 commented May 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Jun 1, 2017

fejta commented Jun 2, 2017

yujuhong commented Jun 2, 2017

yujuhong commented Jun 2, 2017

evie404 commented Jun 2, 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

timstclair commented Jun 2, 2017

thockin commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

evie404 commented Jun 7, 2017

k8s-github-robot commented Jun 7, 2017

evie404 commented May 24, 2017 •

edited

Loading