From ad5266456c72595901f25ef0bdd5e1ce3a5d6c60 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Thu, 5 Oct 2017 09:14:16 -0700 Subject: [PATCH 1/2] windows: Fix a few races Signed-off-by: Kenfe-Mickael Laventure --- windows/process.go | 22 +++++++++++++++------- windows/task.go | 13 ++++++++----- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/windows/process.go b/windows/process.go index 2d80ff5fd..2bcabb946 100644 --- a/windows/process.go +++ b/windows/process.go @@ -5,6 +5,7 @@ package windows import ( "context" "io" + "sync" "syscall" "time" @@ -19,13 +20,14 @@ import ( // process implements containerd.Process and containerd.State type process struct { + sync.Mutex + hcs hcsshim.Process - id string - pid uint32 - io *pipeSet - status runtime.Status - task *task + id string + pid uint32 + io *pipeSet + task *task exitCh chan struct{} exitCode uint32 @@ -93,6 +95,13 @@ func (p *process) ExitCode() (uint32, time.Time, error) { } func (p *process) Start(ctx context.Context) (err error) { + p.Lock() + defer p.Unlock() + + if p.hcs != nil { + return errors.Wrap(errdefs.ErrFailedPrecondition, "process already started") + } + // If we fail, close the io right now defer func() { if err != nil { @@ -132,6 +141,7 @@ func (p *process) Start(ctx context.Context) (err error) { if p.io.stderr != nil { go ioCopy("stderr", p.io.stderr, stderr) } + p.hcs = hp // Wait for the process to exit to get the exit status go func() { @@ -174,8 +184,6 @@ func (p *process) Start(ctx context.Context) (err error) { // Cleanup HCS resources hp.Close() }() - p.status = runtime.RunningStatus - p.hcs = hp return nil } diff --git a/windows/task.go b/windows/task.go index 8b6425786..ac26ff179 100644 --- a/windows/task.go +++ b/windows/task.go @@ -111,6 +111,10 @@ func (t *task) Info() runtime.TaskInfo { } func (t *task) Start(ctx context.Context) error { + if p := t.getProcess(t.id); p != nil { + return errors.Wrap(errdefs.ErrFailedPrecondition, "task already started") + } + conf := newWindowsProcessConfig(t.spec.Process, t.io) p, err := t.newProcess(ctx, t.id, conf, t.io) if err != nil { @@ -136,13 +140,13 @@ func (t *task) Pause(ctx context.Context) error { t.Lock() t.status = runtime.PausedStatus t.Unlock() - } - if err == nil { + t.publisher.Publish(ctx, runtime.TaskPausedEventTopic, &eventsapi.TaskPaused{ ContainerID: t.id, }) + return nil } return errors.Wrap(err, "hcsshim failed to pause task") } @@ -157,13 +161,13 @@ func (t *task) Resume(ctx context.Context) error { t.Lock() t.status = runtime.RunningStatus t.Unlock() - } - if err == nil { + t.publisher.Publish(ctx, runtime.TaskResumedEventTopic, &eventsapi.TaskResumed{ ContainerID: t.id, }) + return nil } return errors.Wrap(err, "hcsshim failed to resume task") } @@ -313,7 +317,6 @@ func (t *task) newProcess(ctx context.Context, id string, conf *hcsshim.ProcessC id: id, pid: pid, io: pset, - status: runtime.CreatedStatus, task: t, exitCh: make(chan struct{}), conf: conf, From cfa87567a09645cff683f519ffc6da2f321cce88 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 6 Oct 2017 13:38:08 -0700 Subject: [PATCH 2/2] windows: Create init process with task This prevents `task.Wait()` to return an error if it is called before the task is started. Signed-off-by: Kenfe-Mickael Laventure --- container_test.go | 4 ++-- windows/process.go | 12 ++++++++++++ windows/runtime.go | 5 +++++ windows/task.go | 13 ++++++------- 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/container_test.go b/container_test.go index f37a9f6bd..b51943fe1 100644 --- a/container_test.go +++ b/container_test.go @@ -203,9 +203,9 @@ func TestContainerOutput(t *testing.T) { } status := <-statusC - code, _, _ := status.Result() + code, _, err := status.Result() if code != 0 { - t.Errorf("expected status 0 but received %d", code) + t.Errorf("expected status 0 but received %d: %v", code, err) } if _, err := task.Delete(ctx); err != nil { t.Error(err) diff --git a/windows/process.go b/windows/process.go index 2bcabb946..539830ee7 100644 --- a/windows/process.go +++ b/windows/process.go @@ -52,6 +52,9 @@ func (p *process) State(ctx context.Context) (runtime.State, error) { } func (p *process) Status() runtime.Status { + p.Lock() + defer p.Unlock() + if p.task.getStatus() == runtime.PausedStatus { return runtime.PausedStatus } @@ -71,15 +74,24 @@ func (p *process) Status() runtime.Status { func (p *process) Kill(ctx context.Context, sig uint32, all bool) error { // On windows all signals kill the process + if p.Status() == runtime.CreatedStatus { + return errors.Wrap(errdefs.ErrFailedPrecondition, "process was not started") + } return errors.Wrap(p.hcs.Kill(), "failed to kill process") } func (p *process) ResizePty(ctx context.Context, size runtime.ConsoleSize) error { + if p.Status() == runtime.CreatedStatus { + return errors.Wrap(errdefs.ErrFailedPrecondition, "process was not started") + } err := p.hcs.ResizeConsole(uint16(size.Width), uint16(size.Height)) return errors.Wrap(err, "failed to resize process console") } func (p *process) CloseIO(ctx context.Context) error { + if p.Status() == runtime.CreatedStatus { + return errors.Wrap(errdefs.ErrFailedPrecondition, "process was not started") + } return errors.Wrap(p.hcs.CloseStdin(), "failed to close stdin") } diff --git a/windows/runtime.go b/windows/runtime.go index 4c0afb4cc..467c22067 100644 --- a/windows/runtime.go +++ b/windows/runtime.go @@ -304,6 +304,11 @@ func (r *windowsRuntime) newTask(ctx context.Context, namespace, id string, spec hcsContainer: ctr, terminateDuration: createOpts.TerminateDuration, } + // Create the new process but don't start it + pconf := newWindowsProcessConfig(t.spec.Process, t.io) + if _, err = t.newProcess(ctx, t.id, pconf, t.io); err != nil { + return nil, err + } r.tasks.Add(ctx, t) var rootfs []*containerdtypes.Mount diff --git a/windows/task.go b/windows/task.go index ac26ff179..362611ff2 100644 --- a/windows/task.go +++ b/windows/task.go @@ -111,17 +111,16 @@ func (t *task) Info() runtime.TaskInfo { } func (t *task) Start(ctx context.Context) error { - if p := t.getProcess(t.id); p != nil { - return errors.Wrap(errdefs.ErrFailedPrecondition, "task already started") + p := t.getProcess(t.id) + if p == nil { + panic("init process is missing") } - conf := newWindowsProcessConfig(t.spec.Process, t.io) - p, err := t.newProcess(ctx, t.id, conf, t.io) - if err != nil { - return err + if p.Status() != runtime.CreatedStatus { + return errors.Wrap(errdefs.ErrFailedPrecondition, "process was already started") } + if err := p.Start(ctx); err != nil { - t.removeProcess(t.id) return err } t.publisher.Publish(ctx,