Skip to content

Revert "Preventing containers from being unable to be deleted"#5153

Merged
cyphar merged 1 commit intoopencontainers:mainfrom
kolyshkin:rev-4757
Mar 6, 2026
Merged

Revert "Preventing containers from being unable to be deleted"#5153
cyphar merged 1 commit intoopencontainers:mainfrom
kolyshkin:rev-4757

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Mar 6, 2026

This fixes random failures to start a container in conmon integration tests (see #5151).

Fixes: #5151.

I guess we need to find another way to fix #4645.

This reverts commit 1b39997 / PR #4757.

This fixes random failures to start a container in conmon integration
tests (see issue 5151).

I guess we need to find another way to fix issue 4645.

This reverts commit 1b39997.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin requested a review from rata March 6, 2026 02:47
@kolyshkin kolyshkin added the backport/1.3-todo A PR in main branch which needs to be backported to release-1.3 label Mar 6, 2026
@kolyshkin kolyshkin added this to the 1.4.1 milestone Mar 6, 2026
@kolyshkin kolyshkin added the backport/1.4-todo A PR in main branch which needs to backported to release-1.4 label Mar 6, 2026
@kolyshkin
Copy link
Copy Markdown
Contributor Author

A quick (under a minute) repro, from runc top source dir:

make
export RUNTIME_BINARY=$(pwd)/runc
$RUNTIME_BINARY --version
git clone https://github.com/containers/conmon && (cd conmon && make)
./conmon/test/run-tests.sh -j $(nproc)

(comment out git clone line and this can be used for git bisect run).

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Or maybe conmon tests or conmon itself is doing something nasty; haven't figured it out yet. In any case it's a regression.

Copy link
Copy Markdown
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm not even sure I was fully on-board with this even back when we merged it.

Copy link
Copy Markdown
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for investigating and finding a fix!

Is it always the same test failing? In that case, maybe we can adapt it and add it to runc?

@cyphar cyphar merged commit 1c35df9 into opencontainers:main Mar 6, 2026
42 checks passed
@rata
Copy link
Copy Markdown
Member

rata commented Mar 6, 2026

@HirazawaUi you worked on the commit we are now reverting due to a regression. Wanna take a look on a way to fix it that doesn't regress? The repro for the regression is here: #5153 (comment)

@kolyshkin
Copy link
Copy Markdown
Contributor Author

LGTM, thanks for investigating and finding a fix!

Is it always the same test failing? In that case, maybe we can adapt it and add it to runc?

The failures are random, I haven't got to the bottom of it, but even if it is something not entirely kosher that conmon (or conmon tests) do (which results in a race), it worked before and it works with crun so this is a regression.

As for adding runc tests, I was just thinking we can run existing conmon tests. The downside it when conmon tests will break, our CI will break, too, but I guess we can live with that (same as we're living with occasional CRIU failures).

Similarly, we can add podman tests (crun does it already), but those are more heavy weight.

@kolyshkin kolyshkin added backport/1.3-done A PR in main branch which has been backported to release-1.3 backport/1.4-done A PR in main branch which has been backported to release-1.4 and removed backport/1.3-todo A PR in main branch which needs to be backported to release-1.3 backport/1.4-todo A PR in main branch which needs to backported to release-1.4 labels Mar 10, 2026
@kolyshkin
Copy link
Copy Markdown
Contributor Author

LGTM, thanks for investigating and finding a fix!
Is it always the same test failing? In that case, maybe we can adapt it and add it to runc?

The failures are random, I haven't got to the bottom of it, but even if it is something not entirely kosher that conmon (or conmon tests) do (which results in a race), it worked before and it works with crun so this is a regression.

As for adding runc tests, I was just thinking we can run existing conmon tests. The downside it when conmon tests will break, our CI will break, too, but I guess we can live with that (same as we're living with occasional CRIU failures).

Opened #5159, PTAL @rata

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

Labels

backport/1.3-done A PR in main branch which has been backported to release-1.3 backport/1.4-done A PR in main branch which has been backported to release-1.4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"cannot start a container that has stopped" in F-43

3 participants