cli/command/container: prevent panic during stats on empty event Actor.ID

[thaJeztah]

relates to

• api/types/events: Message: remove deprecated Status, ID, and From fields moby/moby#50832
• Hide the mutex lock in formatter.ContainerStats moby/moby#26819
• Add support for Names and ID in stats format moby/moby#27797

This code was missing a check for the ID field before truncating it to a shorter length for presentation. This would result in a panic if an event would either have an empty ID field or a shorter length ID;

panic: runtime error: slice bounds out of range [:12] with length 0

goroutine 82 [running]:
github.com/docker/cli/cli/command/container.RunStats.func2({{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x40001fcba0, 0x9}, {0x40001fcba9, 0x5}, ...})
    /go/src/github.com/docker/cli/cli/command/container/stats.go:146 +0x1d0
created by github.com/docker/cli/cli/command/container.(*eventHandler).watch in goroutine 6
    /go/src/github.com/docker/cli/cli/command/container/stats.go:363 +0x1c8

We need to look at this code in general; the truncated ID is passed to NewStats, which uses the ID to propagate the Container field in the StatsEntry struct. which is not used in the default format used by docker stats and, having the same content as the ID field on the same struct, doesn't make it very useful, other than being able to present it under a CONTAINER column (instead of CONTAINER ID); we should consider deprecating it; there may be some subtle things to look into here; the Container field originally held the container name. This was changed in moby@ef915fd, which introduced separate ID and Name fields, renaming the old Name field to container.

Looking at Stats.SetStatistics() and related code in stats_helpers.go, the Container field is used as the "canonical" reference for the stats record; this allows the stats data to be refreshed when a new stats sample arrives for the same container (also see moby@929a77b, which moved locking to the Stats wrapper struct). This construct allows to account for intermediate states, where a stats sample was incomplete or could produce an error; in that case, the reference to the container for which the stats were sampled is kept to allow removing a container from the list once the container was removed. We should consider removing Container as a formatting option, and moving the Container field to the outer struct; this makes the outer struct responsible for keeping a reference to the container, allowing the StatsEntry as a whole to be replaced atomically.

This patch only addresses the panic;

• It changes the logic to preserve the container ID verbatim instead of truncating. This allows stats samples to be matched against the Actor.ID as-is.
• Truncating the Container is moved to the presentation logic; currently this does not take --no-trunc into account to keep the existing behavior, but we can (should) consider adding this.
• Logging is improved to use structured logs, and an extra check is added to prevent empty IDs from being added as watcher.

- What I did

- How I did it

- How to verify it

- Human readable description for the release notes

Fix a panic during `stats` on empty event `Actor.ID`

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cli/command/container/stats.go	0.00%	8 Missing ⚠️

📢 Thoughts on this report? Let us know!

[thaJeztah]

Ah, derp looks like I forgot to apply some changes that I wanted to keep for later;

#18 64.79 === Failed
#18 64.79 === FAIL: cli/command/container TestContainerStatsContext (0.00s)
#18 64.79     formatter_stats_test.go:45: Expected "e093ff3a3f2df7a27b0851c1e0c9e332d78c893c16769ba2c3d3e482b4115b22", got "e093ff3a3f2d"
#18 64.79 
#18 64.79 === FAIL: cli/command/container TestContainerStatsContextWrite (0.00s)
#18 64.79     formatter_stats_test.go:121: assertion failed: 
#18 64.79         --- te.expected
#18 64.79         +++ →
#18 64.79         @@ -1,3 +1,3 @@
#18 64.79         -container1  abcdef  foo
#18 64.79         +container1  abcdef  /foo
#18 64.79          container2    --

[thaJeztah]

Ah, and this was for later 😂

[thaJeztah]

Slightly hacky to keep the old output

[thaJeztah]

Ah, derp and that isn't exactly right either; the old behavior was inconsistent; when passing IDs, they were shown "as-is", but when showing "all containers" it would always truncate;

docker stats --format '{{.ID}}\t{{.Container}}' --no-stream
4b8a430426d0	4b8a430426d0
c74518277ddc	c74518277ddc

docker stats --format '{{.ID}}\t{{.Container}}' --no-trunc --no-stream
4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a	4b8a430426d0
c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65	c74518277ddc

docker stats --format '{{.ID}}\t{{.Container}}' --no-stream 4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65
4b8a430426d0	4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a
c74518277ddc	c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65

docker stats --format '{{.ID}}\t{{.Container}}' --no-trunc --no-stream 4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65
4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a	4b8a430426d0fbc1fb446e42549ca0d56ca783678d3a8eb1171c9f2c2f524b3a
c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65	c74518277ddc15a6afeaaeb06ee5f7433dcb27188224777c1efa7df1e8766d65

Perhaps we should just remove the truncating for Container - it's inconsistent in either case

[thaJeztah]

I'll bring this one in; I think the very slight change in behaviour should probably not be problematic. I'll also look at some of the follow-ups after this.

Ultimately, we should clean up this code, and see if we can move this (back) into the client module so that compose (and others) don't have either use this code, or reinvent the logic used.