diff --git a/pkg/server/container_stop.go b/pkg/server/container_stop.go index bb4763067..4dcf2d521 100644 --- a/pkg/server/container_stop.go +++ b/pkg/server/container_stop.go @@ -65,24 +65,28 @@ func (c *criContainerdService) StopContainer(ctx context.Context, r *runtime.Sto return &runtime.StopContainerResponse{}, nil } - // TODO(random-liu): [P1] Get stop signal from image config. - stopSignal := unix.SIGTERM - glog.V(2).Infof("Stop container %q with signal %v", id, stopSignal) - _, err = c.containerService.Kill(ctx, &execution.KillRequest{ID: id, Signal: uint32(stopSignal)}) - if err != nil { - if isContainerdContainerNotExistError(err) { + if r.GetTimeout() > 0 { + // TODO(random-liu): [P1] Get stop signal from image config. + stopSignal := unix.SIGTERM + glog.V(2).Infof("Stop container %q with signal %v", id, stopSignal) + _, err = c.containerService.Kill(ctx, &execution.KillRequest{ID: id, Signal: uint32(stopSignal)}) + if err != nil { + if isContainerdContainerNotExistError(err) { + return &runtime.StopContainerResponse{}, nil + } + return nil, fmt.Errorf("failed to stop container %q: %v", id, err) + } + + err = c.waitContainerStop(ctx, id, time.Duration(r.GetTimeout())*time.Second) + if err == nil { return &runtime.StopContainerResponse{}, nil } - return nil, fmt.Errorf("failed to stop container %q: %v", id, err) + glog.Errorf("Stop container %q timed out: %v", id, err) } - err = c.waitContainerStop(ctx, id, time.Duration(r.GetTimeout())*time.Second) - if err == nil { - return &runtime.StopContainerResponse{}, nil - } - glog.Errorf("Stop container %q timed out: %v", id, err) - glog.V(2).Infof("Delete container from containerd %q", id) + // Delete sends SIGKILL to the container in the containerd version we are using. + // TODO(random-liu): Replace with `Kill` to avoid race soon. _, err = c.containerService.Delete(ctx, &execution.DeleteRequest{ID: id}) if err != nil { if isContainerdContainerNotExistError(err) { diff --git a/pkg/server/container_stop_test.go b/pkg/server/container_stop_test.go index e1f4ecfe7..85e32ccc1 100644 --- a/pkg/server/container_stop_test.go +++ b/pkg/server/container_stop_test.go @@ -109,6 +109,7 @@ func TestStopContainer(t *testing.T) { killErr error deleteErr error discardEvents int + noTimeout bool expectErr bool expectCalls []string }{ @@ -155,6 +156,12 @@ func TestStopContainer(t *testing.T) { expectErr: true, expectCalls: []string{"kill"}, }, + "should directly kill container if timeout is 0": { + metadata: &testMetadata, + containerdContainer: &testContainer, + noTimeout: true, + expectCalls: []string{"delete", "delete"}, + }, "should return error if delete failed": { metadata: &testMetadata, containerdContainer: &testContainer, @@ -201,11 +208,15 @@ func TestStopContainer(t *testing.T) { } }(eventClient, test.discardEvents) fake.ClearCalls() + timeout := int64(1) + if test.noTimeout { + timeout = 0 + } // 1 second timeout should be enough for the unit test. // TODO(random-liu): Use fake clock for this test. resp, err := c.StopContainer(context.Background(), &runtime.StopContainerRequest{ ContainerId: testID, - Timeout: 1, + Timeout: timeout, }) if test.expectErr { assert.Error(t, err)