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

Is there some operation that needs to be done in a transaction or something? Ideally it wouldn't be possible to introduce an inconsistency between stores that later needs to be cleaned up, just by stopping containerd at the wrong time.

Feels like all these error handling PRs are just to clean up after a problem that shouldn't be occurring in the first place?

Is there some operation that needs to be done in a transaction or something? Ideally it wouldn't be possible to introduce an inconsistency between stores that later needs to be cleaned up, just by stopping containerd at the wrong time.

Feels like all these error handling PRs are just to clean up after a problem that shouldn't be occurring in the first place?

we don't have transaction support across two stores.. wip to ensure we are only modifying one store for both the object and object's meta ... if memory serves there was a split to support new sandbox types..

additionally there was a bug in network CNI tear down of a pod that was causing failure to remove the partially created pod in defer.. was also broken in stop pod when the network was never successfully setup in the first place .. the issue was no cni at all or just an error in running the cni setup/teardown would cause that problem.. this before we had the optional support for doing the minimum of loopback internally....

a number of PRs are all trying to fix the same problem

Unrelated, but I feel like the errors in this section are switched:

Unless I'm misunderstanding, the first operation should log an unable to update extensions error, and the second should log unable to save sandbox %q to store. Add the extensions does not save the store, that's what the second function is doing - as indicated by the comment on the line above it.

AddExtension is just a helper that sets sandboxInfo.Extensions[podsandbox.MetadataKey] = typeurl.MarshalAny(&sandbox.Metadata), it doesn't actually save anything as far as I can tell?

@mikebrow
additionally there was a bug in network CNI tear down of a pod that was causing failure to remove the partially created pod in defer.. was also broken in stop pod when the network was never successfully setup in the first place .. the issue was no cni at all or just an error in running the cni setup/teardown would cause that problem..

Do you have a link to the issue or PR for this one? Is it fixed in containerd 2.0.4?

#10744 yes fixed in all releases see linked PRs..

Unrelated, but I feel like the errors in this section are switched:

containerd/internal/cri/server/sandbox_run.go

Lines 221 to 227 in d20482f

	if err := sandboxInfo.AddExtension(podsandbox.MetadataKey, &sandbox.Metadata); err != nil {
	return nil, fmt.Errorf("unable to save sandbox %q to store: %w", id, err)
	}
	// Save sandbox metadata to store
	if sandboxInfo, err = c.client.SandboxStore().Update(ctx, sandboxInfo, "extensions"); err != nil {
	return nil, fmt.Errorf("unable to update extensions for sandbox %q: %w", id, err)
	}

Unless I'm misunderstanding, the first operation should log an unable to update extensions error, and the second should log unable to save sandbox %q to store. Add the extensions does not save the store, that's what the second function is doing - as indicated by the comment on the line above it.

AddExtension is just a helper that sets sandboxInfo.Extensions[podsandbox.MetadataKey] = typeurl.MarshalAny(&sandbox.Metadata), it doesn't actually save anything as far as I can tell?

original.. https://github.com/containerd/containerd/blob/release/1.5/pkg/cri/server/sandbox_run.go#L372-L388

for real what is going on here. This needs to be merged and backported to 2.0 asap. We have been shipping b9ab7a3 with k3s and rke2 for several months.

/ok-to-test

closing and reopening to attempt to reset the test results that have a bad link / can't be restarted...

/cherry-pick release/2.1

/cherry-pick release/2.0

@estesp: new pull request created: #12076

@estesp: new pull request created: #12077

can you review my pr 11967? @mikebrow