Use native CDI in container runtimes when supported #1285
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,10 +161,11 @@ type ClusterPolicyController struct { | |
| openshift string | ||
| ocpDriverToolkit OpenShiftDriverToolkit | ||
|
|
||
| runtime gpuv1.Runtime | ||
| runtimeSupportsCDI bool | ||
|
Comment on lines
+164
to
+165
There was a problem hiding this comment. Is |
||
| hasGPUNodes bool | ||
| hasNFDLabels bool | ||
| sandboxEnabled bool | ||
| } | ||
|
|
||
| func addState(n *ClusterPolicyController, path string) { | ||
|
|
@@ -580,7 +581,7 @@ func (n *ClusterPolicyController) labelGPUNodes() (bool, int, error) { | |
| return clusterHasNFDLabels, gpuNodesTotal, nil | ||
| } | ||
|
|
||
| func getRuntimeVersionString(node corev1.Node) (gpuv1.Runtime, string, error) { | ||
| // ContainerRuntimeVersion string will look like <runtime>://<x.y.z> | ||
| runtimeVer := node.Status.NodeInfo.ContainerRuntimeVersion | ||
| var runtime gpuv1.Runtime | ||
|
|
@@ -592,9 +593,11 @@ func getRuntimeString(node corev1.Node) (gpuv1.Runtime, error) { | |
| case strings.HasPrefix(runtimeVer, "cri-o"): | ||
| runtime = gpuv1.CRIO | ||
| default: | ||
| return "", "", fmt.Errorf("runtime not recognized: %s", runtimeVer) | ||
| } | ||
| version := strings.SplitAfter(runtimeVer, "//")[1] | ||
| vVersion := "v" + strings.TrimPrefix(version, "v") | ||
| return runtime, vVersion, nil | ||
| } | ||
|
|
||
| func (n *ClusterPolicyController) setPodSecurityLabelsForNamespace() error { | ||
|
|
@@ -706,13 +709,14 @@ func (n *ClusterPolicyController) ocpEnsureNamespaceMonitoring() error { | |
| return nil | ||
| } | ||
|
|
||
| // getContainerRuntimeInfo will detect the container runtime version used by nodes | ||
| // in the cluster and correctly set the value for clusterPolicyController.runtime | ||
| // and clusterPolicyController.runtimeSupportsCDI. On OpenShift, the runtime | ||
| // is always assumed to be cri-o. We assume the runtime supports CDI unless | ||
| // containerd < 1.7.0 is detected. | ||
| func (n *ClusterPolicyController) getContainerRuntimeInfo() error { | ||
| ctx := n.ctx | ||
| n.runtimeSupportsCDI = true | ||
| // assume crio for openshift clusters | ||
| if n.openshift != "" { | ||
| n.runtime = gpuv1.CRIO | ||
|
|
@@ -725,27 +729,26 @@ func (n *ClusterPolicyController) getRuntime() error { | |
| list := &corev1.NodeList{} | ||
| err := n.client.List(ctx, list, opts...) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list nodes: %w", err) | ||
| } | ||
|
|
||
| var runtime gpuv1.Runtime | ||
| for i, node := range list.Items { | ||
| rt, version, err := getRuntimeVersionString(node) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get runtime info for node %s: %w", node.Name, err) | ||
| } | ||
| if i == 0 { | ||
| runtime = rt | ||
| } else if rt != runtime { | ||
| n.logger.Error(nil, "Different runtimes on different worker nodes is not supported") | ||
| return fmt.Errorf("different runtimes on different worker nodes is not supported") | ||
| } | ||
| if runtime == gpuv1.Containerd && semver.Compare(version, "v1.7.0") < 0 { | ||
|
There was a problem hiding this comment. Is it possible to query the CRI API and see if CDI is enabled ? or a similar check? That would be a more robust check IMO. It's possible that we may have forks of containerd running (rehashes based on a vanilla containerd with different versioning). This check wouldn't yield the expected result in those cases There was a problem hiding this comment. I think there was some talk about exposing this for use in DRA too, but it may not officially be part of the CRI. If I recall correctly, runtimes could send this information in the CRI status: https://github.com/containerd/containerd/blob/6f652853f01ef9ba340a860c2f39edf1701102d1/internal/cri/server/status.go#L34 and https://github.com/cri-o/cri-o/blob/02f3400b358159265d28a37df61be430404925e9/server/runtime_status.go#L15 I would be surprised if the There was a problem hiding this comment. Assuming this information is not available via the CRI, is there any other way we could potentially get this information instead of checking version strings here in the controller? There was a problem hiding this comment. For containerd, one could do a config dump and check whether via the CRI api. Not sure if this is visible in the context of the GPU Operator though. There was a problem hiding this comment. Ah, I see. What are your thoughts on adding this logic to the toolkit container? That is, the toolkit container would check if CDI is supported in containerd (by doing a config dump), and if supported it would ensure native-CDI is used for workloads by NOT configuring There was a problem hiding this comment. I think that sounds reasonable. Do we just assume native CDI support for CRI-O? In this mode of operation we would:
For Some questions:
There was a problem hiding this comment.
From the perspective of GPU Operator, I am comfortable saying yes since our next operator release will not support a K8s version that supports docker.
This is what I was envisioning as CRI-O has supported CDI since 1.23.2. Since we are shortening our K8s support matrix to n-3 at the time of release, and CRI-O follows the K8s release cycle with respect to minor versions, I think it is relatively safe to always assume native-CDI support for CRI-O. But maybe I am overlooking something.
I propose triggering this behavior whenever
I am assuming this is with respect to the device list strategy we configure in the plugin. I believe if we push the "native-CDI" detection logic to the toolkit container, we would have to do something similar in the plugin. That is, the behavior of the toolkit container and device-plugin when I'll try to write-up this proposal in more detail before the weekend. |
||
| n.runtimeSupportsCDI = false | ||
| } | ||
| } | ||
|
|
||
| n.runtime = runtime | ||
| return nil | ||
| } | ||
|
|
@@ -868,11 +871,12 @@ func (n *ClusterPolicyController) init(ctx context.Context, reconciler *ClusterP | |
| } | ||
|
|
||
| // detect the container runtime on worker nodes | ||
| err = n.getContainerRuntimeInfo() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get container runtime info: %w", err) | ||
| } | ||
| n.logger.Info(fmt.Sprintf("Using container runtime: %s", n.runtime.String())) | ||
| n.logger.Info(fmt.Sprintf("Container runtime supports CDI: %t", n.runtimeSupportsCDI)) | ||
|
|
||
| // fetch all kernel versions from the GPU nodes in the cluster | ||
| if n.singleton.Spec.Driver.IsEnabled() && n.singleton.Spec.Driver.UsePrecompiledDrivers() { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.