Merge pull request #7684 from mxpv/sb-runtime-fix

Fix sandbox API when calling sandboxed shims

pkg/cri/sbserver/container_create.go

@@ -57,6 +57,8 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		return nil, fmt.Errorf("failed to find sandbox id %q: %w", r.GetPodSandboxId(), err)
 	}
 	sandboxID := sandbox.ID
+
+	// TODO: Add PID endpoint to Controller interface.
 	s, err := sandbox.Container.Task(ctx, nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sandbox container task: %w", err)
@@ -104,11 +106,6 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	}
 
 	start := time.Now()
-	// Run container using the same runtime with sandbox.
-	sandboxInfo, err := sandbox.Container.Info(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get sandbox %q info: %w", sandboxID, err)
-	}
 
 	// Create container root directory.
 	containerRootDir := c.getContainerRootDir(id)
@@ -236,16 +233,18 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 
 	containerLabels := buildLabels(config.Labels, image.ImageSpec.Config.Labels, containerKindContainer)
 
-	runtimeOptions, err := getRuntimeOptions(sandboxInfo)
+	sandboxInfo, err := c.client.SandboxStore().Get(ctx, sandboxID)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get runtime options: %w", err)
+		return nil, fmt.Errorf("unable to get sandbox %q metdata: %w", sandboxID, err)
 	}
 
 	opts = append(opts,
 		containerd.WithSpec(spec, specOpts...),
-		containerd.WithRuntime(sandboxInfo.Runtime.Name, runtimeOptions),
+		containerd.WithRuntime(sandboxInfo.Runtime.Name, sandboxInfo.Runtime.Options),
 		containerd.WithContainerLabels(containerLabels),
-		containerd.WithContainerExtension(containerMetadataExtension, &meta))
+		containerd.WithContainerExtension(containerMetadataExtension, &meta),
+	)
+
 	var cntr containerd.Container
 	if cntr, err = c.client.NewContainer(ctx, id, opts...); err != nil {
 		return nil, fmt.Errorf("failed to create containerd container: %w", err)

pkg/cri/sbserver/podsandbox/helpers.go

@@ -33,18 +33,11 @@ import (
 	criconfig "github.com/containerd/containerd/pkg/cri/config"
 	imagestore "github.com/containerd/containerd/pkg/cri/store/image"
 	ctrdutil "github.com/containerd/containerd/pkg/cri/util"
-	runtimeoptions "github.com/containerd/containerd/pkg/runtimeoptions/v1"
-	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/reference/docker"
-	"github.com/containerd/containerd/runtime/linux/runctypes"
-	runcoptions "github.com/containerd/containerd/runtime/v2/runc/options"
-	"github.com/containerd/typeurl"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 
-	runhcsoptions "github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/options"
 	imagedigest "github.com/opencontainers/go-digest"
-	"github.com/pelletier/go-toml"
 )
 
 const (
@@ -61,8 +54,6 @@ const (
 	containerKindSandbox = "sandbox"
 	// sandboxMetadataExtension is an extension name that identify metadata of sandbox in CreateContainerRequest
 	sandboxMetadataExtension = criContainerdPrefix + ".sandbox.metadata"
-	// runtimeRunhcsV1 is the runtime type for runhcs.
-	runtimeRunhcsV1 = "io.containerd.runhcs.v1"
 	// MetadataKey is the key used for storing metadata in the sandbox extensions
 	MetadataKey = "metadata"
 )
@@ -170,59 +161,6 @@ func parseImageReferences(refs []string) ([]string, []string) {
 	return tags, digests
 }
 
-// generateRuntimeOptions generates runtime options from cri plugin config.
-func generateRuntimeOptions(r criconfig.Runtime, c criconfig.Config) (interface{}, error) {
-	if r.Options == nil {
-		if r.Type != plugin.RuntimeLinuxV1 {
-			return nil, nil
-		}
-		// This is a legacy config, generate runctypes.RuncOptions.
-		return &runctypes.RuncOptions{
-			Runtime:       r.Engine,
-			RuntimeRoot:   r.Root,
-			SystemdCgroup: c.SystemdCgroup,
-		}, nil
-	}
-	optionsTree, err := toml.TreeFromMap(r.Options)
-	if err != nil {
-		return nil, err
-	}
-	options := getRuntimeOptionsType(r.Type)
-	if err := optionsTree.Unmarshal(options); err != nil {
-		return nil, err
-	}
-	return options, nil
-}
-
-// getRuntimeOptionsType gets empty runtime options by the runtime type name.
-func getRuntimeOptionsType(t string) interface{} {
-	switch t {
-	case plugin.RuntimeRuncV1:
-		fallthrough
-	case plugin.RuntimeRuncV2:
-		return &runcoptions.Options{}
-	case plugin.RuntimeLinuxV1:
-		return &runctypes.RuncOptions{}
-	case runtimeRunhcsV1:
-		return &runhcsoptions.Options{}
-	default:
-		return &runtimeoptions.Options{}
-	}
-}
-
-// getRuntimeOptions get runtime options from container metadata.
-func getRuntimeOptions(c containers.Container) (interface{}, error) {
-	from := c.Runtime.Options
-	if from == nil || from.GetValue() == nil {
-		return nil, nil
-	}
-	opts, err := typeurl.UnmarshalAny(from)
-	if err != nil {
-		return nil, err
-	}
-	return opts, nil
-}
-
 // getPassthroughAnnotations filters requested pod annotations by comparing
 // against permitted annotations for the given runtime.
 func getPassthroughAnnotations(podAnnotations map[string]string,

pkg/cri/sbserver/podsandbox/helpers_test.go

@@ -22,21 +22,11 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/containerd/containerd/containers"
 	"github.com/containerd/containerd/oci"
-	criconfig "github.com/containerd/containerd/pkg/cri/config"
-	"github.com/containerd/containerd/plugin"
-	"github.com/containerd/containerd/protobuf/types"
 	"github.com/containerd/containerd/reference/docker"
-	"github.com/containerd/containerd/runtime/linux/runctypes"
-	runcoptions "github.com/containerd/containerd/runtime/v2/runc/options"
-	"github.com/containerd/typeurl"
-
 	imagedigest "github.com/opencontainers/go-digest"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/pelletier/go-toml"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 // TestGetUserFromImage tests the logic of getting image uid or user name of image user.
@@ -158,112 +148,6 @@ func TestParseImageReferences(t *testing.T) {
 	assert.Equal(t, expectedDigests, digests)
 }
 
-func TestGenerateRuntimeOptions(t *testing.T) {
-	nilOpts := `
-systemd_cgroup = true
-[containerd]
-  no_pivot = true
-  default_runtime_name = "default"
-[containerd.runtimes.legacy]
-  runtime_type = "` + plugin.RuntimeLinuxV1 + `"
-[containerd.runtimes.runc]
-  runtime_type = "` + plugin.RuntimeRuncV1 + `"
-[containerd.runtimes.runcv2]
-  runtime_type = "` + plugin.RuntimeRuncV2 + `"
-`
-	nonNilOpts := `
-systemd_cgroup = true
-[containerd]
-  no_pivot = true
-  default_runtime_name = "default"
-[containerd.runtimes.legacy]
-  runtime_type = "` + plugin.RuntimeLinuxV1 + `"
-[containerd.runtimes.legacy.options]
-  Runtime = "legacy"
-  RuntimeRoot = "/legacy"
-[containerd.runtimes.runc]
-  runtime_type = "` + plugin.RuntimeRuncV1 + `"
-[containerd.runtimes.runc.options]
-  BinaryName = "runc"
-  Root = "/runc"
-  NoNewKeyring = true
-[containerd.runtimes.runcv2]
-  runtime_type = "` + plugin.RuntimeRuncV2 + `"
-[containerd.runtimes.runcv2.options]
-  BinaryName = "runc"
-  Root = "/runcv2"
-  NoNewKeyring = true
-`
-	var nilOptsConfig, nonNilOptsConfig criconfig.Config
-	tree, err := toml.Load(nilOpts)
-	require.NoError(t, err)
-	err = tree.Unmarshal(&nilOptsConfig)
-	require.NoError(t, err)
-	require.Len(t, nilOptsConfig.Runtimes, 3)
-
-	tree, err = toml.Load(nonNilOpts)
-	require.NoError(t, err)
-	err = tree.Unmarshal(&nonNilOptsConfig)
-	require.NoError(t, err)
-	require.Len(t, nonNilOptsConfig.Runtimes, 3)
-
-	for desc, test := range map[string]struct {
-		r               criconfig.Runtime
-		c               criconfig.Config
-		expectedOptions interface{}
-	}{
-		"when options is nil, should return nil option for io.containerd.runc.v1": {
-			r:               nilOptsConfig.Runtimes["runc"],
-			c:               nilOptsConfig,
-			expectedOptions: nil,
-		},
-		"when options is nil, should return nil option for io.containerd.runc.v2": {
-			r:               nilOptsConfig.Runtimes["runcv2"],
-			c:               nilOptsConfig,
-			expectedOptions: nil,
-		},
-		"when options is nil, should use legacy fields for legacy runtime": {
-			r: nilOptsConfig.Runtimes["legacy"],
-			c: nilOptsConfig,
-			expectedOptions: &runctypes.RuncOptions{
-				SystemdCgroup: true,
-			},
-		},
-		"when options is not nil, should be able to decode for io.containerd.runc.v1": {
-			r: nonNilOptsConfig.Runtimes["runc"],
-			c: nonNilOptsConfig,
-			expectedOptions: &runcoptions.Options{
-				BinaryName:   "runc",
-				Root:         "/runc",
-				NoNewKeyring: true,
-			},
-		},
-		"when options is not nil, should be able to decode for io.containerd.runc.v2": {
-			r: nonNilOptsConfig.Runtimes["runcv2"],
-			c: nonNilOptsConfig,
-			expectedOptions: &runcoptions.Options{
-				BinaryName:   "runc",
-				Root:         "/runcv2",
-				NoNewKeyring: true,
-			},
-		},
-		"when options is not nil, should be able to decode for legacy runtime": {
-			r: nonNilOptsConfig.Runtimes["legacy"],
-			c: nonNilOptsConfig,
-			expectedOptions: &runctypes.RuncOptions{
-				Runtime:     "legacy",
-				RuntimeRoot: "/legacy",
-			},
-		},
-	} {
-		t.Run(desc, func(t *testing.T) {
-			opts, err := generateRuntimeOptions(test.r, test.c)
-			assert.NoError(t, err)
-			assert.Equal(t, test.expectedOptions, opts)
-		})
-	}
-}
-
 func TestEnvDeduplication(t *testing.T) {
 	for desc, test := range map[string]struct {
 		existing []string
@@ -471,13 +355,3 @@ func TestEnsureRemoveAllWithFile(t *testing.T) {
 		t.Fatal(err)
 	}
 }
-
-func TestGetRuntimeOptions(t *testing.T) {
-	_, err := getRuntimeOptions(containers.Container{})
-	require.NoError(t, err)
-
-	var pbany *types.Any               // This is nil.
-	var typeurlAny typeurl.Any = pbany // This is typed nil.
-	_, err = getRuntimeOptions(containers.Container{Runtime: containers.RuntimeInfo{Options: typeurlAny}})
-	require.NoError(t, err)
-}

pkg/cri/sbserver/podsandbox/sandbox_run.go

@@ -86,7 +86,7 @@ func (c *Controller) Start(ctx context.Context, id string) (resp *api.Controller
 		return nil, fmt.Errorf("failed to get image from containerd %q: %w", image.ID, err)
 	}
 
-	ociRuntime, err := c.getSandboxRuntime(config, sandboxInfo.Runtime.Name)
+	ociRuntime, err := c.getSandboxRuntime(config, metadata.RuntimeHandler)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sandbox runtime: %w", err)
 	}
@@ -131,11 +131,6 @@ func (c *Controller) Start(ctx context.Context, id string) (resp *api.Controller
 
 	sandboxLabels := buildLabels(config.Labels, image.ImageSpec.Config.Labels, containerKindSandbox)
 
-	runtimeOpts, err := generateRuntimeOptions(ociRuntime, c.config)
-	if err != nil {
-		return nil, fmt.Errorf("failed to generate runtime options: %w", err)
-	}
-
 	snapshotterOpt := snapshots.WithLabels(snapshots.FilterInheritedLabels(config.Annotations))
 	opts := []containerd.NewContainerOpts{
 		containerd.WithSnapshotter(c.runtimeSnapshotter(ctx, ociRuntime)),
@@ -143,7 +138,8 @@ func (c *Controller) Start(ctx context.Context, id string) (resp *api.Controller
 		containerd.WithSpec(spec, specOpts...),
 		containerd.WithContainerLabels(sandboxLabels),
 		containerd.WithContainerExtension(sandboxMetadataExtension, &metadata),
-		containerd.WithRuntime(ociRuntime.Type, runtimeOpts)}
+		containerd.WithRuntime(ociRuntime.Type, sandboxInfo.Runtime.Options),
+	}
 
 	container, err := c.client.NewContainer(ctx, id, opts...)
 	if err != nil {
@@ -263,6 +259,7 @@ func (c *Controller) Start(ctx context.Context, id string) (resp *api.Controller
 		SandboxID: id,
 		Pid:       task.Pid(),
 		CreatedAt: protobuf.ToTimestamp(info.CreatedAt),
+		Labels:    labels,
 	}
 
 	return resp, nil

pkg/cri/sbserver/sandbox_run.go

@@ -86,10 +86,29 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 		}
 	}()
 
-	sandboxInfo := sb.Sandbox{
+	var (
-		ID: id,
+		err         error
-		// TODO: runtime handler can be an empty string, should use default one and enable back validation of this field in metadata store.
+		sandboxInfo = sb.Sandbox{ID: id}
-		Runtime: sb.RuntimeOpts{Name: r.GetRuntimeHandler()},
+	)
+
+	ociRuntime, err := c.getSandboxRuntime(config, r.GetRuntimeHandler())
+	if err != nil {
+		return nil, fmt.Errorf("unable to get OCI runtime for sandbox %q: %w", id, err)
+	}
+
+	sandboxInfo.Runtime.Name = ociRuntime.Type
+
+	// Retrieve runtime options
+	runtimeOpts, err := generateRuntimeOptions(ociRuntime, c.config)
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate sandbox runtime options: %w", err)
+	}
+
+	if runtimeOpts != nil {
+		sandboxInfo.Runtime.Options, err = typeurl.MarshalAny(runtimeOpts)
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal runtime options: %w", err)
+		}
 	}
 
 	// Save sandbox name
@@ -127,11 +146,7 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 		}
 	}()
 
-	var (
+	podNetwork := true
-		podNetwork = true
-		err        error
-	)
-
 	if goruntime.GOOS != "windows" &&
 		config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetNetwork() == runtime.NamespaceMode_NODE {
 		// Pod network is not needed on linux with host network.
@@ -143,6 +158,11 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 		podNetwork = false
 	}
 
+	// No CNI on darwin yet
+	if goruntime.GOOS == "darwin" {
+		podNetwork = false
+	}
+
 	if podNetwork {
 		netStart := time.Now()
 		// If it is not in host network namespace then create a namespace and set the sandbox

pkg/cri/sbserver/sandbox_status.go

@@ -45,12 +45,13 @@ func (c *criService) PodSandboxStatus(ctx context.Context, r *runtime.PodSandbox
 	status := toCRISandboxStatus(sandbox.Metadata, sandbox.Status.Get(), ip, additionalIPs)
 	if status.GetCreatedAt() == 0 {
 		// CRI doesn't allow CreatedAt == 0.
-		info, err := sandbox.Container.Info(ctx)
+		sandboxInfo, err := c.client.SandboxStore().Get(ctx, r.GetPodSandboxId())
 		if err != nil {
-			return nil, fmt.Errorf("failed to get CreatedAt for sandbox container in %q state: %w", status.State, err)
+			return nil, fmt.Errorf("failed to get sandbox %q from metadata store: %w", r.GetPodSandboxId(), err)
 		}
-		status.CreatedAt = info.CreatedAt.UnixNano()
+		status.CreatedAt = sandboxInfo.CreatedAt.UnixNano()
 	}
+
 	if !r.GetVerbose() {
 		return &runtime.PodSandboxStatusResponse{Status: status}, nil
 	}
@@ -76,10 +77,15 @@ func (c *criService) getIPs(sandbox sandboxstore.Sandbox) (string, []string, err
 		// responsible for reporting the IP.
 		return "", nil, nil
 	}
+
 	if goruntime.GOOS == "windows" && config.GetWindows().GetSecurityContext().GetHostProcess() {
 		return "", nil, nil
 	}
 
+	if goruntime.GOOS == "darwin" {
+		return "", nil, nil
+	}
+
 	if closed, err := sandbox.NetNS.Closed(); err != nil {
 		return "", nil, fmt.Errorf("check network namespace closed: %w", err)
 	} else if closed {

pkg/cri/sbserver/service.go

@@ -27,14 +27,12 @@ import (
 	"time"
 
 	"github.com/containerd/containerd"
-	sandboxapi "github.com/containerd/containerd/api/services/sandbox/v1"
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/pkg/cri/sbserver/podsandbox"
 	"github.com/containerd/containerd/pkg/cri/streaming"
 	"github.com/containerd/containerd/pkg/kmutex"
 	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/sandbox"
-	"github.com/containerd/containerd/sandbox/proxy"
 	runtime_alpha "github.com/containerd/containerd/third_party/k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 	"github.com/containerd/go-cni"
 	"github.com/sirupsen/logrus"
@@ -191,7 +189,7 @@ func NewCRIService(config criconfig.Config, client *containerd.Client) (CRIServi
 
 	// Load all sandbox controllers(pod sandbox controller and remote shim controller)
 	c.sandboxControllers[criconfig.ModePodSandbox] = podsandbox.New(config, client, c.sandboxStore, c.os, c, c.baseOCISpecs)
-	c.sandboxControllers[criconfig.ModeShim] = proxy.NewSandboxController(sandboxapi.NewControllerClient(client.Conn()))
+	c.sandboxControllers[criconfig.ModeShim] = client.SandboxController()
 
 	return c, nil
 }

services/sandbox/controller_local.go

@@ -112,6 +112,7 @@ func (c *controllerLocal) Create(ctx context.Context, in *api.ControllerCreateRe
 		Rootfs:     in.Rootfs,
 		Options:    in.Options,
 	}); err != nil {
+		// TODO: Delete sandbox shim here.
 		return nil, fmt.Errorf("failed to start sandbox %s: %w", in.SandboxID, err)
 	}