Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Increase port-forward timeout to 1s to fix e2e test#1477

Merged
mikebrow merged 1 commit intocontainerd:masterfrom
saschagrunert:port-forward-timeout
May 12, 2020
Merged

Increase port-forward timeout to 1s to fix e2e test#1477
mikebrow merged 1 commit intocontainerd:masterfrom
saschagrunert:port-forward-timeout

Conversation

@saschagrunert
Copy link
Contributor

@saschagrunert saschagrunert commented May 12, 2020 •
edited
Loading

We encountered two failing end-to-end tests after the adoption of #1470 in cri-o/cri-o#3749:

Summarizing 2 Failures:
[Fail] [sig-cli] Kubectl Port forwarding With a server listening on 0.0.0.0 that expects a client request [It] should support a client that connects,
sends DATA, and disconnects
test/e2e/kubectl/portforward.go:343

[Fail] [sig-cli] Kubectl Port forwarding With a server listening on localhost that expects a client request [It] should support a client that connects
, sends DATA, and disconnects
test/e2e/kubectl/portforward.go:343

Increasing the timeout to 1s fixes the issue.

cc @aojea

@k8s-ci-robot
Copy link

k8s-ci-robot commented May 12, 2020

Hi @saschagrunert. Thanks for your PR.

I'm waiting for a containerd 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.

Details

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.

saschagrunert
saschagrunert commented May 12, 2020
View reviewed changes
@saschagrunert saschagrunert mentioned this pull request May 12, 2020
Merged
@aojea
Copy link
Contributor

aojea commented May 12, 2020

I didn't know there were kubernetes tests covering this, otherwise, I should've tested the patch, sorry.

Anyway, I can confirm that the timeout it's too short and make the e2e Kubernetes fail, using 1 second the tests pass.

@dims
Copy link
Member

dims commented May 12, 2020

LGTM

/ok-to-test

@k8s-ci-robot
Copy link

k8s-ci-robot commented May 12, 2020

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

Details

In response to this:

LGTM

/ok-to-test

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.

@dims
Copy link
Member

dims commented May 12, 2020

LGTM

yes please!

@saschagrunert
Increase port-forward timeout to 1s to fix e2e test
e2cedb9 
We encountered two failing end-to-end tests after the adoption of
containerd#1470 in
cri-o/cri-o#3749:

```
Summarizing 2 Failures:
[Fail] [sig-cli] Kubectl Port forwarding With a server listening on 0.0.0.0 that expects a client request [It] should support a client that connects,
sends DATA, and disconnects
test/e2e/kubectl/portforward.go:343

[Fail] [sig-cli] Kubectl Port forwarding With a server listening on localhost that expects a client request [It] should support a client that connects
, sends DATA, and disconnects
test/e2e/kubectl/portforward.go:343
```

Increasing the timeout to 1s fixes the issue.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@saschagrunert saschagrunert force-pushed the port-forward-timeout branch from a9dfd88 to e2cedb9 Compare May 12, 2020 10:43
@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 12, 2020

/ok-to-test

thaJeztah
thaJeztah approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah mentioned this pull request May 12, 2020
Merged
mikebrow
mikebrow approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow mikebrow merged commit 64aa9da into containerd:master May 12, 2020
@saschagrunert saschagrunert deleted the port-forward-timeout branch May 12, 2020 12:59
@aojea
Copy link
Contributor

aojea commented May 12, 2020

It seems that socat approach had these failures too.
Definitively this timeout seems related kubernetes/kubernetes#86355 and maybe something we should tune in the future?

@saschagrunert
Copy link
Contributor Author

saschagrunert commented May 12, 2020

It seems that socat approach had these failures too.
Definitively this timeout seems related kubernetes/kubernetes#86355 and maybe something we should tune in the future?

Is the actual issue that the client (container runtime) does not know when there is no more content to be expected from the streaming server? If yes, then I'm wondering if we can optimize there:
https://github.com/kubernetes/kubernetes/blob/70948498bcc696e66780edb4e4652366d4862e65/pkg/kubelet/server/portforward/httpstream.go#L36-L68

Or there: https://github.com/kubernetes/kubernetes/blob/70948498bcc696e66780edb4e4652366d4862e65/pkg/kubelet/server/portforward/httpstream.go#L208-L234

@mikebrow
Copy link
Member

mikebrow commented May 12, 2020 •
edited
Loading

It seems that socat approach had these failures too.
Definitively this timeout seems related kubernetes/kubernetes#86355 and maybe something we should tune in the future?

Is the actual issue that the client (container runtime) does not know when there is no more content to be expected from the streaming server? If yes, then I'm wondering if we can optimize there:
https://github.com/kubernetes/kubernetes/blob/70948498bcc696e66780edb4e4652366d4862e65/pkg/kubelet/server/portforward/httpstream.go#L36-L68

Or there: https://github.com/kubernetes/kubernetes/blob/70948498bcc696e66780edb4e4652366d4862e65/pkg/kubelet/server/portforward/httpstream.go#L208-L234

IMO It's a test case error.
https://github.com/kubernetes/kubernetes/blob/89ba90573f163ee3452b526f30348a035d54e870/test/e2e/kubectl/portforward.go#L322-L344

@saschagrunert The test sends 'abc' to the stream for it's writer on the port, then immediately closes their writer. Then the test starts reading. At some point it is expected that the code in the container is getting ready to get use abc and do something like send data back over the port in response, but we have received the close already in the forwarder because they closed their writer. Now it's a race condition. If the test had waited to close the writer there would be no race.

@saschagrunert
Copy link
Contributor Author

saschagrunert commented May 12, 2020

Thank you for the insights @mikebrow, I can propose a PR to fix that and we can reduce the timeout here again afterwards. :)

@mikebrow
Copy link
Member

mikebrow commented May 12, 2020

Thank you for the insights @mikebrow, I can propose a PR to fix that and we can reduce the timeout here again afterwards. :)

Sure, thx. I like your idea for using an http idle wait after one or the other side has been closed, I had considered that as well but figured it would need to be in this short time range anyway so.. Maybe a config value for the timeout..

@saschagrunert saschagrunert mentioned this pull request May 13, 2020
Merged
@saschagrunert
Copy link
Contributor Author

saschagrunert commented May 13, 2020

Good, a fix the e2e test is incoming here: kubernetes/kubernetes#91045

@mikebrow mikebrow mentioned this pull request Sep 2, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@aojea aojea aojea left review comments

@thaJeztah thaJeztah thaJeztah approved these changes

@mikebrow mikebrow mikebrow approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test size/XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@saschagrunert @k8s-ci-robot @aojea @dims @AkihiroSuda @mikebrow @thaJeztah