Skip to content

OCPNODE-2408: internal/oci: remove redundant ShouldBeStopped check for stopping containers #8300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 17, 2024
Merged

OCPNODE-2408: internal/oci: remove redundant ShouldBeStopped check for stopping containers #8300

merged 4 commits into from
Jul 17, 2024

Conversation

@sohankunkerkar
Copy link
Member

@sohankunkerkar sohankunkerkar commented Jun 18, 2024 •
edited by kwilczynski
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

This commit removes the ShouldBeStopped function to address the issue where stopping container blocks all further stop attempts for the same container. That function acquired the opLock mutex to check the container's state, which could lead to contention and potential deadlocks if other goroutines were waiting to acquire the same lock.

Additionally, the container's state is checked later in the StopContainer function by calling the Living method, which checks if the container's process is still running. Therefore, the ShouldBeStopped function is redundant and can be removed.

Which issue(s) this PR fixes:

Fixes #8030.

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

Fixed a bug where stopping a container would block all further stop attempts for the same container.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Jun 18, 2024
@openshift-ci openshift-ci bot requested review from klihub and kwilczynski June 18, 2024 21:43
@sohankunkerkar sohankunkerkar force-pushed the stop-container branch 2 times, most recently from 9583567 to e7feb48 Compare June 18, 2024 21:49
@codecov
Copy link

codecov bot commented Jun 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 41.66667% with 21 lines in your changes missing coverage. Please review.

Project coverage is 49.49%. Comparing base (503d183) to head (5f66ac0).
Report is 12 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8300      +/-   ##
==========================================
- Coverage   49.52%   49.49%   -0.03%     
==========================================
  Files         153      153              
  Lines       17085    17091       +6     
==========================================
- Hits         8462     8460       -2     
- Misses       7559     7567       +8     
  Partials     1064     1064

@sohankunkerkar sohankunkerkar marked this pull request as draft June 20, 2024 14:34
@openshift-ci openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 20, 2024
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jun 20, 2024
@sohankunkerkar sohankunkerkar force-pushed the stop-container branch 2 times, most recently from 968a36c to 5bebf14 Compare June 21, 2024 17:23
@openshift-ci openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 21, 2024
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Jun 21, 2024
@sohankunkerkar sohankunkerkar force-pushed the stop-container branch 6 times, most recently from 3f90ccb to 407edb6 Compare June 24, 2024 16:25
@haircommander
Copy link
Member

haircommander commented Jul 16, 2024

/retest

1 similar comment
@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 16, 2024

/retest

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 16, 2024

/test ci-fedora-critest

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

The test failure is not related to this PR.

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

@cri-o/cri-o-maintainers PTAL

@haircommander
Copy link
Member

haircommander commented Jul 17, 2024

/override ci/prow/ci-fedora-critest
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2024

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-fedora-critest

In response to this:

/override ci/prow/ci-fedora-critest
/lgtm

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-sigs/prow repository.

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/retest

4 similar comments
@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/retest

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/retest

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/retest

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/retest

@haircommander
Copy link
Member

haircommander commented Jul 17, 2024

/override ci/prow/ci-cgroupv2-integration
/override ci/prow/ci-fedora-kata

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2024

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-cgroupv2-integration, ci/prow/ci-fedora-kata

In response to this:

/override ci/prow/ci-cgroupv2-integration
/override ci/prow/ci-fedora-kata

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-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 7347ea6 into cri-o:main Jul 17, 2024
@sohankunkerkar sohankunkerkar deleted the stop-container branch July 17, 2024 19:28
@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/cherry-pick release-1.30

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jul 17, 2024

@sohankunkerkar: #8300 failed to apply on top of branch "release-1.30":

Applying: oci: remove redundant ShouldBeStopped check for stopping containers
Using index info to reconstruct a base tree...
M	internal/oci/container.go
M	internal/oci/container_test.go
M	internal/oci/runtime_oci.go
M	internal/oci/runtime_oci_test.go
M	internal/oci/runtime_vm.go
Falling back to patching base and 3-way merge...
Auto-merging internal/oci/runtime_vm.go
Auto-merging internal/oci/runtime_oci_test.go
Auto-merging internal/oci/runtime_oci.go
Auto-merging internal/oci/container_test.go
Auto-merging internal/oci/container.go
CONFLICT (content): Merge conflict in internal/oci/container.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 oci: remove redundant ShouldBeStopped check for stopping containers
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.30

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-sigs/prow repository.

@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 17, 2024

/cherry-pick release-1.29

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jul 17, 2024

@sohankunkerkar: #8300 failed to apply on top of branch "release-1.29":

Applying: oci: remove redundant ShouldBeStopped check for stopping containers
Using index info to reconstruct a base tree...
M	internal/oci/container.go
M	internal/oci/container_test.go
M	internal/oci/runtime_oci.go
M	internal/oci/runtime_oci_test.go
M	internal/oci/runtime_vm.go
Falling back to patching base and 3-way merge...
Auto-merging internal/oci/runtime_vm.go
Auto-merging internal/oci/runtime_oci_test.go
CONFLICT (content): Merge conflict in internal/oci/runtime_oci_test.go
Auto-merging internal/oci/runtime_oci.go
Auto-merging internal/oci/container_test.go
CONFLICT (content): Merge conflict in internal/oci/container_test.go
Auto-merging internal/oci/container.go
CONFLICT (content): Merge conflict in internal/oci/container.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 oci: remove redundant ShouldBeStopped check for stopping containers
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.29

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-sigs/prow repository.

This was referenced Jul 17, 2024
Merged
Merged
Merged
Merged
Merged
@github-actions github-actions bot mentioned this pull request Sep 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haircommander haircommander haircommander left review comments

@fidencio fidencio Awaiting requested review from fidencio fidencio is a code owner

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@klihub klihub Awaiting requested review from klihub

@wgahnagl wgahnagl Awaiting requested review from wgahnagl

@QiWang19 QiWang19 Awaiting requested review from QiWang19

@littlejawa littlejawa Awaiting requested review from littlejawa

+1 more reviewer

@kwilczynski kwilczynski kwilczynski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kwilczynski kwilczynski

@haircommander haircommander

@sohankunkerkar sohankunkerkar

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Stopping a container blocks all further stop attempts for the same container

6 participants

@sohankunkerkar @haircommander @kwilczynski @openshift-ci-robot @openshift-cherrypick-robot @openshift-merge-robot