Merge pull request #931 from katiewasnothere/task_update_implementation

support pod and container updates

cmd/containerd-shim-runhcs-v1/service_internal.go

@@ -380,7 +380,17 @@ func (s *service) closeIOInternal(ctx context.Context, req *task.CloseIORequest)
 }
 
 func (s *service) updateInternal(ctx context.Context, req *task.UpdateTaskRequest) (*google_protobuf1.Empty, error) {
-	return nil, errdefs.ErrNotImplemented
+	if req.Resources == nil {
+		return nil, errors.Wrapf(errdefs.ErrInvalidArgument, "resources cannot be empty, updating container %s resources failed", req.ID)
+	}
+	t, err := s.getTask(req.ID)
+	if err != nil {
+		return nil, err
+	}
+	if err := t.Update(ctx, req); err != nil {
+		return nil, err
+	}
+	return empty, nil
 }
 
 func (s *service) waitInternal(ctx context.Context, req *task.WaitRequest) (*task.WaitResponse, error) {

cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/runtime/v2/task"
 	"github.com/containerd/typeurl"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
 func setupPodServiceWithFakes(t *testing.T) (*service, *testShimTask, *testShimTask, *testShimExec) {
@@ -573,15 +574,46 @@ func Test_PodShim_closeIOInternal_2ndTaskID_2ndExecID_Success(t *testing.T) {
 	}
 }
 
-func Test_PodShim_updateInternal_Error(t *testing.T) {
-	s := service{
-		tid:       t.Name(),
-		isSandbox: true,
+func Test_PodShim_updateInternal_Success(t *testing.T) {
+	s, t1, _, _ := setupPodServiceWithFakes(t)
+
+	var limit uint64 = 100
+	resources := &specs.WindowsResources{
+		Memory: &specs.WindowsMemoryResources{
+			Limit: &limit,
+		},
 	}
 
-	resp, err := s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t.Name()})
+	any, err := typeurl.MarshalAny(resources)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	verifyExpectedError(t, resp, err, errdefs.ErrNotImplemented)
+	resp, err := s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t1.ID(), Resources: any})
+	if err != nil {
+		t.Fatalf("should not have failed with error, got: %v", err)
+	}
+	if resp == nil {
+		t.Fatalf("should have returned an empty resp")
+	}
+}
+
+func Test_PodShim_updateInternal_Error(t *testing.T) {
+	s, t1, _, _ := setupPodServiceWithFakes(t)
+
+	// resources must be of type *WindowsResources or *LinuxResources
+	resources := &specs.Process{}
+	any, err := typeurl.MarshalAny(resources)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t1.ID(), Resources: any})
+	if err == nil {
+		t.Fatal("expected to get an error for incorrect resource's type")
+	}
+	if err != errNotSupportedResourcesRequest {
+		t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
+	}
 }
 
 func Test_PodShim_waitInternal_NoTask_Error(t *testing.T) {

cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/runtime/v2/task"
 	"github.com/containerd/typeurl"
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 func setupTaskServiceWithFakes(t *testing.T) (*service, *testShimTask, *testShimExec) {
@@ -493,15 +494,47 @@ func Test_TaskShim_closeIOInternal_InitTaskID_2ndExecID_Success(t *testing.T) {
 	}
 }
 
-func Test_TaskShim_updateInternal_Error(t *testing.T) {
-	s := service{
-		tid:       t.Name(),
-		isSandbox: true,
+func Test_TaskShim_updateInternal_Success(t *testing.T) {
+	s, t1, _ := setupTaskServiceWithFakes(t)
+
+	var limit uint64 = 100
+	resources := &specs.WindowsResources{
+		Memory: &specs.WindowsMemoryResources{
+			Limit: &limit,
+		},
 	}
 
-	resp, err := s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t.Name()})
+	any, err := typeurl.MarshalAny(resources)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	verifyExpectedError(t, resp, err, errdefs.ErrNotImplemented)
+	resp, err := s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t1.ID(), Resources: any})
+	if err != nil {
+		t.Fatalf("should not have failed with error, got: %v", err)
+	}
+	if resp == nil {
+		t.Fatalf("should have returned an empty resp")
+	}
+}
+
+func Test_TaskShim_updateInternal_Error(t *testing.T) {
+	s, t1, _ := setupTaskServiceWithFakes(t)
+
+	// resources must be of type *WindowsResources or *LinuxResources
+	resources := &specs.Process{}
+	any, err := typeurl.MarshalAny(resources)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = s.updateInternal(context.TODO(), &task.UpdateTaskRequest{ID: t1.ID(), Resources: any})
+	if err == nil {
+		t.Fatal("expected to get an error for incorrect resource's type")
+	}
+	if err != errNotSupportedResourcesRequest {
+		t.Fatalf("expected to get errNotSupportedResourcesRequest, instead got %v", err)
+	}
 }
 
 func Test_TaskShim_waitInternal_NoTask_Error(t *testing.T) {

cmd/containerd-shim-runhcs-v1/task.go

@@ -15,7 +15,10 @@ import (
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 )
 
-var errTaskNotIsolated = errors.New("task is not isolated")
+var (
+	errTaskNotIsolated              = errors.New("task is not isolated")
+	errNotSupportedResourcesRequest = errors.New("update resources must be of type *WindowsResources or *LinuxResources")
+)
 
 type shimTask interface {
 	// ID returns the original id used at `Create`.
@@ -86,6 +89,18 @@ type shimTask interface {
 	// If the host is hypervisor isolated and this task owns the host additional
 	// metrics on the UVM may be returned as well.
 	Stats(ctx context.Context) (*stats.Statistics, error)
+	// Update updates a task's container
+	Update(ctx context.Context, req *task.UpdateTaskRequest) error
+}
+
+func verifyTaskUpdateResourcesType(data interface{}) error {
+	switch data.(type) {
+	case *specs.WindowsResources:
+	case *specs.LinuxResources:
+	default:
+		return errNotSupportedResourcesRequest
+	}
+	return nil
 }
 
 // isStatsNotFound returns true if the err corresponds to a scenario

cmd/containerd-shim-runhcs-v1/task_hcs.go

@@ -23,18 +23,24 @@ import (
 	"github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/stats"
 	"github.com/Microsoft/hcsshim/internal/cmd"
 	"github.com/Microsoft/hcsshim/internal/cow"
+	"github.com/Microsoft/hcsshim/internal/guestrequest"
 	"github.com/Microsoft/hcsshim/internal/hcs"
+	"github.com/Microsoft/hcsshim/internal/hcs/resourcepaths"
 	"github.com/Microsoft/hcsshim/internal/hcs/schema1"
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
 	"github.com/Microsoft/hcsshim/internal/hcsoci"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/oci"
+	"github.com/Microsoft/hcsshim/internal/processorinfo"
+	"github.com/Microsoft/hcsshim/internal/requesttype"
 	"github.com/Microsoft/hcsshim/internal/resources"
 	"github.com/Microsoft/hcsshim/internal/shimdiag"
 	"github.com/Microsoft/hcsshim/internal/uvm"
 	"github.com/Microsoft/hcsshim/osversion"
 )
 
+const bytesPerMB = 1024 * 1024
+
 func newHcsStandaloneTask(ctx context.Context, events publisher, req *task.CreateTaskRequest, s *specs.Spec) (shimTask, error) {
 	log.G(ctx).WithField("tid", req.ID).Debug("newHcsStandaloneTask")
 
@@ -784,25 +790,7 @@ func (ht *hcsTask) Share(ctx context.Context, req *shimdiag.ShareRequest) error
 	if ht.host == nil {
 		return errTaskNotIsolated
 	}
-	// For hyper-v isolated WCOW the task used isn't the standard hcsTask so we
-	// only have to deal with the LCOW case here.
-	st, err := os.Stat(req.HostPath)
-	if err != nil {
-		return fmt.Errorf("could not open '%s' path on host: %s", req.HostPath, err)
-	}
-	var (
-		hostPath       string = req.HostPath
-		restrictAccess bool
-		fileName       string
-		allowedNames   []string
-	)
-	if !st.IsDir() {
-		hostPath, fileName = filepath.Split(hostPath)
-		allowedNames = append(allowedNames, fileName)
-		restrictAccess = true
-	}
-	_, err = ht.host.AddPlan9(ctx, hostPath, req.UvmPath, req.ReadOnly, restrictAccess, allowedNames)
-	return err
+	return ht.host.Share(ctx, req.HostPath, req.UvmPath, req.ReadOnly)
 }
 
 func hcsPropertiesToWindowsStats(props *hcsschema.Properties) *stats.Statistics_Windows {
@@ -859,3 +847,110 @@ func (ht *hcsTask) Stats(ctx context.Context) (*stats.Statistics, error) {
 	}
 	return s, nil
 }
+
+func (ht *hcsTask) Update(ctx context.Context, req *task.UpdateTaskRequest) error {
+	resources, err := typeurl.UnmarshalAny(req.Resources)
+	if err != nil {
+		return errors.Wrapf(err, "failed to unmarshal resources for container %s update request", req.ID)
+	}
+
+	if err := verifyTaskUpdateResourcesType(resources); err != nil {
+		return err
+	}
+
+	if ht.ownsHost && ht.host != nil {
+		return ht.host.UpdateConstraints(ctx, resources, req.Annotations)
+	}
+
+	return ht.updateTaskContainerResources(ctx, resources, req.Annotations)
+}
+
+func (ht *hcsTask) updateTaskContainerResources(ctx context.Context, data interface{}, annotations map[string]string) error {
+	if ht.isWCOW {
+		return ht.updateWCOWResources(ctx, data, annotations)
+	}
+
+	return ht.updateLCOWResources(ctx, data, annotations)
+}
+
+func (ht *hcsTask) updateWCOWContainerCPU(ctx context.Context, cpu *specs.WindowsCPUResources) error {
+	// if host is 20h2+ then we can make a request directly to hcs
+	if osversion.Get().Build >= osversion.V20H2 {
+		req := &hcsschema.Processor{}
+		if cpu.Count != nil {
+			procCount := int32(*cpu.Count)
+			hostProcs := processorinfo.ProcessorCount()
+			if ht.host != nil {
+				hostProcs = ht.host.ProcessorCount()
+			}
+			req.Count = hcsoci.NormalizeProcessorCount(ctx, ht.id, procCount, hostProcs)
+		}
+		if cpu.Maximum != nil {
+			req.Maximum = int32(*cpu.Maximum)
+		}
+		if cpu.Shares != nil {
+			req.Weight = int32(*cpu.Shares)
+		}
+		return ht.requestUpdateContainer(ctx, resourcepaths.SiloProcessorResourcePath, req)
+	}
+
+	return errdefs.ErrNotImplemented
+}
+
+func isValidWindowsCPUResources(c *specs.WindowsCPUResources) bool {
+	return (c.Count != nil && (c.Shares == nil && c.Maximum == nil)) ||
+		(c.Shares != nil && (c.Count == nil && c.Maximum == nil)) ||
+		(c.Maximum != nil && (c.Count == nil && c.Shares == nil))
+}
+
+func (ht *hcsTask) updateWCOWResources(ctx context.Context, data interface{}, annotations map[string]string) error {
+	resources, ok := data.(*specs.WindowsResources)
+	if !ok {
+		return errors.New("must have resources be type *WindowsResources when updating a wcow container")
+	}
+	if resources.Memory != nil && resources.Memory.Limit != nil {
+		newMemorySizeInMB := *resources.Memory.Limit / bytesPerMB
+		memoryLimit := hcsoci.NormalizeMemorySize(ctx, ht.id, newMemorySizeInMB)
+		if err := ht.requestUpdateContainer(ctx, resourcepaths.SiloMemoryResourcePath, memoryLimit); err != nil {
+			return err
+		}
+	}
+	if resources.CPU != nil {
+		if !isValidWindowsCPUResources(resources.CPU) {
+			return fmt.Errorf("invalid cpu resources request for container %s: %v", ht.id, resources.CPU)
+		}
+		if err := ht.updateWCOWContainerCPU(ctx, resources.CPU); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (ht *hcsTask) updateLCOWResources(ctx context.Context, data interface{}, annotations map[string]string) error {
+	resources, ok := data.(*specs.LinuxResources)
+	if !ok || resources == nil {
+		return errors.New("must have resources be non-nil and type *LinuxResources when updating a lcow container")
+	}
+	settings := guestrequest.LCOWContainerConstraints{
+		Linux: *resources,
+	}
+	return ht.requestUpdateContainer(ctx, "", settings)
+}
+
+func (ht *hcsTask) requestUpdateContainer(ctx context.Context, resourcePath string, settings interface{}) error {
+	var modification interface{}
+	if ht.isWCOW {
+		modification = &hcsschema.ModifySettingRequest{
+			ResourcePath: resourcePath,
+			RequestType:  requesttype.Update,
+			Settings:     settings,
+		}
+	} else {
+		modification = guestrequest.GuestRequest{
+			ResourceType: guestrequest.ResourceTypeContainerConstraints,
+			RequestType:  requesttype.Update,
+			Settings:     settings,
+		}
+	}
+	return ht.c.Modify(ctx, modification)
+}

cmd/containerd-shim-runhcs-v1/task_test.go

@@ -10,6 +10,7 @@ import (
 	v1 "github.com/containerd/cgroups/stats/v1"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/runtime/v2/task"
+	"github.com/containerd/typeurl"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 )
@@ -91,6 +92,14 @@ func (tst *testShimTask) DumpGuestStacks(ctx context.Context) string {
 	return ""
 }
 
+func (tst *testShimTask) Update(ctx context.Context, req *task.UpdateTaskRequest) error {
+	data, err := typeurl.UnmarshalAny(req.Resources)
+	if err != nil {
+		return errors.Wrapf(err, "failed to unmarshal resources for container %s update request", req.ID)
+	}
+	return verifyTaskUpdateResourcesType(data)
+}
+
 func (tst *testShimTask) Share(ctx context.Context, req *shimdiag.ShareRequest) error {
 	return errors.New("not implemented")
 }

cmd/containerd-shim-runhcs-v1/task_wcow_podsandbox.go

@@ -16,6 +16,7 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/runtime"
 	"github.com/containerd/containerd/runtime/v2/task"
+	"github.com/containerd/typeurl"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/pkg/errors"
 	"go.opencensus.io/trace"
@@ -257,6 +258,23 @@ func (wpst *wcowPodSandboxTask) DumpGuestStacks(ctx context.Context) string {
 	return ""
 }
 
+func (wpst *wcowPodSandboxTask) Update(ctx context.Context, req *task.UpdateTaskRequest) error {
+	if wpst.host == nil {
+		return errTaskNotIsolated
+	}
+
+	resources, err := typeurl.UnmarshalAny(req.Resources)
+	if err != nil {
+		return errors.Wrapf(err, "failed to unmarshal resources for container %s update request", req.ID)
+	}
+
+	if err := verifyTaskUpdateResourcesType(resources); err != nil {
+		return err
+	}
+
+	return wpst.host.UpdateConstraints(ctx, resources, req.Annotations)
+}
+
 func (wpst *wcowPodSandboxTask) Share(ctx context.Context, req *shimdiag.ShareRequest) error {
 	if wpst.host == nil {
 		return errTaskNotIsolated