[release/1.7] Handle teardown failure to avoid blocking cleanup by matteopulcini6 · Pull Request #10770 · containerd/containerd

matteopulcini6 · 2024-10-04T15:10:08Z

MIke: The scenario here is CNI is not configured / installed .. there are no plugins.. cni setup fails.. returns error and CNIResult is nil... While processing the defer chain to clean up the partially created pod.. cni tear down will also fail because.. Subsequently netns is not destroyed and then because netns is not destroyed the pod is created in the not cleaned up state. The user must manually attempt stop at this point. In the case where cni is not configured, we should not require it to be configured to clean up.

k8s-ci-robot · 2024-10-04T15:10:19Z

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

Details

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.

mikebrow

looking good need to also put this new block down on line 451 in the user ns networking block

mikebrow · 2024-10-04T16:22:05Z

/ok-to-test

mikebrow

LGTM on green

matteopulcini6 · 2024-10-04T16:58:13Z

/retest

mikebrow · 2024-10-04T17:01:28Z

failures are not related to PR.. looks like the container registry is blocking.. will try again later.

sameersaeed · 2024-10-04T17:58:07Z

Sample flow

pod-config.json:, per crictl docs

{
  "metadata": {
    "name": "nginx-sandbox",
    "namespace": "default",
    "attempt": 1,
    "uid": "65fb5845de97d8a4"
  },
  "log_directory": "/tmp",
  "linux": {
  }
}

Current behaviour from running sandbox pod - receive CNI plugin error, but pod still comes up:

root@32748c76b2ba:/src# crictl pods
POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME

root@32748c76b2ba:/src# crictl runp pod-config.json
E1004 17:42:14.071632   31488 remote_runtime.go:176] "RunPodSandbox from runtime service failed" err="rpc error: code = Unknown desc = failed to setup network for sandbox \"803615d3f284991954f660a4d85295c15839ba8fa574640d58dd02de6a248d80\": cni plugin not initialized"
FATA[0000] run pod sandbox: rpc error: code = Unknown desc = failed to setup network for sandbox "803615d3f284991954f660a4d85295c15839ba8fa574640d58dd02de6a248d80": cni plugin not initialized

root@32748c76b2ba:/src# crictl pods
POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME
803615d3f2849       1 second ago        NotReady            nginx-sandbox       default             1                   (default)

Changes made

Current code (release 1.7):

containerd/pkg/cri/server/sandbox_run.go

Lines 323 to 326 in 45967a7

    
           // Teardown network if an error is returned. 
        
           if cleanupErr = c.teardownPodNetwork(deferCtx, sandbox); cleanupErr != nil { 
        
           	log.G(ctx).WithError(cleanupErr).Errorf("Failed to destroy network for sandbox %q", id) 
        
           }

containerd/pkg/cri/server/sandbox_run.go

Lines 444 to 447 in 45967a7

    
           // Teardown network if an error is returned. 
        
           if cleanupErr = c.teardownPodNetwork(deferCtx, sandbox); cleanupErr != nil { 
        
           	log.G(ctx).WithError(cleanupErr).Errorf("Failed to destroy network for sandbox %q", id) 
        
           }

Changes were added to the above sandbox_run.go code to skip the pod network teardown if no CNI plugins are initialized. This allows for the rest of the deferred cleanup steps to run so that the CNI plugins failure does not block the pod from being fully cleaned up as expected:

// Teardown network if an error is returned.
if cleanupErr = c.teardownPodNetwork(deferCtx, sandbox); cleanupErr != nil {
	log.G(ctx).WithError(cleanupErr).Errorf("Failed to destroy network for sandbox %q", id)

	// ignoring failed to destroy networks when we failed to setup networks
	if sandbox.CNIResult == nil {
		cleanupErr = nil
	}
}

// Teardown network if an error is returned.
if cleanupErr = c.teardownPodNetwork(deferCtx, sandbox); cleanupErr != nil {
	log.G(ctx).WithError(cleanupErr).Errorf("Failed to destroy network for sandbox %q", id)

	// ignoring failed to destroy networks when we failed to setup networks
	if sandbox.CNIResult == nil {
		cleanupErr = nil
	}
}

This makes it so that a pod cannot be started if no CNI plugins are initialized:

root@32748c76b2ba:/src# crictl pods
POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME

root@32748c76b2ba:/src# crictl runp pod-config.json
E1004 17:40:26.373297   29655 remote_runtime.go:176] "RunPodSandbox from runtime service failed" err="rpc error: code = Unknown desc = failed to setup network for sandbox \"bd9a6545dc2f9f1484b36a3b72b2c87d2e44b9f74fe5ffc90f53fa220d73462b\": cni plugin not initialized"
FATA[0000] run pod sandbox: rpc error: code = Unknown desc = failed to setup network for sandbox "bd9a6545dc2f9f1484b36a3b72b2c87d2e44b9f74fe5ffc90f53fa220d73462b": cni plugin not initialized

root@32748c76b2ba:/src# crictl pods
POD ID              CREATED             STATE               NAME                NAMESPACE           ATTEMPT             RUNTIME

fuweid

would you please squash the commits? Thanks

matteopulcini6 · 2024-10-07T23:13:10Z

@fuweid commits are squashed

dmcgowan · 2024-10-08T00:51:21Z

This had the 1.7 cherry pick label, is this change already in main?

Squashed

matteopulcini6 · 2024-10-08T15:11:02Z

@mikebrow @dmcgowan I didn't see the change in main. Also submitted this for release/1.6 [Release/1.6 Handle teardown failure to avoid blocking cleanup #10778

Please note: I only saw one teardownPodNetwork for 1.6 hence why the the cleanupErr is set to nil only once.

dmcgowan

This will deviate from main, a change needs to get merged there first

matteopulcini6 · 2024-10-08T19:09:51Z

@dmcgowan Added the PR here #10800, but fyi it looks like there is one teardown call in release/1.6, but there are two for release/1.7

mikebrow · 2024-10-15T14:02:40Z

This will deviate from main, a change needs to get merged there first

Nod.. main first.. overlooked release in my review when I tagged it cherry pick.

kzys · 2024-10-18T23:52:43Z

I've merged #10778 and realized this discussion 🤦

So, since #10839 has been merged into main. Are we good to go? The commit message could be cleaned up though.

mikebrow

LGTM with the commit header cleanup

mikebrow · 2024-10-22T13:42:48Z

@matteopulcini6 pls do a commit --amend and remove the extra commit header signatures and only need one 69 char description line for it .. Cheers
from:

Signed-off-by: Matteo.Pulcini@ibm.com

Fixed Linting: gofmt

Signed-off-by: Matteo.Pulcini@ibm.com

Handle teardown failure in user ns networking block

Signed-off-by:  Matteo.Pulcini@ibm.com
Signed-off-by: Matteo Pulcini <Matteo.Pulcini@ibm.com>

to

Handle teardown failure in user ns networking block

Signed-off-by: Matteo Pulcini <Matteo.Pulcini@ibm.com>

Signed-off-by: Matteo Pulcini <Matteo.Pulcini@ibm.com>

matteopulcini6 · 2024-10-22T15:06:25Z

/retest

akhilerm · 2025-01-27T07:17:40Z

Ping @mikebrow @dmcgowan . Are we good with the changes here? The cherry-pick already got merged in 1.6 branch, but this PR is still open to 1.7. Can we get this in?

aojea · 2025-07-18T18:53:35Z

@mikebrow @dmcgowan please hold on this until we can investigate a bit more, we have an internal issue that is causing CNI plugin to leak IPs and this may be related,

cc: @zihongz

MikeZappa87 · 2025-07-18T19:00:12Z

@mikebrow @dmcgowan please hold on this until we can investigate a bit more, we have an internal issue that is causing CNI plugin to leak IPs and this may be related,

cc: @zihongz

It looks like the scenario for this is where the CNI was never setup in the first place however RunPodSandbox was still called. I am guessing this is coming from using crictl runp directly? @mikebrow

@aojea this might be different?

aojea · 2025-07-19T09:10:22Z

@zihongz found the problem, it turned out to be #10744.

This PR skips teardownPodNetwork() when sandbox.CNIResult is nil. And sandbox.CNIResultwill only be written if the CNI operation succeeds .

If the CNIs plugins timeouts the cleanup process does not happen

k8s-ci-robot added the needs-ok-to-test label Oct 4, 2024

k8s-ci-robot added the size/XS label Oct 4, 2024

dosubot bot added the area/cri Container Runtime Interface (CRI) label Oct 4, 2024

matteopulcini6 force-pushed the sandbox-deferring-teardown branch from 7e5a8b8 to 50dc942 Compare October 4, 2024 15:20

mikebrow reviewed Oct 4, 2024

View reviewed changes

k8s-ci-robot added ok-to-test size/S and removed needs-ok-to-test size/XS labels Oct 4, 2024

mikebrow approved these changes Oct 4, 2024

View reviewed changes

mikebrow assigned fuweid and MikeZappa87 Oct 4, 2024

mikebrow added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Oct 4, 2024

sameersaeed mentioned this pull request Oct 4, 2024

Add regression tests for sandbox pods when no CNI plugins are initialized #10771

Open

matteopulcini6 mentioned this pull request Oct 5, 2024

[release/1.6] Handle teardown failure to avoid blocking cleanup #10778

Merged

matteopulcini6 changed the title ~~Handle teardown failure to avoid blocking cleanup~~ [Release/1.7] Handle teardown failure to avoid blocking cleanup Oct 5, 2024

fuweid previously requested changes Oct 7, 2024

View reviewed changes

matteopulcini6 force-pushed the sandbox-deferring-teardown branch from d277591 to 9fe6976 Compare October 7, 2024 22:48

matteopulcini6 requested a review from fuweid October 7, 2024 23:41

dmcgowan approved these changes Oct 8, 2024

View reviewed changes

dmcgowan added impact/changelog and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Oct 8, 2024

dmcgowan requested changes Oct 8, 2024

View reviewed changes

matteopulcini6 requested a review from dmcgowan October 11, 2024 19:01

estesp mentioned this pull request Oct 16, 2024

Handle teardown failure to avoid blocking cleanup #10839

Merged

kzys changed the title ~~[Release/1.7] Handle teardown failure to avoid blocking cleanup~~ [release/1.7] Handle teardown failure to avoid blocking cleanup Oct 18, 2024

mikebrow approved these changes Oct 22, 2024

View reviewed changes

Handle teardown failure in user ns networking block

59d7eed

Signed-off-by: Matteo Pulcini <Matteo.Pulcini@ibm.com>

matteopulcini6 force-pushed the sandbox-deferring-teardown branch from 9fe6976 to 59d7eed Compare October 22, 2024 14:14

dmcgowan removed the cherry-pick/1.6.x label Nov 6, 2025

Conversation

matteopulcini6 commented Oct 4, 2024 • edited by mikebrow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 4, 2024

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Oct 4, 2024

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matteopulcini6 commented Oct 4, 2024

Uh oh!

mikebrow commented Oct 4, 2024

Uh oh!

sameersaeed commented Oct 4, 2024

Sample flow

Changes made

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

matteopulcini6 commented Oct 7, 2024

Uh oh!

dmcgowan commented Oct 8, 2024

Uh oh!

matteopulcini6 commented Oct 8, 2024

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

matteopulcini6 commented Oct 8, 2024

Uh oh!

mikebrow commented Oct 15, 2024

Uh oh!

kzys commented Oct 18, 2024

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matteopulcini6 commented Oct 22, 2024

Uh oh!

akhilerm commented Jan 27, 2025

Uh oh!

aojea commented Jul 18, 2025

Uh oh!

MikeZappa87 commented Jul 18, 2025

Uh oh!

aojea commented Jul 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

matteopulcini6 commented Oct 4, 2024 •

edited by mikebrow

Loading

mikebrow left a comment •

edited

Loading

mikebrow left a comment •

edited

Loading

mikebrow commented Oct 22, 2024 •

edited

Loading