Skip to content

Default to cdi #910

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Default to cdi #910

wants to merge 3 commits into from

Conversation

elezar
Copy link
Member

@elezar elezar commented Feb 7, 2025

No description provided.

@elezar elezar marked this pull request as draft February 10, 2025 12:32
@elezar elezar mentioned this pull request Feb 11, 2025
Merged
@elezar elezar force-pushed the default-to-cdi branch 2 times, most recently from 323f7b8 to 3e2c4d9 Compare February 11, 2025 21:44
@elezar elezar added this to the Disable legacy code path by default milestone Feb 17, 2025
jgehrcke
jgehrcke reviewed Mar 21, 2025
View reviewed changes
internal/modifier/cdi.go
@@ -116,6 +117,24 @@ func getDevicesFromSpec(logger logger.Interface, ociSpec oci.Spec, cfg *config.C
return nil, nil
}

func normalizeDeviceList(logger logger.Interface, defaultKind string, devices ...string) []string {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a code comment showing an example of what this function is doing (supposed to do).

jgehrcke
jgehrcke reviewed Mar 21, 2025
View reviewed changes
cmd/nvidia-container-runtime/main_test.go
require.NotEmpty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 1, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Empty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's invert the world

jgehrcke
jgehrcke approved these changes Mar 21, 2025
View reviewed changes
Copy link

@jgehrcke jgehrcke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a drafty draft and I don't know what I am talking about. But we need to make progress here and you know what you are doing. So, approved!

cdesiniotis
cdesiniotis approved these changes Mar 21, 2025
View reviewed changes
Copy link
Contributor

@cdesiniotis cdesiniotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cmd/nvidia-container-runtime/main_test.go
@@ -125,8 +125,8 @@ func TestGoodInput(t *testing.T) {
// Check config.json for NVIDIA prestart hook
spec, err = cfg.getRuntimeSpec()
require.NoError(t, err, "should be no errors when reading and parsing spec from config.json")
require.NotEmpty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 1, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Empty(t, spec.Hooks, "there should be hooks in config.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require.Empty(t, spec.Hooks, "there should be hooks in config.json")
require.Empty(t, spec.Hooks, "there should be no hooks in config.json")

cmd/nvidia-container-runtime/main_test.go
require.NotEmpty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 1, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Empty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "no nvidia prestart hook should be inserted into config.json")

cmd/nvidia-container-runtime/main_test.go
require.NotEmpty(t, spec.Hooks, "there should be hooks in config.json")
require.Equal(t, 1, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Empty(t, spec.Hooks, "there should be no hooks in config.json")
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "exactly one nvidia prestart hook should be inserted correctly into config.json")
require.Equal(t, 0, nvidiaHookCount(spec.Hooks), "no nvidia prestart hook should be inserted into config.json")

internal/modifier/cdi.go
logger.Debugf("Ignoring duplicate device %q", name)
continue
if cfg.AcceptDeviceListAsVolumeMounts {
devices = normalizeDeviceList(logger, defaultKind, append(container.DevicesFromMounts(), container.CDIDevicesFromMounts()...)...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change I notice that we were only calling container.CDIDevicesFromMounts() to construct the list of devices specified as volume mounts, but now we are also calling container.DeviceFromMounts() -- is there a reason we weren't calling both functions prior?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@ArangoGutierrez ArangoGutierrez self-requested a review May 6, 2025 17:23
@ArangoGutierrez
Copy link
Collaborator

@elezar is this PR Ready for review, or why is it still set as Draft?

ArangoGutierrez
ArangoGutierrez approved these changes May 9, 2025
View reviewed changes
Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Just a couple comments

internal/modifier/cdi.go
logger.Debugf("Ignoring duplicate device %q", name)
continue
if cfg.AcceptDeviceListAsVolumeMounts {
devices = normalizeDeviceList(logger, defaultKind, append(container.DevicesFromMounts(), container.CDIDevicesFromMounts()...)...)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

internal/modifier/cdi.go
@@ -165,6 +184,8 @@ func filterAutomaticDevices(devices []string) []string {

func newAutomaticCDISpecModifier(logger logger.Interface, cfg *config.Config, devices []string) (oci.SpecModifier, error) {
logger.Debugf("Generating in-memory CDI specs for devices %v", devices)
// TODO: We should try to load the kernel modules and create the device nodes here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can edit this TODO now, after #976 (comment)

@elezar elezar modified the milestones: Disable legacy code path by default, v1.18.0 May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgehrcke jgehrcke jgehrcke approved these changes

@ArangoGutierrez ArangoGutierrez ArangoGutierrez approved these changes

@cdesiniotis cdesiniotis cdesiniotis approved these changes

Assignees

@elezar elezar

Labels
None yet
Projects
None yet
Milestone
v1.18.0
Development

Successfully merging this pull request may close these issues.
4 participants
@elezar @ArangoGutierrez @jgehrcke @cdesiniotis