Always consume shim logs #4546

cpuguy83 · 2020-09-09T23:48:10Z

These fifos fill up if unconsumed, so always consume them.
Before this change, it was only being consumed if shim debug is set.

Fixes #4509
Closes #4545

This issue became evident in e3ab8bd where a new log entry was added that happens to get logged every time an exec exits when the client deletes the exec before the event is processed in the shim.

theopenlab-ci · 2020-09-09T23:54:44Z

Build succeeded.

containerd-build-arm64 : FAILURE in 5m 02s (non-voting)

fuweid · 2020-09-10T00:02:26Z

do we need this useless log? https://github.com/containerd/containerd/blob/master/runtime/v1/shim/service.go#L517

I think we should remove it because it doesn't help.

mikebrow

I like this commit ... and I Agree with @fuweid that we don't need: https://github.com/containerd/containerd/blob/master/runtime/v1/shim/service.go#L517

cpuguy83 · 2020-09-10T17:03:41Z

@fuweid At least it's only debug now, it was info level.

runtime/v1/shim/client/client.go

These fifos fill up if unconsumed, so always consume them. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2020-09-10T17:25:51Z

runtime/v1/shim/client/client.go

+		if err != nil {
+			return nil, nil, errors.Wrapf(err, "failed to create stderr log")
 		}
+		if debug {


I think this is pointless. We are already copying, and there can be some errors/warnings from the shim that would be good to have.

I agree, I feel like this was an intentional decision made in the past though. Let's see if there was a reason this wasn't done before.

But it is what loadTasks is doing (Called on containerd restart)

@dmcgowan's comment wasn't there when I posted mine... just to disambiguate, my 2nd comment is intended as a follow-up to my first comment.

I checked with @crosbymichael on this one as well, he wasn't aware of a reason it is this way. I think we should be OK to just make the behavior the same as loadTasks and include that change here.

@dmcgowan I don't understand what change needs to be made.
The PR brings this in line with loadTasks, but thinking it just doesn't make sense to send to /dev/null in non-debug mode since we are already incurring the cost of creating the pipes and copying over.

theopenlab-ci · 2020-09-10T17:30:35Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 30s (non-voting)

fuweid

LGTM

Just still feel that the log https://github.com/containerd/containerd/blob/master/runtime/v1/shim/service.go#L517 is useless because each runc call will create >=1 log information. Even if we open debug mode, it still doesn't provide useful information.

mikebrow

LGTM

dmcgowan

LGTM

containerd 1.4.1 Welcome to the v1.4.1 release of containerd! The first patch release for `containerd` 1.4 includes a fix for v1 shims hanging on exit and exec when the log pipe fills up along with other minor changes. * Always consume shim logs to prevent logs in the shim from blocking [containerd#4546](containerd#4546) * Fix error deleting v2 bundle directory when removing rootfs returns `ErrNotExist` [containerd#4472](containerd#4472) * Fix metrics monitoring of v2 runtime tasks [containerd#4486](containerd#4486) * Fix incorrect stat for Windows containers [containerd#4468](containerd#4468) * Fix devmapper device deletion on rollback [containerd#4437](containerd#4437) * Update seccomp default profile [containerd#4481](containerd#4481) [containerd#4491](containerd#4491) [containerd#4492](containerd#4492) [containerd#4493](containerd#4493) Please try out the release binaries and report any issues at https://github.com/containerd/containerd/issues. * Sebastiaan van Stijn * Derek McGowan * Wei Fu * Brian Goff * Akihiro Suda * Antonio Ojea * Jintao Zhang * Phil Estes * Kazuyoshi Kato * Li Yuxuan * Mike Brown * Prashant Bhutani <details><summary>36 commits</summary> <p> * [`c623d1b3`](containerd@c623d1b) Merge pull request [containerd#4564](containerd#4564) from dmcgowan/prepare-1.4.1 * [`97d690d2`](containerd@97d690d) Prepare v1.4.1 release * [`910da2fb`](containerd@910da2f) Merge pull request [containerd#4555](containerd#4555) from thaJeztah/1.4_backport_bumpcni * [`ca3b91d8`](containerd@ca3b91d) Merge pull request [containerd#4560](containerd#4560) from dmcgowan/backport-4546 * [`42f38718`](containerd@42f3871) Always consume shim logs * [`ea29a60a`](containerd@ea29a60) Merge pull request [containerd#4558](containerd#4558) from thaJeztah/1.4_backport_winstats * [`db931948`](containerd@db93194) Merge pull request [containerd#4557](containerd#4557) from thaJeztah/1.4_backport_makefile_test_tags * [`9b5066aa`](containerd@9b5066a) Merge pull request [containerd#4556](containerd#4556) from thaJeztah/1.4_backport_fix_static_plugin * [`3bcce819`](containerd@3bcce81) Merge pull request [containerd#4554](containerd#4554) from thaJeztah/1.4_backport_add_openat2_syscall * [`98a733e0`](containerd@98a733e) Merge pull request [containerd#4552](containerd#4552) from thaJeztah/1.4_backport_shim_exec_p_debug * [`f247618a`](containerd@f247618) Report correct stats for windows containers * [`cc5d1518`](containerd@cc5d151) Update go list to respect build tags * [`086e859d`](containerd@086e859) BUILDING.md: fix description about static builds * [`16712ae4`](containerd@16712ae) bump cni version to v0.8.0 * [`1575c88c`](containerd@1575c88) seccomp: add `faccessat2` syscall. * [`8bd2bece`](containerd@8bd2bec) seccomp: add `openat2` syscall. * [`4e3397e0`](containerd@4e3397e) shimv1: downgrade poroccess missing log to debug * [`6b5fc7f2`](containerd@6b5fc7f) Merge pull request [containerd#4542](containerd#4542) from thaJeztah/1.4_backport_forward_signal_not_found * [`d118c90d`](containerd@d118c90) Ignore SIGURG signals in signal forwarder * [`3ee6189f`](containerd@3ee6189) Exit signal forward if process not found * [`1a367762`](containerd@1a36776) Merge pull request [containerd#4512](containerd#4512) from fuweid/14-cherry-pick-4486 * [`a1289d6b`](containerd@a1289d6) tasks: Monitor v2 tasks in initFunc as well * [`12f20c99`](containerd@12f20c9) Merge pull request [containerd#4503](containerd#4503) from thaJeztah/1.4_backport_seccomp_updates * [`1f823f76`](containerd@1f823f7) seccomp: allow io-uring related system calls * [`3d28944b`](containerd@3d28944) seccomp: allow clock_settime when CAP_SYS_TIME is added * [`e5cc7d52`](containerd@e5cc7d5) seccomp: allow quotactl with CAP_SYS_ADMIN * [`20273a80`](containerd@20273a8) seccomp: allow sync_file_range2 on supported architectures. * [`357d1002`](containerd@357d100) seccomp: allow personality with UNAME26 bit set * [`0c9de662`](containerd@0c9de66) seccomp: allow syscall membarrier * [`caa46116`](containerd@caa4611) seccomp: allow adjtimex get time operation * [`2b80b7dc`](containerd@2b80b7d) seccomp: allow add preadv2 and pwritev2 syscalls * [`e71eccbc`](containerd@e71eccb) seccomp: move the syslog syscall to be gated by CAP_SYS_ADMIN or CAP_SYSLOG * [`881db9b5`](containerd@881db9b) Merge pull request [containerd#4499](containerd#4499) from fuweid/cherry-pick-4472 * [`feff914a`](containerd@feff914) runtime: ignore ErrNotExist when remove rootfs * [`94c8bd94`](containerd@94c8bd9) Merge pull request [containerd#4496](containerd#4496) from kzys/backport-1.4-4437 * [`23e0ea27`](containerd@23e0ea2) snapshots/devmapper: fix rollback </p> </details> <details><summary>4 commits</summary> <p> * [`8fbf363`](containerd/go-cni@8fbf363) Merge pull request [containerd#56](containerd/go-cni#56) from aojea/bumpcni * [`49657db`](containerd/go-cni@49657db) bump containernetworking/cni dependency to 0.8.0 * [`1582593`](containerd/go-cni@1582593) Merge pull request [containerd#58](containerd/go-cni#58) from fuweid/update-readme-usage * [`8ffba88`](containerd/go-cni@8ffba88) README.md: update Usage case </p> </details> * **github.com/containerd/go-cni** v1.0.0 -> v1.0.1 * **github.com/containernetworking/cni** v0.7.1 -> v0.8.0 * **github.com/containernetworking/plugins** v0.7.6 -> v0.8.6 Previous release can be found at [v1.4.0](https://github.com/containerd/containerd/releases/tag/v1.4.0)

alindeman · 2020-11-16T17:54:25Z

What symptoms would this cause? Could it cause, e.g., Docker to not be able to properly kill containers after a while?

cubic3d · 2020-11-16T18:05:11Z

What symptoms would this cause? Could it cause, e.g., Docker to not be able to properly kill containers after a while?

Yes

BSWANG · 2021-11-26T03:50:08Z

Fixes #4434

cpuguy83 added this to the 1.4.1 milestone Sep 9, 2020

cpuguy83 added the cherry-pick/1.4.x label Sep 9, 2020

cpuguy83 mentioned this pull request Sep 9, 2020

Containers with curl based health checks becoming unresponsive #4509

Closed

mikebrow reviewed Sep 10, 2020

View reviewed changes

dmcgowan reviewed Sep 10, 2020

View reviewed changes

runtime/v1/shim/client/client.go Outdated Show resolved Hide resolved

Always consume shim logs

dab7bd0

These fifos fill up if unconsumed, so always consume them. Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the consume_shim_log branch from 25be9dd to dab7bd0 Compare September 10, 2020 17:23

cpuguy83 commented Sep 10, 2020

View reviewed changes

cpuguy83 mentioned this pull request Sep 10, 2020

update containerd binary to v1.4.0 moby/moby#40982

Merged

fuweid approved these changes Sep 11, 2020

View reviewed changes

mikebrow approved these changes Sep 11, 2020

View reviewed changes

dmcgowan approved these changes Sep 11, 2020

View reviewed changes

dmcgowan merged commit 9efd738 into containerd:master Sep 11, 2020

dmcgowan mentioned this pull request Sep 11, 2020

[release/1.4] Always consume shim logs #4560

Merged

dmcgowan added cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.4.x labels Sep 11, 2020

cpuguy83 deleted the consume_shim_log branch October 20, 2020 16:48

cpuguy83 mentioned this pull request Nov 19, 2020

Stopped container is shown in docker ps moby/moby#38501

Open

cpuguy83 mentioned this pull request Jul 28, 2023

containerd-shim hangs #4434

Closed

Always consume shim logs #4546

Always consume shim logs #4546

Uh oh!

Conversation

cpuguy83 commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Sep 9, 2020

Uh oh!

fuweid commented Sep 10, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Sep 10, 2020

Uh oh!

Uh oh!

cpuguy83 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

dmcgowan Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

dmcgowan Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Sep 10, 2020

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

alindeman commented Nov 16, 2020

Uh oh!

cubic3d commented Nov 16, 2020

Uh oh!

BSWANG commented Nov 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cpuguy83 commented Sep 9, 2020 •

edited

Loading