migrate TestAPIStatsNetworkStats to integration by deahtstroke · Pull Request #51800 · moby/moby

deahtstroke · 2026-01-04T12:05:27Z

…n/ suite

- What I did
Migrated the test TestAPIStatsNetworkStats to integration/ from the integration-cli/ suite in reference to epic #50159
- How I did it
Using test on integration-cli/docker_api_stats_test.go and slightly tweaking it to use the go-client instead of the direct CLI
- How to verify it
Run using the integration test directive in the makefile:

TESTDIRS='./integration/container/' TESTFLAGS='-test.run TestStatsNetworkStats' make test-integration

- Human readable description for the release notes

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

thaJeztah

Thanks! Overall looking good; I left some suggestions, let me know what you think; I opened a PR against your branch with my suggestions; if those changes look good to you, you can merge that PR, which will make them show up here; feel free to squash my commit though, as we don't need the extra commit 😅

deahtstroke#1

integration/container/stats_test.go

deahtstroke · 2026-01-05T12:40:24Z

Saw the changes to my branch and I think they look good! Just for guidance, in deahstroke/moby I need to squash + merge, then those changes will appear here? And additionally, I need force push the removal of the code that polls for the container to be ready in that specific order?

thaJeztah · 2026-01-05T12:58:50Z

Thanks! Yes, you can (rebase, and) squash your branch; also happy to do so for you; looks like the DCO check doesn't like the merge-commit, so I can do a quick rebase and squash for you.

thaJeztah · 2026-01-05T13:00:43Z

Rebased; DCO-check should probably be happy now 🤞

deahtstroke · 2026-01-05T13:04:39Z

When you said that the DCO did not like the merge-commit, is that something I did on my end?

thaJeztah · 2026-01-05T13:10:33Z

No, not really a mistake you made; the DCO check checks for every commit-message in the PR to have a Signed-Off-By line. When merging my PR into your branch, GitHub will create a merge commit, using a generic merge XXX into XXX commit-message, and it doesn't include such a line in the commit-message.

thaJeztah · 2026-01-05T13:15:09Z

integration-cli/docker_api_stats_test.go

 }

-func (s *DockerAPISuite) TestAPIStatsNetworkStats(c *testing.T) {
-	skip.If(c, RuntimeIsWindowsContainerd(), "FIXME: Broken on Windows + containerd combination")


I just realised that there was a FIXME comment here; apparently something wasn't right when using containerd as runtime (see 1285c6d).

Perhaps good to preserve that comment so that it's clear why we're skipping it currently (and to look into the failure to see if we can un-skip it)

Let me push a quick change to restore that comment.

thaJeztah

LGTM

but perhaps @vvoland can give a second review because I pushed the latest changes 😅

vvoland

LGTM

thaJeztah · 2026-01-05T13:43:13Z

Interesting; getting a failure now 🤔

=== RUN   TestStatsNetworkStats
    stats_test.go:167: assertion failed: postTxPackets is less than expTxPkts: Reported less TxPackets than expected. Expected >= 3. Found 1. PING 172.17.0.2 (172.17.0.2): 56 data bytes
        64 bytes from 172.17.0.2: icmp_seq=0 ttl=64 time=0.092 ms
        --- 172.17.0.2 ping statistics ---
        1 packets transmitted, 1 packets received, 0% packet loss
        round-trip min/avg/max/stddev = 0.092/0.092/0.092/0.000 ms

deahtstroke · 2026-01-05T13:56:39Z

Yes I was checking this while the CI tests were running but I'm not sure how it fails in Ubuntu-22.04 🤔 only difference between the old and new test is just how we get the network stats.

thaJeztah · 2026-01-05T14:11:05Z

Yeah, not sure either. Perhaps it's cgroups v1 vs v2; ISTR we may be skipping (some of) the integration-cli tests on cgroups v2. Although in that case, I'd expected the reverse to happen (and it failing on 24.04) 🤔

Could still be some race-condition of course.

deahtstroke · 2026-01-06T04:13:37Z

Is there a way I can replicate this myself? I was just reading through the docs for testing but I'm unsure how to target a specific distro while being rootless

deahtstroke · 2026-01-07T18:49:26Z

Hello again! Yesterday I worked on trying to replicate this as much as I could on my own local environment by running it on Ubuntu 22.04 LTS while using Docker Rootless mode. Perhaps something from my environment was missing to fully replicate it 100% as it is in the CI, but the test passed fully. I was thinking if I should add some slight delay before running the assertions or worst comes to worst, skip in rootless mode which I don't think this is the best way to go. Any thoughts?

vvoland

CI red

…n/suite Signed-off-by: Daniel Villavicencio <dvm3099@pm.me> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Follow-up to fadc29b Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The driver was removed in f725489, so users won't be able to have it configured. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The oracle vm tests are frequently timing out; let's see if it's just slowness. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

While these fields are only propagated on Linux, there should be no need to only conditionally include the fields in the inspect response; just pass through the information we have (which should be empty on Windows). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The log would log an info about skipping a missing directory together with a "not found" error (which was redundant), but discard the error for other problems. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

When shutting down the daemon, containers are stopped/killed, which calls `container.ExitOnNext()` to stop the restart manager to prevent containers with a restart policy from being restarted. However, when the daemon handles the container exit (Daemon.handleContainerExit), it checks whether the container should be restarted (`container.RestartManager().ShouldRestart`), which returns an `ErrRestartCanceled` if the restart manager is stopped. This patch: - Prevents the `ErrRestartCanceled` from being logged as a warning; for other errors, a log is still produced; also splitting the `exitStatus` field to `exitCode` and `exitedAt` as the string presentation of it was not human-readable. - Adds an "info" log to log when the restart-manager is disabled for a container with a restart-policy. - Changes the confusing "ignoring event" log for a more informative message ("received task-delete event from containerd"). We don't act on this event, but do want to log it to verify containerd handled the shutdown. - Changes the "restarting container" log from "Debug" to "Info"; this allows verifying that the restart-monitor handled a container-exit Before this patch: ^CINFO[2026-02-21T20:04:42.937057877Z] Processing signal 'interrupt' INFO[2026-02-21T20:04:43.074175085Z] ignoring event container=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 module=libcontainerd namespace=moby topic=/tasks/delete type="*events.TaskDelete" INFO[2026-02-21T20:04:43.073736502Z] shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T20:04:43.074380960Z] cleaning up after shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T20:04:43.074393169Z] cleaning up dead shim id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby WARN[2026-02-21T20:04:43.084973627Z] ShouldRestart failed, container will not be restarted container=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 daemonShuttingDown=true error="restart canceled" execDuration=4.868244252s exitStatus="{0 2026-02-21 20:04:43.061117752 +0000 UTC}" hasBeenManuallyStopped=false restartCount=0 INFO[2026-02-21T20:04:43.231618294Z] stopping event stream following graceful shutdown error="<nil>" module=libcontainerd namespace=moby INFO[2026-02-21T20:04:43.232019419Z] stopping healthcheck following graceful shutdown module=libcontainerd INFO[2026-02-21T20:04:43.232244877Z] stopping event stream following graceful shutdown error="context canceled" module=libcontainerd namespace=plugins.moby INFO[2026-02-21T20:04:44.238598878Z] Daemon shutdown complete With this patch: ^CINFO[2026-02-21T20:07:39.524114750Z] Processing signal 'interrupt' INFO[2026-02-21T20:07:39.525043125Z] stopping restart-manager container=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 INFO[2026-02-21T20:07:39.592829959Z] received task-delete event from containerd container=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 module=libcontainerd namespace=moby topic=/tasks/delete type="*events.TaskDelete" INFO[2026-02-21T20:07:39.592859084Z] shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T20:07:39.593019334Z] cleaning up after shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T20:07:39.593038667Z] cleaning up dead shim id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T20:07:39.710357042Z] stopping event stream following graceful shutdown error="<nil>" module=libcontainerd namespace=moby INFO[2026-02-21T20:07:39.710912875Z] stopping healthcheck following graceful shutdown module=libcontainerd INFO[2026-02-21T20:07:39.710989959Z] stopping event stream following graceful shutdown error="context canceled" module=libcontainerd namespace=plugins.moby INFO[2026-02-21T20:07:40.724286834Z] Daemon shutdown complete Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The Cleanup function oportunistically tries to cleanup old rule (if any). In cases where the rules didn't exist, it would log the error returned by the command, but this would always be `exit status 1`, which doesn't provide details about the actual failure (i.e., if it's the expected "does not exist"). We could improve the code by first calling `nft list table` to check if rules were found, but that would expose a similar problem (error could be due to the rules not present, or "other cause". This patch just changes the logs to be more informative, and passes the context to allow cancelling the command if the context is cancelled. Before this patch: INFO[2026-02-21T13:30:10.428457589Z] Deleting nftables IPv4 rules error="exit status 1" INFO[2026-02-21T13:30:10.453012214Z] Deleting nftables IPv6 rules error="exit status 1" With this patch: INFO[2026-02-21T14:10:06.183933878Z] Deleting nftables IPv4 rules error="exit status 1" output="Error: Could not process rule: No such file or directory\ndelete table ip docker-bridges" INFO[2026-02-21T14:10:06.212198878Z] Deleting nftables IPv6 rules error="exit status 1" output="Error: Could not process rule: No such file or directory\ndelete table ip6 docker-bridges" Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Some services are shut-down in a defer, resulting in logs for them being shut down logged after the "Daemon shutdown complete" log; ^CINFO[2026-02-21T16:57:55.312312593Z] Processing signal 'interrupt' INFO[2026-02-21T16:57:55.424494218Z] shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T16:57:55.424642052Z] cleaning up after shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T16:57:55.424655718Z] cleaning up dead shim id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby ... INFO[2026-02-21T16:57:55.535564427Z] stopping event stream following graceful shutdown error="<nil>" module=libcontainerd namespace=moby INFO[2026-02-21T16:57:55.536002469Z] Daemon shutdown complete INFO[2026-02-21T16:57:55.536108635Z] stopping healthcheck following graceful shutdown module=libcontainerd INFO[2026-02-21T16:57:55.536799719Z] stopping event stream following graceful shutdown error="context canceled" module=libcontainerd namespace=plugins.moby Move printing the log to a defer so that it's printed last: ^CINFO[2026-02-21T17:30:32.762078180Z] Processing signal 'interrupt' INFO[2026-02-21T17:30:32.844606222Z] shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T17:30:32.844780972Z] cleaning up after shim disconnected id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby INFO[2026-02-21T17:30:32.844787555Z] cleaning up dead shim id=1ed1fcfd71c27b5ff4f04c1b33104c17725a841fba7661acea205879fdd687c8 namespace=moby ... INFO[2026-02-21T17:30:32.962354138Z] stopping event stream following graceful shutdown error="<nil>" module=libcontainerd namespace=moby INFO[2026-02-21T17:30:32.962715347Z] stopping event stream following graceful shutdown error="context canceled" module=libcontainerd namespace=plugins.moby INFO[2026-02-21T17:30:32.962760680Z] stopping healthcheck following graceful shutdown module=libcontainerd INFO[2026-02-21T17:30:33.970766458Z] Daemon shutdown complete Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The daemon previously signaled systemd (READY=1) before the API Serve loops were started. Move the readiness notification until after the API listeners are serving to reduce the window where systemd considers the daemon ready but the HTTP accept loop has not yet started. While modifying, also use `WaitGroup.Go` for a slight cleanup. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

It's more idiomatic to not return non-exported types for an exported function. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The `DisplayJSONMessages` only accepted a `JSONMessagesStream`. This meant that it would not accept the output from (for example) [client.ImagePushResponse]. This patch: - Makes `DisplayJSONMessages` accept any `iter.Seq2[jsonstream.Message, error]` - Makes `JSONMessagesStream` an alias instead of a concrete type (TBD if we actually need the type). - Some minor cleanups in the implementation. [client.ImagePushResponse]: https://github.com/moby/moby/blob/fa87120ae64a170fd42096de3b873b439b95016b/client/image_push.go#L20-L24 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…n/suite Signed-off-by: Daniel Villavicencio <dvm3099@pm.me> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…s-to-integration

github-actions bot added the area/testing label Jan 4, 2026

thaJeztah mentioned this pull request Jan 5, 2026

review suggestions deahtstroke/moby#1

Merged

thaJeztah requested changes Jan 5, 2026

View reviewed changes

thaJeztah force-pushed the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch from f60e3eb to 2fbd1b4 Compare January 5, 2026 13:00

thaJeztah reviewed Jan 5, 2026

View reviewed changes

thaJeztah force-pushed the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch from 2fbd1b4 to 4df22f2 Compare January 5, 2026 13:17

thaJeztah approved these changes Jan 5, 2026

View reviewed changes

vvoland approved these changes Jan 5, 2026

View reviewed changes

vvoland added this to the 29.2.0 milestone Jan 5, 2026

vvoland added the kind/refactor PR's that refactor, or clean-up code label Jan 5, 2026

vvoland mentioned this pull request Jan 5, 2026

integration-cli migration #50159

Open

vvoland requested changes Jan 22, 2026

View reviewed changes

vvoland modified the milestones: 29.2.0, 29.3.0, 29.2.1 Jan 22, 2026

vvoland removed this from the 29.2.1 milestone Feb 2, 2026

deahtstroke force-pushed the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch from 4df22f2 to 993d69e Compare February 4, 2026 06:52

integration/container: Migrate TestAPIStatsNetworkStats to integratio…

1846aa6

…n/suite Signed-off-by: Daniel Villavicencio <dvm3099@pm.me> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

deahtstroke force-pushed the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch from 993d69e to 1846aa6 Compare February 4, 2026 07:04

thaJeztah and others added 14 commits February 24, 2026 00:49

api/types/jsonstream: add sanity-check for Message marshaling

4155b0b

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/libnetwork/osl: don't chain "WithXXX" for logs

05b638c

Follow-up to fadc29b Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: remove obsolete warning for legacy "overlay"

9cc4bc0

The driver was removed in f725489, so users won't be able to have it configured. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

integration: raise timeout for "vm" tests

58786fa

The oracle vm tests are frequently timing out; let's see if it's just slowness. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: CDI: fix missing error in logs

2b7624a

The log would log an info about skipping a missing directory together with a "not found" error (which was redundant), but discard the error for other problems. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/logger: cleanup some tests

945b536

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client/internal: export stream type

9f411e1

It's more idiomatic to not return non-exported types for an exported function. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

integration/container: Migrate TestAPIStatsNetworkStats to integratio…

68e1bab

…n/suite Signed-off-by: Daniel Villavicencio <dvm3099@pm.me> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

deahtstroke requested a review from tianon as a code owner February 24, 2026 08:50

github-actions bot added area/builder Build area/networking Networking area/volumes Volumes area/daemon Core Engine area/builder/buildkit Build containerd-integration Issues and PRs related to containerd integration area/ci module/api module/client labels Feb 24, 2026

Merge branch 'moby:master' into 50159-migrate-TestAPIStatsNetworkStat…

5c4ae22

…s-to-integration

github-actions bot added the area/dependencies label Feb 24, 2026

deahtstroke closed this Feb 24, 2026

deahtstroke deleted the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch February 24, 2026 09:15

deahtstroke restored the 50159-migrate-TestAPIStatsNetworkStats-to-integration branch February 24, 2026 23:43

deahtstroke mentioned this pull request Feb 26, 2026

integration/container: Migrate TestAPIStatsNetworkStats to integratio… #52096

Open

Conversation

deahtstroke commented Jan 4, 2026

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deahtstroke commented Jan 5, 2026

Uh oh!

thaJeztah commented Jan 5, 2026

Uh oh!

thaJeztah commented Jan 5, 2026

Uh oh!

deahtstroke commented Jan 5, 2026

Uh oh!

thaJeztah commented Jan 5, 2026

Uh oh!

thaJeztah Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 5, 2026

Uh oh!

deahtstroke commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jan 5, 2026

Uh oh!

deahtstroke commented Jan 6, 2026

Uh oh!

deahtstroke commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

deahtstroke commented Jan 5, 2026 •

edited

Loading

deahtstroke commented Jan 7, 2026 •

edited

Loading