Hi @HirazawaUi. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

FAIL - does not have a valid DCO

Please sign-off your commit.

Please sign-off your commit.

Signed.

/ok-to-test

why not add check in handleSandboxTaskExit? why sb.Container = nil ? @HirazawaUi

why not add check in handleSandboxTaskExit? why sb.Container = nil ? @HirazawaUi

ref: #11588 (comment)

I find will call handleSandboxTaskExit twice, I don't know why. It should be anoter bug.

I find will call handleSandboxTaskExit twice, I don't know why. It should be anoter bug.

In the logs I’ve observed, every panic seems to stem from a call to HandleEvent -> handleSandboxTaskExit, where a panic occurs. Additionally, in all other callers of handleSandboxTaskExit, the Container is never nil. I think handling nil in either HandleEvent or handleSandboxTaskExit would work fine, and as noted in the comment #11588 (comment), it’s really just a matter of code preference.

If you think there’s anything incorrect in my response, I’d greatly appreciate it if you could point it out. That said, I’m a bit puzzled as to why you opened another PR to address this issue, especially when there doesn’t seem to be a clear difference between the two PRs.

I close my pr,it should be a nother bug

Will this cover up other issues? In fact, this paragraph was moved from https://github.com/containerd/containerd/blob/v1.7.27/pkg/cri/server/sandbox_run.go#L196C1-L205C5

	defer func() {
		// Delete container only if all the resource cleanup is done.
		if retErr != nil && cleanupErr == nil {
			deferCtx, deferCancel := util.DeferContext()
			defer deferCancel()
			if cleanupErr = container.Delete(deferCtx, containerd.WithSnapshotCleanup); cleanupErr != nil {
				log.G(ctx).WithError(cleanupErr).Errorf("Failed to delete containerd container %q", id)
			}
+      podSandbox.Container = nil
		}
	}()

But after the move, a nil was added. I don't know why this sentence was added here, but it looks different from the original processing.

If we make changes here, may similar problems actually occur in other unknown places?

@estesp It seems some test job failed due to some unrelated reasons, preventing this PR from being successfully merged. Is there anything I can do at this point to help? Or should I wait for other maintainers to re-add it to merge queue?

Why hasn't this been backported to the 2.0 branch yet?

Why hasn't this been backported to the 2.0 branch yet?

Thank you for the reminder, I’ll cherry-pick this to the 2.0 branch.

@brandond PR has been opened.

ref: #12047

Thanks. How about getting #11576 / b9ab7a3 merged and backported too? We've had this pulled into our fork for months, containerd is basically unusable without it for a large subset of our users.

It looks like 2.0 hasn't seen a patch release tagged since 2.1 came out, is this intentional? @austinvazquez @estesp

It looks like 2.0 hasn't seen a patch release tagged since 2.1 came out, is this intentional?

Definitely not intentional. We attempt to properly mark PRs for cherry-pick that are bug fixes, but we are definitely imperfect. I think having two 2.x lines of support has made things a bit more challenging lately, but hopefully we can make sure to get 2.0.x in better shape and cut a release soon.

We tend to be driven by customer demand on minor/point releases, so if there is ever a time you feel like a release is warranted please open an issue or ping us on #containerd-dev Slack channel.