From 5a43067915c7aaccbbdeddd6552028959a8a88c2 Mon Sep 17 00:00:00 2001 From: Jack Francis Date: Fri, 12 Mar 2021 13:47:56 -0800 Subject: [PATCH] respect ExecProbeTimeout --- pkg/kubelet/dockershim/exec.go | 4 +- pkg/kubelet/dockershim/exec_test.go | 67 +++++++++++++++++++------ test/e2e/common/node/container_probe.go | 20 ++++++++ 3 files changed, 75 insertions(+), 16 deletions(-) diff --git a/pkg/kubelet/dockershim/exec.go b/pkg/kubelet/dockershim/exec.go index b59867156ff..8a59a22b0b3 100644 --- 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 index cc7e00bac50..13509740aa5 100644 --- 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/node/container_probe.go b/test/e2e/common/node/container_probe.go index eadf89b3e32..afd60bae770 100644 --- a/test/e2e/common/node/container_probe.go +++ b/test/e2e/common/node/container_probe.go @@ -246,6 +246,26 @@ var _ = SIGDescribe("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