[sandbox] Address PR review comments

Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>

api/next.pb.txt

@@ -22,7 +22,7 @@ file {
     java_package: "com.google.protobuf"
     java_outer_classname: "AnyProto"
     java_multiple_files: true
-    go_package: "google.golang.org/protobuf/types/known/anypb"
+    go_package: "github.com/golang/protobuf/ptypes/any"
     objc_class_prefix: "GPB"
     csharp_namespace: "Google.Protobuf.WellKnownTypes"
   }
@@ -458,7 +458,7 @@ file {
     java_package: "com.google.protobuf"
     java_outer_classname: "TimestampProto"
     java_multiple_files: true
-    go_package: "google.golang.org/protobuf/types/known/timestamppb"
+    go_package: "github.com/golang/protobuf/ptypes/timestamp"
     cc_enable_arenas: true
     objc_class_prefix: "GPB"
     csharp_namespace: "Google.Protobuf.WellKnownTypes"
@@ -800,7 +800,7 @@ file {
     java_package: "com.google.protobuf"
     java_outer_classname: "EmptyProto"
     java_multiple_files: true
-    go_package: "google.golang.org/protobuf/types/known/emptypb"
+    go_package: "github.com/golang/protobuf/ptypes/empty"
     cc_enable_arenas: true
     objc_class_prefix: "GPB"
     csharp_namespace: "Google.Protobuf.WellKnownTypes"
@@ -824,7 +824,7 @@ file {
     java_package: "com.google.protobuf"
     java_outer_classname: "FieldMaskProto"
     java_multiple_files: true
-    go_package: "google.golang.org/protobuf/types/known/fieldmaskpb"
+    go_package: "google.golang.org/genproto/protobuf/field_mask;field_mask"
     cc_enable_arenas: true
     objc_class_prefix: "GPB"
     csharp_namespace: "Google.Protobuf.WellKnownTypes"

api/types/sandbox.pb.go

@@ -30,14 +30,14 @@ var _ = time.Kitchen
 const _ = proto.GoGoProtoPackageIsVersion3 // please upgrade the proto package
 
 // Sandbox represents a sandbox metadata object that keeps all info required by controller to
-// work with a particular instance. containerd keeps this in metadata store and sends it to a controller with
+// work with a particular instance.
-// each request, so they can reflect sandbox state changes (by updating the corresponding fields).
 type Sandbox struct {
 	// SandboxID is a unique instance identifier within namespace
 	SandboxID string `protobuf:"bytes,1,opt,name=sandbox_id,json=sandboxId,proto3" json:"sandbox_id,omitempty"`
 	// Runtime specifies which runtime to use for executing this container.
 	Runtime Sandbox_Runtime `protobuf:"bytes,2,opt,name=runtime,proto3" json:"runtime"`
-	// Spec is sandbox configuration (kin of OCI runtime spec, but for VM)
+	// Spec is sandbox configuration (kin of OCI runtime spec), spec's data will be written to a config.json file in the
+	// bundle directory (similary to OCI spec).
 	Spec *types.Any `protobuf:"bytes,3,opt,name=spec,proto3" json:"spec,omitempty"`
 	// Labels provides an area to include arbitrary data on containers.
 	Labels map[string]string `protobuf:"bytes,4,rep,name=labels,proto3" json:"labels" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
@@ -87,7 +87,8 @@ var xxx_messageInfo_Sandbox proto.InternalMessageInfo
 type Sandbox_Runtime struct {
 	// Name is the name of the runtime.
 	Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
-	// Options specify additional runtime initialization options.
+	// Options specify additional runtime initialization options for the shim (this data will be available in StartShim).
+	// Typically this data expected to be runtime shim implementation specific.
 	Options              *types.Any `protobuf:"bytes,2,opt,name=options,proto3" json:"options,omitempty"`
 	XXX_NoUnkeyedLiteral struct{}   `json:"-"`
 	XXX_unrecognized     []byte     `json:"-"`

api/types/sandbox.proto

@@ -25,20 +25,21 @@ import "google/protobuf/timestamp.proto";
 option go_package = "github.com/containerd/containerd/api/types;types";
 
 // Sandbox represents a sandbox metadata object that keeps all info required by controller to
-// work with a particular instance. containerd keeps this in metadata store and sends it to a controller with
+// work with a particular instance.
-// each request, so they can reflect sandbox state changes (by updating the corresponding fields).
 message Sandbox {
 	// SandboxID is a unique instance identifier within namespace
 	string sandbox_id = 1;
 	message Runtime {
 		// Name is the name of the runtime.
 		string name = 1;
-		// Options specify additional runtime initialization options.
+		// Options specify additional runtime initialization options for the shim (this data will be available in StartShim).
+		// Typically this data expected to be runtime shim implementation specific.
 		google.protobuf.Any options = 2;
 	}
 	// Runtime specifies which runtime to use for executing this container.
 	Runtime runtime = 2 [(gogoproto.nullable) = false];
-	// Spec is sandbox configuration (kin of OCI runtime spec, but for VM)
+	// Spec is sandbox configuration (kin of OCI runtime spec), spec's data will be written to a config.json file in the
+	// bundle directory (similary to OCI spec).
 	google.protobuf.Any spec = 3;
 	// Labels provides an area to include arbitrary data on containers.
 	map<string, string> labels  = 4 [(gogoproto.nullable) = false];

metadata/boltutil/helpers.go

@@ -213,11 +213,11 @@ func WriteAny(bkt *bolt.Bucket, name []byte, any typeurl.Any) error {
 
 	data, err := proto.Marshal(pbany)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to marshal: %w", err)
 	}
 
 	if err := bkt.Put(name, data); err != nil {
-		return err
+		return fmt.Errorf("put failed: %w", err)
 	}
 
 	return nil

metadata/sandbox.go

@@ -18,11 +18,13 @@ package metadata
 
 import (
 	"context"
+	"fmt"
 	"strings"
 	"time"
 
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/filters"
+	"github.com/containerd/containerd/identifiers"
 	"github.com/containerd/containerd/metadata/boltutil"
 	"github.com/containerd/containerd/namespaces"
 	api "github.com/containerd/containerd/sandbox"
@@ -59,11 +61,11 @@ func (s *sandboxStore) Create(ctx context.Context, sandbox api.Sandbox) (api.San
 	if err := s.db.Update(func(tx *bbolt.Tx) error {
 		parent, err := createSandboxBucket(tx, ns)
 		if err != nil {
-			return err
+			return fmt.Errorf("create error: %w", err)
 		}
 
 		if err := s.write(parent, &sandbox, false); err != nil {
-			return err
+			return fmt.Errorf("write error: %w", err)
 		}
 
 		return nil
@@ -200,7 +202,7 @@ func (s *sandboxStore) List(ctx context.Context, fields ...string) ([]api.Sandbo
 	if err := view(ctx, s.db, func(tx *bbolt.Tx) error {
 		bucket := getSandboxBucket(tx, ns)
 		if bucket == nil {
-			return errors.Wrap(errdefs.ErrNotFound, "not sandbox buckets")
+			return errors.Wrap(errdefs.ErrNotFound, "no sandbox buckets")
 		}
 
 		if err := bucket.ForEach(func(k, v []byte) error {
@@ -354,8 +356,8 @@ func (s *sandboxStore) read(parent *bbolt.Bucket, id []byte) (api.Sandbox, error
 }
 
 func (s *sandboxStore) validate(new *api.Sandbox) error {
-	if new.ID == "" {
+	if err := identifiers.Validate(new.ID); err != nil {
-		return errors.Wrap(errdefs.ErrInvalidArgument, "instance ID must not be empty")
+		return errors.Wrap(err, "invalid sandbox ID")
 	}
 
 	if new.CreatedAt.IsZero() {

sandbox.go

@@ -44,6 +44,8 @@ type Sandbox interface {
 	Resume(ctx context.Context) error
 	// Status will return current sandbox status (provided by shim runtime)
 	Status(ctx context.Context, status interface{}) error
+	// Ping will check whether existing sandbox instance alive
+	Ping(ctx context.Context) error
 }
 
 type sandboxClient struct {

sandbox/controller.go

@@ -37,7 +37,7 @@ import (
 // 	1. Client calls `client.Controller.Start()` to launch new shim and create sandbox process
 // 	2. Run containers with `shim.TaskService.RunContainer(id=1)` and another one `shim.TaskService.RunContainer(id=2)`
 //	3. ... usual container lifecycle calls to `shim.TaskService`
-// 	4. Client calls shim to stop the VM with `client.SandboxService.Shutdown()`
+// 	4. Client calls shim to stop the sandbox with `client.SandboxService.Shutdown()`
 //  5. Shim implementation will perform cleanup similar to regular task service (e.g. shutdown, clean, and `shim_binary --delete`)
 type Controller interface {
 	// Start will start new sandbox instance.
@@ -52,9 +52,10 @@ type Controller interface {
 	Pause(ctx context.Context, sandboxID string) error
 	// Resume will unfreeze previously paused sandbox instance
 	Resume(ctx context.Context, sandboxID string) error
-	// Ping is a lightweight API call to check whether sandbox instance is still alive.
+	// Ping is a lightweight API call to check whether sandbox instance is still alive (e.g. quick livability check).
 	// This should not involve any complex logic and containerd will not debug log it as it might be called quite often.
 	Ping(ctx context.Context, sandboxID string) error
-	// Status will query sandbox process status
+	// Status will query sandbox process status. It is heavier than Ping call and must be used whenever you need to
+	// gather metadata about current sandbox state (status, uptime, resource use, etc).
 	Status(ctx context.Context, sandboxID string) (*types.Any, error)
 }

sandbox/helpers.go

@@ -32,6 +32,7 @@ func ToProto(s *Sandbox) types.Sandbox {
 		CreatedAt:  s.CreatedAt,
 		UpdatedAt:  s.UpdatedAt,
 		Extensions: s.Extensions,
+		Spec:       s.Spec,
 	}
 }
 

services/sandbox/controller_local.go

@@ -1,9 +1,12 @@
 /*
    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.

services/sandbox/controller_service.go

@@ -1,9 +1,12 @@
 /*
    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.
@@ -27,7 +30,7 @@ import (
 func init() {
 	plugin.Register(&plugin.Registration{
 		Type: plugin.GRPCPlugin,
-		ID:   "controllers",
+		ID:   "sandbox-controllers",
 		Requires: []plugin.Type{
 			plugin.ServicePlugin,
 		},