From cb9f51ccb7763f17777e79ce04a09968114c968b 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/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 a735759a61514..f93fff3790571 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 cc7e00bac50a0..13509740aa564 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/container_probe.go b/test/e2e/common/container_probe.go index 59e0315ad9aaf..f899fc0e076ed 100644 --- 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