test(deployments): keep the image pull out of the observe-wait timing window

[SandyChapman]

What

test_never_deployment_outlives_observe_wait_then_succeeds asserts that create_deployment returns within observe_timeout + 2.0s. It pre-pulls alpine:3.20 with a comment saying this keeps an uncached pull out of the timed window — but that is not what happens.

The backend is built with pull_images=True, and create_deployment pulls unconditionally rather than only when the image is missing locally. The warm-up avoids re-downloading layers; it does nothing about the registry round-trip, which still lands inside the measurement.

Why now

A fully cached alpine:3.20 pull measures ~1.7s locally. That puts the unfixed test at 2.85s against a 3.0s budget on a fast machine with a warm cache — 0.15s of margin, all of it hostage to Docker Hub latency. CI has been tipping over it on main and on unrelated branches:

run	branch	create_elapsed
30554298719	main	4.39s
30550043391	fix-stop-instance-lock-race/schapman (#987)	3.84s
30549563915	feat/experimentalist-run-progress	3.42s

There is no @pytest.mark.flaky on this test — reruns are opt-in per-test in this repo — so a single slow pull is a hard CI failure on whatever PR happens to be running.

The fix

Build the backend with pull_images=False in _docker_backend_with_observe_timeout, so the test's own pre-pull is what puts the image on the host and the timed window covers container create, start, and the observe wait — what the assertion is actually about.

The helper is used only by this test; the other three tests in the file keep the default pull_images=True.

Verification

Measured create_elapsed locally, 3 runs each:

	run 1	run 2	run 3	budget
before	2.86s	2.84s	2.83s	3.0s
after	1.18s	1.18s	1.18s	3.0s

Full test_docker_backend.py: 4 passed. ruff check / ruff format --check clean; tools/lint/lint-python-types.sh exits 0 with this file unimplicated; lint-copyright-headers, lint-merge-conflict, lint-no-nmp-common-in-plugins pass.

Out of scope

pull_images is described as "Pull container images before run when missing locally", but the implementation pulls unconditionally. That mismatch is real and worth a look, but changing it alters production pull behaviour for mutable tags, so it does not belong in a test-flake fix.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Tests
  • Improved Docker deployment timing measurements by pulling images before the timed observation period.
  • Measurements now focus on deployment observation time instead of image download latency.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 787321a7-a25b-40c3-bee3-762b94baffcd

📥 Commits

Reviewing files that changed from the base of the PR and between 1063d06 and 16cd6d6.

📒 Files selected for processing (1)

• plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py

---

📝 Walkthrough

Walkthrough

The Docker observe-timeout test now sets pull_images=False, ensuring image retrieval occurs outside the measured observe-wait interval. Its comment was updated to document this timing behavior.

Changes

Docker observe-timeout testing

Layer / File(s)	Summary
Configure timing test plugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.py	The test backend disables image pulling while retaining the observe-timeout override, and the in-test comment documents why the pull occurs outside the measured interval.

Possibly related PRs

• NVIDIA-NeMo/nemo-platform#925: Adds related one-shot “Never” observe-timeout tests and elapsed observe-wait assertions.

Suggested labels: fix

Suggested reviewers: ironcommit, benmccown, tylersbray

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR only changes Docker timing tests and does not implement the flock-release stop_instance fix required by #987.	Implement the stop_instance and orphan-cleanup lock-wait changes from #987, plus the bounded timeout and regression/unit tests.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: excluding Docker image pull time from the observe-wait timing window.
Out of Scope Changes check	✅ Passed	The only code change is the Docker test timing adjustment described by the PR objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-docker-observe-wait-flake/schapman

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	28214/36050	78.3%	62.7%
Integration Tests	16766/34768	48.2%	20.8%

[SandyChapman]

Superseded by #987, which now carries this commit.

#987 and this PR were blocking each other: #987 fixes the daemon-lifecycle flake but was failing on the docker flake (deterministic, 4/4 runs), while this PR fixes the docker flake but failed on the daemon-lifecycle flake. Neither could go green alone, so both fixes now live on #987.

The fix here was validated in CI on this PR's run — the docker test passed and dropped off the slowest-25 durations. Cherry-picked verbatim to fix-stop-instance-lock-race/schapman as 1d66b01bd2.