diff --git a/containers/docker/container_stop.go b/containers/docker/container_stop.go index 08d01e3..640af77 100644 --- a/containers/docker/container_stop.go +++ b/containers/docker/container_stop.go @@ -16,6 +16,22 @@ import ( // 10 (seconds) is chosen as this is the default timeout used by container.StopOptions const maximumStopTimeout = 10 +// minimumStopTimeout is the lower bound (in seconds) for the timeout that is +// derived from the context deadline. It gives the container at least some time +// to handle SIGTERM, and avoids passing 0 (immediate SIGKILL) or a negative +// value (no SIGKILL at all) to docker when the deadline is close or has passed. +const minimumStopTimeout = 1 + +// forcedStopTimeout returns the number of seconds docker should wait after +// SIGTERM before sending SIGKILL, given the time remaining until the RPC +// deadline. It uses half of the remaining time, so that the RPC context has not +// expired when ContainerStop returns, bounded by [minimumStopTimeout, +// maximumStopTimeout]. +func forcedStopTimeout(remaining time.Duration) int { + seconds := int((remaining / 2).Round(time.Second) / time.Second) + return max(minimumStopTimeout, min(seconds, maximumStopTimeout)) +} + // ContainerStop stops a container. If the Force option is set and a timeout // is specified in the context, the contains if forcefully terminated after that timeout. // If the Force option is set but no timeout is provided the container's StopTimeout @@ -39,24 +55,16 @@ func (m *Manager) ContainerStop(ctx context.Context, instance string, opts ...op } // a negative timeout indicates to docker that no forceful termination should - // occur. + // occur. A nil timeout makes docker use the container's StopTimeout, if set, + // otherwise the engine default. duration := -1 - if optionz.Force { - // compute duration from context deadline - duration = 0 - timeoutTime, ok := ctx.Deadline() - if ok { - // set the duration to half of the timeout, to ensure that after - // ContainerStop runs, the RPC context won't have expired - duration = int(time.Until(timeoutTime)) / 2 - // cap duration at based on max timeout - duration = min(duration, maximumStopTimeout) - } - } - pDuration := &duration - if duration == 0 { + if optionz.Force { pDuration = nil + if timeoutTime, ok := ctx.Deadline(); ok { + duration = forcedStopTimeout(time.Until(timeoutTime)) + pDuration = &duration + } } if optionz.Restart { diff --git a/containers/docker/container_stop_test.go b/containers/docker/container_stop_test.go index 9b3417f..147e867 100644 --- a/containers/docker/container_stop_test.go +++ b/containers/docker/container_stop_test.go @@ -55,6 +55,7 @@ func TestContainerStop(t *testing.T) { tests := []struct { name string inTimeout time.Duration + inExpired bool inOpts []options.Option inInstance string inCnts []types.Container @@ -155,6 +156,36 @@ func TestContainerStop(t *testing.T) { inOpts: []options.Option{options.Restart()}, wantErr: status.Errorf(codes.NotFound, "container restart-no-such-instance was not found"), }, + { + name: "stop-with-force-and-short-duration", + inInstance: "stop-with-force-and-short-duration", + inTimeout: 10 * time.Second, + inOpts: []options.Option{options.Force()}, + inCnts: []types.Container{ + types.Container{ + Names: []string{"stop-with-force-and-short-duration"}, + }, + }, + wantState: &fakeStoppingDocker{ + Instance: "stop-with-force-and-short-duration", + Duration: 5, + }, + }, + { + name: "stop-with-force-and-expired-deadline", + inInstance: "stop-with-force-and-expired-deadline", + inExpired: true, + inOpts: []options.Option{options.Force()}, + inCnts: []types.Container{ + types.Container{ + Names: []string{"stop-with-force-and-expired-deadline"}, + }, + }, + wantState: &fakeStoppingDocker{ + Instance: "stop-with-force-and-expired-deadline", + Duration: minimumStopTimeout, + }, + }, } for _, tc := range tests { @@ -165,6 +196,11 @@ func TestContainerStop(t *testing.T) { ctx, cancel = context.WithTimeout(context.Background(), tc.inTimeout) defer cancel() } + if tc.inExpired { + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + defer cancel() + } fsd := &fakeStoppingDocker{ cnts: tc.inCnts, } @@ -188,3 +224,26 @@ func TestContainerStop(t *testing.T) { }) } } + +func TestForcedStopTimeout(t *testing.T) { + tests := []struct { + name string + inRemaining time.Duration + want int + }{ + {name: "long-deadline-is-capped", inRemaining: time.Minute, want: maximumStopTimeout}, + {name: "half-of-remaining", inRemaining: 10 * time.Second, want: 5}, + {name: "rounds-to-nearest-second", inRemaining: 9*time.Second - 10*time.Millisecond, want: 4}, + {name: "short-deadline-uses-minimum", inRemaining: 500 * time.Millisecond, want: minimumStopTimeout}, + {name: "zero-uses-minimum", inRemaining: 0, want: minimumStopTimeout}, + {name: "expired-deadline-uses-minimum", inRemaining: -5 * time.Second, want: minimumStopTimeout}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := forcedStopTimeout(tc.inRemaining); got != tc.want { + t.Errorf("forcedStopTimeout(%v) = %d, want %d", tc.inRemaining, got, tc.want) + } + }) + } +}