More testcases for GOAWAY

[answer1991]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

1. Add a testcase(TestGOAWAYConcurrency) to check if GOAWAY affect concurrency requests
2. Refactor GOAWAY testcases

Which issue(s) this PR fixes:

Fixes #91131

Special notes for your reviewer:

1. Golang version go1.14.7 linux/amd64(has not fixed GOAWAY issue), testcase TestGOAWAYConcurrency failed with error like:

--- FAIL: TestGOAWAYConcurrency (5.19s)
    goaway_test.go:487: failed to request "/post", err: expect response status code: 200, but got: 500. response body: request declared a Content-Length of 5 but only wrote 0 bytes
    goaway_test.go:487: failed to request "/post", err: expect response status code: 200, but got: 500. response body: request declared a Content-Length of 5 but only wrote 0 bytes
    goaway_test.go:487: failed to request "/post", err: expect response status code: 200, but got: 500. response body: request declared a Content-Length of 5 but only wrote 0 bytes
    goaway_test.go:487: failed to request "/post", err: expect response status code: 200, but got: 500. response body: request declared a Content-Length of 5 but only wrote 0 bytes

2. Golang version go1.15 linux/amd64, testcase TestGOAWAYConcurrency succeed.

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

[k8s-ci-robot]

Hi @answer1991. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[answer1991]

/ok-to-test
/sig api-machinery

[k8s-ci-robot]

@answer1991: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test
/sig api-machinery

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.

[eddiezane]

/ok-to-test

[fedebongio]

/assign @deads2k

[answer1991]

/test pull-kubernetes-e2e-gce-ubuntu-containerd

[answer1991]

As we expected, after upgrading to golang 1.15, GOAWAY feature will not affect concurrency requests. We now are ready to enable GOAWAY feature.

PTAL @lavalamp @deads2k @wojtek-t

[dims]

response -> responded

[dims]

Also what time period? if you see the previous 2, they say in last second

[dims]

Looks like we are adding something new apiserver_goaway_response so this is not just adding test cases. am i reading this right?

[answer1991]

Sorry, I used a wrong metrics type. I will fixed it later.

so this is not just adding test cases. am i reading this right? - yes, 2 commits.

[lavalamp]

Also what time period? if you see the previous 2, they say in last second

The last two metrics are gauges, this is a counter, counters don't have time frames.

However I do think it'd be good to add this metric in a separate PR. I'd like to take the tests first.

The metric is about connections (right?), but the other metrics here are about requests. So it's confusing and I think we should talk about that more. Looks like the tests don't depend on this metric, so hopefully it won't be much trouble to send it in a separate PR.

[lavalamp]

(nit: "response" is a noun, "respond" is the verb)

[answer1991]

Fixed the nit and removed the metrics commit.

[lavalamp]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: answer1991, lavalamp

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:

• staging/src/k8s.io/apiserver/OWNERS [lavalamp]

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

[answer1991]

/test pull-kubernetes-e2e-kind

[wojtek-t]

Adding milestone - this is purely test change.

/milestone v1.20

[answer1991]

/kind cleanup

[liggitt]

this test fails 100% of the time when run with go test -race k8s.io/apiserver/pkg/server/filters -run TestGOAWAYConcurrency, disabling in #94529 and opened #94532 to track fixing the test or the component

[answer1991]

caused by golang http/http2 pkg, I have already opened an issue #41234 at golang repo .

[liggitt]

this added a data race on unguarded localAddr, fixed in #94530

[answer1991]

As far as I know, it's not the root cause of TestClientReceivedGOAWAY flake, because dial a new connection only when client send a new request, and in this case, requests are sent one by one. In other words, if this line cause data race, then the additional unexpected connection set up, which means the test case or the configuration has problem.

I'm still debug the root cause of TestClientReceivedGOAWAY flake, for now, I doubt that I used an incorrect http2 server/client configuration(http.Transport.MaxConnsPerHost=1 or SETTINGS_MAX_CONCURRENT_STREAMS limited), or the dial function ignored context.Context which will cause more than one goroutine dial the connection when the first canceled.

Another interesting thing is I got a data race result that read at if tc.expectConnections != len(localAddr)(main testing goroutine) and write at localAddr = append(localAddr, conn.LocalAddr().String()), I'm very confused why the client try to set-up a new connection when we finished all requests.