Fix the forced stop timeout in the docker manager

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 {

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)
+			}
+		})
+	}
+}