Skip to content

[release 1.26] internal/oci: remove redundant ShouldBeStopped check for stopping containers #8396

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
Sep 18, 2024
Merged

[release 1.26] internal/oci: remove redundant ShouldBeStopped check for stopping containers #8396

merged 4 commits into from
Sep 18, 2024

Conversation

@sohankunkerkar
Copy link
Member

@sohankunkerkar sohankunkerkar commented Jul 17, 2024

This is a manual cherry-pick of #8300

/assign sohankunkerkar

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

@sohankunkerkar
oci: remove redundant ShouldBeStopped check for stopping containers
80274e6 
Fixes cri-o#8030
This commit removes the ShouldBeStopped function to address the
issue where stopping a 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.

Signed-off-by: Sohan Kunkerkar <[email protected]>
@sohankunkerkar
test: add test coverage for multiple stop calls
d111cb0 
Signed-off-by: Sohan Kunkerkar <[email protected]>
@sohankunkerkar
oci: move oci-specific code into their respective runtime
eb2bfce 
Signed-off-by: Sohan Kunkerkar <[email protected]>
@sohankunkerkar
oci: separate out timer logic from the backoff manager
3f8b44d 
Signed-off-by: Sohan Kunkerkar <[email protected]>
@openshift-ci openshift-ci bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 17, 2024
@openshift-ci openshift-ci bot requested review from QiWang19 and wgahnagl July 17, 2024 20:18
@openshift-ci openshift-ci bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 17, 2024
@sohankunkerkar
Copy link
Member Author

sohankunkerkar commented Jul 25, 2024

@cri-o/cri-o-maintainers PTAL

@kwilczynski
Copy link
Contributor

kwilczynski commented Jul 26, 2024

/approve
/lgtm

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

kwilczynski commented Aug 2, 2024

/retest

@github-actions
Copy link

github-actions bot commented Sep 2, 2024

A friendly reminder that this PR had no activity for 30 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 2, 2024
@kwilczynski kwilczynski removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 2, 2024
@kwilczynski
Copy link
Contributor

kwilczynski commented Sep 2, 2024

@cri-o/cri-o-maintainers, please have a look. Thank you!

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2024
@kwilczynski
Copy link
Contributor

kwilczynski commented Sep 8, 2024

/unhold

@kwilczynski
Copy link
Contributor

kwilczynski commented Sep 9, 2024

@saschagrunert, please have a look! Thank you!

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwilczynski, 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:

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

@kwilczynski
Copy link
Contributor

kwilczynski commented Sep 18, 2024

@cri-o/cri-o-maintainers, please have a look. Thank you!

@haircommander
Copy link
Member

haircommander commented Sep 18, 2024

/override ci/prow/e2e-gcp
/override ci/prow/e2e-agnostic
/override ci/prow/images

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 18, 2024

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-agnostic, ci/prow/e2e-gcp, ci/prow/images

In response to this:

/override ci/prow/e2e-gcp
/override ci/prow/e2e-agnostic
/override ci/prow/images

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 efe9233 into cri-o:release-1.26 Sep 18, 2024
@rphillips
Copy link
Contributor

rphillips commented Jun 6, 2025

/cherry-pick release-1.25

@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jun 6, 2025

@rphillips: #8396 failed to apply on top of branch "release-1.25":

Applying: oci: remove redundant ShouldBeStopped check for stopping containers
Applying: test: add test coverage for multiple stop calls
Applying: oci: move oci-specific code into their respective runtime
Using index info to reconstruct a base tree...
M	internal/oci/runtime_oci.go
M	internal/oci/runtime_vm.go
M	server/container_stop.go
Falling back to patching base and 3-way merge...
Auto-merging server/container_stop.go
CONFLICT (content): Merge conflict in server/container_stop.go
Auto-merging internal/oci/runtime_vm.go
Auto-merging internal/oci/runtime_oci.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 oci: move oci-specific code into their respective runtime

In response to this:

/cherry-pick release-1.25

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

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

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

@QiWang19 QiWang19 Awaiting requested review from QiWang19

@wgahnagl wgahnagl Awaiting requested review from wgahnagl

Assignees

@kwilczynski kwilczynski

@saschagrunert saschagrunert

@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. 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.

6 participants

@sohankunkerkar @kwilczynski @haircommander @rphillips @openshift-cherrypick-robot @saschagrunert