[release/1.2 backport] backport exec fixes

[thaJeztah]

Backports of:

• Add timeout for I/O waitgroups #3361 Add timeout for I/O waitgroups
  • fixes docker exec hang if earlier docker exec left a zombie process #3286 docker exec hang if earlier docker exec left a zombie process
• Robust pid locking for shim processes #3366 Robust pid locking for shim processes
  • closes fixes: pid reuse attack when kill a exec process #2832 fixes: pid reuse attack when kill a exec process
• Use cached state instead of runc state. #3711 Use cached state instead of runc state
  • relates to High system load/CPU utilization with trivial liveness and readiness exec probes kubernetes/kubernetes#82440 High system load/CPU utilization with trivial liveness and readiness probes
  • relates to Docker healthcheck causes high CPU utilization moby/moby#39102 Docker healthcheck causes high CPU utilization

First two cherry-picks were clean:

# https://github.com/containerd/containerd/pull/3361 Add timeout for I/O waitgroups
git cherry-pick -s -S -x 245052243d23c8de21fcc95bbf47fb1dbc731ab4

# https://github.com/containerd/containerd/pull/3366 Robust pid locking for shim processes
git cherry-pick -s -S -x 719a2c594e4aad6a2de5cd9c298ab95309c2135c

# https://github.com/containerd/containerd/pull/3711 Use cached state instead of `runc state`
git cherry-pick -s -S -x 18be6e37140e778dffd91804dab2bc66ba54493f

Last cherry-pick didn't apply clean:

both modified:   runtime/v1/linux/proc/init.go

This was because #3085 (Shim pluggable logging) and #3374 (Refactor runtime package for code usage) are not in the 1.2 branch,
and respectively changed runc.IO -> *processIO, and proc.Platform->stdio.Platform`:

diff --cc runtime/v1/linux/proc/init.go
index 4a87b2455,539f9c24a..000000000
--- a/runtime/v1/linux/proc/init.go
+++ b/runtime/v1/linux/proc/init.go
@@@ -59,12 -56,14 +59,23 @@@ type Init struct 
  
        WorkDir string
  
++<<<<<<< HEAD:runtime/v1/linux/proc/init.go
 +      id           string
 +      Bundle       string
 +      console      console.Console
 +      Platform     proc.Platform
 +      io           runc.IO
 +      runtime      *runc.Runc
++=======
+       id       string
+       Bundle   string
+       console  console.Console
+       Platform stdio.Platform
+       io       *processIO
+       runtime  *runc.Runc
+       // pausing preserves the pausing state.
+       pausing      *atomicBool
++>>>>>>> 18be6e371... Use cached state instead of `runc state`.:pkg/process/init.go
        status       int
        exited       time.Time
        pid          safePid

[thaJeztah]

Temporarily marked as "WIP", because the first two backports were not marked for backporting, so I want to be sure they look reasonable for backporting to this branch.

I'll remove "WIP" if they are ok, and non-risky to backport

ping @Random-Liu @crosbymichael @estesp PTAL

[thaJeztah]

Keeping as "WIP" per a discussion on Slack with Michael Crosby;

we were not going to cherry pick that change just yet, we wanted more testing in master before the runc state changes are added to a release

[Random-Liu]

we were not going to cherry pick that change just yet, we wanted more testing in master before the runc state changes are added to a release

We don't see obvious test failures caused by that change in HEAD. It seems safe to cherrypick.

[thaJeztah]

removed "WIP"

@crosbymichael @dmcgowan PTAL

[Random-Liu]

LGTM

[estesp]

LGTM

[codecov-io]

Codecov Report

Merging #3755 into release/1.2 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release/1.2    #3755   +/-   ##
============================================
  Coverage        44.19%   44.19%           
============================================
  Files              100      100           
  Lines            10847    10847           
============================================
  Hits              4794     4794           
  Misses            5313     5313           
  Partials           740      740

Flag	Coverage Δ
#linux	47.87% <ø> (ø)	⬆️
#windows	41% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0d7ef6...0877136. Read the comment docs.

[thaJeztah]

Don't merge yet; discussing with @crosbymichael, and he's still on the fence on backporting this one in a patch release

[sorenmogensen]

Assuming this does not get backported to 1.2 - when is a rough estimate for when a containerd with the fix will get released? (We are within last 2 hardening sprints before a product release, and had hoped - maybe naively - that we could get this fix in)

[thaJeztah]

I'm not a maintainer, but from the quick chats I had with @crosbymichael, the main concern was that there may be risk involved in this patch (or at least, it should be battle tested). Which brings us in a bit of a chicken & egg situation; to get the patch widely tested, ideally there would be packages for people to test (beyond the automated tests in this repository and the kubernetes test suites).

There's been a proposal (and implementation) to have "nightly" builds of containerd (see #3702, and https://github.com/kind-ci/containerd-nightlies/releases), which would have this patch, but the master branch has progressed quite a bit since 1.2.x, so (without having looked closely how much this particular codepath has diverged) no guarantees that it works equally well on the 1.2 codebase.

I'm open to suggestions on how to move this forward; perhaps a runtime options (env var?) to enable/disable this could be considered, but that's just thinking out loud 😂

[crosbymichael]

LGTM

[sorenmogensen]

Just wanted to update you that this (git build after merge) did indeed fix the high CPU issue we were experiencing.