Remove UBI sufix from image build #493

ArangoGutierrez · 2025-08-23T05:51:14Z

Fixes #489

Copilot

Pull Request Overview

This PR removes the "ubi9" suffix from container image tags as part of fixing issue #489. The change simplifies the image tagging strategy by moving away from distribution-specific suffixes to a cleaner tag format.

Updated Makefile to use version-only tags instead of version-distribution format
Changed default target from "ubi9" to "distroless"
Modified CI configuration to remove UBI-specific tag handling

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
deployments/container/Makefile	Updated image tagging logic and changed default distribution target
demo/clusters/kind/scripts/common.sh	Added TODO comment about removing UBI tag suffix
demo/clusters/gke/install-dra-driver-gpu.sh	Added TODO comment about removing UBI tag suffix
.nvidia-ci.yml	Removed UBI-specific tag suffix handling from CI pipeline

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

demo/clusters/kind/scripts/common.sh

demo/clusters/gke/install-dra-driver-gpu.sh

klueska · 2025-08-25T08:13:25Z

.nvidia-ci.yml

-# IN_IMAGE="${IN_IMAGE_NAME}:${IN_IMAGE_TAG}${TAG_SUFFIX}"
+# IN_IMAGE="${IN_IMAGE_NAME}:${IN_IMAGE_TAG}"
 # to
-# OUT_IMAGE="${OUT_IMAGE_NAME}:${OUT_IMAGE_TAG}${TAG_SUFFIX}"
+# OUT_IMAGE="${OUT_IMAGE_NAME}:${OUT_IMAGE_TAG}"


Why do we need this at all anymore? IN_IMAGE seems to be the same as OUT_IMAGE

About the diff: it's fine I think in this case -- the ${TAG_SUFFIX} concept I think was historically added in hindsight to this .copy-images this job. And this diff I believe effectively reverts that change.

More importantly, I think:

I have advocated before to not use the term "image name" to encode the full location including registry, to me that's the full image spec or even image URL (not just the name).

This is a reminder. Because:

IN_IMAGE seems to be the same as OUT_IMAGE

Good point.

AFAIU, the main purpose of this CI job is to copy an image from one registry to another, retaining the (what I would call) name and tag.

Here, we do encode the registry in the name. That is, IN_IMAGE and OUT_IMAGE differ in precisely (only) the registry. Example: copy from ghcr.io/nvidia/k8s-dra-driver-gpu:06db1a78-ubi9 to <nvidia-internal-registry>/k8s-dra-driver-gpu:06db1a78-ubi9

More context: I think .copy-images is somewhat a legacy-ish job, that is self-contained (provides an API that is useful). I think it's smart to not change it too much here. This API is consumed in the pull-images job defined elsewhere in this file:

pull-images: extends: - .copy-images

klueska · 2025-08-25T08:14:24Z

.nvidia-ci.yml

-      export SCAN_IMAGE=${IMAGE}${TAG_SUFFIX}
+      export SCAN_IMAGE=${IMAGE}


No need for a new variable if we are not adding the tag suffix

klueska · 2025-08-25T08:16:09Z

.nvidia-ci.yml

      rm -f ${VERSION_FILE}
-      echo "${IN_IMAGE_TAG}-ubi9 ${OUT_IMAGE_TAG}-ubi9" >> ${VERSION_FILE}
-      echo "${IN_IMAGE_TAG}-ubi9 ${OUT_IMAGE_TAG}" >> ${VERSION_FILE}
+      echo "${IN_IMAGE_TAG} ${OUT_IMAGE_TAG}" >> ${VERSION_FILE}
      cat ${VERSION_FILE}


What is in this VERSION_FILE exactly and how is it used? It feels unnecessary to copy both ${IN_IMAGE_TAG} and ${OUT_IMAGE_TAG} in here since they are now identical.

This is a templated default, see https://github.com/NVIDIA/nvidia-container-toolkit/blob/main/.nvidia-ci.yml#L224-L230. We also have IN and OUT variables, even if they are identical. Removing these should be treated as an independent followup PR

The discussed change looks OK-ish to me for now I think.

What is in this VERSION_FILE exactly and how is it used?

Good question (we should add more code comments here I think): this file is input to internal release tooling (internal ref). More specifically, this CI job is exercising our (team-internal) tooling for automatically submitting a merge request against an NGC publishing config repo. The output of this CI job is literally a merge request that needs to be reviewed. We exercise this CI job for every commit to test as much as we can about the actual release pipeline for every commit to main (without going all the way of actually releasing).

And to add more detail: the current left-over line

echo "${IN_IMAGE_TAG} ${OUT_IMAGE_TAG}" >> ${VERSION_FILE}

can be thought of as eventually building an instruction to copy an image from one registry to another.

Hence, this is not happening:

unnecessary to copy both ${IN_IMAGE_TAG} and ${OUT_IMAGE_TAG} in here since they are now identical

(I however empathize with much of this being barely readable / understandable :P).

What is in this VERSION_FILE exactly and how is it used? It feels unnecessary to copy both ${IN_IMAGE_TAG} and ${OUT_IMAGE_TAG} in here since they are now identical.

These are not identical. The IN_IMAGE_TAG is the short-commit-ref of the current HEAD and OUT_IMAGE_TAG is the human-readable version that we are releasing.

We are not copying anything here, we are adding a line:

aabbccdd v99.88.00

to a file which is processed by release tooling release the image with tag aabbccdd as v99.88.00 externally.

demo/clusters/gke/install-dra-driver-gpu.sh

demo/clusters/kind/scripts/common.sh

klueska · 2025-08-25T08:19:04Z

deployments/container/Makefile

+REGCTL ?= regctl
+
+BUILDX =


Suggested change

REGCTL ?= regctl

BUILDX =

REGCTL ?= regctl

BUILDX =

klueska · 2025-08-25T08:19:59Z

deployments/container/Makefile

-IMAGE_TAG ?= $(IMAGE_VERSION)-$(DIST)
+IMAGE_TAG ?= $(IMAGE_VERSION)
 IMAGE = $(IMAGE_NAME):$(IMAGE_TAG)

 OUT_IMAGE_NAME ?= $(IMAGE_NAME)
 OUT_IMAGE_VERSION ?= $(IMAGE_VERSION)
-OUT_IMAGE_TAG = $(OUT_IMAGE_VERSION)-$(DIST)
+OUT_IMAGE_TAG = $(OUT_IMAGE_VERSION)


It feels unnecessary to have both an IMAGE_TAG and an OUT_IMAGE_TAG now that they are identical.

Agree, but as commented on the .nvidia.yaml file, we need these IN and OUT variables for our templated GitLab CI pipelines. Modifying this is out of scope for this PR, as it would require a commit/PR specifically targeted at modifying our GitLab Pipeline

I am right now confused where this makefile flow is used at all (need to inspect that).

But generally, as pointed out in another comment here, I believe that these IN_ and OUT_ variables might differ in precisely the registry (and not image name/tag).

Note that before we had to switch to using the new release flow, we would copy images from our CI registries to NGC using regctl. In this case IMAGE_TAG and OUT_IMAGE_TAG did differ ince the former was derrived from the short-commit reference and the latter was the version that we were releasing. The tag also differed when "retagging" an image to drop the distribution-specific suffix, for example.

We did not properly audit the Makefile after the (somewhat rushed) migration to the new release flow.

klueska · 2025-08-25T08:20:39Z

deployments/container/Makefile

-DEFAULT_PUSH_TARGET := ubi9
-DISTRIBUTIONS = $(DEFAULT_PUSH_TARGET)
+DEFAULT_PUSH_TARGET := distroless
+DISTRIBUTIONS := $(DEFAULT_PUSH_TARGET)


What is the rationale behind a "default" push target now taht we only have one push target at all? It feels like this can be simplified.

I have simplified the Makefile now that we don't have the need for dynamic targets

klueska · 2025-08-25T08:22:02Z

deployments/container/Makefile

-.PHONY: $(DISTRIBUTIONS) $(PUSH_TARGETS) $(BUILD_TARGETS) $(TEST_TARGETS) $(BUILD_TARGETS)
+.PHONY: $(DISTRIBUTIONS) $(PUSH_TARGETS) $(BUILD_TARGETS) $(TEST_TARGETS)


Why is BUILD_TARGETS being dropped here? I see that it was listed twice previously and now we are reducing it to only be called once. @elezar was this an oversight or on purpose?

Now that we only have 1 target (no dynamic targets) I have simplified this

klueska · 2025-08-25T08:22:58Z

deployments/container/Makefile

-$(DEFAULT_PUSH_TARGET): DIST = $(DEFAULT_PUSH_TARGET)
+$(DEFAULT_PUSH_TARGET):


What is the purpose of having a make target here with no dependencies and no rules?

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

klueska · 2025-08-25T10:24:10Z

I'll leave the rest of the review to @elezar as he's more familiar with the inner working of this and what makes sense to change vs. leave as is, even in the presence of all these redundant variables.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

jgehrcke · 2025-09-03T11:38:12Z

For the record: an important question here is "do we want to replace -ubi9?" (with a different tag). Not discussed here I think, so I want to quickly add that. I think the answer is "no", and to me personally the strongest argument is that we have previously made that decision for the GPU Operator: https://github.com/NVIDIA/gpu-operator/pkgs/container/gpu-operator

There, we do something like this:

The image without the suffix is a multi-arch image, example: https://github.com/NVIDIA/gpu-operator/pkgs/container/gpu-operator/503315840?tag=c65b8bee.

I believe for multi-arch images to work, we do not need to explicitly push the per-architecture tags.

deployments/container/Makefile

jgehrcke · 2025-09-08T12:36:01Z

@ArangoGutierrez could you review this again today, and after all discussion double-check if this is in a state that is mergeable from your point of view (that might be the current state)?

IMO, it is OK to leave some (rarely or never exercised) local dev workflows behind knowingly broken, we can fix them out-of-band. The focus for this patch should be on what's exercised by CI.

Evan says he can maybe add feedback ~tomorrow, we can incorporate his feedback also for follow-up patches.

We need to land a pragmatic version ASAP. Reason for mild urgency: the earlier we try to land this, the earlier we see what we have forgotten to think about. We want to cut an RC release from the main branch any day now, and this change should be part of it.

jgehrcke · 2025-09-09T14:48:41Z

deployments/container/Makefile


 ifneq ($(BUILD_MULTI_ARCH_IMAGES),true)
 include $(CURDIR)/deployments/container/native-only.mk
 else
 include $(CURDIR)/deployments/container/multi-arch.mk
 endif

-# For the default push target we also push a short tag equal to the version.
+# For the main push target we also push a short tag equal to the version.


In one of the next iterations on this, we should reword this comment:

we also push a short tag equal to the version

I read this and wonder: What is a "short tag"? Where do we (want to) push and when?

Agree, this should be a followup TODO

If we don't have a distribution suffix, then we don't have a short tag. I would argue that we could remove all "push" logic (with the exception of push=true when building) from this makefile to simplify it significantly.

jgehrcke · 2025-09-09T14:50:04Z

deployments/container/Makefile


-# Use a generic build target to build the relevant images
-$(IMAGE_TARGETS): image-%:
+# Build the container image


For the record: this is also the target we use in CI to push (using push=true in the DOCKER_BUILD_OPTIONS).

None of the other push-related targets in this Makefile are used in CI, and I have personally so far never used them locally.

The push targets are a remnant of earlier release processes that have not yet been removed.

jgehrcke

Thanks!

This patch was sitting too long, and we want to make progress to get the new tag structure in place for the first 25.8.0 release candidate. Hence, I want to merge this now and deal with any potential fallout asap post-merge.

Considerations:

To focus on the most important aspects of this patch, we need post-merge signal (does this actually work when it runs on main?).
We can take more time in the future to review which Makefile targets are really still needed.
We should review & fix periphery scripts in independent work.

github-project-automation bot added this to Planning Board: k8s-dra-driver-gpu Aug 23, 2025

github-project-automation bot moved this to Backlog in Planning Board: k8s-dra-driver-gpu Aug 23, 2025

ArangoGutierrez requested a review from Copilot August 23, 2025 06:16

ArangoGutierrez self-assigned this Aug 23, 2025

ArangoGutierrez requested a review from jgehrcke August 23, 2025 06:16

Copilot AI reviewed Aug 23, 2025

View reviewed changes

demo/clusters/kind/scripts/common.sh Show resolved Hide resolved

demo/clusters/gke/install-dra-driver-gpu.sh Show resolved Hide resolved

klueska reviewed Aug 25, 2025

View reviewed changes

ArangoGutierrez force-pushed the i489 branch from c0e2775 to 3b835cc Compare August 25, 2025 09:13

ArangoGutierrez requested review from klueska and Copilot August 25, 2025 09:14

Remove UBI sufix from image build

4a3b87c

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the i489 branch from 3b835cc to 4a3b87c Compare August 25, 2025 09:38

klueska added this to the v25.8.0 milestone Aug 25, 2025

klueska added cleanup ci/testing issue/PR related to CI and/or testing build artifacts task/issue related to build artifacts (cont. images) labels Aug 25, 2025

Copilot AI reviewed Aug 25, 2025

View reviewed changes

jgehrcke reviewed Sep 8, 2025

View reviewed changes

deployments/container/Makefile Show resolved Hide resolved

jgehrcke reviewed Sep 9, 2025

View reviewed changes

jgehrcke approved these changes Sep 9, 2025

View reviewed changes

jgehrcke merged commit 9569c5b into NVIDIA:main Sep 9, 2025
13 checks passed

This was referenced Sep 10, 2025

[no-relnote] Fix ghcr Helm chart #553

Merged

[no-relnote] Cleanup makefile to build container image #554

Open

		export SCAN_IMAGE=${IMAGE}${TAG_SUFFIX}
		export SCAN_IMAGE=${IMAGE}

		.PHONY: $(DISTRIBUTIONS) $(PUSH_TARGETS) $(BUILD_TARGETS) $(TEST_TARGETS) $(BUILD_TARGETS)
		.PHONY: $(DISTRIBUTIONS) $(PUSH_TARGETS) $(BUILD_TARGETS) $(TEST_TARGETS)

		$(DEFAULT_PUSH_TARGET): DIST = $(DEFAULT_PUSH_TARGET)
		$(DEFAULT_PUSH_TARGET):

Remove UBI sufix from image build #493

Remove UBI sufix from image build #493

Conversation

ArangoGutierrez commented Aug 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klueska commented Aug 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

jgehrcke commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jgehrcke commented Sep 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke left a comment

jgehrcke Sep 3, 2025 •

edited

Loading

jgehrcke Sep 3, 2025 •

edited

Loading

jgehrcke commented Sep 3, 2025 •

edited

Loading

elezar Sep 10, 2025 •

edited

Loading