Skip to content
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.

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

Add NRI device injector package #345

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

Jiaqicao257
Copy link
Contributor

No description provided.

log.Infof("Container %s: annotated device %q...", ctrName, d.Path)
deviceNRI, err := d.toNRIDevice()
if err != nil {
return nil, nil, err
Copy link

Choose a reason for hiding this comment

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

is there any difference on returning nil or the empty value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, as long as the plugin returns error, there won't be any adjustment made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samuelkarp @aojea added readme for the directory, PTAL, thanks!

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

This looks really good. A few suggestions, but otherwise looks good.

nri_device_injector/nri_device_injector.go Outdated Show resolved Hide resolved
nri_device_injector/nri_device_injector.go Outdated Show resolved Hide resolved
nri_device_injector/nri_device_injector.go Outdated Show resolved Hide resolved
nri_device_injector/nri_device_injector.go Outdated Show resolved Hide resolved
samuelkarp
samuelkarp previously approved these changes Dec 29, 2023
nri_device_injector/nri_device_injector.go Outdated Show resolved Hide resolved
@Jiaqicao257 Jiaqicao257 merged commit ff676b5 into GoogleCloudPlatform:master Jan 5, 2024
1 of 2 checks passed
@klueska
Copy link

klueska commented Jan 22, 2024

Can someone add some context on what this newly integrated NRI plugin is designed to accomplish? On the surface it seems like CDI is a better fit (and how we now do things in NVIDIA's GPU device plugin).

If we have any hope of eventually merging the GKE GPU plugin with NVIDIA's we will need to understand what is being added here and why it is necessary / how it differs from just using CDI.

@klueska
Copy link

klueska commented Jan 22, 2024

Also worth noting -- CDI devices were added to the device plugin API in Kubernetes 1.28, graduated to beta in 1.29, and will be promoted to GA in 1.30. And both containerd 1.7+ and cri-o support injecting them natively (without the need for a custom NRI plugin).

kubernetes/enhancements#4009

@Jiaqicao257
Copy link
Contributor Author

@klueska Hi Kevin, thanks for your comments! This NRI plugin is not related to GPU device plugin itself, but is related to the GPU feature TCPX, that's only supported on GKE 1.27 for now, and this plugin is used to provide unprivileged GPU access on a TCPX sidecar container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants