Allow cdi mode to work with --gpus flag #894
Conversation
elezar
added
the
must-backport
|
cc @tmonty12 |
elezar
force-pushed
the
remove-nvidia-container-runtime-hook-in-cdi-mode
branch
from
d7ffa9d to
b7762b1
Compare
elezar
requested review from
klueska,
tariq1890,
cdesiniotis and
ArangoGutierrez
elezar
force-pushed
the
remove-nvidia-container-runtime-hook-in-cdi-mode
branch
3 times, most recently
from
0ea4964 to
cb7c1cf
Compare
Signed-off-by: Evan Lezar <[email protected]>
This changes ensures that the cdi modifier also removes the NVIDIA Container Runtime Hook from the incoming spec. This aligns with what is done for CSV modifications and prevents an error when starting the container. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
remove-nvidia-container-runtime-hook-in-cdi-mode
branch
from
cb7c1cf to
03152db
Compare
| @@ -74,6 +74,12 @@ var _ = Describe("docker", Ordered, func() { | |||
| Expect(containerOutput).To(Equal(hostOutput)) | |||
| }) | |||
|
|
|||
| It("should support automatic CDI spec generation with the --gpus flag", func(ctx context.Context) { | |||
There was a problem hiding this comment.
Judging by the CI check names I see have run on this PR -- this test wasn't executed on those, right? (I was looking for some kind of e2e name).
If this test ran on this PR -- can you point me to a log?
If this test did not run on this PR -- can you briefly explain where/when we run this test? Also, did you run this manually?
(I trust that you did all the right things here; this is just a good opportunity for me to learn what we do/have)
There was a problem hiding this comment.
I don't think that this triggered the e2e tests, and if it did, I think it probably failed. We definitely need to improve the visibility / traceability of the tests.
One issue is that we trigger the tests once the images are built, but this does not seem to trigger a "checks" entry at a PR level.
| @@ -165,3 +166,181 @@ func TestFactoryMethod(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestNewSpecModifier(t *testing.T) { | |||
There was a problem hiding this comment.
Did this run as part of the CI checks on this PR?
Need to get used to interpreting such test runner log. I see ok github.com/NVIDIA/nvidia-container-toolkit/internal/runtime 0.059s coverage: 39.6% of statements, but I see no mention of TestNewSpecModifier, or of any of the strings below such as "csv mode removes nvidia-container-runtime-hook".
There was a problem hiding this comment.
We don't currently have verbose output enabled for the tests. Running locally with the diff:
diff --git a/Makefile b/Makefile
index be9da4bc..d2cf9b18 100644
--- a/Makefile
+++ b/Makefile
@@ -103,7 +103,7 @@ licenses:
COVERAGE_FILE := coverage.out
test: build cmds
- go test -coverprofile=$(COVERAGE_FILE) $(MODULE)/...
+ go test -v -coverprofile=$(COVERAGE_FILE) $(MODULE)/...
coverage: test
cat $(COVERAGE_FILE) | grep -v "_mock.go" > $(COVERAGE_FILE).no-mocks
$ make test > test-output-txt
$ grep TestNewSpecModifier test-output.txt
=== RUN TestNewSpecModifier
=== RUN TestNewSpecModifier/csv_mode_removes_nvidia-container-runtime-hook
=== RUN TestNewSpecModifier/csv_mode_removes_nvidia-container-toolkit
=== RUN TestNewSpecModifier/cdi_mode_removes_nvidia-container-runtime-hook
=== RUN TestNewSpecModifier/cdi_mode_removes_nvidia-container-toolkit
=== RUN TestNewSpecModifier/legacy_mode_keeps_nvidia-container-runtime-hook
=== RUN TestNewSpecModifier/legacy_mode_keeps_nvidia-container-toolkit
--- PASS: TestNewSpecModifier (0.00s)
--- PASS: TestNewSpecModifier/csv_mode_removes_nvidia-container-runtime-hook (0.00s)
--- PASS: TestNewSpecModifier/csv_mode_removes_nvidia-container-toolkit (0.00s)
--- PASS: TestNewSpecModifier/cdi_mode_removes_nvidia-container-runtime-hook (0.00s)
--- PASS: TestNewSpecModifier/cdi_mode_removes_nvidia-container-toolkit (0.00s)
--- PASS: TestNewSpecModifier/legacy_mode_keeps_nvidia-container-runtime-hook (0.00s)
--- PASS: TestNewSpecModifier/legacy_mode_keeps_nvidia-container-toolkit (0.00s)
This changes ensures that the cdi modifier also removes the NVIDIA
Container Runtime Hook from the incoming spec. This aligns with what is
done for CSV modifications and prevents an error when starting the
container.