Merge pull request #7731 from mxpv/cri

[Sandbox API] CRI status cleanup

pkg/cri/sbserver/container_create.go

@@ -56,14 +56,21 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	if err != nil {
 		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)
+	controller, err := c.getSandboxController(sandbox.Config, sandbox.RuntimeHandler)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get sandbox container task: %w", err)
+		return nil, fmt.Errorf("failed to get sandbox controller: %w", err)
 	}
-	sandboxPid := s.Pid()
+
+	statusResponse, err := controller.Status(ctx, sandbox.ID, false)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get controller status: %w", err)
+	}
+
+	var (
+		sandboxID  = statusResponse.GetID()
+		sandboxPid = statusResponse.GetPid()
+	)
 
 	// Generate unique id and name for the container and reserve the name.
 	// Reserve the container name to avoid concurrent `CreateContainer` request creating
@@ -245,6 +252,11 @@ func (c *criService) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		containerd.WithContainerExtension(containerMetadataExtension, &meta),
 	)
 
+	// When using sandboxed shims, containerd's runtime needs to know which sandbox shim instance to use.
+	if ociRuntime.SandboxMode == string(criconfig.ModeShim) {
+		opts = append(opts, containerd.WithSandbox(sandboxID))
+	}
+
 	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/controller.go

@@ -84,26 +84,6 @@ func New(
 
 var _ sandbox.Controller = (*Controller)(nil)
 
-func (c *Controller) Status(ctx context.Context, sandboxID string) (*api.ControllerStatusResponse, error) {
-	sandbox, err := c.sandboxStore.Get(sandboxID)
-	if err != nil {
-		return nil, fmt.Errorf("an error occurred while trying to find sandbox %q: %w",
-			sandboxID, err)
-	}
-	status := sandbox.Status.Get()
-	resp := &api.ControllerStatusResponse{
-		ID:         sandboxID,
-		Pid:        status.Pid,
-		State:      status.State.String(),
-		ExitStatus: status.ExitStatus,
-		Extra:      nil,
-	}
-	if !status.ExitedAt.IsZero() {
-		resp.ExitedAt = protobuf.ToTimestamp(status.ExitedAt)
-	}
-	return resp, nil
-}
-
 func (c *Controller) Wait(ctx context.Context, sandboxID string) (*api.ControllerWaitResponse, error) {
 	status := c.store.Get(sandboxID)
 	if status == nil {

pkg/cri/sbserver/podsandbox/controller_test.go

@@ -78,12 +78,11 @@ func Test_Status(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	s, err := controller.Status(context.Background(), sandboxID)
+	s, err := controller.Status(context.Background(), sandboxID, false)
 	if err != nil {
 		t.Fatal(err)
 	}
 	assert.Equal(t, s.Pid, pid)
-	assert.Equal(t, s.ExitStatus, exitStatus)
 	assert.Equal(t, s.ExitedAt, protobuf.ToTimestamp(exitedAt))
 	assert.Equal(t, s.State, sandboxstore.StateReady.String())
 }

pkg/cri/sbserver/podsandbox/sandbox_status.go

@@ -0,0 +1,182 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package podsandbox
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+
+	"github.com/containerd/containerd"
+	api "github.com/containerd/containerd/api/services/sandbox/v1"
+	"github.com/containerd/containerd/containers"
+	"github.com/containerd/containerd/errdefs"
+	sandboxstore "github.com/containerd/containerd/pkg/cri/store/sandbox"
+	"github.com/containerd/containerd/protobuf"
+	"github.com/containerd/go-cni"
+	"github.com/containerd/typeurl"
+	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+)
+
+// SandboxInfo is extra information for sandbox.
+// TODO (mikebrow): discuss predefining constants structures for some or all of these field names in CRI
+type SandboxInfo struct {
+	Pid         uint32 `json:"pid"`
+	Status      string `json:"processStatus"`
+	NetNSClosed bool   `json:"netNamespaceClosed"`
+	Image       string `json:"image"`
+	SnapshotKey string `json:"snapshotKey"`
+	Snapshotter string `json:"snapshotter"`
+	// Note: a new field `RuntimeHandler` has been added into the CRI PodSandboxStatus struct, and
+	// should be set. This `RuntimeHandler` field will be deprecated after containerd 1.3 (tracked
+	// in https://github.com/containerd/cri/issues/1064).
+	RuntimeHandler string                    `json:"runtimeHandler"` // see the Note above
+	RuntimeType    string                    `json:"runtimeType"`
+	RuntimeOptions interface{}               `json:"runtimeOptions"`
+	Config         *runtime.PodSandboxConfig `json:"config"`
+	// Note: RuntimeSpec may not be populated if the sandbox has not been fully created.
+	RuntimeSpec *runtimespec.Spec      `json:"runtimeSpec"`
+	CNIResult   *cni.Result            `json:"cniResult"`
+	Metadata    *sandboxstore.Metadata `json:"sandboxMetadata"`
+}
+
+func (c *Controller) Status(ctx context.Context, sandboxID string, verbose bool) (*api.ControllerStatusResponse, error) {
+	sandbox, err := c.sandboxStore.Get(sandboxID)
+	if err != nil {
+		return nil, fmt.Errorf("an error occurred while trying to find sandbox %q: %w",
+			sandboxID, err)
+	}
+
+	status := sandbox.Status.Get()
+	resp := &api.ControllerStatusResponse{
+		ID:        sandboxID,
+		Pid:       status.Pid,
+		State:     status.State.String(),
+		CreatedAt: protobuf.ToTimestamp(status.CreatedAt),
+		Extra:     nil,
+	}
+
+	if !status.ExitedAt.IsZero() {
+		resp.ExitedAt = protobuf.ToTimestamp(status.ExitedAt)
+	}
+
+	if verbose {
+		info, err := toCRISandboxInfo(ctx, sandbox)
+		if err != nil {
+			return nil, err
+		}
+
+		resp.Info = info
+	}
+
+	return resp, nil
+}
+
+// toCRISandboxInfo converts internal container object information to CRI sandbox status response info map.
+func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[string]string, error) {
+	si := &SandboxInfo{
+		Pid:            sandbox.Status.Get().Pid,
+		Config:         sandbox.Config,
+		RuntimeHandler: sandbox.RuntimeHandler,
+		CNIResult:      sandbox.CNIResult,
+	}
+
+	if container := sandbox.Container; container != nil {
+		task, err := container.Task(ctx, nil)
+		if err != nil && !errdefs.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to get sandbox container task: %w", err)
+		}
+
+		var processStatus containerd.ProcessStatus
+		if task != nil {
+			if taskStatus, err := task.Status(ctx); err != nil {
+				if !errdefs.IsNotFound(err) {
+					return nil, fmt.Errorf("failed to get task status: %w", err)
+				}
+				processStatus = containerd.Unknown
+			} else {
+				processStatus = taskStatus.Status
+			}
+		}
+		si.Status = string(processStatus)
+
+		spec, err := container.Spec(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get sandbox container runtime spec: %w", err)
+		}
+		si.RuntimeSpec = spec
+
+		ctrInfo, err := container.Info(ctx)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get sandbox container info: %w", err)
+		}
+		// Do not use config.SandboxImage because the configuration might
+		// be changed during restart. It may not reflect the actual image
+		// used by the sandbox container.
+		si.Image = ctrInfo.Image
+		si.SnapshotKey = ctrInfo.SnapshotKey
+		si.Snapshotter = ctrInfo.Snapshotter
+
+		runtimeOptions, err := getRuntimeOptions(ctrInfo)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get runtime options: %w", err)
+		}
+
+		si.RuntimeType = ctrInfo.Runtime.Name
+		si.RuntimeOptions = runtimeOptions
+	}
+
+	if si.Status == "" {
+		// If processStatus is empty, it means that the task is deleted. Apply "deleted"
+		// status which does not exist in containerd.
+		si.Status = "deleted"
+	}
+
+	if sandbox.NetNS != nil {
+		// Add network closed information if sandbox is not using host network.
+		closed, err := sandbox.NetNS.Closed()
+		if err != nil {
+			return nil, fmt.Errorf("failed to check network namespace closed: %w", err)
+		}
+		si.NetNSClosed = closed
+	}
+
+	si.Metadata = &sandbox.Metadata
+
+	infoBytes, err := json.Marshal(si)
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal info %v: %w", si, err)
+	}
+
+	return map[string]string{
+		"info": string(infoBytes),
+	}, nil
+}
+
+// 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
+}

pkg/cri/sbserver/sandbox_run.go

@@ -257,12 +257,15 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 		}
 		return nil, fmt.Errorf("failed to start sandbox %q: %w", id, err)
 	}
+
 	// TODO: get rid of this. sandbox object should no longer have Container field.
-	container, err := c.client.LoadContainer(ctx, id)
-	if err != nil {
-		return nil, fmt.Errorf("failed to load container %q for sandbox: %w", id, err)
+	if ociRuntime.SandboxMode == string(criconfig.ModePodSandbox) {
+		container, err := c.client.LoadContainer(ctx, id)
+		if err != nil {
+			return nil, fmt.Errorf("failed to load container %q for sandbox: %w", id, err)
+		}
+		sandbox.Container = container
 	}
-	sandbox.Container = container
 
 	labels := resp.GetLabels()
 	if labels == nil {
@@ -293,17 +296,20 @@ func (c *criService) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
 	// but we don't care about sandbox TaskOOM right now, so it is fine.
 	go func() {
 		resp, err := controller.Wait(context.Background(), id)
-		if err != nil && err != context.Canceled && err != context.DeadlineExceeded {
-			e := &eventtypes.TaskExit{
-				ContainerID: id,
-				ID:          id,
-				// Pid is not used
-				Pid:        0,
-				ExitStatus: resp.ExitStatus,
-				ExitedAt:   resp.ExitedAt,
-			}
-			c.eventMonitor.backOff.enBackOff(id, e)
+		if err != nil {
+			log.G(ctx).WithError(err).Error("failed to wait for sandbox controller, skipping exit event")
+			return
 		}
+
+		e := &eventtypes.TaskExit{
+			ContainerID: id,
+			ID:          id,
+			// Pid is not used
+			Pid:        0,
+			ExitStatus: resp.ExitStatus,
+			ExitedAt:   resp.ExitedAt,
+		}
+		c.eventMonitor.backOff.enBackOff(id, e)
 	}()
 
 	sandboxRuntimeCreateTimer.WithValues(labels["oci_runtime_type"]).UpdateSince(runtimeStart)

pkg/cri/sbserver/sandbox_status.go

@@ -18,17 +18,12 @@ package sbserver
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	goruntime "runtime"
-
-	"github.com/containerd/containerd"
-	"github.com/containerd/containerd/errdefs"
-	"github.com/containerd/go-cni"
-	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
-	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+	"time"
 
 	sandboxstore "github.com/containerd/containerd/pkg/cri/store/sandbox"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
 )
 
 // PodSandboxStatus returns the status of the PodSandbox.
@@ -42,7 +37,18 @@ func (c *criService) PodSandboxStatus(ctx context.Context, r *runtime.PodSandbox
 	if err != nil {
 		return nil, fmt.Errorf("failed to get sandbox ip: %w", err)
 	}
-	status := toCRISandboxStatus(sandbox.Metadata, sandbox.Status.Get(), ip, additionalIPs)
+
+	controller, err := c.getSandboxController(sandbox.Config, sandbox.RuntimeHandler)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get sandbox controller: %w", err)
+	}
+
+	statusResponse, err := controller.Status(ctx, r.GetPodSandboxId(), r.GetVerbose())
+	if err != nil {
+		return nil, fmt.Errorf("failed to query controller status: %w", err)
+	}
+
+	status := toCRISandboxStatus(sandbox.Metadata, statusResponse.State, statusResponse.GetCreatedAt().AsTime(), ip, additionalIPs)
 	if status.GetCreatedAt() == 0 {
 		// CRI doesn't allow CreatedAt == 0.
 		sandboxInfo, err := c.client.SandboxStore().Get(ctx, r.GetPodSandboxId())
@@ -52,19 +58,9 @@ func (c *criService) PodSandboxStatus(ctx context.Context, r *runtime.PodSandbox
 		status.CreatedAt = sandboxInfo.CreatedAt.UnixNano()
 	}
 
-	if !r.GetVerbose() {
-		return &runtime.PodSandboxStatusResponse{Status: status}, nil
-	}
-
-	// Generate verbose information.
-	info, err := toCRISandboxInfo(ctx, sandbox)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get verbose sandbox container info: %w", err)
-	}
-
 	return &runtime.PodSandboxStatusResponse{
 		Status: status,
-		Info:   info,
+		Info:   statusResponse.GetInfo(),
 	}, nil
 }
 
@@ -96,11 +92,11 @@ func (c *criService) getIPs(sandbox sandboxstore.Sandbox) (string, []string, err
 }
 
 // toCRISandboxStatus converts sandbox metadata into CRI pod sandbox status.
-func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status, ip string, additionalIPs []string) *runtime.PodSandboxStatus {
+func toCRISandboxStatus(meta sandboxstore.Metadata, status string, createdAt time.Time, ip string, additionalIPs []string) *runtime.PodSandboxStatus {
 	// Set sandbox state to NOTREADY by default.
 	state := runtime.PodSandboxState_SANDBOX_NOTREADY
-	if status.State == sandboxstore.StateReady {
-		state = runtime.PodSandboxState_SANDBOX_READY
+	if value, ok := runtime.PodSandboxState_value[status]; ok {
+		state = runtime.PodSandboxState(value)
 	}
 	nsOpts := meta.Config.GetLinux().GetSecurityContext().GetNamespaceOptions()
 	var ips []*runtime.PodIP
@@ -111,7 +107,7 @@ func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status,
 		Id:        meta.ID,
 		Metadata:  meta.Config.GetMetadata(),
 		State:     state,
-		CreatedAt: status.CreatedAt.UnixNano(),
+		CreatedAt: createdAt.UnixNano(),
 		Network: &runtime.PodSandboxNetworkStatus{
 			Ip:            ip,
 			AdditionalIps: ips,
@@ -130,105 +126,3 @@ func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status,
 		RuntimeHandler: meta.RuntimeHandler,
 	}
 }
-
-// SandboxInfo is extra information for sandbox.
-// TODO (mikebrow): discuss predefining constants structures for some or all of these field names in CRI
-type SandboxInfo struct {
-	Pid         uint32 `json:"pid"`
-	Status      string `json:"processStatus"`
-	NetNSClosed bool   `json:"netNamespaceClosed"`
-	Image       string `json:"image"`
-	SnapshotKey string `json:"snapshotKey"`
-	Snapshotter string `json:"snapshotter"`
-	// Note: a new field `RuntimeHandler` has been added into the CRI PodSandboxStatus struct, and
-	// should be set. This `RuntimeHandler` field will be deprecated after containerd 1.3 (tracked
-	// in https://github.com/containerd/cri/issues/1064).
-	RuntimeHandler string                    `json:"runtimeHandler"` // see the Note above
-	RuntimeType    string                    `json:"runtimeType"`
-	RuntimeOptions interface{}               `json:"runtimeOptions"`
-	Config         *runtime.PodSandboxConfig `json:"config"`
-	// Note: RuntimeSpec may not be populated if the sandbox has not been fully created.
-	RuntimeSpec *runtimespec.Spec      `json:"runtimeSpec"`
-	CNIResult   *cni.Result            `json:"cniResult"`
-	Metadata    *sandboxstore.Metadata `json:"sandboxMetadata"`
-}
-
-// toCRISandboxInfo converts internal container object information to CRI sandbox status response info map.
-func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[string]string, error) {
-	si := &SandboxInfo{
-		Pid:            sandbox.Status.Get().Pid,
-		Config:         sandbox.Config,
-		RuntimeHandler: sandbox.RuntimeHandler,
-		CNIResult:      sandbox.CNIResult,
-	}
-
-	if sandbox.Container != nil {
-		container := sandbox.Container
-		task, err := container.Task(ctx, nil)
-		if err != nil && !errdefs.IsNotFound(err) {
-			return nil, fmt.Errorf("failed to get sandbox container task: %w", err)
-		}
-
-		var processStatus containerd.ProcessStatus
-		if task != nil {
-			if taskStatus, err := task.Status(ctx); err != nil {
-				if !errdefs.IsNotFound(err) {
-					return nil, fmt.Errorf("failed to get task status: %w", err)
-				}
-				processStatus = containerd.Unknown
-			} else {
-				processStatus = taskStatus.Status
-			}
-		}
-		si.Status = string(processStatus)
-
-		spec, err := container.Spec(ctx)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get sandbox container runtime spec: %w", err)
-		}
-		si.RuntimeSpec = spec
-
-		ctrInfo, err := container.Info(ctx)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get sandbox container info: %w", err)
-		}
-		// Do not use config.SandboxImage because the configuration might
-		// be changed during restart. It may not reflect the actual image
-		// used by the sandbox container.
-		si.Image = ctrInfo.Image
-		si.SnapshotKey = ctrInfo.SnapshotKey
-		si.Snapshotter = ctrInfo.Snapshotter
-
-		runtimeOptions, err := getRuntimeOptions(ctrInfo)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get runtime options: %w", err)
-		}
-		si.RuntimeType = ctrInfo.Runtime.Name
-		si.RuntimeOptions = runtimeOptions
-	}
-
-	if si.Status == "" {
-		// If processStatus is empty, it means that the task is deleted. Apply "deleted"
-		// status which does not exist in containerd.
-		si.Status = "deleted"
-	}
-
-	if sandbox.NetNS != nil {
-		// Add network closed information if sandbox is not using host network.
-		closed, err := sandbox.NetNS.Closed()
-		if err != nil {
-			return nil, fmt.Errorf("failed to check network namespace closed: %w", err)
-		}
-		si.NetNSClosed = closed
-	}
-
-	si.Metadata = &sandbox.Metadata
-
-	infoBytes, err := json.Marshal(si)
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal info %v: %w", si, err)
-	}
-	return map[string]string{
-		"info": string(infoBytes),
-	}, nil
-}

pkg/cri/sbserver/sandbox_status_test.go

@@ -88,29 +88,25 @@ func TestPodSandboxStatus(t *testing.T) {
 		RuntimeHandler: "test-runtime-handler",
 	}
 	for desc, test := range map[string]struct {
-		state         sandboxstore.State
+		state         string
 		expectedState runtime.PodSandboxState
 	}{
 		"sandbox state ready": {
-			state:         sandboxstore.StateReady,
+			state:         sandboxstore.StateReady.String(),
 			expectedState: runtime.PodSandboxState_SANDBOX_READY,
 		},
 		"sandbox state not ready": {
-			state:         sandboxstore.StateNotReady,
+			state:         sandboxstore.StateNotReady.String(),
 			expectedState: runtime.PodSandboxState_SANDBOX_NOTREADY,
 		},
 		"sandbox state unknown": {
-			state:         sandboxstore.StateUnknown,
+			state:         sandboxstore.StateUnknown.String(),
 			expectedState: runtime.PodSandboxState_SANDBOX_NOTREADY,
 		},
 	} {
 		t.Run(desc, func(t *testing.T) {
-			status := sandboxstore.Status{
-				CreatedAt: createdAt,
-				State:     test.state,
-			}
 			expected.State = test.expectedState
-			got := toCRISandboxStatus(metadata, status, ip, additionalIPs)
+			got := toCRISandboxStatus(metadata, test.state, createdAt, ip, additionalIPs)
 			assert.Equal(t, expected, got)
 		})
 	}