Update container with sandbox metadata after NetNS is created by qiutongs · Pull Request #7481 · containerd/containerd

qiutongs · 2022-10-05T14:51:58Z

This is to fix a bug introduced in #5904.

When the network namespace is created, we need to update both container spec and sandbox metadata so that both information can persist on disk. If sandbox metadata is missing, the NetNS is considered as closed when loading sandbox during restart recovery because the NetNS.path is empty. Closed NetNS will affect network teardown in sandbox stop.

Manual Testing

Add a sleep in setupPodNetwork, e.g. 60s
sudo crictl runp sandbox.json
Restart containerd
sudo crictl pods shows the

Before fix: sudo crictl inspectp <POD ID> shows "netNamespaceClosed": true

After fix: sudo crictl inspectp <POD ID> shows "netNamespaceClosed": false

New Integration Test

ENABLE_CRI_SANDBOXES="" CONTAINERD_RUNTIME=runc FOCUS=TestRunPodSandboxWithShimStartAndTeardownCNISlow make cri-integration

qiutongs · 2022-10-05T14:52:24Z

/cc @fuweid @samuelkarp @dmcgowan

k8s-ci-robot · 2022-10-05T15:04:12Z

Hi @qiutongs. 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/test-infra repository.

samuelkarp · 2022-10-06T01:30:33Z

/ok-to-test

samuelkarp · 2022-10-06T01:44:09Z

updateContainerSpec (in pkg/cri/server/container_update_resources_other.go) is now no longer called and is causing linter errors. That will need to be removed.

fuweid · 2022-10-08T04:37:12Z

+
+	time.Sleep(5 * time.Second)
+	t.Log("Restart containerd after 5s")
+	RestartContainerd(t)


synced with @qiutongs last night. We need to send sigkill to prevent containerd from graceful shutdown. The graceful shutdown will cancel the context to the inflight rpc call, which might invoke the defer to cleanup the resource.

qiutongs · 2022-10-09T00:28:41Z

[It] runtime should support portforward in host network

Timed out after 60.000s.
  Expected
      <*url.Error | 0xc0007b4ab0>: {
          Op: "Get",
          URL: "http://127.0.0.1:12002/",
          Err: <*net.OpError | 0xc0007c2190>{
              Op: "dial",
              Net: "tcp",
              Source: nil,
              Addr: <*net.TCPAddr | 0xc0007b4a80>{
                  IP: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 127, 0, 0, 1],
                  Port: 12002,
                  Zone: "",
              },
              Err: <*os.SyscallError | 0xc00014b4a0>{
                  Syscall: "connect",
                  Err: <syscall.Errno>0x6f,
              },
          },
      }
  to be nil
  In [It] at: github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

qiutongs · 2022-10-09T00:39:23Z

[It] runtime should support portforward in host network

Timed out after 60.000s.
  Expected
      <*url.Error | 0xc0007b4ab0>: {
          Op: "Get",
          URL: "http://127.0.0.1:12002/",
          Err: <*net.OpError | 0xc0007c2190>{
              Op: "dial",
              Net: "tcp",
              Source: nil,
              Addr: <*net.TCPAddr | 0xc0007b4a80>{
                  IP: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 127, 0, 0, 1],
                  Port: 12002,
                  Zone: "",
              },
              Err: <*os.SyscallError | 0xc00014b4a0>{
                  Syscall: "connect",
                  Err: <syscall.Errno>0x6f,
              },
          },
      }
  to be nil
  In [It] at: github.com/kubernetes-sigs/cri-tools/pkg/validate/networking.go:253

Signed-off-by: Qiutong Song <[email protected]>

qiutongs · 2022-10-09T11:11:27Z

/retest

fuweid

LGTM

estesp

LGTM

samuelkarp

Apologies for the late review, but LGTM. One small nit remains, but that can be addressed separately.

samuelkarp · 2022-10-11T00:49:03Z

+  # The command may return non-zero and we want to continue this script.
+  # e.g. containerd receives SIGKILL
+  set +e


Bash options (which are set with set) do not have a function scope, so this set +e affects execution for all commands afterward. There's no set -e (or set -o errexit) in the script, so we're not changing behavior, but it'd be better to move this to the top of the script so it's clear that it applies to everything.

Here's a little script demonstrating:

#!/bin/bash # This should exit non-zero, but not cause the script to exit. false echo "false: $?" # Abort the script when the first error occurs set -e echo "set -e" # false # This would cause the script to abort myfunc() { echo "myfunc" # Allow errors set +e false echo "false: $?" } # call myfunc myfunc echo "calling false again" false echo "false: $?" echo "If 'set -e' were in effect, we wouldn't reach this line." set -e false echo "Unreachable"

Great suggestion. Captured in #7441

The keepalive is running in background, which is in other process. It doesn't impact the following statements. But it needs doc or uses separate bash script to ship it.

k8s-ci-robot added the needs-ok-to-test label Oct 5, 2022

k8s-ci-robot requested review from dmcgowan, fuweid and samuelkarp October 5, 2022 15:04

samuelkarp added the area/cri Container Runtime Interface (CRI) label Oct 6, 2022

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 6, 2022

samuelkarp reviewed Oct 6, 2022

View reviewed changes

Comment thread pkg/cri/server/sandbox_run.go Outdated

Comment thread pkg/cri/server/sandbox_run.go Outdated

samuelkarp added this to the 1.7 milestone Oct 6, 2022

samuelkarp added the kind/bug label Oct 6, 2022

qiutongs force-pushed the netns-fix branch 3 times, most recently from f9884df to 3c3711c Compare October 6, 2022 13:47

samuelkarp reviewed Oct 6, 2022

View reviewed changes

qiutongs force-pushed the netns-fix branch from 3c3711c to b40e6a3 Compare October 7, 2022 03:37

fuweid reviewed Oct 7, 2022

View reviewed changes

Comment thread integration/sandbox_run_rollback_test.go Outdated

fuweid added the cherry-pick/1.6.x label Oct 7, 2022

samuelkarp added the cherry-pick/1.5.x label Oct 7, 2022

fuweid reviewed Oct 8, 2022

View reviewed changes

qiutongs force-pushed the netns-fix branch from b40e6a3 to 06cab00 Compare October 8, 2022 14:48

qiutongs force-pushed the netns-fix branch from 06cab00 to cac4e51 Compare October 9, 2022 00:30

Update container with sandbox metadata after NetNS is created

b41d6f4

Signed-off-by: Qiutong Song <[email protected]>

qiutongs force-pushed the netns-fix branch from cac4e51 to b41d6f4 Compare October 9, 2022 01:15

qiutongs requested review from fuweid and samuelkarp and removed request for dmcgowan, fuweid and samuelkarp October 9, 2022 14:54

fuweid approved these changes Oct 10, 2022

View reviewed changes

estesp approved these changes Oct 10, 2022

View reviewed changes

estesp merged commit 9b0ab84 into containerd:main Oct 10, 2022

samuelkarp reviewed Oct 11, 2022

View reviewed changes

This was referenced Oct 11, 2022

[release/1.6] Update container with sandbox metadata after NetNS is created #7505

Merged

Follow-ups on PR #5904 - Setup pod network after creating the sandbox container #7441

Closed

qiutongs deleted the netns-fix branch October 13, 2022 01:37

samuelkarp added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.6.x labels Oct 13, 2022

samuelkarp mentioned this pull request Oct 13, 2022

[port #5904 to sbserver] Setup pod network after creating the sandbox container #7426

Merged

mxpv added the cherry-pick/sbserver label Nov 22, 2022

samuelkarp added cherry-picked/sbserver Changes are backported to sbserver and removed cherry-pick/sbserver labels Nov 23, 2022

dcantah mentioned this pull request Dec 20, 2022

CRI: Fix no CNI info for pod sandbox on restart #7845

Merged

yylt mentioned this pull request Mar 28, 2025

[CRI] store extension when create sandbox in metadata store #11612

Merged

Conversation

qiutongs commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual Testing

New Integration Test

Uh oh!

qiutongs commented Oct 5, 2022

Uh oh!

k8s-ci-robot commented Oct 5, 2022

Uh oh!

samuelkarp commented Oct 6, 2022

Uh oh!

Uh oh!

Uh oh!

samuelkarp commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fuweid Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

qiutongs commented Oct 9, 2022

Uh oh!

qiutongs commented Oct 9, 2022

Uh oh!

qiutongs commented Oct 9, 2022

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

samuelkarp Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

qiutongs Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

fuweid Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

qiutongs commented Oct 5, 2022 •

edited

Loading

samuelkarp commented Oct 6, 2022 •

edited

Loading