docker fixtures: unique container names, ownership-scoped cleanup, unconditional removal - #7427
Draft
bm1549 wants to merge 1 commit into
Draft
docker fixtures: unique container names, ownership-scoped cleanup, unconditional removal#7427bm1549 wants to merge 1 commit into
bm1549 wants to merge 1 commit into
Conversation
…conditional removal
Three related defects let one parametric test destroy another's container and fail
it. Observed in a dd-trace-php parametric run, where
Test_Trace_Sampling_Tags_Feb2024_Revision::test_metric_existence failed in setup
with `409 Conflict. The container name "/php-test-client-<id>" is already in use`
while Test_Trace_Sampling_Tags::test_trace_dropped_by_trace_sampling_rule_tags
failed in teardown on that same container. Neither is a sampling bug; any two
tests sharing the scenario can hit this.
1. `test_id` was `str(uuid.uuid4())[0:6]`, a 24-bit space, and it is the only
variable part of every container and network name derived from it. Those names
are shared by all xdist workers against one docker daemon, so at ~800
container-creating tests per session the birthday collision chance is ~1.8%
per session. Two different node ids drew the same value 2.1s apart on gw5 and
gw3. `new_test_id()` now yields 64 bits from one shared definition, so the
parametric and integration-frameworks fixtures cannot drift apart again -- the
latter carried a byte-identical copy of the defect. 16 hex chars rather than
the full 32 keeps the longest generated name (integration-frameworks client,
`{library}-test-library-{framework}-{framework_version}-{test_id}`) inside the
63-char DNS label limit.
2. On a failed create, `docker_run` force-removed every container matching the
name, with no check that it had created them. When the name was taken, that
destroyed the owning worker's *live* container. This is what actually killed
the container in the run above: the losing test's cleanup removed the winner's
container while the winner was inside `stop(timeout=5)`, so the winner's
`logs()` then returned `409 ... dead or marked for removal`. Containers now
carry a per-invocation label and cleanup selects on that label, never on the
name, so a foreign container is never touched.
3. `docker_run`'s teardown gated `container.remove(force=True)` on
`container.logs()`. Any error capturing logs skipped removal, leaking the
container and holding its name for the rest of the session. Removal now runs
in its own `finally`. The capture error still propagates: swallowing it would
turn an unexplained dead container into a green test, and `logger` is built
`use_stdout=False`, so a warning would reach only the log artifact -- not the
terminal, junit, or the job log. `NotFound` is suppressed because removal is
now reached in states where docker reports the container already gone, and
failing there would be spurious.
Adds TEST_THE_TEST coverage; 5 of the 8 cases fail against the previous
behaviour, including both ownership cases and both leak paths.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bm1549
added
the
ai-generated
Contributor
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Two parametric tests failed in a dd-trace-php run: one in setup with
409 Conflict. The container name "/php-test-client-<id>" is already in use, the other inteardown on that same container. Neither is a sampling bug; any two tests in the scenario can
hit it. Three defects combine, and the third is what actually killed the container:
test_idwasstr(uuid.uuid4())[0:6]. That 24-bit value is the only variable part of everycontainer and network name built from it, and those names are shared by all xdist workers
against one docker daemon. At ~800 container-creating tests per session that is a ~1.8%
collision chance per session. Two node ids drew the same value 2.1s apart, on gw5 and gw3.
tests/integration_frameworks/conftest.pycarried a byte-identical copy of the same fixture,so the scenario had the same exposure.
On a failed create,
docker_runforce-removed every container matching the name withoutchecking that it had created them. When the name was already taken, that destroyed the owning
worker's live container. The losing test's cleanup removed the winner's container while the
winner was inside
stop(timeout=5), so the winner'slogs()then returned409 ... dead or marked for removal.Separately,
docker_run's teardown gatedcontainer.remove(force=True)oncontainer.logs(),so any log-capture error skipped removal and leaked the container. Its name stayed taken for the
rest of the session, and the log file needed to explain the death was discarded.
Changes
new_test_id()inutils/docker_fixtures/_core.pyis now the single definition used by boththe parametric and integration-frameworks fixtures, so the two cannot drift apart again. It
yields 64 bits. 16 hex chars rather than the full 32 keeps the longest generated name
(
{library}-test-library-{framework}-{framework_version}-{test_id}) inside the 63-char DNSlabel limit.
label instead of the name, so another worker's container is never removed.
container.remove(force=True)moved into its ownfinally. The log-capture error stillpropagates: swallowing it would turn an unexplained dead container into a green test, and
loggeris builtuse_stdout=False, so a warning would reach only the log artifact, not theterminal, junit, or the job log.
NotFoundis suppressed because removal is now reached instates where docker reports the container already gone, where failing would be spurious.
tests/test_the_test/test_docker_run_cleanup.py: 8 cases. 5 fail against the previousbehaviour: both ownership cases, both leak paths, and the already-removed case.
Verified:
ruff format --check,ruff check,mypyclean; fullTEST_THE_TEST372 passed,1 xfailed.
Note for reviewers
This changes
docker_run's create-failure and teardown-failure contracts, which every tracer'sparametric run depends on. A test that previously passed after a swallowed teardown error will now
fail loudly. That is the intent, but it may surface pre-existing flakiness on the first runs.
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teamutils/docker_fixtures/_core.pyis modified — R&P approval still needed, not yet obtained.build-XXX-imagelabel is present