Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support pod and container updates #931

Merged
merged 1 commit into from
May 19, 2021
Merged

support pod and container updates #931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion cmd/containerd-shim-runhcs-v1/service_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
44 changes: 38 additions & 6 deletions cmd/containerd-shim-runhcs-v1/service_internal_podshim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
katiewasnothere marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand Down
45 changes: 39 additions & 6 deletions cmd/containerd-shim-runhcs-v1/service_internal_taskshim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
17 changes: 16 additions & 1 deletion cmd/containerd-shim-runhcs-v1/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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
Expand Down
133 changes: 114 additions & 19 deletions cmd/containerd-shim-runhcs-v1/task_hcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming ownsHost will only be true for LCOW if this is the pause container task, do we not support updating the pause container resources? It seems we won't ever call into updateTaskContainerResources in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we support updating the pause container resources? What would the scenario be?

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 {
ambarve marked this conversation as resolved.
Show resolved Hide resolved
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 {
kevpar marked this conversation as resolved.
Show resolved Hide resolved
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 {
katiewasnothere marked this conversation as resolved.
Show resolved Hide resolved
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 {
dcantah marked this conversation as resolved.
Show resolved Hide resolved
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{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using a guestrequest.GuestRequest for a hypervisor-isolated Windows container? Or does this request work for all Windows containers (process and hypervisor isolated)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hcsschema.ModifySettingRequest can be used to issue a modify request to a hypervisor isolated container. In particular, we need to provide the container resource path ResourcePath.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this works for both hypervisor and process isolated Windows container?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

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)
}
9 changes: 9 additions & 0 deletions cmd/containerd-shim-runhcs-v1/task_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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")
}
Expand Down
18 changes: 18 additions & 0 deletions cmd/containerd-shim-runhcs-v1/task_wcow_podsandbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
katiewasnothere marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
Loading