Skip to content

Use Docker client's ContainerWait

Arran Walker requested to merge 25312-migrate-waitforcontainer-to-docker-api-waitforcontainer into master

What does this MR do?

Replaces waitForContainer (a loop of InspectContainer) with the Docker client's own ContainerWait. The same retry handling is used in event a network error stops the long polling.

Why was this MR needed?

Reduces the requests and retry delay to the Docker daemon, and responds immediately when a container has exited.

In my tests, the simplest build job would take 12 seconds. This reduces the time to 5 seconds.

Some of our docker tests also finish faster:

Test Name Old Wait New Wait
TestCacheInContainer 43s 19s
TestDockerServiceNameFromVariable 13s 7s
TestDockerCommandWithHelperImageConfig 15s 8s
TestDockerCommandOutput 13s 6s
TestDockerCommandTwoServicesFromOneImage 30s 15s
TestDockerCommandWithHelperImageConfig 15s 8s
TestDockerCommandDisableEntrypointOverwrite 1m21s 55s

Are there points in the code the reviewer needs to double check?

  • Previously the code in watchContainer would wait for the container to exit whilst the job was running. We don't actually need to check for that, because the stdout stream will return an error (or nil if successful) when we're ready to detach.

    After we've detached, we kill (just in case there was an error that didn't result in the container exiting) and wait to return the status code.

  • Some tests in Wait() were no longer relevant, as there's no "still running" states. ContainerWait only responds when the container has stopped, or there is an error.

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Added tests for this feature/bug
  • In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Closes #25312 (closed) Closes #3918 (closed)

Edited by Arran Walker

Merge request reports