kubelet: dockershim ExecSync should return context.DeadlineExeceeded on timeout #96495

andrewsykim · 2020-11-12T03:29:45Z

Signed-off-by: Andrew Sy Kim [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:
In #94115 we fixed a bug where kubelet did not respect exec probe timeouts. That PR also re-enabled some tests to ensure we don't regress. So far all the tests using containerd have been passing, but some tests using dockershim started to fail (see #96463). This wasn't caught during presubmit since jobs that run dockershim only run e2es marked with [NodeConformance].

The bug is in the CRI implementation of dockershim where it should be returning context.Deadline in the ExecSync response instead of exec.TimedoutError. TimedoutError is actually the error as expected by the prober, which is what should be returned by the gRPC client, not the server. The server is expected to return context.DeadlineExeceeded, which results in the cri client returning TimedoutError back to the prober.

Which issue(s) this PR fixes:
Fixes #96463

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

andrewsykim · 2020-11-12T03:35:04Z

/assign @SergeyKanzhelev

hasheddan

@andrewsykim looks like this may break some of the other probing container tests

andrewsykim · 2020-11-12T14:38:34Z

@hasheddan that's interesting, I thought none of the presubmits run docker (see #96463 (comment)), but this PR only touches dockreshim.

hasheddan · 2020-11-12T16:52:40Z

@andrewsykim so it looks like the presubmit does use dockershim, but our periodic does not. From the failing test logs:

NodeSystemInfo{MachineID:3733c809a1e5b150506bec770a5c5c70,SystemUUID:3733c809-a1e5-b150-506b-ec770a5c5c70,BootID:4cd37059-30d4-4a94-86d5-9a1c18597431,KernelVersion:5.3.0-1016-gke,OSImage:Ubuntu 18.04.4 LTS,ContainerRuntimeVersion:docker://19.3.2,KubeletVersion:v1.20.0-beta.1.466+011d0e2ffaf85f,KubeProxyVersion:v1.20.0-beta.1.466+011d0e2ffaf85f,OperatingSystem:linux,Architecture:amd64,}

I know @oomichi tested out that the test did in fact fail without your PR but it wasn't pull-kubernetes-node-e2e, it was pull-kubernetes-e2e-gce-ubuntu-containerd (see #96178). I am looking at differences between the pull-kubernetes-node-e2e presubmit and the gce-ubuntu-master-default periodic now. I do notice that we are using different Ubuntu versions in each:

presubmit: ubuntu-gke-1804-1-16-v20200330
periodic: ubuntu-2004-focal-v20200423

hasheddan · 2020-11-12T17:06:13Z

@andrewsykim ah found it, we are skipping the test that is failing in the periodic

hasheddan · 2020-11-12T20:17:38Z

xref kubernetes/test-infra#19934

andrewsykim · 2020-11-16T17:35:13Z

/milestone v1.20

SergeyKanzhelev · 2020-11-17T01:06:09Z

pkg/kubelet/dockershim/exec.go

nothing bad will happen, but maybe need to call cancel()?

cancel is called with defer on line 112

yes, totally. My comment is that we can cancel timeout in case the exec has already finished =)

Signed-off-by: Andrew Sy Kim <[email protected]>

SergeyKanzhelev · 2020-11-17T07:17:42Z

/test pull-kubernetes-e2e-azure-disk-windows

derekwaynecarr

just the one comment about not adding NodeConformance tag.

derekwaynecarr · 2020-11-17T14:42:11Z

test/e2e/common/container_probe.go

i agree we should defer labeling this NodeConformance

Signed-off-by: Andrew Sy Kim <[email protected]>

andrewsykim · 2020-11-17T16:02:21Z

@derekwaynecarr removed exec probe timeout tests from [NodeConformance], PTAL

SergeyKanzhelev · 2020-11-18T00:21:42Z

/lgtm

andrewsykim · 2020-11-18T22:01:39Z

/kind failing-test
/priority critical-urgent
/triage accepted

hasheddan

/lgtm

@derekwaynecarr could you take another look here and approve if looks good to you?

derekwaynecarr · 2020-11-19T16:08:46Z

/approve
/lgtm

k8s-ci-robot · 2020-11-19T16:09:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, derekwaynecarr, hasheddan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~test/e2e/common/OWNERS~~ [andrewsykim,derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewsykim mentioned this pull request Nov 12, 2020

[Failing Test] [k8s.io] Probing container should be restarted with a docker exec liveness probe with timeout [ci-kubernetes-e2e-ubuntu-gce] #96463

Closed

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 12, 2020

k8s-ci-robot requested review from derekwaynecarr and matthyx November 12, 2020 03:31

andrewsykim force-pushed the dockershim-exec-context branch from 5b79be9 to 5a1b531 Compare November 12, 2020 03:34

k8s-ci-robot assigned SergeyKanzhelev Nov 12, 2020

andrewsykim force-pushed the dockershim-exec-context branch 2 times, most recently from 3517e5f to 9a03598 Compare November 12, 2020 03:46

hasheddan reviewed Nov 12, 2020

View reviewed changes

hasheddan mentioned this pull request Nov 12, 2020

Explicitly enable probing container test for dockershim job kubernetes/test-infra#19934

Closed

andrewsykim force-pushed the dockershim-exec-context branch 3 times, most recently from ed47ee3 to ab34ee5 Compare November 13, 2020 00:20

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Nov 13, 2020

andrewsykim force-pushed the dockershim-exec-context branch from ab34ee5 to 56fd817 Compare November 13, 2020 01:54

k8s-ci-robot added this to the v1.20 milestone Nov 16, 2020

SergeyKanzhelev reviewed Nov 17, 2020

View reviewed changes

kubelet: dockershim should return grpc status with DeadlineExceeded code

a59189e

Signed-off-by: Andrew Sy Kim <[email protected]>

andrewsykim force-pushed the dockershim-exec-context branch from bbdcd20 to 3391546 Compare November 17, 2020 03:52

derekwaynecarr suggested changes Nov 17, 2020

View reviewed changes

test/e2e/common/container_probe.go Outdated

Copy link

Member

derekwaynecarr Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree we should defer labeling this NodeConformance

e2e/node: increase timeouts seconds to 5 for liveness probe restart test

f5a82f7

Signed-off-by: Andrew Sy Kim <[email protected]>

andrewsykim force-pushed the dockershim-exec-context branch from 3391546 to f5a82f7 Compare November 17, 2020 15:02

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2020

andrewsykim mentioned this pull request Nov 18, 2020

e2e/node: add exec probe timeout tests to NodeConformance #96694

Merged

hasheddan approved these changes Nov 19, 2020

View reviewed changes

k8s-ci-robot assigned hasheddan Nov 19, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2020

k8s-ci-robot merged commit 18099e1 into kubernetes:master Nov 19, 2020

This was referenced Dec 18, 2020

the pod container quickly exited postStart will block forever,then pod keep containerCreating forever #97364

Closed

Automated cherry pick of #96495 upstream release 1.19 #97388

Closed

hasheddan mentioned this pull request Feb 5, 2021

[Failing Test] [sig-network] HostPort validates that there is no conflict between pods with same hostPort but different hostIP and protocol #98648

Closed

jackfrancis mentioned this pull request Mar 12, 2021

respect ExecProbeTimeout=false for dockershim #100200

Merged

SergeyKanzhelev mentioned this pull request Aug 5, 2024

adding SergeyKanzhelev as SIG Node approver #126551

Merged

25 tasks

kubelet: dockershim ExecSync should return context.DeadlineExeceeded on timeout #96495

kubelet: dockershim ExecSync should return context.DeadlineExeceeded on timeout #96495

Uh oh!

Conversation

andrewsykim commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewsykim commented Nov 12, 2020

Uh oh!

hasheddan left a comment

Choose a reason for hiding this comment

Uh oh!

andrewsykim commented Nov 12, 2020

Uh oh!

hasheddan commented Nov 12, 2020

Uh oh!

hasheddan commented Nov 12, 2020

Uh oh!

hasheddan commented Nov 12, 2020

Uh oh!

andrewsykim commented Nov 16, 2020

Uh oh!

SergeyKanzhelev Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

andrewsykim Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev commented Nov 17, 2020

Uh oh!

derekwaynecarr left a comment

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

andrewsykim commented Nov 17, 2020

Uh oh!

SergeyKanzhelev commented Nov 18, 2020

Uh oh!

andrewsykim commented Nov 18, 2020

Uh oh!

hasheddan left a comment

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Nov 19, 2020

Uh oh!

k8s-ci-robot commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

andrewsykim commented Nov 12, 2020 •

edited

Loading