Skip to content

server: run post stop hooks before updating container status #9422

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
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

server: run post stop hooks before updating container status #9422

wants to merge 1 commit into from

Conversation

haircommander
Copy link
Member

/kind bug

What type of PR is this?

What this PR does / why we need it:

ContainerStateToDisk will call UpdateContainerStatus which will update the internal representation of the container state to be stopped. While correct, it opens a race where the kubelet may take action on a container that seems stopped before post stop hooks are run.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a bug where CRI-O reports a container is stopped before post stop hooks are run

@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. labels Aug 25, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 25, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 25, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 25, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2025
@haircommander
Copy link
Member Author

cc @andreaskaris @MarSik

@haircommander haircommander force-pushed the post-stop-sync branch 3 times, most recently from 16f489f to a34023d Compare August 25, 2025 17:45
@haircommander
server: run post stop hooks before updating container status
5223c2d 
ContainerStateToDisk will call UpdateContainerStatus which will
update the internal representation of the container state to be
stopped. While correct, it opens a race where the kubelet may take action
on a container that seems stopped before post stop hooks are run.

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@codecov Codecov
Copy link

codecov bot commented Aug 25, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.54%. Comparing base (9f047e1) to head (5223c2d).
⚠️ Report is 56 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (9f047e1) and HEAD (5223c2d). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (9f047e1) HEAD (5223c2d)
18 7
Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #9422       +/-   ##
===========================================
- Coverage   67.13%   39.54%   -27.60%     
===========================================
  Files         202      191       -11     
  Lines       27939    27756      -183     
===========================================
- Hits        18756    10975     -7781     
- Misses       7610    15290     +7680     
+ Partials     1573     1491       -82
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions GitHub Actions
Copy link

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 25, 2025
@andreaskaris
Copy link
Contributor

@haircommander #9405 merged now, so I guess it's time to revisit this one, here :-)

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

andreaskaris commented Oct 2, 2025 •
edited
Loading

So I applied this on top of OCPBUGS-59416 (#9351):
https://github.com/andreaskaris/cri-o/tree/OCPBUGS-59416-pr9422

It works fine, but I still have no way of testing the original customer issue, as PR9351 by itself fixes my own test case, too (meaning that my reproducer doesn't catch 100% of possible issues)

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

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp will be requested when the pull request is marked ready for review mrunalp is a code owner

Assignees
No one assigned
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. 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.
3 participants
@haircommander @andreaskaris @openshift-merge-robot