Skip to content

fix(ci): skip DGDR deploy tests without planner image - #15201

Closed
xianlubird wants to merge 3 commits into
ai-dynamo:mainfrom
xianlubird:bug/skip-dgdr-without-planner-image
Closed

xianlubird wants to merge 3 commits into
ai-dynamo:mainfrom
xianlubird:bug/skip-dgdr-without-planner-image

Conversation

@xianlubird

@xianlubird xianlubird commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Build the Planner image when DGDR files change, so DGDR-only changes still have the image they need.
  • Run the DGDR deployment tests only after the Planner image copy succeeds and returns a nonempty tag. This prevents a cancelled Planner build from producing unrelated DGDR admission errors.

Why

In PR #14681's CI run, the Planner build was cancelled at its 60-minute limit. The Planner image copy was skipped, but the DGDR job still started with an empty image tag (dynamo-planner:). The webhook then rejected DGDRs because spec.image had no parseable version tag and spec.runtimeVersionOverride was absent:

The deploy status check then failed as a consequence. This PR prevents those secondary errors; it does not change the Planner build timeout.

Validation

  • Parsed .github/workflows/pr.yaml as YAML and checked the new dependency conditions.
  • git diff --check passed.

Where should the reviewer start?

The planner-build and deploy-test-dgdr conditions in .github/workflows/pr.yaml.

Related Issues

Summary by CodeRabbit

  • Chores
    • Updated automated build workflows to also run when DGDR components change.
    • Added a validation step so test DGDR deployments run only after a successful planner image publication with a valid image tag.

Signed-off-by: xianlubird <xianlubird@gmail.com>
@xianlubird
xianlubird requested a review from a team as a code owner September 23, 2026 01:43
@copy-pr-bot

copy-pr-bot Bot commented Sep 23, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@xianlubird
xianlubird deployed to external_collaborator September 23, 2026 01:43 — with GitHub Actions Active
@xianlubird
xianlubird deployed to external_collaborator September 23, 2026 01:43 — with GitHub Actions Active
@github-actions github-actions Bot added external-contribution Pull request is from an external contributor trusted-contributor Org-External user who is trusted to run CI without Org-member approval fix labels Sep 23, 2026
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test b9da294

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e8b48b7d-d1bc-4afd-86b0-717e5f1c14bf

📥 Commits

Reviewing files that changed from the base of the PR and between f804218 and b9da294.

📒 Files selected for processing (1)
  • .github/workflows/pr.yaml

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The pull request updates .github/workflows/pr.yaml. Planner builds now include DGDR changes as triggers. DGDR deployment tests now require a successful planner image copy with a non-empty image tag.

Changes

Planner workflow updates

Layer / File(s) Summary
Planner build triggers and DGDR deployment gates
.github/workflows/pr.yaml
planner-build runs when dgdr changes. deploy-test-dgdr runs only after planner-copy-to-acr succeeds and provides a non-empty image tag.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to b9da2

DGDR changes trigger the planner workflow, and deployment tests run only after a successful image copy with a valid tag.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main CI change: skipping DGDR deployment tests when the Planner image is unavailable.
Description check ✅ Passed The description clearly explains the change, motivation, validation, and reviewer starting point. The Related Issues section references #14681 but does not use the required issue-link format or confir…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@xianlubird
xianlubird deployed to external_collaborator September 23, 2026 03:20 — with GitHub Actions Active
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test 19cad4f

@xianlubird
xianlubird deployed to external_collaborator September 23, 2026 03:36 — with GitHub Actions Active
@dynamo-ops

Copy link
Copy Markdown
Contributor

/ok to test e2452a2

@dmitry-tokarev-nv dmitry-tokarev-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve e2452a270c2c9c7352dd7cc665ba4aa6aa5fbeba. It has no P0, P1, or P2 defect. Two P3 items follow.

main already has most of this change. Commit 3079ce52 (#15394) adds the same dgdr condition to planner-build. It also runs deploy-test-dgdr only after planner-copy-to-acr succeeds. This head merges into main at 522096a8e666 with no conflict. After that merge, this PR adds only the two conditions below.

  • [P3] The merge with main repeats needs.planner-copy-to-acr.result == 'success'. .github/workflows/pr.yaml:1570 (lines 1585 and 1588 after the merge). Please merge main and remove the repeated line. If main is enough, please close this PR instead.
  • [P3] The image_tag != '' test changes a missing Planner tag into a silent skip. .github/workflows/pr.yaml:1571. If planner-copy-to-acr succeeds with an empty tag, no DGDR test runs, and deploy-status-check and backend-status-check both pass. Please remove the line, or make an empty tag fail the DGDR job with a clear error.

No CI run covers the merge with main. The last PR run on this head is 35815040589, from 2026-09-23.

Five Planner cases: the DGDR tests run only in the last row, and `backend-status-check` fails for a cancelled or failed build.

The results are the same at this head and on the merge with main at 522096a8e666. Each row assumes that deploy-operator succeeds. Rows 1, 2, 4, and 5 also assume a change that sets operator or dgdr.

Planner build planner-copy-to-acr deploy-test-dgdr deploy-status-check backend-status-check
cancelled skipped skipped passes fails
failed skipped skipped passes fails
skipped skipped skipped passes passes
success, empty tag success skipped passes passes
success, tag set success runs fails on a DGDR test failure passes
  • The Planner build skips only for a change with no core, planner, operator, or dgdr file. Then deploy-test-dgdr also skips, because its own condition needs operator or dgdr.
  • On main without this PR, only the empty-tag row differs. The DGDR tests run with the image dynamo-planner: and fail, so deploy-status-check fails.
  • The code in shared-copy.yml never sets an empty tag. It writes image_tag at line 97, or it exits 1 at line 91.
  • Run 35692044251 (#14681, before this fix) shows the cancelled case on the old code. The Planner build stopped at 60 minutes, the copy skipped, the three DGDR tests ran and failed, and both status checks failed.
  • deploy-status-check, backend-status-check, and planner-copy-to-acr are the same in that run, at this head, and on the merge.
  • Run 35815040589 shows the last row on this head. The Planner build and the copy succeeded, and the three DGDR tests ran and passed.
  • The other cells come from the if: conditions, not from a run.
actionlint 1.7.12 gives the same 22 messages on `main`, on this head, and on the merge.
  • The 22 messages are 21 unknown runner labels and the constant if: false on allure-report. Only the line numbers differ between the three trees.
  • A planted unknown job name in the new if: adds one error. A planted missing ) adds one error.
  • A planted unknown output name adds no error, because actionlint cannot read the outputs of a $/ workflow. I read image_tag in shared-copy.yml lines 36 to 38 instead.

@xianlubird

Copy link
Copy Markdown
Contributor Author

Thanks, I checked main and confirmed #15394 already covers the DGDR planner build trigger and the copy-success gate.

This branch was successfully deployed

1 active deployment
external_collaborator — e2452a27 Deployed Sep 23, 2026 by xianlubird via ok-to-test #22236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actions external-contribution Pull request is from an external contributor fix size/XS trusted-contributor Org-External user who is trusted to run CI without Org-member approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants