Merge pull request #7481 from qiutongs/netns-fix

Update container with sandbox metadata after NetNS is created

integration/main_test.go

@@ -17,20 +17,24 @@
 package integration
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
 	"flag"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	goruntime "runtime"
 	"strconv"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
 	"github.com/containerd/containerd"
+	"github.com/containerd/containerd/containers"
 	cri "github.com/containerd/containerd/integration/cri-api/pkg/apis"
 	_ "github.com/containerd/containerd/integration/images" // Keep this around to parse `imageListFile` command line var
 	"github.com/containerd/containerd/integration/remote"
@@ -403,12 +407,12 @@ func Randomize(str string) string {
 }
 
 // KillProcess kills the process by name. pkill is used.
-func KillProcess(name string) error {
+func KillProcess(name string, signal syscall.Signal) error {
 	var command []string
 	if goruntime.GOOS == "windows" {
 		command = []string{"taskkill", "/IM", name, "/F"}
 	} else {
-		command = []string{"pkill", "-x", fmt.Sprintf("^%s$", name)}
+		command = []string{"pkill", "-" + strconv.Itoa(int(signal)), "-x", fmt.Sprintf("^%s$", name)}
 	}
 
 	output, err := exec.Command(command[0], command[1:]...).CombinedOutput()
@@ -444,6 +448,80 @@ func PidOf(name string) (int, error) {
 	return strconv.Atoi(output)
 }
 
+// PidsOf returns pid(s) of a process by name
+func PidsOf(name string) ([]int, error) {
+	if len(name) == 0 {
+		return []int{}, fmt.Errorf("name is required")
+	}
+
+	procDirFD, err := os.Open("/proc")
+	if err != nil {
+		return nil, fmt.Errorf("failed to open /proc: %w", err)
+	}
+	defer procDirFD.Close()
+
+	res := []int{}
+	for {
+		fileInfos, err := procDirFD.Readdir(100)
+		if err != nil {
+			if err == io.EOF {
+				break
+			}
+			return nil, fmt.Errorf("failed to readdir: %w", err)
+		}
+
+		for _, fileInfo := range fileInfos {
+			if !fileInfo.IsDir() {
+				continue
+			}
+
+			pid, err := strconv.Atoi(fileInfo.Name())
+			if err != nil {
+				continue
+			}
+
+			exePath, err := os.Readlink(filepath.Join("/proc", fileInfo.Name(), "exe"))
+			if err != nil {
+				continue
+			}
+
+			if strings.HasSuffix(exePath, name) {
+				res = append(res, pid)
+			}
+		}
+	}
+	return res, nil
+}
+
+// PidEnvs returns the environ of pid in key-value pairs.
+func PidEnvs(pid int) (map[string]string, error) {
+	envPath := filepath.Join("/proc", strconv.Itoa(pid), "environ")
+
+	b, err := os.ReadFile(envPath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read %s: %w", envPath, err)
+	}
+
+	values := bytes.Split(b, []byte{0})
+	if len(values) == 0 {
+		return nil, nil
+	}
+
+	res := make(map[string]string)
+	for _, value := range values {
+		value := strings.TrimSpace(string(value))
+		if len(value) == 0 {
+			continue
+		}
+
+		parts := strings.SplitN(value, "=", 2)
+		if len(parts) == 2 {
+			res[parts[0]] = parts[1]
+		}
+	}
+	return res, nil
+}
+
 // RawRuntimeClient returns a raw grpc runtime service client.
 func RawRuntimeClient() (runtime.RuntimeServiceClient, error) {
 	addr, dialer, err := dialer.GetAddressAndDialer(*criEndpoint)
@@ -500,8 +578,8 @@ func SandboxInfo(id string) (*runtime.PodSandboxStatus, *server.SandboxInfo, err
 	return status, &info, nil
 }
 
-func RestartContainerd(t *testing.T) {
-	require.NoError(t, KillProcess(*containerdBin))
+func RestartContainerd(t *testing.T, signal syscall.Signal) {
+	require.NoError(t, KillProcess(*containerdBin, signal))
 
 	// Use assert so that the 3rd wait always runs, this makes sure
 	// containerd is running before this function returns.
@@ -534,3 +612,7 @@ func EnsureImageExists(t *testing.T, imageName string) string {
 
 	return imgID
 }
+
+func GetContainer(id string) (containers.Container, error) {
+	return containerdClient.ContainerService().Get(context.Background(), id)
+}

integration/restart_test.go

@@ -165,7 +165,7 @@ func TestContainerdRestart(t *testing.T) {
 	assert.NoError(t, err)
 
 	t.Logf("Restart containerd")
-	RestartContainerd(t)
+	RestartContainerd(t, syscall.SIGTERM)
 
 	t.Logf("Check sandbox and container state after restart")
 	loadedSandboxes, err := runtimeService.ListPodSandbox(&runtime.PodSandboxFilter{})

integration/sandbox_run_rollback_test.go

@@ -26,16 +26,24 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
+	"syscall"
 	"testing"
+	"time"
 
+	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	criapiv1 "k8s.io/cri-api/pkg/apis/runtime/v1"
 
+	"github.com/containerd/containerd/pkg/cri/store/sandbox"
 	"github.com/containerd/containerd/pkg/failpoint"
+	"github.com/containerd/typeurl"
 )
 
 const (
 	failpointRuntimeHandler = "runc-fp"
+	failpointCNIBinary      = "cni-bridge-fp"
 
 	failpointShimPrefixKey = "io.containerd.runtime.v2.shim.failpoint."
 
@@ -138,7 +146,7 @@ func TestRunPodSandboxWithShimDeleteFailure(t *testing.T) {
 
 			if restart {
 				t.Log("Restart containerd")
-				RestartContainerd(t)
+				RestartContainerd(t, syscall.SIGTERM)
 
 				t.Log("ListPodSandbox with the specific label")
 				l, err = runtimeService.ListPodSandbox(&criapiv1.PodSandboxFilter{Id: sb.Id})
@@ -211,7 +219,7 @@ func TestRunPodSandboxWithShimStartAndTeardownCNIFailure(t *testing.T) {
 
 			if restart {
 				t.Log("Restart containerd")
-				RestartContainerd(t)
+				RestartContainerd(t, syscall.SIGTERM)
 
 				t.Log("ListPodSandbox with the specific label")
 				l, err = runtimeService.ListPodSandbox(&criapiv1.PodSandboxFilter{Id: sb.Id})
@@ -229,6 +237,132 @@ func TestRunPodSandboxWithShimStartAndTeardownCNIFailure(t *testing.T) {
 	t.Run("JustCleanup", testCase(false))
 }
 
+// TestRunPodSandboxWithShimStartAndTeardownCNISlow should keep the sandbox
+// record if failed to rollback CNI API.
+func TestRunPodSandboxAndTeardownCNISlow(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skip()
+	}
+	if os.Getenv("ENABLE_CRI_SANDBOXES") != "" {
+		t.Skip()
+	}
+
+	t.Log("Init PodSandboxConfig with specific key")
+	sbName := t.Name()
+	labels := map[string]string{
+		sbName: "true",
+	}
+	sbConfig := PodSandboxConfig(sbName, "failpoint", WithPodLabels(labels))
+
+	t.Log("Inject CNI failpoint")
+	conf := &failpointConf{
+		// Delay 1 day
+		Add: "1*delay(86400000)",
+	}
+	injectCNIFailpoint(t, sbConfig, conf)
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+
+	go func() {
+		defer wg.Done()
+		t.Log("Create a sandbox")
+		_, err := runtimeService.RunPodSandbox(sbConfig, failpointRuntimeHandler)
+		require.Error(t, err)
+		require.ErrorContains(t, err, "error reading from server: EOF")
+	}()
+
+	assert.NoError(t, ensureCNIAddRunning(t, sbName), "check that failpoint CNI.Add is running")
+
+	// Use SIGKILL to prevent containerd server gracefulshutdown which may cause indeterministic invocation of defer functions
+	t.Log("Restart containerd")
+	RestartContainerd(t, syscall.SIGKILL)
+
+	wg.Wait()
+
+	t.Log("ListPodSandbox with the specific label")
+	l, err := runtimeService.ListPodSandbox(&criapiv1.PodSandboxFilter{LabelSelector: labels})
+	require.NoError(t, err)
+	require.Len(t, l, 1)
+
+	sb := l[0]
+
+	defer func() {
+		t.Log("Cleanup leaky sandbox")
+		err := runtimeService.StopPodSandbox(sb.Id)
+		assert.NoError(t, err)
+		err = runtimeService.RemovePodSandbox(sb.Id)
+		require.NoError(t, err)
+	}()
+
+	assert.Equal(t, sb.State, criapiv1.PodSandboxState_SANDBOX_NOTREADY)
+	assert.Equal(t, sb.Metadata.Name, sbConfig.Metadata.Name)
+	assert.Equal(t, sb.Metadata.Namespace, sbConfig.Metadata.Namespace)
+	assert.Equal(t, sb.Metadata.Uid, sbConfig.Metadata.Uid)
+	assert.Equal(t, sb.Metadata.Attempt, sbConfig.Metadata.Attempt)
+
+	t.Log("Get sandbox info")
+	_, info, err := SandboxInfo(sb.Id)
+	require.NoError(t, err)
+	require.False(t, info.NetNSClosed)
+
+	var netNS string
+	for _, n := range info.RuntimeSpec.Linux.Namespaces {
+		if n.Type == runtimespec.NetworkNamespace {
+			netNS = n.Path
+		}
+	}
+	assert.NotEmpty(t, netNS, "network namespace should be set")
+
+	t.Log("Get sandbox container")
+	c, err := GetContainer(sb.Id)
+	require.NoError(t, err)
+	any, ok := c.Extensions["io.cri-containerd.sandbox.metadata"]
+	require.True(t, ok, "sandbox metadata should exist in extension")
+	i, err := typeurl.UnmarshalAny(any)
+	require.NoError(t, err)
+	require.IsType(t, &sandbox.Metadata{}, i)
+	metadata, ok := i.(*sandbox.Metadata)
+	require.True(t, ok)
+	assert.NotEmpty(t, metadata.NetNSPath)
+	assert.Equal(t, netNS, metadata.NetNSPath, "network namespace path should be the same in runtime spec and sandbox metadata")
+}
+
+func ensureCNIAddRunning(t *testing.T, sbName string) error {
+	return Eventually(func() (bool, error) {
+		pids, err := PidsOf(failpointCNIBinary)
+		if err != nil || len(pids) == 0 {
+			return false, err
+		}
+
+		for _, pid := range pids {
+			envs, err := PidEnvs(pid)
+			if err != nil {
+				t.Logf("failed to read environ of pid %v: %v: skip it", pid, err)
+				continue
+			}
+
+			args, ok := envs["CNI_ARGS"]
+			if !ok {
+				t.Logf("expected CNI_ARGS env but got nothing, skip pid=%v", pid)
+				continue
+			}
+
+			for _, arg := range strings.Split(args, ";") {
+				kv := strings.SplitN(arg, "=", 2)
+				if len(kv) != 2 {
+					continue
+				}
+
+				if kv[0] == "K8S_POD_NAME" && kv[1] == sbName {
+					return true, nil
+				}
+			}
+		}
+		return false, nil
+	}, time.Second, 30*time.Second)
+}
+
 // failpointConf is used to describe cmdAdd/cmdDel/cmdCheck command's failpoint.
 type failpointConf struct {
 	Add   string `json:"cmdAdd"`

pkg/cri/server/container_update_resources_other.go

@@ -23,10 +23,6 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/containerd/containerd"
-	"github.com/containerd/containerd/containers"
-	"github.com/containerd/typeurl"
-	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
 
 	containerstore "github.com/containerd/containerd/pkg/cri/store/container"
@@ -48,19 +44,3 @@ func (c *criService) UpdateContainerResources(ctx context.Context, r *runtime.Up
 	}
 	return &runtime.UpdateContainerResourcesResponse{}, nil
 }
-
-// TODO: Copied from container_update_resources.go because that file is not built for darwin.
-// updateContainerSpec updates container spec.
-func updateContainerSpec(ctx context.Context, cntr containerd.Container, spec *runtimespec.Spec) error {
-	any, err := typeurl.MarshalAny(spec)
-	if err != nil {
-		return fmt.Errorf("failed to marshal spec %+v: %w", spec, err)
-	}
-	if err := cntr.Update(ctx, func(ctx context.Context, client *containerd.Client, c *containers.Container) error {
-		c.Spec = any
-		return nil
-	}); err != nil {
-		return fmt.Errorf("failed to update container spec: %w", err)
-	}
-	return nil
-}

pkg/cri/server/sandbox_run.go

@@ -290,7 +290,12 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 
 		// Update network namespace in the container's spec
 		c.updateNetNamespacePath(spec, sandbox.NetNSPath)
-		if err := updateContainerSpec(ctx, container, spec); err != nil {
+
+		if err := container.Update(ctx,
+			// Update spec of the container
+			containerd.UpdateContainerOpts(containerd.WithSpec(spec)),
+			// Update sandbox metadata to include NetNS info
+			containerd.UpdateContainerOpts(containerd.WithContainerExtension(sandboxMetadataExtension, &sandbox.Metadata))); err != nil {
 			return nil, fmt.Errorf("failed to update the network namespace for the sandbox container %q: %w", id, err)
 		}
 

script/test/utils.sh

@@ -275,6 +275,9 @@ run_crictl() {
 # keepalive runs a command and keeps it alive.
 # keepalive process is eventually killed in test_teardown.
 keepalive() {
+  # The command may return non-zero and we want to continue this script.
+  # e.g. containerd receives SIGKILL
+  set +e
   local command=$1
   echo "${command}"
   local wait_period=$2