respect ExecProbeTimeout

openshift · Apr 14, 2021 · cb9f51c · cb9f51c
1 parent 1484454
commit cb9f51c
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 16 deletions.
diff --git a/pkg/kubelet/dockershim/exec.go b/pkg/kubelet/dockershim/exec.go
@@ -26,8 +26,10 @@ import (
 
 	dockertypes "github.com/docker/docker/api/types"
 
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/remotecommand"
 	"k8s.io/klog/v2"
+	"k8s.io/kubernetes/pkg/features"
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 )
@@ -106,7 +108,7 @@ func (*NativeExecHandler) ExecInContainer(ctx context.Context, client libdocker.
 		ExecStarted:  execStarted,
 	}
 
-	if timeout > 0 {
+	if timeout > 0 && utilfeature.DefaultFeatureGate.Enabled(features.ExecProbeTimeout) {
 		var cancel context.CancelFunc
 		ctx, cancel = context.WithTimeout(ctx, timeout)
 		defer cancel()

diff --git a/pkg/kubelet/dockershim/exec_test.go b/pkg/kubelet/dockershim/exec_test.go
@@ -29,7 +29,11 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/remotecommand"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
+	"k8s.io/kubernetes/pkg/features"
+	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 	mockclient "k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker/testing"
 )
 
@@ -43,6 +47,8 @@ func TestExecInContainer(t *testing.T) {
 		returnStartExec    error
 		returnInspectExec1 *dockertypes.ContainerExecInspect
 		returnInspectExec2 error
+		execProbeTimeout   bool
+		startExecDelay     time.Duration
 		expectError        error
 	}{{
 		description:       "ExecInContainer succeeds",
@@ -57,6 +63,7 @@ func TestExecInContainer(t *testing.T) {
 			ExitCode:    0,
 			Pid:         100},
 		returnInspectExec2: nil,
+		execProbeTimeout:   true,
 		expectError:        nil,
 	}, {
 		description:        "CreateExec returns an error",
@@ -66,6 +73,7 @@ func TestExecInContainer(t *testing.T) {
 		returnStartExec:    nil,
 		returnInspectExec1: nil,
 		returnInspectExec2: nil,
+		execProbeTimeout:   true,
 		expectError:        fmt.Errorf("failed to exec in container - Exec setup failed - error in CreateExec()"),
 	}, {
 		description:        "StartExec returns an error",
@@ -75,6 +83,7 @@ func TestExecInContainer(t *testing.T) {
 		returnStartExec:    fmt.Errorf("error in StartExec()"),
 		returnInspectExec1: nil,
 		returnInspectExec2: nil,
+		execProbeTimeout:   true,
 		expectError:        fmt.Errorf("error in StartExec()"),
 	}, {
 		description:        "InspectExec returns an error",
@@ -84,6 +93,7 @@ func TestExecInContainer(t *testing.T) {
 		returnStartExec:    nil,
 		returnInspectExec1: nil,
 		returnInspectExec2: fmt.Errorf("error in InspectExec()"),
+		execProbeTimeout:   true,
 		expectError:        fmt.Errorf("error in InspectExec()"),
 	}, {
 		description:       "ExecInContainer returns context DeadlineExceeded",
@@ -98,7 +108,30 @@ func TestExecInContainer(t *testing.T) {
 			ExitCode:    0,
 			Pid:         100},
 		returnInspectExec2: nil,
+		execProbeTimeout:   true,
 		expectError:        context.DeadlineExceeded,
+	}, {
+		description:        "[ExecProbeTimeout=true] StartExec that takes longer than the probe timeout returns context.DeadlineExceeded",
+		timeout:            1 * time.Second,
+		returnCreateExec1:  &dockertypes.IDResponse{ID: "12345678"},
+		returnCreateExec2:  nil,
+		startExecDelay:     5 * time.Second,
+		returnStartExec:    fmt.Errorf("error in StartExec()"),
+		returnInspectExec1: nil,
+		returnInspectExec2: nil,
+		execProbeTimeout:   true,
+		expectError:        context.DeadlineExceeded,
+	}, {
+		description:        "[ExecProbeTimeout=false] StartExec that takes longer than the probe timeout returns a error",
+		timeout:            1 * time.Second,
+		returnCreateExec1:  &dockertypes.IDResponse{ID: "12345678"},
+		returnCreateExec2:  nil,
+		startExecDelay:     5 * time.Second,
+		returnStartExec:    fmt.Errorf("error in StartExec()"),
+		returnInspectExec1: nil,
+		returnInspectExec2: nil,
+		execProbeTimeout:   false,
+		expectError:        fmt.Errorf("error in StartExec()"),
 	}}
 
 	eh := &NativeExecHandler{}
@@ -110,23 +143,27 @@ func TestExecInContainer(t *testing.T) {
 	var resize <-chan remotecommand.TerminalSize
 
 	for _, tc := range testcases {
-		t.Logf("TestCase: %q", tc.description)
+		tc := tc
+		t.Run(tc.description, func(t *testing.T) {
+			t.Parallel()
+			defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ExecProbeTimeout, tc.execProbeTimeout)()
 
-		mockClient := mockclient.NewMockInterface(ctrl)
-		mockClient.EXPECT().CreateExec(gomock.Any(), gomock.Any()).Return(
-			tc.returnCreateExec1,
-			tc.returnCreateExec2)
-		mockClient.EXPECT().StartExec(gomock.Any(), gomock.Any(), gomock.Any()).Return(tc.returnStartExec)
-		mockClient.EXPECT().InspectExec(gomock.Any()).Return(
-			tc.returnInspectExec1,
-			tc.returnInspectExec2)
+			mockClient := mockclient.NewMockInterface(ctrl)
+			mockClient.EXPECT().CreateExec(gomock.Any(), gomock.Any()).Return(
+				tc.returnCreateExec1,
+				tc.returnCreateExec2)
+			mockClient.EXPECT().StartExec(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(_ string, _ dockertypes.ExecStartCheck, _ libdocker.StreamOptions) { time.Sleep(tc.startExecDelay) }).Return(tc.returnStartExec)
+			mockClient.EXPECT().InspectExec(gomock.Any()).Return(
+				tc.returnInspectExec1,
+				tc.returnInspectExec2)
 
-		// use parent context of 2 minutes since that's the default remote
-		// runtime connection timeout used by dockershim
-		ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
-		defer cancel()
-		err := eh.ExecInContainer(ctx, mockClient, container, cmd, stdin, stdout, stderr, false, resize, tc.timeout)
-		assert.Equal(t, tc.expectError, err)
+			// use parent context of 2 minutes since that's the default remote
+			// runtime connection timeout used by dockershim
+			ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+			defer cancel()
+			err := eh.ExecInContainer(ctx, mockClient, container, cmd, stdin, stdout, stderr, false, resize, tc.timeout)
+			assert.Equal(t, tc.expectError, err)
+		})
 	}
 }
 

diff --git a/test/e2e/common/container_probe.go b/test/e2e/common/container_probe.go
@@ -242,6 +242,26 @@ var _ = framework.KubeDescribe("Probing container", func() {
 		runReadinessFailTest(f, pod, time.Minute)
 	})
 
+	/*
+		Release: v1.21
+		Testname: Pod liveness probe, container exec timeout, restart
+		Description: A Pod is created with liveness probe with a Exec action on the Pod. If the liveness probe call does not return within the timeout specified, liveness probe MUST restart the Pod. When ExecProbeTimeout feature gate is disabled and cluster is using dockershim, the timeout is ignored BUT a failing liveness probe MUST restart the Pod.
+	*/
+	ginkgo.It("should be restarted with a failing exec liveness probe that took longer than the timeout", func() {
+		// The ExecProbeTimeout feature gate exists to allow backwards-compatibility with pre-1.20 cluster behaviors using dockershim, where livenessProbe timeouts were ignored
+		// If ExecProbeTimeout feature gate is disabled on a dockershim cluster, timeout enforcement for exec livenessProbes is disabled, but a failing liveness probe MUST still trigger a restart
+		// Note ExecProbeTimeout=false is not recommended for non-dockershim clusters (e.g., containerd), and this test will fail if run against such a configuration
+		cmd := []string{"/bin/sh", "-c", "sleep 600"}
+		livenessProbe := &v1.Probe{
+			Handler:             execHandler([]string{"/bin/sh", "-c", "sleep 10 & exit 1"}),
+			InitialDelaySeconds: 15,
+			TimeoutSeconds:      1,
+			FailureThreshold:    1,
+		}
+		pod := busyBoxPodSpec(nil, livenessProbe, cmd)
+		RunLivenessTest(f, pod, 1, defaultObservationTimeout)
+	})
+
 	/*
 		Release: v1.14
 		Testname: Pod http liveness probe, redirected to a local address