Repository navigation
ci: Replace Harbor with Artifactory for Docker images - #1688
Conversation
|
👋 Hi ntsemah! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughJenkins CI matrix YAMLs updated to use NVIDIA Artifactory: registry host/auth/path changed, Kubernetes imagePullSecrets added, BASE_IMAGE build-args and docker login targets refactored to use ${registry_host}, and compile-step credentials switched to svc-nixl-new-artifactory-token. ChangesCI Registry Configuration Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ci/jenkins/lib/test-matrix.yaml:
- Around line 25-27: The YAML has a credential mismatch: update any references
to the old nixl_harbor_credentials to use the new svc-nixl-new-artifactory-token
value—specifically change the credentials entry (where credentials: references
nixl_harbor_credentials) and the build step credentialsId (where credentialsId:
nixl_harbor_credentials) to svc-nixl-new-artifactory-token so the registry_auth
(svc-nixl-new-artifactory-token) is used consistently for authentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58cf7bca-c87e-4bd5-a727-7d81293fae28
📒 Files selected for processing (1)
.ci/jenkins/lib/test-matrix.yaml
ab6ba31 to
ff6dadb
Compare
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ci/jenkins/lib/build-wheel-matrix.yaml:
- Line 129: The docker login invocation uses "https://${registry_host}" which
mismatches how credentials are looked up for pulls; update the docker/login call
that uses the string with registry_host (the line invoking docker login and the
variable ${registry_host}) to pass the registry server name only (i.e.,
"${registry_host}" or include ":port" only if pulls include it) so the auth key
matches image pulls; ensure the same registry_host variable is used without the
https:// scheme in the docker login command.
In @.ci/jenkins/lib/test-dl-matrix.yaml:
- Around line 100-108: The BASE_IMAGE reference is pulling from
${HARBOR_REGISTRY}/nixl/base/... even though this job publishes the fresh base
to Artifactory earlier; update the BASE_IMAGE build-arg or add an explicit sync
so the job uses the freshly produced Artifactory image instead of a potentially
stale Harbor copy. Specifically, change the --build-arg BASE_IMAGE assignment
used in the podman build command (the BASE_IMAGE variable) to point at the
Artifactory output registry/hostname used earlier in the matrix (or insert a
step that pushes the produced nixl-ci-dl-gpu-base-25.10-cuda13.0-ubuntu24.04
image into ${HARBOR_REGISTRY} before the podman build) so the same-job handoff
is deterministic.
In @.ci/jenkins/lib/test-matrix.yaml:
- Around line 97-105: The job is using
BASE_IMAGE=${HARBOR_REGISTRY}/nixl/base/... which points at Harbor instead of
the freshly published base image produced earlier; update the podman build
invocation to reference the new base image location (the Artifactory/ matrix
output variable that holds the just-built
nixl-ci-gpu-base-25.10-cuda13.0-ubuntu24.04 image) or add a mirror/push step
that copies the just-built base into ${HARBOR_REGISTRY} before the podman build;
modify the BASE_IMAGE argument used in the podman build (and any use of
${HARBOR_REGISTRY}) so the job consumes the current run's base image rather than
a stale Harbor artifact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 665e197e-6809-423c-a7f3-7cb7529da061
📒 Files selected for processing (4)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.ci/jenkins/lib/test-matrix.yaml (1)
96-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the repo-qualified Artifactory path for these image refs.
PR_IMAGE,BASE_IMAGE, and the downstreamdockerImagevalues now point at${registry_host}/nixl/..., but the Artifactory migration elsewhere in this PR uses a repo-qualified prefix like${registry_host}/sw-nbu-swx-nixl-docker-local/.... That drops the repository segment configured byregistry_path, so this step can push to one location while later consumers pull from another or from a non-existent repo. Build these refs from the shared Artifactory prefix instead of hardcoding/nixl/....Also applies to: 141-141, 156-156, 171-171, 186-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.ci/jenkins/lib/test-matrix.yaml around lines 96 - 104, The image refs are hardcoded to ${registry_host}/nixl/... which ignores the repo-qualified prefix used elsewhere; update the assignments and any downstream dockerImage references (specifically PR_IMAGE, BASE_IMAGE and the dockerImage variables) to build from the shared Artifactory prefix by inserting the configured registry path (e.g. use ${registry_host}/${registry_path}/nixl/...) so pushes and pulls use the same repo-qualified location across the pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.ci/jenkins/lib/test-matrix.yaml:
- Around line 96-104: The image refs are hardcoded to ${registry_host}/nixl/...
which ignores the repo-qualified prefix used elsewhere; update the assignments
and any downstream dockerImage references (specifically PR_IMAGE, BASE_IMAGE and
the dockerImage variables) to build from the shared Artifactory prefix by
inserting the configured registry path (e.g. use
${registry_host}/${registry_path}/nixl/...) so pushes and pulls use the same
repo-qualified location across the pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4dfbdeda-1cd9-4c25-9f86-cc1b120a71ac
📒 Files selected for processing (4)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml
|
/build |
|
/build |
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ci/jenkins/lib/test-matrix.yaml:
- Around line 95-107: The pipeline exposes Artifactory credentials by using
--creds ${REPO_USER}:${REPO_PASS} with set -x and passing them on the podman
build/push command lines; replace this by performing an authenticated session
before the build: run podman login against ${registry_host} using --username
${REPO_USER} and --password-stdin (feed ${REPO_PASS} via stdin or a masked
credential variable) and then remove the --creds arguments from the podman build
and podman push invocations (affecting the podman build --creds and podman push
--creds usages), and ensure any set -x tracing is disabled or scoped away from
the login step so secrets are not echoed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d41a5802-8484-4d5a-bdc4-7dbc5e8b6b43
📒 Files selected for processing (4)
.ci/jenkins/lib/build-matrix.yaml.ci/jenkins/lib/build-wheel-matrix.yaml.ci/jenkins/lib/test-dl-matrix.yaml.ci/jenkins/lib/test-matrix.yaml
|
/build |
|
/build |
1 similar comment
|
/build |
|
/ok to test 640e9da |
Use Artifactory instead of Harbor for Docker image operations Signed-off-by: Noam Tsemah <ntsemah@nvidia.com>
|
/build |
1 similar comment
|
/build |
|
Build with base images pushed to Artifactory: |
NirWolfer
left a comment
NirWolfer
left a comment
There was a problem hiding this comment.
Did you see that in some instances the upload itself takes over 20 minutes? seems very slow and maybe worth investigating further
|
/ok to test 9774890 |
## Summary Move the `nixl-ci-test-sanitizers` base images (`-asan` / `-tsan`) from Harbor to Artifactory, matching the other CI base images (#1688). ## Changes - `test-sanitizer-matrix.yaml`: switch `registry_host` / `registry_auth` / `registry_path` to `artifactory.nvidia.com`, `svc-nixl-new-artifactory-token`, `/sw-nbu-swx-nixl-docker-local/ci` - Add the `artifactory-pull-secret` image pull secret to the Kubernetes config - No cleanup change needed: the images land under `ci/x86_64/`, already covered by the `ci/**` base-image rule in `.ci/cleanup-spec.json` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated the sanitizer test environment’s container image source and pull credentials. This affects automated testing infrastructure; no changes to end-user features or behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: NirWolfer <nwolfer@nvidia.com>
What?
Use Artifactory instead of Harbor for Docker image operations
Why?
Justification for the PR. If there is an existing issue/bug, please reference it. For
bug fixes, the 'Why?' and 'What?' can be merged into a single item.
How?
It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.
Summary by CodeRabbit