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

Remove token from ContainerIO read_loop #739

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Sep 21, 2022

What type of PR is this?

/kind bug

What this PR does / why we need it:

This allows the read loop to finish one there is nothing more to read.

Which issue(s) this PR fixes:

Fixes #738

Special notes for your reviewer:

This is fixing the GitHub actions e2e tests in CRI-O: https://github.com/cri-o/cri-o/actions/runs/3098295877/jobs/5017090180

Does this PR introduce a user-facing change?

Fixed bug where read loop races with the container exit.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 21, 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 Report

Merging #739 (814c50a) into main (8e4cec9) will decrease coverage by 1.64%.
The diff coverage is 19.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #739      +/-   ##
==========================================
- Coverage   34.04%   32.40%   -1.65%     
==========================================
  Files          13       13              
  Lines        1125     1108      -17     
  Branches      417      416       -1     
==========================================
- Hits          383      359      -24     
- Misses        453      467      +14     
+ Partials      289      282       -7     

@saschagrunert saschagrunert changed the title Remove token from ContainerIO read_loop WIP: Remove token from ContainerIO read_loop Sep 21, 2022
@saschagrunert saschagrunert force-pushed the read-loop-token branch 4 times, most recently from 0107f94 to 2ae593f Compare September 21, 2022 08:43
This allows the read loop to finish one there is nothing more to read.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert saschagrunert changed the title WIP: Remove token from ContainerIO read_loop Remove token from ContainerIO read_loop Sep 21, 2022
@saschagrunert
Copy link
Member Author

@rphillips @haircommander PTAL

@haircommander
Copy link
Collaborator

hm I would have expected this routine to leak, but the stress memory test isn't failing

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 21, 2022
@saschagrunert saschagrunert merged commit fe5ceea into containers:main Sep 21, 2022
@saschagrunert saschagrunert deleted the read-loop-token branch September 21, 2022 15:01
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.

Container logs related tests fail
3 participants