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

Read once again after stdin token cancelled #746

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

saschagrunert
Copy link
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

This fixes the first test of should support inline execution and attach:

https://github.com/kubernetes/kubernetes/blob/7f129f1c9af62cc3cd4f6b754dacdf5932f39d5c/test/e2e/kubectl/kubectl.go#L603-L612

Short lived attach sessions may have some more content to read after they exited, which is now fixed by trying another read after the token has been canceled via the waitpid exit.

Which issue(s) this PR fixes:

Refers to #736

Special notes for your reviewer:

The --leave-stdin-open=true tests are still failing, but those have to be fixed in another PR:

https://github.com/kubernetes/kubernetes/blob/7f129f1c9af62cc3cd4f6b754dacdf5932f39d5c/test/e2e/kubectl/kubectl.go#L614-L648

Does this PR introduce a user-facing change?

Fixed race between container exit and stdin attach data to be processed.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

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:

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Merging #746 (7c35832) into main (fe5ceea) will increase coverage by 0.06%.
The diff coverage is 25.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #746      +/-   ##
==========================================
+ Coverage   32.34%   32.40%   +0.06%     
==========================================
  Files          13       13              
  Lines        1110     1117       +7     
  Branches      418      420       +2     
==========================================
+ Hits          359      362       +3     
- Misses        468      471       +3     
- Partials      283      284       +1     

This fixes the first test of `should support inline execution and
attach`:

https://github.com/kubernetes/kubernetes/blob/7f129f1c9af62cc3cd4f6b754dacdf5932f39d5c/test/e2e/kubectl/kubectl.go#L603-L612

Short lived attach sessions may have some more content to read after
they exited, which is now fixed by trying another read after the token
has been canceled via the waitpid exit.

Signed-off-by: Sascha Grunert <[email protected]>
@haircommander
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 22, 2022
@openshift-merge-robot openshift-merge-robot merged commit 750ca5a into containers:main Sep 22, 2022
@saschagrunert saschagrunert deleted the stdin-wait branch September 22, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants