Repository navigation
ci: publish build images to east and west ECR - #15331
Conversation
Signed-off-by: Lavanya <lvijayakrish@nvidia.com>
|
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 configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe shared image build workflow now logs in to the derived us-east-1 ECR registry when image pushing is enabled. It generates image tags for both regions, including tags used for test-image builds. ChangesMulti-region image publishing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The workflow is configured to publish runtime and test images to both east and west while retaining the west tags. The available source supports the authentication and tag paths, with no concrete merge blocker identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved. The change does what the description says, and I found no defects. I read commit 6d83144.
- PR builds: in the
PRrun 36466014262 on this commit, the east login passed in all 7 build jobs. Every image that the jobs pushed got the same tags in both regions. - Not run yet: the
post-merge-ci.ymlandnightly-ci.ymlbuilds push themain-*and*-nightlytags to east for the first time. I ran their tag scripts locally on the base and on this commit (first table). The firstpost-merge-ci.ymlrun onmainafter the merge is the end-to-end test for these tags. - Cost, for information only: the push to east adds time to each build job. On this commit, the push phase of five runtime images took 57 to 184 s longer than on other PRs today (second table). This is one run. When the east repository already holds the base layers, later runs can be faster.
Tag lists on the base and on this commit, with the same inputs.
I ran the scripts with bash 5.3 and -e -o pipefail, the flags that GitHub uses for shell: bash. The registry host was a fake one, and docker was a stub that prints its arguments.
| Step and case | Base | This commit | Result |
|---|---|---|---|
| Extra tags, PR build, CUDA 13 | 1 tag | 3 tags | The same 3 tags as in the log of run 36466014262 |
Extra tags, post-merge on main |
3 tags | 7 tags | Each west tag has one east copy, plus the east SHA tag |
| Extra tags, post-merge on a release branch | 1 tag | 3 tags | Each west tag has one east copy, plus the east SHA tag |
| Extra tags, nightly, CUDA 13 | 3 tags | 7 tags | Each west tag has one east copy, plus the east SHA tag |
| Extra tags, CPU image (planner) | 0 tags | 1 tag | The east SHA tag only |
Extra tags, primary registry not in us-west-2 |
exit 0 | exit 1 | The new guard stops the step |
| Extra tags, empty registry variable | exit 0 | exit 1 | The new guard stops the step |
| Test image, PR build | 1 tag | 2 tags | Each west tag has one east copy |
Test image, post-merge on main |
2 tags | 4 tags | Each west tag has one east copy |
| Test image, release branch | 1 tag | 2 tags | Each west tag has one east copy |
Push phase in seconds, this commit against the same jobs on other PRs today.
The push phase is the time from the last exporting manifest list line to the end of the build step. The other PRs are 6 green PR runs from today (4 for frontend). They push to west only.
| Image | Other PRs, median (max) | This commit |
|---|---|---|
| trtllm-runtime | 14.0 (18.4) | 134.8 |
| vllm-runtime | 8.4 (9.9) | 107.3 |
| sglang-runtime | 6.7 (7.8) | 190.7 |
| triton-runtime | 8.6 (9.1) | 72.2 |
| dynamo-runtime | 55.5 (68.0) | 112.1 |
| dynamo-planner | 9.6 (13.4) | 18.7 |
| dynamo-frontend | 62.2 (68.1) | 76.3 |
| Test images | 3.0 (5.1) | 7.3 to 13.4 |
|
Since both registries use the same AWS account, could we define the east registry once in env:
ECR_REGISTRY_EAST: ${{ secrets.AWS_ACCOUNT_ID }}.dkr.ecr.us-east-1.amazonaws.comThis would remove the three repeated hostname substitutions and unchanged-value checks. |
Signed-off-by: Lavanya <lvijayakrish@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved again. The new commit keeps the behavior of 6d83144, the commit that I approved before, and I found no defects in the code. I read commit a3c170a.
- The commit does what sara4dev asked. Line 214 defines the east registry once in
jobs.build.env. The three places that built it from the west registry, and the three region tests, are gone. - [P3] The description still has the bullet
Fail early if the configured primary registry is not in us-west-2.This commit removed that test, so please delete the bullet. This item does not block. - PR builds: in the
PRrun 36482208834 on this commit, the east login passed in all 7 build jobs. Each runtime image got the same tags in both regions, as in run 36466014262 on the approved commit. - Not run yet: the
post-merge-ci.ymlandnightly-ci.ymlbuilds. My local runs of the changed steps give the same tag lists as the approved commit (first table). The firstpost-merge-ci.ymlrun onmainafter the merge is the end-to-end test for these tags. - For information: the job now reads the
AWS_ACCOUNT_IDsecret, so GitHub masks the account in the logs of this job. Each image name in the build logs now shows***in place of the account (second table). The pushed tags do not change.
Local runs of the three changed steps, on the approved commit and on this commit.
I ran each step with bash 5.3 and -e -o pipefail, the flags that GitHub uses for shell: bash. I took each step from the workflow file with a YAML parser. The account was a fake one, and docker and aws were stubs that print their arguments.
| Step and case | Approved commit | This commit |
|---|---|---|
| Extra tags, PR build, CUDA 13 | 3 tags | The same 3 tags, in the same order |
Extra tags, post-merge on main |
7 tags | The same 7 tags, in the same order |
| Extra tags, post-merge on a release branch | 3 tags | The same 3 tags, in the same order |
| Extra tags, nightly, CUDA 13 | 7 tags | The same 7 tags, in the same order |
| Extra tags, CPU image (planner) | 1 tag | The same tag |
Test image, PR, post-merge on main and release branch |
2, 4 and 2 tags | The same docker arguments |
| East login | The east host and a token | The same host and token |
East login, aws fails |
Exit 255 | Exit 255 |
Primary registry not in us-west-2 |
Exit 1 | Exit 0. The east copy goes to us-east-1. |
| Registry variable empty | Exit 1 | Exit 0, as before this PR |
| Secret empty | Not used | Exit 0. The east tags start with .dkr.ecr, and Docker rejects them with invalid reference format. |
| Job env missing | Not used | Exit 1, ECR_REGISTRY_EAST: unbound variable |
The workflow declares the secret as required, and all 40 calls of this workflow pass it with secrets: inherit. For the empty secret, I ran only the tag parser of Docker (docker tag, with no daemon). I did not run buildx or docker login.
Build logs of this commit, of the approved commit, and of two other PRs from the same hour.
I counted the lines that name the west registry in each build log.
| Build logs | East login passed | Account shown | Account shown as *** |
|---|---|---|---|
| This commit, run 36482208834 | 7 of 7 | 0 lines in each log | 21 to 64 lines per log |
| Approved commit, run 36466014262 | 7 of 7 | 22 to 65 lines per log | 0 lines in each log |
Other PRs, runs 36482302808 and 36483610279, vllm-runtime build |
No east login step | 35 lines per log | 0 lines in each log |
In both runs of this PR, each runtime image got one east tag for each west tag. The tags are the same in the two runs. Five of the six test images print their tags, and each shows one east tag for each west tag.
The mask covers the whole account, so the secret holds the same account as the west registry. No job output of this workflow holds an image name. No log shows the GitHub warning for a dropped output. The step summary that lists image names runs only for callers that set show_summary, and no caller sets it.
Signed-off-by: Lavanya <lvijayakrish@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved again. I found no new defects in the two new commits. I read commit 9f34a9e.
- The new commit 105ece3 does what sara4dev asked in the open thread on
shared-build-image.yml. I leave that thread for sara4dev to resolve. - Callers that do not pass
ecr_registry_eastkeep their old logins. In thePRrun 36493581092 on this commit, the east login ran in the 6 image builds that finished. It was skipped in the 5 jobs of other callers (first table). - With
push_image: false, the workflow passes an empty input, and the action skips the east login. No job on this commit used that path. The second table shows each part of it in a runner log. - The tag steps did not change since the approved commit. Each west tag still gets exactly one east copy, in my local runs and in the build logs of run 36493581092 (third table).
- [P3] Still open from my last review: the description promises the removed
us-west-2check. It also names anEast ECR Loginstep inshared-build-image.yml, which is now indocker-login. This does not block. - The two red
vllm-runtime / Testjobs fail two tests intest_vllm_engine_generate.py. The same two tests fail onmainin runs 36479595583 and 36488605795, after #15179. They do not come from this PR. - Not run yet: the
pr.yamldev builds withpush_image: false, and thepost-merge-ci.ymlandnightly-ci.ymlbuilds.
Logins per job in the PR runs on the approved commit (36482208834) and on this commit (36493581092).
| Jobs | Caller of docker-login |
ecr_registry_east |
East login, approved commit | East login, this commit | Login Succeeded lines, both commits |
|---|---|---|---|---|---|
| 6 image builds that finished | shared-build-image.yml |
Set | Ran as a separate step | Ran in the action | 3: west ECR, east ECR, ACR |
Operator |
build-deploy-component |
Not passed | None | Skipped | 2: west ECR, ACR |
Power Agent |
build-deploy-component |
Not passed | None | Skipped | 1: west ECR |
Operator Integration |
pr.yaml |
Not passed | None | Skipped | 1: west ECR |
planner / Compliance cpu, amd64 and arm64 |
shared-compliance.yml |
Not passed | None | Skipped | 2: west ECR, ACR |
For each step inside a composite action, the job log has a line with its outcome, for example outcome=skipped. The table reads those lines. The two other callers, build-on-demand.yml and shared-build-sidecar.yml, do not pass ecr_registry_east and did not run in this PR. The amd64 compliance job on this commit was later cancelled at its 15-minute limit, in the scan step after its logins.
The two parts of the push_image: false path, each in a runner log.
| Part | Runner log |
|---|---|
false && X || '' gives an empty string, not false. |
Run 35456343466 of post-merge-ci.yml on release/1.5.0: github.ref_name == 'main' && 'main-vllm-runtime' || '' gave blank EXTRA_TAGS. The log has no tag that ends in :false. |
An empty input skips a step with if: ${{ inputs.X != '' }}. |
Run 36493581092, Operator job: build-deploy-component passes its empty default for ngc_ci_access_token, and NGC Login was skipped. |
If the AWS_ACCOUNT_ID secret is empty, ECR_REGISTRY_EAST is still not empty, because the job env adds the host suffix. The action then gets the same host with no account that the approved commit used. My local runs with stub aws and docker show this. I did not run a real docker login.
Local runs of the east login and the two tag steps, with bash 5.3 and stub aws and docker.
I took each step from the YAML files with a YAML parser. I ran each step with -e -o pipefail, as the runner does for shell: bash. The account was a fake one.
| Case | Approved commit | This commit |
|---|---|---|
East login, aws works |
docker login gets the east host and the token |
The same |
East login, aws fails |
Exit 255 | Exit 255 |
East login, aws fails and docker rejects an empty token |
Exit 1 | Exit 1 |
| Tag steps, 11 cases of PR, post-merge, nightly, CPU, and extra-tag builds | Each west tag has exactly one east copy | The same tags, in the same order |
As a control, I removed set -euo pipefail and the shell flags from a copy of the login step. When aws fails, that copy exits 0, so the test can see a step that does not stop. Three changed copies of the tag steps, with one east tag removed or repeated, each failed the tag check. In run 36493581092, the logs of 5 image builds show each runtime and test tag in both regions.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved again. The new commit only merges main, and the two files of this PR did not change. I read commit 788dde9.
- GitHub moved my last approval from 9f34a9e onto this commit. I approve again now that I read this commit.
- The merge has no manual changes. It changes nothing that the two files use, and no job or step that calls them (first table).
- In the
PRrun 36519848517 on this commit, the 7 image builds ran the east login. The 5 jobs of other callers skipped it (second table). - [P3] Still open: the description promises the removed
us-west-2check. It also names anEast ECR Loginstep inshared-build-image.yml, which is now indocker-login. This does not block. - The two red
vllm-runtime / Testjobs fail two cases oftest_tito_adapter_rejects_asymmetric_image_feature_objects. The same two cases fail onmainin run 36515585850. They do not come from this PR. - Not run yet: the
pr.yamldev builds withpush_image: false, and thepost-merge-ci.ymlandnightly-ci.ymlbuilds.
The merge of main changes nothing that this PR depends on.
| What I compared | Result |
|---|---|
This commit, and my own merge of 9f34a9e with main at 51b83df |
The same tree |
.github/ at 9f34a9e and at this commit |
Only filters.yaml, nightly-ci.yml, and post-merge-ci.yml changed, all from #15081 |
The 43 jobs that call shared-build-image.yml or dynamo-pipeline.yml |
No change |
The 6 steps that use docker-login |
No change |
| The 9 jobs that #15081 added or changed in those two workflows | None of them calls shared-build-image.yml or has a docker-login step. The two sidecar builds call shared-build-sidecar.yml, which does not pass ecr_registry_east, so they skip the east login. |
Local runs of the east login and the two tag steps, with bash 5.3 and stub aws and docker |
The same results as for 9f34a9e. Each west tag has exactly one east copy. |
actionlint 1.7.12 on .github/ of main, of 9f34a9e, and of this commit |
The same 99 findings on all three trees. None come from this PR. |
main is now at 8e41fd8. That is one docs commit later, with no change under .github/.
Logins per job in the PR run 36519848517 on this commit.
| Jobs | Caller of docker-login |
East ECR Login | Login Succeeded lines |
|---|---|---|---|
| 7 image builds | shared-build-image.yml |
Ran and passed | 3: west ECR, east ECR, ACR |
Operator |
build-deploy-component |
Skipped | 2: west ECR, ACR |
Power Agent |
build-deploy-component |
Skipped | 1: west ECR |
Operator Integration |
pr.yaml |
Skipped | 1: west ECR |
planner / Compliance cpu, amd64 and arm64 |
shared-compliance.yml |
Skipped | 2: west ECR, ACR |
In the logs of the 6 multi-arch builds, each tag of the runtime, frontend, planner, and test images has one copy in each region. The seventh build, triton-runtime, builds one platform. Its build command has both tags, but its log has no line that lists the pushed tags.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved again. The new commit only merges main, and it changes nothing that this PR uses. I read commit 7e8e1f3.
- GitHub moved my last approval from 788dde9 onto this commit. I approve again now that I read this commit.
- The merge has no manual changes, and it changes no file under
.github/. The two files of this PR and all of their callers are the same as in 788dde9. - In the
PRrun 36609080493 on this commit, the 6 image builds that finished passed the east login. They pushed each tag to both regions. When I read this commit, thefrontendbuild was still running. - [P3] Still open: the description promises the removed
us-west-2check. It also names anEast ECR Loginstep inshared-build-image.yml, which is now indocker-login. This does not block. - Not run on this PR: the dev builds with
push_image: false, the EFA builds, and the builds inpost-merge-ci.ymlandnightly-ci.yml.
What I compared for this commit.
| What I compared | Result |
|---|---|
This commit, and my own merge of 788dde9 with main at 86d68a4 |
The same tree, with no conflicts |
.github/ at 788dde9 and at this commit |
The same tree |
| The 5 files that the merge brought in | Rust code, a vLLM test, and docs. None of them is a workflow or an action. |
main now, at 4dc45f7 |
One more commit, #15244. It changes only the code owner files, and no caller of the two files of this PR reads them. This PR merges onto it with no conflicts. |
| Logs of the 6 image builds that finished | The east login passed. In 5 logs, each pushed tag has one copy in each region. The triton-runtime build makes one platform, and its build command has both tags. |
Logs of Operator and planner / Compliance cpu, arm64 |
The east login step did not run, because these callers do not pass the east registry. |
Overview:
Summary
Publish Dynamo runtime and test images to both west and east AWS ECR registries. West ECR remains the
primary registry and existing image output contract.
Details:
us-west-2.Where should the reviewer start?
Start with
.github/workflows/shared-build-image.yml, specifically:East ECR LoginCalculate extra tagsBuild and Push Test ImageThese sections contain the registry authentication and dual-region tag publication changes.
Validation:
pre-commit run --files .github/workflows/shared-build-image.ymlgit diff --checkRelated Issues
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit