Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Adds large requests tests #83680

Open
wants to merge 1 commit into
base: master
from

Conversation

@bclau
Copy link
Contributor

commented Oct 9, 2019

What type of PR is this?

/sig testing

What this PR does / why we need it:

Ensures that requests that require large packets work properly, and that they are not dropped.

Which issue(s) this PR fixes:

Related: #83739
Depends On: #84045

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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


@alejandrox1

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

/retest

@alejandrox1

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

/kind bug
/priority critical-urgent
/milestone v1.17
/assign @timothysc @johnSchnake

/remove-area e2e-test-framework

@alejandrox1

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

@bclau

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

the tests are failing because this PR bumps the agnhost image version.

@bclau bclau force-pushed the bclau:tests/network-large-requests branch from a229186 to df45aba Oct 17, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bclau
To complete the pull request process, please assign timothysc
You can assign the PR to them by writing /assign @timothysc in a comment when ready.

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

Needs approval from an approver in each of these files:

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

Ensures that requests that require large packets work properly, and that
they are not dropped.
@johnSchnake

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2019

/lgtm

nits: 99% of the tests are duplicaeted so it seems like something could be reused but it isn't a big deal; the tests are small and easy enough to follow. It also seems like the tests should send the exact same message in each case, but all just nits.

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 21, 2019
Copy link
Member

left a comment

It would be good to fixup the documentation, otherwise this part of the testing code is going to be harder to understand for those who are not intimately familiar with the behavior of echo server.

@@ -199,18 +212,19 @@ func (config *NetworkingTestConfig) EndpointHostnames() sets.String {
// maxTries == minTries will confirm that we see the expected endpoints and no
// more for maxTries. Use this if you want to eg: fail a readiness check on a
// pod and confirm it doesn't show up as an endpoint.
func (config *NetworkingTestConfig) DialFromContainer(protocol, containerIP, targetIP string, containerHTTPPort, targetPort, maxTries, minTries int, expectedEps sets.String) {
func (config *NetworkingTestConfig) DialFromContainer(protocol, dialCommand, containerIP, targetIP string, containerHTTPPort, targetPort, maxTries, minTries int, expectedResponses sets.String) {

This comment has been minimized.

Copy link
@bowei

bowei Oct 21, 2019

Member

You need to update the comment to reflect the new param name, also, what is the content of expectedResponses supposed to remain the same? The new variable name is pretty generic and the old name specifically states that these are endpoints.

This comment has been minimized.

Copy link
@bowei

bowei Oct 21, 2019

Member

dialCommand is not documented

This comment has been minimized.

Copy link
@bowei

bowei Oct 21, 2019

Member

can we make the dial command more of an enum with documentation?

type EchoServerCommand string
var (
  /* some enum for the default behavior?*/
  // EchoHostname tells the echo server to ...
  EchoHostname = "hostname" 
)

This comment has been minimized.

Copy link
@bclau

bclau Oct 22, 2019

Author Contributor

Indeed. I wanted to reuse this method, there was no reason to duplicate it. Some changes were required though, like the dialCommand (it was only hostname before), and the expected response.

Will document dialCommand.

As for enumerating, sounds good, but it won't work. unless we also make the message constant. I'm thinking that maybe at some point we'll also want to test for jumbo frames, so the message size would be different anyways.

expectedResponse := sets.NewString()
expectedResponse.Insert(echoMessage)
var dialCommand string
if protocol == "http" {

This comment has been minimized.

Copy link
@bowei

bowei Oct 21, 2019

Member

This is pretty obscure -- I'm assuming this triggers some path in the echo container. Can you document that

This comment has been minimized.

Copy link
@bclau

bclau Oct 22, 2019

Author Contributor

Well, netexec will just call the given targetIP and targetPort with this dial command with this protocol. For http, netexec will just send a web request to http://targetIP:targetPort/dialCommand", while for UDP, the message dialCommandis sent. The receiver will look for a few starting strings, includingecho`` and will treat them accordingly.

@bowei

This comment has been minimized.

Copy link
Member

commented Oct 21, 2019

/hold

@bclau bclau force-pushed the bclau:tests/network-large-requests branch from df45aba to 24839a3 Oct 22, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 22, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2019

New changes are detected. LGTM label has been removed.

@bclau bclau force-pushed the bclau:tests/network-large-requests branch 2 times, most recently from 718fca2 to a637a34 Oct 22, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2019

@bclau: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-100-performance a637a34 link /test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-e2e-kind a637a34 link /test pull-kubernetes-e2e-kind
pull-kubernetes-integration a637a34 link /test pull-kubernetes-integration
pull-kubernetes-e2e-gce a637a34 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.