From e3be90595643c86dc8b6071cc1f92e6a6bd90fff Mon Sep 17 00:00:00 2001 From: Maksim An Date: Fri, 15 Apr 2022 01:37:35 -0700 Subject: [PATCH 1/7] Allow multiple CreateContainer operations at the same time. Prior to this change, GCS allowed only one CreateContainer operation at a time. This isn't an issue in general case, however this doesn't work properly with synchronization via OCI runtime hook. Synchronization via runtime hook was introduced in: https://github.com/microsoft/hcsshim/pull/1258 It injects a CreateRuntime OCI hook, if security policy provides wait paths. This allows container-A to run after container-B, where container-B writes to an empty directory volume shared between the two containers to signal that it's done some setup container-A depends on. In general case, container-A can be started before container-B which results in a deadlock, because CreateContainer request holds a lock to a map, which keeps track of running containers. To resolve the issue, the code has been updated to do a more granular locking when reading/updating the containers map: - Add a new "Status" field to Container object, which can be either "Running" or "Creating". - Remove locking from CreateContainer function - Rework "GetContainer" to "GetRunningContainer", which returns the container object only when it's in "Running" state, otherwise either `gcserr.HrVmcomputeSystemNotFound` or `gcserr.HrVmcomputeInvalidState` error returned. - Add new "AddContainer(id, container)" function, which updates the containers map with new container instances. - Rework CreateContainer to initially add new container objects into the containers map and set the "Status" to "Creating" at the start of the function and set it to "Running" only when the container successfully starts. Reworking "GetContainer" to "GetRunningContainer" seemed to be the least invasive change, which allows us to limit updates in the affected places. If "GetContainer" is left unchanged, then handling of containers in status "Creating" needs to take place and this requires handling cases when (e.g.) a modification request is sent to a container which isn't yet running. Signed-off-by: Maksim An --- internal/guest/bridge/bridge_v2.go | 14 ++--- internal/guest/gcserr/errors.go | 8 +-- internal/guest/runtime/hcsv2/container.go | 9 ++++ internal/guest/runtime/hcsv2/uvm.go | 62 +++++++++++++++-------- test/cri-containerd/policy_test.go | 8 +-- 5 files changed, 66 insertions(+), 35 deletions(-) diff --git a/internal/guest/bridge/bridge_v2.go b/internal/guest/bridge/bridge_v2.go index f7924a2576..c0b9b55546 100644 --- a/internal/guest/bridge/bridge_v2.go +++ b/internal/guest/bridge/bridge_v2.go @@ -199,7 +199,7 @@ func (b *Bridge) execProcessV2(r *Request) (_ RequestResponse, err error) { var c *hcsv2.Container if params.IsExternal || request.ContainerID == hcsv2.UVMContainerID { pid, err = b.hostState.RunExternalProcess(ctx, params, conSettings) - } else if c, err = b.hostState.GetContainer(request.ContainerID); err == nil { + } else if c, err = b.hostState.GetRunningContainer(request.ContainerID); err == nil { // We found a V2 container. Treat this as a V2 process. if params.OCIProcess == nil { pid, err = c.Start(ctx, conSettings) @@ -267,7 +267,7 @@ func (b *Bridge) signalContainerV2(ctx context.Context, span *trace.Span, r *Req b.quitChan <- true b.hostState.Shutdown() } else { - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } @@ -296,7 +296,7 @@ func (b *Bridge) signalProcessV2(r *Request) (_ RequestResponse, err error) { trace.Int64Attribute("pid", int64(request.ProcessID)), trace.Int64Attribute("signal", int64(request.Options.Signal))) - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } @@ -344,7 +344,7 @@ func (b *Bridge) getPropertiesV2(r *Request) (_ RequestResponse, err error) { return nil, errors.New("getPropertiesV2 is not supported against the UVM") } - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } @@ -407,7 +407,7 @@ func (b *Bridge) waitOnProcessV2(r *Request) (_ RequestResponse, err error) { } exitCodeChan, doneChan = p.Wait() } else { - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } @@ -453,7 +453,7 @@ func (b *Bridge) resizeConsoleV2(r *Request) (_ RequestResponse, err error) { trace.Int64Attribute("height", int64(request.Height)), trace.Int64Attribute("width", int64(request.Width))) - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } @@ -514,7 +514,7 @@ func (b *Bridge) deleteContainerStateV2(r *Request) (_ RequestResponse, err erro return nil, errors.Wrapf(err, "failed to unmarshal JSON in message \"%s\"", r.Message) } - c, err := b.hostState.GetContainer(request.ContainerID) + c, err := b.hostState.GetRunningContainer(request.ContainerID) if err != nil { return nil, err } diff --git a/internal/guest/gcserr/errors.go b/internal/guest/gcserr/errors.go index 0e13e21ef1..b042776800 100644 --- a/internal/guest/gcserr/errors.go +++ b/internal/guest/gcserr/errors.go @@ -87,14 +87,14 @@ func BaseStackTrace(e error) errors.StackTrace { return tracer.StackTrace() } -type baseHresultError struct { +type BaseHresultError struct { hresult Hresult } -func (e *baseHresultError) Error() string { +func (e *BaseHresultError) Error() string { return fmt.Sprintf("HRESULT: 0x%x", uint32(e.Hresult())) } -func (e *baseHresultError) Hresult() Hresult { +func (e *BaseHresultError) Hresult() Hresult { return e.hresult } @@ -139,7 +139,7 @@ func (e *wrappingHresultError) StackTrace() errors.StackTrace { // NewHresultError produces a new error with the given HRESULT. func NewHresultError(hresult Hresult) error { - return &baseHresultError{hresult: hresult} + return &BaseHresultError{hresult: hresult} } // WrapHresult produces a new error with the given HRESULT and wrapping the diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index d890d0c2b0..3968cd080f 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -28,6 +28,13 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) +type containerStatus int + +const ( + containerCreating containerStatus = iota + containerRunning +) + type Container struct { id string vsock transport.Transport @@ -43,6 +50,8 @@ type Container struct { processesMutex sync.Mutex processes map[uint32]*containerProcess + + Status containerStatus } func (c *Container) Start(ctx context.Context, conSettings stdio.ConnectionSettings) (int, error) { diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 568f0ed448..6a50456651 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -123,19 +123,29 @@ func (h *Host) RemoveContainer(id string) { delete(h.containers, id) } -func (h *Host) getContainerLocked(id string) (*Container, error) { +func (h *Host) GetRunningContainer(id string) (*Container, error) { + h.containersMutex.Lock() + defer h.containersMutex.Unlock() + if c, ok := h.containers[id]; !ok { return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound) } else { + if c.Status != containerRunning { + return nil, gcserr.NewHresultError(gcserr.HrVmcomputeInvalidState) + } return c, nil } } -func (h *Host) GetContainer(id string) (*Container, error) { +func (h *Host) AddContainer(id string, c *Container) error { h.containersMutex.Lock() defer h.containersMutex.Unlock() - return h.getContainerLocked(id) + if _, ok := h.containers[id]; ok { + return gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists) + } + h.containers[id] = c + return nil } func setupSandboxMountsPath(id string) (err error) { @@ -162,11 +172,13 @@ func setupSandboxHugePageMountsPath(id string) error { } func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VMHostedContainerSettingsV2) (_ *Container, err error) { - h.containersMutex.Lock() - defer h.containersMutex.Unlock() - - if _, ok := h.containers[id]; ok { + if _, err := h.GetRunningContainer(id); err == nil { return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists) + } else { + herr := err.(*gcserr.BaseHresultError) + if herr.Hresult() == gcserr.HrVmcomputeInvalidState { + return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists) + } } err = h.securityPolicyEnforcer.EnforceCreateContainerPolicy( @@ -180,8 +192,26 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM return nil, errors.Wrapf(err, "container creation denied due to policy") } - var namespaceID string criType, isCRI := settings.OCISpecification.Annotations[annotations.KubernetesContainerType] + c := &Container{ + id: id, + vsock: h.vsock, + spec: settings.OCISpecification, + isSandbox: criType == "sandbox", + exitType: prot.NtUnexpectedExit, + processes: make(map[uint32]*containerProcess), + Status: containerCreating, + } + if err := h.AddContainer(id, c); err != nil { + return nil, err + } + defer func() { + if err != nil { + h.RemoveContainer(id) + } + }() + + var namespaceID string // for sandbox container sandboxID is same as container id sandboxID := id if isCRI { @@ -290,15 +320,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM return nil, errors.Wrapf(err, "failed to get container init process") } - c := &Container{ - id: id, - vsock: h.vsock, - spec: settings.OCISpecification, - isSandbox: criType == "sandbox", - container: con, - exitType: prot.NtUnexpectedExit, - processes: make(map[uint32]*containerProcess), - } + c.container = con c.initProcess = newProcess(c, settings.OCISpecification.Process, init, uint32(c.container.Pid()), true) // Sandbox or standalone, move the networks to the container namespace @@ -318,7 +340,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - h.containers[id] = c + c.Status = containerRunning return c, nil } @@ -337,7 +359,7 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * case guestresource.ResourceTypeVPCIDevice: return modifyMappedVPCIDevice(ctx, req.RequestType, req.Settings.(*guestresource.LCOWMappedVPCIDevice)) case guestresource.ResourceTypeContainerConstraints: - c, err := h.GetContainer(containerID) + c, err := h.GetRunningContainer(containerID) if err != nil { return err } @@ -355,7 +377,7 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * } func (h *Host) modifyContainerSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) error { - c, err := h.GetContainer(containerID) + c, err := h.GetRunningContainer(containerID) if err != nil { return err } diff --git a/test/cri-containerd/policy_test.go b/test/cri-containerd/policy_test.go index 9d1348218d..332e107e4d 100644 --- a/test/cri-containerd/policy_test.go +++ b/test/cri-containerd/policy_test.go @@ -232,13 +232,13 @@ func Test_RunContainers_WithSyncHooks_ValidWaitPath(t *testing.T) { cidWriter := createContainer(t, client, ctx, writerReq) cidWaiter := createContainer(t, client, ctx, waiterReq) - startContainer(t, client, ctx, cidWriter) - defer removeContainer(t, client, ctx, cidWriter) - defer stopContainer(t, client, ctx, cidWriter) - startContainer(t, client, ctx, cidWaiter) defer removeContainer(t, client, ctx, cidWaiter) defer stopContainer(t, client, ctx, cidWaiter) + + startContainer(t, client, ctx, cidWriter) + defer removeContainer(t, client, ctx, cidWriter) + defer stopContainer(t, client, ctx, cidWriter) } func Test_RunContainers_WithSyncHooks_InvalidWaitPath(t *testing.T) { From b7cdeacc5099d09484f99af296d0fa5a08d32e87 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 18 Apr 2022 12:53:38 -0700 Subject: [PATCH 2/7] Move AddContainer to top to address potential race condition. Signed-off-by: Maksim An --- internal/guest/gcserr/errors.go | 8 ++++---- internal/guest/runtime/hcsv2/uvm.go | 31 ++++++++++------------------- 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/internal/guest/gcserr/errors.go b/internal/guest/gcserr/errors.go index b042776800..0e13e21ef1 100644 --- a/internal/guest/gcserr/errors.go +++ b/internal/guest/gcserr/errors.go @@ -87,14 +87,14 @@ func BaseStackTrace(e error) errors.StackTrace { return tracer.StackTrace() } -type BaseHresultError struct { +type baseHresultError struct { hresult Hresult } -func (e *BaseHresultError) Error() string { +func (e *baseHresultError) Error() string { return fmt.Sprintf("HRESULT: 0x%x", uint32(e.Hresult())) } -func (e *BaseHresultError) Hresult() Hresult { +func (e *baseHresultError) Hresult() Hresult { return e.hresult } @@ -139,7 +139,7 @@ func (e *wrappingHresultError) StackTrace() errors.StackTrace { // NewHresultError produces a new error with the given HRESULT. func NewHresultError(hresult Hresult) error { - return &BaseHresultError{hresult: hresult} + return &baseHresultError{hresult: hresult} } // WrapHresult produces a new error with the given HRESULT and wrapping the diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 6a50456651..8039470658 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -172,26 +172,6 @@ func setupSandboxHugePageMountsPath(id string) error { } func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VMHostedContainerSettingsV2) (_ *Container, err error) { - if _, err := h.GetRunningContainer(id); err == nil { - return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists) - } else { - herr := err.(*gcserr.BaseHresultError) - if herr.Hresult() == gcserr.HrVmcomputeInvalidState { - return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemAlreadyExists) - } - } - - err = h.securityPolicyEnforcer.EnforceCreateContainerPolicy( - id, - settings.OCISpecification.Process.Args, - settings.OCISpecification.Process.Env, - settings.OCISpecification.Process.Cwd, - ) - - if err != nil { - return nil, errors.Wrapf(err, "container creation denied due to policy") - } - criType, isCRI := settings.OCISpecification.Annotations[annotations.KubernetesContainerType] c := &Container{ id: id, @@ -202,6 +182,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM processes: make(map[uint32]*containerProcess), Status: containerCreating, } + if err := h.AddContainer(id, c); err != nil { return nil, err } @@ -211,6 +192,16 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } }() + err = h.securityPolicyEnforcer.EnforceCreateContainerPolicy( + id, + settings.OCISpecification.Process.Args, + settings.OCISpecification.Process.Env, + settings.OCISpecification.Process.Cwd, + ) + if err != nil { + return nil, errors.Wrapf(err, "container creation denied due to policy") + } + var namespaceID string // for sandbox container sandboxID is same as container id sandboxID := id From bf0724d9a60d42e37639fc1be518b6121e0c258c Mon Sep 17 00:00:00 2001 From: Maksim An Date: Mon, 18 Apr 2022 16:54:01 -0700 Subject: [PATCH 3/7] pr feedback Signed-off-by: Maksim An --- internal/guest/runtime/hcsv2/container.go | 4 ++-- internal/guest/runtime/hcsv2/uvm.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index 3968cd080f..c156b84e26 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -28,7 +28,7 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) -type containerStatus int +type containerStatus uint8 const ( containerCreating containerStatus = iota @@ -51,7 +51,7 @@ type Container struct { processesMutex sync.Mutex processes map[uint32]*containerProcess - Status containerStatus + status containerStatus } func (c *Container) Start(ctx context.Context, conSettings stdio.ConnectionSettings) (int, error) { diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 8039470658..ff7256ee52 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -130,8 +130,9 @@ func (h *Host) GetRunningContainer(id string) (*Container, error) { if c, ok := h.containers[id]; !ok { return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound) } else { - if c.Status != containerRunning { - return nil, gcserr.NewHresultError(gcserr.HrVmcomputeInvalidState) + if c.status != containerRunning { + return nil, fmt.Errorf("container is not in state running: %w", + gcserr.NewHresultError(gcserr.HrVmcomputeInvalidState)) } return c, nil } @@ -180,7 +181,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM isSandbox: criType == "sandbox", exitType: prot.NtUnexpectedExit, processes: make(map[uint32]*containerProcess), - Status: containerCreating, + status: containerCreating, } if err := h.AddContainer(id, c); err != nil { @@ -331,7 +332,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - c.Status = containerRunning + c.status = containerRunning return c, nil } From 3b02532555aa145c58b74cb06384f21a315e825d Mon Sep 17 00:00:00 2001 From: Maksim An Date: Wed, 20 Apr 2022 10:21:43 -0700 Subject: [PATCH 4/7] pr feedback: change containerRunning to containerCreated Additionally update synchronization CRI tests to use go routines to properly reproduce the scenario. Signed-off-by: Maksim An --- internal/guest/bridge/bridge_v2.go | 14 +++++----- internal/guest/runtime/hcsv2/container.go | 7 ++++- internal/guest/runtime/hcsv2/uvm.go | 19 +++++++------- test/cri-containerd/policy_test.go | 32 ++++++++++++++++++----- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/internal/guest/bridge/bridge_v2.go b/internal/guest/bridge/bridge_v2.go index c0b9b55546..99777ca84a 100644 --- a/internal/guest/bridge/bridge_v2.go +++ b/internal/guest/bridge/bridge_v2.go @@ -199,7 +199,7 @@ func (b *Bridge) execProcessV2(r *Request) (_ RequestResponse, err error) { var c *hcsv2.Container if params.IsExternal || request.ContainerID == hcsv2.UVMContainerID { pid, err = b.hostState.RunExternalProcess(ctx, params, conSettings) - } else if c, err = b.hostState.GetRunningContainer(request.ContainerID); err == nil { + } else if c, err = b.hostState.GetCreatedContainer(request.ContainerID); err == nil { // We found a V2 container. Treat this as a V2 process. if params.OCIProcess == nil { pid, err = c.Start(ctx, conSettings) @@ -267,7 +267,7 @@ func (b *Bridge) signalContainerV2(ctx context.Context, span *trace.Span, r *Req b.quitChan <- true b.hostState.Shutdown() } else { - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } @@ -296,7 +296,7 @@ func (b *Bridge) signalProcessV2(r *Request) (_ RequestResponse, err error) { trace.Int64Attribute("pid", int64(request.ProcessID)), trace.Int64Attribute("signal", int64(request.Options.Signal))) - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } @@ -344,7 +344,7 @@ func (b *Bridge) getPropertiesV2(r *Request) (_ RequestResponse, err error) { return nil, errors.New("getPropertiesV2 is not supported against the UVM") } - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } @@ -407,7 +407,7 @@ func (b *Bridge) waitOnProcessV2(r *Request) (_ RequestResponse, err error) { } exitCodeChan, doneChan = p.Wait() } else { - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } @@ -453,7 +453,7 @@ func (b *Bridge) resizeConsoleV2(r *Request) (_ RequestResponse, err error) { trace.Int64Attribute("height", int64(request.Height)), trace.Int64Attribute("width", int64(request.Width))) - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } @@ -514,7 +514,7 @@ func (b *Bridge) deleteContainerStateV2(r *Request) (_ RequestResponse, err erro return nil, errors.Wrapf(err, "failed to unmarshal JSON in message \"%s\"", r.Message) } - c, err := b.hostState.GetRunningContainer(request.ContainerID) + c, err := b.hostState.GetCreatedContainer(request.ContainerID) if err != nil { return nil, err } diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index c156b84e26..96d1a13548 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -28,11 +28,16 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) +// containerStatus has been introduced to enable parallel container creation type containerStatus uint8 const ( + // containerCreating is the default status set on a Container object, when + // no underlying runtime container or init process has been assigned containerCreating containerStatus = iota - containerRunning + // containerCreated is the status when a runtime container and init process + // have been assigned, but runtime start command has not been issued yet + containerCreated ) type Container struct { diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index ff7256ee52..30209e7b91 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -16,11 +16,8 @@ import ( "syscall" "time" - "github.com/Microsoft/hcsshim/internal/guest/policy" - "github.com/mattn/go-shellwords" - "github.com/pkg/errors" - "github.com/Microsoft/hcsshim/internal/guest/gcserr" + "github.com/Microsoft/hcsshim/internal/guest/policy" "github.com/Microsoft/hcsshim/internal/guest/prot" "github.com/Microsoft/hcsshim/internal/guest/runtime" "github.com/Microsoft/hcsshim/internal/guest/spec" @@ -36,6 +33,8 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" "github.com/Microsoft/hcsshim/pkg/annotations" "github.com/Microsoft/hcsshim/pkg/securitypolicy" + "github.com/mattn/go-shellwords" + "github.com/pkg/errors" ) // UVMContainerID is the ContainerID that will be sent on any prot.MessageBase @@ -123,15 +122,15 @@ func (h *Host) RemoveContainer(id string) { delete(h.containers, id) } -func (h *Host) GetRunningContainer(id string) (*Container, error) { +func (h *Host) GetCreatedContainer(id string) (*Container, error) { h.containersMutex.Lock() defer h.containersMutex.Unlock() if c, ok := h.containers[id]; !ok { return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound) } else { - if c.status != containerRunning { - return nil, fmt.Errorf("container is not in state running: %w", + if c.status != containerCreated { + return nil, fmt.Errorf("container is not in state \"created\": %w", gcserr.NewHresultError(gcserr.HrVmcomputeInvalidState)) } return c, nil @@ -332,7 +331,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - c.status = containerRunning + c.status = containerCreated return c, nil } @@ -351,7 +350,7 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * case guestresource.ResourceTypeVPCIDevice: return modifyMappedVPCIDevice(ctx, req.RequestType, req.Settings.(*guestresource.LCOWMappedVPCIDevice)) case guestresource.ResourceTypeContainerConstraints: - c, err := h.GetRunningContainer(containerID) + c, err := h.GetCreatedContainer(containerID) if err != nil { return err } @@ -369,7 +368,7 @@ func (h *Host) modifyHostSettings(ctx context.Context, containerID string, req * } func (h *Host) modifyContainerSettings(ctx context.Context, containerID string, req *guestrequest.ModificationRequest) error { - c, err := h.GetRunningContainer(containerID) + c, err := h.GetCreatedContainer(containerID) if err != nil { return err } diff --git a/test/cri-containerd/policy_test.go b/test/cri-containerd/policy_test.go index 332e107e4d..2892c774e4 100644 --- a/test/cri-containerd/policy_test.go +++ b/test/cri-containerd/policy_test.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" "testing" + "time" runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2" @@ -232,13 +233,30 @@ func Test_RunContainers_WithSyncHooks_ValidWaitPath(t *testing.T) { cidWriter := createContainer(t, client, ctx, writerReq) cidWaiter := createContainer(t, client, ctx, waiterReq) - startContainer(t, client, ctx, cidWaiter) - defer removeContainer(t, client, ctx, cidWaiter) - defer stopContainer(t, client, ctx, cidWaiter) - - startContainer(t, client, ctx, cidWriter) - defer removeContainer(t, client, ctx, cidWriter) - defer stopContainer(t, client, ctx, cidWriter) + errChan := make(chan error) + go func() { + _, err := client.StartContainer(ctx, &runtime.StartContainerRequest{ContainerId: cidWaiter}) + errChan <- err + defer removeContainer(t, client, ctx, cidWaiter) + defer stopContainer(t, client, ctx, cidWaiter) + }() + + // give some time for the first go routine to kick in. + time.Sleep(time.Second) + + go func() { + _, err := client.StartContainer(ctx, &runtime.StartContainerRequest{ContainerId: cidWriter}) + errChan <- err + defer removeContainer(t, client, ctx, cidWriter) + defer stopContainer(t, client, ctx, cidWriter) + }() + + for i := 0; i < 2; i++ { + if err := <-errChan; err != nil { + close(errChan) + t.Fatalf("failed to start container: %s", err) + } + } } func Test_RunContainers_WithSyncHooks_InvalidWaitPath(t *testing.T) { From 291066be654f411f81bc57233be74b35c3581d54 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Thu, 21 Apr 2022 18:45:00 -0700 Subject: [PATCH 5/7] pr feedback: atomic update of container status Signed-off-by: Maksim An --- internal/guest/runtime/hcsv2/container.go | 7 ++----- internal/guest/runtime/hcsv2/uvm.go | 3 ++- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index 96d1a13548..022c046b17 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -28,13 +28,10 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) -// containerStatus has been introduced to enable parallel container creation -type containerStatus uint8 - const ( // containerCreating is the default status set on a Container object, when // no underlying runtime container or init process has been assigned - containerCreating containerStatus = iota + containerCreating uint32 = iota // containerCreated is the status when a runtime container and init process // have been assigned, but runtime start command has not been issued yet containerCreated @@ -56,7 +53,7 @@ type Container struct { processesMutex sync.Mutex processes map[uint32]*containerProcess - status containerStatus + status uint32 } func (c *Container) Start(ctx context.Context, conSettings stdio.ConnectionSettings) (int, error) { diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 30209e7b91..795d5a16b1 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -13,6 +13,7 @@ import ( "path" "path/filepath" "sync" + "sync/atomic" "syscall" "time" @@ -331,7 +332,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - c.status = containerCreated + atomic.StoreUint32(&c.status, containerCreated) return c, nil } From 34521cbe1200d717bd6217873988a3802639e7f2 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Fri, 22 Apr 2022 10:31:21 -0700 Subject: [PATCH 6/7] add container status getter/setter Signed-off-by: Maksim An --- internal/guest/runtime/hcsv2/container.go | 25 +++++++++++++++++++++-- internal/guest/runtime/hcsv2/uvm.go | 7 ++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index 022c046b17..0a40e4f664 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -5,7 +5,9 @@ package hcsv2 import ( "context" + "fmt" "sync" + "sync/atomic" "syscall" "github.com/containerd/cgroups" @@ -28,10 +30,13 @@ import ( "github.com/Microsoft/hcsshim/internal/protocol/guestresource" ) +// containerStatus has been introduced to enable parallel container creation +type containerStatus uint32 + const ( // containerCreating is the default status set on a Container object, when // no underlying runtime container or init process has been assigned - containerCreating uint32 = iota + containerCreating containerStatus = iota // containerCreated is the status when a runtime container and init process // have been assigned, but runtime start command has not been issued yet containerCreated @@ -53,7 +58,7 @@ type Container struct { processesMutex sync.Mutex processes map[uint32]*containerProcess - status uint32 + status containerStatus } func (c *Container) Start(ctx context.Context, conSettings stdio.ConnectionSettings) (int, error) { @@ -231,3 +236,19 @@ func (c *Container) GetStats(ctx context.Context) (*v1.Metrics, error) { func (c *Container) modifyContainerConstraints(ctx context.Context, rt guestrequest.RequestType, cc *guestresource.LCOWContainerConstraints) (err error) { return c.Update(ctx, cc.Linux) } + +func (c *Container) getStatus() containerStatus { + val := atomic.LoadUint32((*uint32)(&c.status)) + return containerStatus(val) +} + +func (c *Container) setStatus(st containerStatus) error { + switch st { + case containerCreating, containerCreated: + break + default: + return fmt.Errorf("unknown status: %d", st) + } + atomic.StoreUint32((*uint32)(&c.status), uint32(st)) + return nil +} diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index 795d5a16b1..fce37b01be 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -13,7 +13,6 @@ import ( "path" "path/filepath" "sync" - "sync/atomic" "syscall" "time" @@ -130,7 +129,7 @@ func (h *Host) GetCreatedContainer(id string) (*Container, error) { if c, ok := h.containers[id]; !ok { return nil, gcserr.NewHresultError(gcserr.HrVmcomputeSystemNotFound) } else { - if c.status != containerCreated { + if c.getStatus() != containerCreated { return nil, fmt.Errorf("container is not in state \"created\": %w", gcserr.NewHresultError(gcserr.HrVmcomputeInvalidState)) } @@ -332,7 +331,9 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - atomic.StoreUint32(&c.status, containerCreated) + if err := c.setStatus(containerCreated); err != nil { + return nil, err + } return c, nil } From 727827c1986e348f1d8276182dd507c74f24142b Mon Sep 17 00:00:00 2001 From: Maksim An Date: Fri, 22 Apr 2022 13:09:32 -0700 Subject: [PATCH 7/7] pr feedback: comment and remove switch Signed-off-by: Maksim An --- internal/guest/runtime/hcsv2/container.go | 11 ++--------- internal/guest/runtime/hcsv2/uvm.go | 4 +--- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/internal/guest/runtime/hcsv2/container.go b/internal/guest/runtime/hcsv2/container.go index 0a40e4f664..cc4304c2f3 100644 --- a/internal/guest/runtime/hcsv2/container.go +++ b/internal/guest/runtime/hcsv2/container.go @@ -5,7 +5,6 @@ package hcsv2 import ( "context" - "fmt" "sync" "sync/atomic" "syscall" @@ -58,6 +57,7 @@ type Container struct { processesMutex sync.Mutex processes map[uint32]*containerProcess + // Only access atomically through getStatus/setStatus. status containerStatus } @@ -242,13 +242,6 @@ func (c *Container) getStatus() containerStatus { return containerStatus(val) } -func (c *Container) setStatus(st containerStatus) error { - switch st { - case containerCreating, containerCreated: - break - default: - return fmt.Errorf("unknown status: %d", st) - } +func (c *Container) setStatus(st containerStatus) { atomic.StoreUint32((*uint32)(&c.status), uint32(st)) - return nil } diff --git a/internal/guest/runtime/hcsv2/uvm.go b/internal/guest/runtime/hcsv2/uvm.go index fce37b01be..380112a00d 100644 --- a/internal/guest/runtime/hcsv2/uvm.go +++ b/internal/guest/runtime/hcsv2/uvm.go @@ -331,9 +331,7 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM } } - if err := c.setStatus(containerCreated); err != nil { - return nil, err - } + c.setStatus(containerCreated) return c, nil }