Drain stdin on attach

[saschagrunert]

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

The Kubernetes e2e test flakes because of a token cancellation race between stdin and attach read:

[FAIL] [sig-cli] Kubectl client Simple pod [It] should support inline execution and attach
…
  << Timeline

  [FAILED] Expected
      <string>: read:stdin closed

  to contain substring
      <string>: read:value
  In [It] at: test/e2e/kubectl/kubectl.go:764 @ 01/04/23 15:09:48.618

https://github.com/kubernetes/kubernetes/blob/5cbd6960c805c22e1c741dde3ce9f2e10f869bce/test/e2e/kubectl/kubectl.go#L757-L766

The issue is that we cancel the token immediately which stops reading from attach. But the container command echo -n read: && cat && echo 'stdin closed' within the test provides additional data, which gets no time to be delivered.

We now drain stdin accordingly and wait for all data to be passed down to the receiver.

Which issue(s) this PR fixes:

Refers to #736

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Correctly drain stdin on attach.

[sohankunkerkar]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, sohankunkerkar

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:

• OWNERS [saschagrunert]

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

[cgwalters]

Doesn't this just make flakes less likely? I am not familiar with all the details here, just a general suspicion of fixing race conditions with sleep(). Offhand wouldn't we need to read from the fd to end?

[saschagrunert]

Doesn't this just make flakes less likely?

That's true. One way to fix it would be to increase the complexity by having cancellation tokens for input and output streams. I can follow-up on that, but I'm not sure if the effort is comparable to the test fix.

[rphillips]

+1 to not sleeping.

The cancellation routine could loop on the data read to drain the buffer, and return on error or EOF.

[haircommander]

+1 to not sleeping.

The cancellation routine could loop on the data read to drain the buffer, and return on error or EOF.

I think that's basically what current conmon does

[saschagrunert]

I think we can simply attach.read() and wait blocking because we already drop the read half in case of EOF. PTAL again @rphillips @haircommander @cgwalters

[rphillips]

lgtm

[haircommander]

/lgtm

good idea!