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

Support the DevicePluginCDIDevices feature gate #1007

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jfroy
Copy link

@jfroy jfroy commented Sep 23, 2024

This patch adds support for the DevicePluginCDIDevices feature gate by adding spec.operator.useDevicePluginCDIDevicesFeature to ClusterPolicy. When this field is set, the operator sets the DEVICE_LIST_STRATEGY device plug-in environment variable to cdi-cri.

@jfroy jfroy force-pushed the deviceplugincdidevices branch 2 times, most recently from 435a38f to 0b0151d Compare September 23, 2024 14:58
@jfroy jfroy changed the title Support for the DevicePluginCDIDevices feature Support the DevicePluginCDIDevices feature gate Sep 23, 2024
@jfroy
Copy link
Author

jfroy commented Sep 23, 2024

@cdesiniotis @elezar

@@ -148,6 +148,9 @@ type OperatorSpec struct {
// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.displayName="On OpenShift, enable DriverToolkit image to build and install driver modules"
// +operator-sdk:gen-csv:customresourcedefinitions.specDescriptors.x-descriptors="urn:alm:descriptor:com.tectonic.ui:booleanSwitch"
UseOpenShiftDriverToolkit *bool `json:"use_ocp_driver_toolkit,omitempty"`

// UseDevicePluginCDIDevicesFeature indicates if the device plug-in should be configured to use the CDI devices feature
Copy link
Member

Choose a reason for hiding this comment

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

One question on the UX for this: Should this be under the cdi object in the cluster policy?

Copy link
Author

Choose a reason for hiding this comment

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

It's an option that only affects the operator, but is related to CDI. I don't know to which it binds more strongly. So I picked one. It's easy enough to change that in the PR, but of course hard after. Maybe other folks can chime in. Ultimately I will defer to you and your team.

setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), DeviceListStrategyEnvName, "cdi-cri")
} else {
setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), DeviceListStrategyEnvName, "envvar,cdi-annotations")
}
setContainerEnv(&(obj.Spec.Template.Spec.Containers[0]), CDIAnnotationPrefixEnvName, "nvidia.cdi.k8s.io/")
Copy link
Member

Choose a reason for hiding this comment

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

This block is also not relevant when using cdi-cri.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean to say that useDevicePluginCDIDevicesFeature should be stronger than cdi.enabled? The current design and implementation is that useDevicePluginCDIDevicesFeature does nothing unless cdi.enabled is set.

This patch adds support for the `DevicePluginCDIDevices` feature gate by
adding `spec.operator.useDevicePluginCDIDevicesFeature` to
`ClusterPolicy`.  When this field is set, the operator sets the
`DEVICE_LIST_STRATEGY` device plug-in environment variable to `cdi-cri`.

Signed-off-by: Jean-Francois Roy <jeroy@nvidia.com>
Copy link

copy-pr-bot bot commented Nov 6, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

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.

2 participants