[no-relnote] Add E2E for libnvidia-container #1118

ArangoGutierrez · 2025-05-30T14:35:15Z

This patch adds an E2E test for the nvidia-container-cli that will allow us to catch regressions on libnvidia-container

coveralls · 2025-05-30T14:37:27Z

Pull Request Test Coverage Report for Build 16348776804

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 35.012%

Totals
Change from base Build 16219798607:	0.0%
Covered Lines:	4442
Relevant Lines:	12687

💛 - Coveralls

tests/e2e/nvidia-container-cli_test.go

ArangoGutierrez · 2025-06-03T19:18:09Z

Tests pass, PR ready for review @elezar

elezar · 2025-06-05T15:04:10Z

tests/e2e/nvidia-container-cli_test.go

+docker run -d --name test-nvidia-container-cli \
+  --privileged \
+  --runtime=nvidia \


It is not scalable to have to MOUNT everything into this container. Note that when we still had some simple integration tests in the toolkit we used

nvidia-container-toolkit/tests/container/docker_test.sh

Line 20 in 27f5ec8

testing::docker::dind::setup() {

Can we rather adapt this?

sure, will work on that. we need to make this test more robust and scalable

elezar · 2025-07-17T08:27:55Z

tests/e2e/installer.go

-# Create a temporary directory
-TEMP_DIR="/tmp/ctk_e2e.$(date +%s)_$RANDOM"
-mkdir -p "$TEMP_DIR"
+: ${IMAGE:={{.Image}}}


Nit: Why did we swap the ordering of these?

As a general note to the scripts -- why are we using envvars at all and don't we just use {{.Image}} everywhere? Is there a case where IMAGE is already set to something else?

switched back

elezar · 2025-07-17T08:29:14Z

tests/e2e/nvidia-container-cli_test.go

+	var (
+		runner        Runner
+		testScript    = "/tmp/libnvidia-container-cli.sh"
+		dockerImage   = "ghcr.io/nvidia/container-toolkit:5e8c1411-ubuntu20.04"


Why are we hardcoding this?

Also, does this break once we switch to distroless?

The hardcoded image was a mistake; it was an experimental feature introduced by me to iterate.

On distroless, it should work as we are setting the --entrypoint /libnvidia-container-cli.sh , so as long as the distroless supports /usr/bin/env bash scripts, this test should work

I have now edited to adjust for the distroless image

elezar · 2025-07-17T08:30:29Z

tests/e2e/e2e_test.go

+	imageName = getRequiredEnvvar[string]("E2E_IMAGE_NAME")
+	imageTag = getRequiredEnvvar[string]("E2E_IMAGE_TAG")


Is there a reason we removed the conditional?

Yes, regardless of if we want to install or not the toolkit on the host, I want to be able to get these 2 variables.

elezar · 2025-07-17T08:32:59Z

tests/e2e/installer.go

@@ -28,11 +28,20 @@ var dockerInstallTemplate = `
 #! /usr/bin/env bash
 set -xe

-: ${IMAGE:={{.Image}}}
+# if the TEMP_DIR is already set, use it
+if [ -f /tmp/ctk_e2e_temp_dir.txt ]; then


Why are we managing this state at a shell-script level? Should we not create the temp dir once in the go code and then use it here in our template?

elezar · 2025-07-17T08:34:45Z

tests/e2e/nvidia-container-cli_test.go

+	// script are therefore a good indicator of whether the NVIDIA Container
+	// Toolkit is functioning correctly inside the container.


This is not what we're testing. We're testing the nvidia-container-cli specifically.

elezar · 2025-07-17T08:36:16Z

tests/e2e/nvidia-container-cli_test.go

+apt-get update -y && apt-get install -y curl gnupg2
+
+WORKDIR="$(mktemp -d)"
+ROOTFS="${WORKDIR}/rootfs"


Why do we need two directories? What about:

Suggested change

ROOTFS="${WORKDIR}/rootfs"

ROOTFS="$(mktemp -d)/rootfs"

agreed, added

elezar · 2025-07-17T08:37:15Z

tests/e2e/nvidia-container-cli_test.go

+var _ = Describe("nvidia-container-cli", Ordered, ContinueOnFailure, func() {
+	var (
+		runner        Runner
+		testScript    = "/tmp/libnvidia-container-cli.sh"


Are we guaranteed a single script across all tests?

elezar · 2025-07-17T08:38:35Z

tests/e2e/nvidia-container-cli_test.go

+		testScript    = "/tmp/libnvidia-container-cli.sh"
+		dockerImage   = "ghcr.io/nvidia/container-toolkit:5e8c1411-ubuntu20.04"
+		containerName = "nvidia-cli-e2e"
+		dockerRunCmd  string


Is a variable at this scope required?

elezar · 2025-07-17T08:39:12Z

tests/e2e/nvidia-container-cli_test.go

+		createScriptCmd := fmt.Sprintf(
+			"cat > %s <<'EOF'\n%s\nEOF\nchmod +x %s",
+			testScript, libnvidiaContainerCliTestTemplate, testScript,
+		)
+
+		_, _, err := runner.Run(createScriptCmd)
+		Expect(err).ToNot(HaveOccurred())


Should this not be in a BeforeAll?

No, as this comment made me realize #1118 (comment) and #1118 (comment) This test script is for this test case only. Yes, right now is the only test under this Describe, but I prefer to keep it local to the It so we are clear to add further test cases

elezar · 2025-07-17T08:40:14Z

tests/e2e/nvidia-container-cli_test.go

+		// If a container with the same name exists from a previous test run, remove it first.
+		runner.Run(fmt.Sprintf("docker rm -f %s", containerName))
+
+		// Build the docker run command (detached mode) from the template so it
+		// stays readable while still resulting in a single-line invocation.
+		dockerRunCmd = fmt.Sprintf(dockerRunCmdTemplate, containerName, testScript, dockerImage)
+
+		// Launch the container in detached mode.
+		_, _, err = runner.Run(dockerRunCmd)
+		Expect(err).ToNot(HaveOccurred())


Should this also be BeforeAll?

same reason as in #1118 (comment)

elezar · 2025-07-17T08:40:58Z

tests/e2e/nvidia-container-cli_test.go

+		hostOutput, _, err := runner.Run("nvidia-smi -L")
+		Expect(err).ToNot(HaveOccurred())
+
+		hostOutput = strings.TrimSpace(strings.ReplaceAll(hostOutput, "\r", ""))


Why are we replacing returns with "" and not " "?

We replace \r with the empty string to obtain a whitespace-minimal representation of each line before comparison; inserting a space would alter the content rather than merely normalizing it.

elezar · 2025-07-17T08:49:07Z

tests/e2e/nvidia-container-cli_test.go

+		// Poll the logs of the already running container until we observe
+		// the GPU list matching the host or until a 5-minute timeout elapses.
+		Eventually(func() string {
+			logs, _, err := runner.Run(fmt.Sprintf("docker logs %s", containerName))


If we're just looking for the last log line, does docker logs --tail 1 work?

I was not aware of the flag, thanks!

Copilot

Pull Request Overview

Adds an end-to-end test for the nvidia-container-cli to catch regressions in the libnvidia-container library.

Introduces a Ginkgo-driven E2E test that mounts a minimal Ubuntu rootfs and runs nvidia-smi inside a container.
Updates the CTK installer script to persist and reuse a temp directory across runs.
Simplifies environment variable loading and adds a focus filter in the Makefile for selective test runs.

Reviewed Changes

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

File	Description
tests/e2e/nvidia-container-cli_test.go	Adds a Ginkgo test that builds a minimal rootfs, configures it with `nvidia-container-cli`, and verifies GPU visibility.
tests/e2e/installer.go	Modifies the CTK installation template to read/write a persistent temp directory file.
tests/e2e/e2e_test.go	Removes conditional guards around image name/tag env vars so they’re always loaded.
tests/e2e/Makefile	Introduces `GINKGO_FOCUS` for targeted test execution and updates the ginkgo invocation.

Comments suppressed due to low confidence (2)

tests/e2e/Makefile:23

[nitpick] The comment says all tests run when GINKGO_FOCUS is unset, but the default is nvidia-container-cli; update the comment to match or adjust the default to actually run all tests.

# If GINKGO_FOCUS is not set, run all tests

tests/e2e/nvidia-container-cli_test.go:37

The apt-get update command doesn’t accept -y and will error; remove the -y flag so it runs successfully, e.g., apt-get update && apt-get install -y curl gnupg2.

apt-get update -y && apt-get install -y curl gnupg2

tests/e2e/installer.go

tests/e2e/nvidia-container-cli_test.go

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

ArangoGutierrez requested review from elezar and Copilot May 30, 2025 14:35

ArangoGutierrez self-assigned this May 30, 2025

This comment was marked as outdated.

Sign in to view

elezar reviewed May 30, 2025

View reviewed changes

tests/e2e/nvidia-container-cli_test.go Outdated Show resolved Hide resolved

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from 67cc2ec to 903737e Compare May 30, 2025 15:32

ArangoGutierrez requested a review from elezar May 30, 2025 15:32

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch 5 times, most recently from d0a338e to 8c42c14 Compare June 3, 2025 18:51

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from 8c42c14 to d905a49 Compare June 4, 2025 08:13

elezar reviewed Jun 5, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from d905a49 to 9674787 Compare June 5, 2025 16:18

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from 9674787 to 1a72738 Compare July 16, 2025 14:03

ArangoGutierrez requested review from elezar and Copilot July 16, 2025 14:03

This comment was marked as outdated.

Sign in to view

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from 1a72738 to f84c038 Compare July 16, 2025 15:27

elezar reviewed Jul 17, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from f84c038 to ecf9d64 Compare July 17, 2025 09:08

ArangoGutierrez requested review from elezar and Copilot July 17, 2025 09:09

This comment was marked as outdated.

Sign in to view

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch 2 times, most recently from 0181dde to 3cf4410 Compare July 17, 2025 09:14

ArangoGutierrez requested a review from Copilot July 17, 2025 09:14

This comment was marked as outdated.

Sign in to view

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from 3cf4410 to bf65552 Compare July 17, 2025 09:15

ArangoGutierrez requested a review from Copilot July 17, 2025 09:16

This comment was marked as outdated.

Sign in to view

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from bf65552 to 72738db Compare July 17, 2025 09:20

ArangoGutierrez requested a review from Copilot July 17, 2025 09:21

Copilot AI reviewed Jul 17, 2025

View reviewed changes

tests/e2e/installer.go Show resolved Hide resolved

tests/e2e/nvidia-container-cli_test.go Outdated Show resolved Hide resolved

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch 9 times, most recently from ace18c6 to f0de6c1 Compare July 17, 2025 14:43

[no-relnote] Add E2E for libnvidia-container

ec003a9

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

ArangoGutierrez force-pushed the e2e/nvidia-container-cli branch from f0de6c1 to ec003a9 Compare July 17, 2025 15:05

		imageName = getRequiredEnvvar[string]("E2E_IMAGE_NAME")
		imageTag = getRequiredEnvvar[string]("E2E_IMAGE_TAG")

		// script are therefore a good indicator of whether the NVIDIA Container
		// Toolkit is functioning correctly inside the container.

[no-relnote] Add E2E for libnvidia-container #1118

Are you sure you want to change the base?

[no-relnote] Add E2E for libnvidia-container #1118

Conversation

ArangoGutierrez commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

coveralls commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16348776804

Details

💛 - Coveralls

Uh oh!

Uh oh!

ArangoGutierrez commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

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!

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!

ArangoGutierrez Jul 17, 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!

elezar Jul 17, 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!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

ArangoGutierrez commented May 30, 2025 •

edited

Loading

coveralls commented May 30, 2025 •

edited

Loading

ArangoGutierrez Jul 17, 2025 •

edited

Loading

elezar Jul 17, 2025 •

edited

Loading