Replace exec, pause pod with platform agnostic agnhost pause pod #79012

mgdevstack · 2019-06-14T08:04:18Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
E2E shouldn't be verifying failure reasons and messages which are optional and might change in future release of k8s. (Ref: #75324 (comment))

~~This PR removes "error message" checking codeline and~~ replaces underlying pause and exec pod images with agnhost image. (Image consolidation issue ref: #76342)

These changes would make e2e eligible as a candidate for Conformance.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

/area conformance
/sig testing
@kubernetes/sig-node-pr-reviews
@kubernetes/sig-architecture-pr-reviews
@kubernetes/cncf-conformance-wg

danwinship · 2019-06-14T14:33:14Z

test/e2e/framework/pod/resource.go

-					Image:   BusyBoxImage,
-					Command: []string{"sh", "-c", "trap exit TERM; while true; do sleep 5; done"},
+					Name:  "agnhost-pause",
+					Image: imageutils.GetE2EImage(imageutils.Agnhost),


This won't work; newExecPodSpec is being used to create pods for kubectl exec-ing into, so it needs to have an image that has stuff in it

oh, i see agnhost has some binaries in it... but I'm assuming that the reason why a whole bunch of tests failed on this PR is that it doesn't have everything needed...

I'm troubleshooting failing E2Es. But effort is to use agnhost. agnhost:2.0 or further release holds various cli based app and system binaries. (#78389)

All tests are passing now with the introduction of agnhost:2.0 from #78389.

This change looks valid. Can we keep this and drop the change to service.go?

danwinship · 2019-06-14T14:35:58Z

test/e2e/network/service.go

@@ -1836,21 +1836,17 @@ var _ = SIGDescribe("Services", func() {
 		cmd := fmt.Sprintf(`wget -T 3 -qO- %v`, serviceAddress)

 		ginkgo.By(fmt.Sprintf("hitting service %v from pod %v on node %v", serviceAddress, podName, nodeName))
-		expectedErr := "connection refused"


This test specifically needs to distinguish different kinds of failure ("connection refused" vs "timed out") and the underlying binaries don't provide any way to do that other than by looking at the error messages.

Good explanation is documented here in #75324 (comment)

Events and contents of condition reasons and messages are not covered by API compatibility guarantees. They are passed through for human consumption, not machine consumption.

The error message it's comparing against is from "curl", not from kubernetes, so kubernetes's API compatibility guarantees aren't relevant.

Alternatively, we could build our own test binary into agnhost and have it exit with different error codes in the two cases. Would that be possible?

error messages itself are "breaking changes" and should be avoided in such situation.

build our own test binary into agnhost and have it exit with different error codes in the two cases

What could be the benefit? ExitCode "zero" and "non-zero" are sufficient enough to verify SUCCESS and FAILURE behaviour or state. Desired state must meet regardless of reasons of success and failure of E2E.

We aren't testing "is the service reachable", we are testing whether the iptables rules are set up correctly. The way we test that currently is by seeing whether a connection attempt is rejected ("connection refused") or dropped ("timeout"). Your rewrite is not correct. It will always report success when the service is not reachable, regardless of whether the iptables rules are correct or not.

At the moment, the only way to test this correctly is by looking at the error string. If you are going to remove the error string check, then you need to remove the entire test and file an issue indicating that you have done so and that a new test needs to be written.

There is no HTTP server running. If we get an HTTP status code then something has gone terribly terribly wrong with the test case.

Clarifying this test a bit more: in the past, if you tried to connect to a service that had no endpoints, the connection attempt would hang for a while, and then eventually time out and fail, after you hit some kernel/client timeout. (eg, like what happens if you try "curl http://2.2.2.2/") This was a little bit annoying, so we tweaked the iptables rules so that now if you try to connect to a service with no endpoints, it will fail immediately. (eg, like if you try "curl http://127.0.0.1:1"). And there was much rejoicing. (:tada:)

But then, someone changed something in our iptables rules in a way that made these rules not work. And nobody noticed for a while, and we shipped a release where trying to connect to inactive services would hang rather than immediately failing and it was annoying and we were sad. So after it got fixed, we added a test to make sure it stayed fixed. And that's this test.

So, the goal of the test is to make sure that if you try to connect to a service when that service is unavailable, the connection attempt should fail "immediately" rather than "eventually". We can't do that just by looking at the iptables rules, because we want to test that they're working, not just that they're there. In theory maybe we could test it by timing the connection attempt and seeing if it fails "immediately" or "eventually", but that's tricky because sometimes the test machine might be hosed and so it would take it a moment to return the "immediate" error, and other times the kernel might have already cached the fact that the destination is unreachable and so will quickly return the "eventually" error. So it's hard to pick a timeout that we can be certain won't cause flakes. It's more reliable to test if we got "connection refused" (pass) or "no route to host"/"timeout" (fail).

As I said a few comments back, if we could add a tiny test program (like, 10 lines of go) to agnhost then we could use that instead, and have it exit with different status codes depending on the errno value returned by the net.Dial() call, and then there'd be no need for potentially-flaky string comparisons.

But if we're sticking with curl, then I believe we have to stick with string comparisons.

So I was thinking/hoping that curl/wget would be setting some sort of httpstatus code based on the failure it encountered (timeout vs refused) but that doesnt seem to be the case.

The exit code of curl is, in both cases, 7 and wget returns 4 in both cases.

Meanwhile, as you proposed the go solution does have a non-string based solution since the net errors have a Timeout() method.

https://play.golang.org/p/Fdhb72pm7CV is a copy of what I did locally. [Note: it doesnt work on the playground, saying not a timeout in both cases, but thats just the playground env).

This seems like the only solution proposed that checks everyone's boxes.

[BTW you've mentioned curl but it looks like its actually using wget, but all the same points apply].

OK, I've filed #79423 with a new agnhost connect command, and updating this test to use it. Feel free to just absorb those commits into this PR though if you'd prefer.

I agree that #79243 looks like the way to go, I think I'd rather see this PR drop changes to this file and merge

Dropping this change in favour of #79423 and once this is merged, respective e2e can be picked up for Conformance promotion.

neolit123 · 2019-06-14T23:57:33Z

test/e2e/network/service.go

+		})
+
+		if err != wait.ErrWaitTimeout {
+			framework.ExpectNoError(err)


the error should be annotated.

spiffxp · 2019-06-15T02:11:12Z

/priority important-soon
/milestone v1.16

mgdevstack · 2019-06-15T04:08:40Z

/work-in-progress
Troubleshooting failing e2e.

mgdevstack · 2019-06-15T04:09:00Z

/hold

mgdevstack · 2019-06-17T06:01:41Z

/test pull-kubernetes-e2e-gce

mgdevstack · 2019-06-17T08:54:47Z

/hold cancel
Annotated error and tests are passing.

spiffxp

Can we re-scope this so it can merge?

spiffxp · 2019-06-27T22:42:51Z

test/e2e/network/service.go

@@ -1836,21 +1836,17 @@ var _ = SIGDescribe("Services", func() {
 		cmd := fmt.Sprintf(`wget -T 3 -qO- %v`, serviceAddress)

 		ginkgo.By(fmt.Sprintf("hitting service %v from pod %v on node %v", serviceAddress, podName, nodeName))
-		expectedErr := "connection refused"


I agree that #79243 looks like the way to go, I think I'd rather see this PR drop changes to this file and merge

spiffxp · 2019-06-27T22:43:51Z

test/e2e/framework/pod/resource.go

-					Image:   BusyBoxImage,
-					Command: []string{"sh", "-c", "trap exit TERM; while true; do sleep 5; done"},
+					Name:  "agnhost-pause",
+					Image: imageutils.GetE2EImage(imageutils.Agnhost),


This change looks valid. Can we keep this and drop the change to service.go?

spiffxp · 2019-06-28T02:39:21Z

/approve
/lgtm

k8s-ci-robot · 2019-06-28T02:40:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgdevstack, spiffxp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/framework/OWNERS~~ [spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2019-06-28T05:49:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-06-28T10:01:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from danwinship and neolit123 June 14, 2019 08:05

k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test labels Jun 14, 2019

danwinship suggested changes Jun 14, 2019

View reviewed changes

neolit123 reviewed Jun 14, 2019

View reviewed changes

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 15, 2019

k8s-ci-robot added this to the v1.16 milestone Jun 15, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 15, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2019

mgdevstack force-pushed the master-service-errmsg branch from a2d34df to 0d4799e Compare June 17, 2019 07:36

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2019

k8s-ci-robot assigned bgrant0607 and timothysc Jun 17, 2019

spiffxp added this to To Triage in conformance-definition Jun 20, 2019

timothysc removed their assignment Jun 20, 2019

timothysc assigned johnSchnake and spiffxp Jun 20, 2019

timothysc moved this from To Triage to In Review in conformance-definition Jun 20, 2019

danwinship mentioned this pull request Jun 26, 2019

Add new agnhost subcommand for a networking test #79423

Merged

spiffxp reviewed Jun 27, 2019

View reviewed changes

spiffxp moved this from In Review to In Progress in conformance-definition Jun 27, 2019

Replace buzybox and pause pod image with agnhost

49cc8ef

mgdevstack force-pushed the master-service-errmsg branch from 0d4799e to 49cc8ef Compare June 28, 2019 02:15

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 28, 2019

mgdevstack changed the title ~~Remove error message check lines from e2e and replace exec, pause pod with platform agnostic agnhost pause pod~~ Replace exec, pause pod with platform agnostic agnhost pause pod Jun 28, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2019

k8s-ci-robot merged commit 3bd11af into kubernetes:master Jun 28, 2019

conformance-definition automation moved this from In Progress to Done Jun 28, 2019

danwinship mentioned this pull request Jul 12, 2019

Make service "no endpoints" test use agnhost connect #80086

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace exec, pause pod with platform agnostic agnhost pause pod #79012

Replace exec, pause pod with platform agnostic agnhost pause pod #79012

mgdevstack commented Jun 14, 2019 •

edited

danwinship Jun 14, 2019

danwinship Jun 14, 2019

mgdevstack Jun 15, 2019

mgdevstack Jun 17, 2019

spiffxp Jun 27, 2019

mgdevstack Jun 28, 2019

danwinship Jun 14, 2019

mgdevstack Jun 15, 2019

danwinship Jun 15, 2019

mgdevstack Jun 17, 2019

danwinship Jun 17, 2019 •

edited

danwinship Jun 25, 2019

johnSchnake Jun 25, 2019

danwinship Jun 26, 2019

spiffxp Jun 27, 2019

mgdevstack Jun 28, 2019

neolit123 Jun 14, 2019

mgdevstack Jun 17, 2019

spiffxp commented Jun 15, 2019

mgdevstack commented Jun 15, 2019

mgdevstack commented Jun 15, 2019

mgdevstack commented Jun 17, 2019

mgdevstack commented Jun 17, 2019

spiffxp left a comment

spiffxp Jun 27, 2019

spiffxp Jun 27, 2019

spiffxp commented Jun 28, 2019

k8s-ci-robot commented Jun 28, 2019

fejta-bot commented Jun 28, 2019

fejta-bot commented Jun 28, 2019

Replace exec, pause pod with platform agnostic agnhost pause pod #79012

Replace exec, pause pod with platform agnostic agnhost pause pod #79012

Conversation

mgdevstack commented Jun 14, 2019 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship Jun 17, 2019 • edited

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

Choose a reason for hiding this comment

spiffxp commented Jun 15, 2019

mgdevstack commented Jun 15, 2019

mgdevstack commented Jun 15, 2019

mgdevstack commented Jun 17, 2019

mgdevstack commented Jun 17, 2019

spiffxp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spiffxp commented Jun 28, 2019

k8s-ci-robot commented Jun 28, 2019

fejta-bot commented Jun 28, 2019

fejta-bot commented Jun 28, 2019

mgdevstack commented Jun 14, 2019 •

edited

danwinship Jun 17, 2019 •

edited