diff --git a/controllers/object_controls.go b/controllers/object_controls.go index 28d39b130..61afcaa78 100644 --- a/controllers/object_controls.go +++ b/controllers/object_controls.go @@ -18,6 +18,8 @@ package controllers import ( "context" + "crypto/sha256" + "encoding/hex" "errors" "fmt" "maps" @@ -3271,10 +3273,32 @@ func getSanitizedKernelVersion(kernelVersion string) string { return strings.ToLower(sanitizedVersion) } +// kernelSuffixForPrecompiledDaemonSet is the kernel portion of a precompiled +// driver DaemonSet name. It rewrites '_' so the result is a legal name, and +// it keeps architecture and page-size tokens. getSanitizedKernelVersion drops +// those tokens, but the node selector and the image tag keep the raw kernel, +// so kernels that sanitize together still need different objects. +// +// Replacing '_' with '.' would give 5.14.0-el9.x86_64 and 5.14.0-el9.x86.64 +// the same name. A kernel that contains '_' also gets a short hash of the +// lowercased raw string. A kernel with no '_' keeps the name it had before. +func kernelSuffixForPrecompiledDaemonSet(kernelVersion string) string { + lower := strings.ToLower(kernelVersion) + suffix := strings.Trim(strings.ReplaceAll(lower, "_", "."), ".") + if !strings.Contains(lower, "_") { + return suffix + } + sum := sha256.Sum256([]byte(lower)) + return suffix + "-" + hex.EncodeToString(sum[:4]) +} + +func precompiledDriverDaemonSetName(kernelVersion, nodeOS string) string { + return commonDriverDaemonsetName + "-" + kernelSuffixForPrecompiledDaemonSet(kernelVersion) + "-" + nodeOS +} + func transformPrecompiledDriverDaemonset(obj *appsv1.DaemonSet, n ClusterPolicyController) (err error) { - sanitizedVersion := getSanitizedKernelVersion(n.currentKernelVersion) // prepare the DaemonSet to be kernel-version specific - obj.Name += "-" + sanitizedVersion + "-" + n.kernelVersionMap[n.currentKernelVersion] + obj.Name = precompiledDriverDaemonSetName(n.currentKernelVersion, n.kernelVersionMap[n.currentKernelVersion]) // add unique labels for each kernel-version specific Daemonset obj.Labels[precompiledIdentificationLabelKey] = precompiledIdentificationLabelValue @@ -4376,7 +4400,9 @@ func precompiledDriverDaemonsets(ctx context.Context, n ClusterPolicyController) } n.logger.V(1).Info("preparing pre-compiled driver daemonsets") + desired := make(map[string]struct{}, len(n.kernelVersionMap)) for kernelVersion, os := range n.kernelVersionMap { + desired[precompiledDriverDaemonSetName(kernelVersion, os)] = struct{}{} // set current kernel version n.currentKernelVersion = kernelVersion @@ -4394,11 +4420,46 @@ func precompiledDriverDaemonsets(ctx context.Context, n ClusterPolicyController) } } + // A name that only used the arch-stripped kernel is not in desired. + // Leave it in place when this pass failed to create the replacements. + if len(errs) == 0 && len(desired) > 0 { + if err := n.deleteUndesiredPrecompiledDaemonsets(ctx, desired); err != nil { + errs = append(errs, err) + } + } + // reset current kernel version n.currentKernelVersion = "" return overallState, errs } +func (n ClusterPolicyController) deleteUndesiredPrecompiledDaemonsets(ctx context.Context, desired map[string]struct{}) error { + list := &appsv1.DaemonSetList{} + err := n.client.List(ctx, list, + client.InNamespace(n.operatorNamespace), + client.MatchingLabels{ + precompiledIdentificationLabelKey: precompiledIdentificationLabelValue, + }) + if err != nil { + return fmt.Errorf("list precompiled driver daemonsets: %w", err) + } + var deleteErrs []error + for i := range list.Items { + ds := &list.Items[i] + if _, ok := desired[ds.Name]; ok { + continue + } + if !strings.HasPrefix(ds.Name, commonDriverDaemonsetName) { + continue + } + n.logger.Info("Deleting precompiled driver DaemonSet that does not match a current kernel", "Name", ds.Name) + if err := n.client.Delete(ctx, ds); err != nil && !apierrors.IsNotFound(err) { + deleteErrs = append(deleteErrs, fmt.Errorf("delete precompiled driver daemonset %s: %w", ds.Name, err)) + } + } + return errors.Join(deleteErrs...) +} + // ocpDriverToolkitDaemonSets goes through all the RHCOS versions // found in the cluster, sets `currentRhcosVersion` and calls the // original DaemonSet() function to create/update the RHCOS-specific diff --git a/controllers/object_controls_test.go b/controllers/object_controls_test.go index fab4485ca..f14986493 100644 --- a/controllers/object_controls_test.go +++ b/controllers/object_controls_test.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "log" + "maps" "os" "path/filepath" goruntime "runtime" @@ -47,6 +48,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/log/zap" gpuv1 "github.com/NVIDIA/gpu-operator/api/nvidia/v1" @@ -1939,6 +1941,154 @@ func TestGetSanitizedKernelVersion(t *testing.T) { } } +func TestPrecompiledDriverDaemonSetsStayDistinctWhenKernelsSanitizeTogether(t *testing.T) { + ctx := context.Background() + // TestGetSanitizedKernelVersion treats these as one string. RHEL uname -r + // and the NFD kernel-version.full label include the architecture. + const ( + x86Kernel = "5.14.0-427.37.1.el9_4.x86_64" + armKernel = "5.14.0-427.37.1.el9_4.aarch64" + ) + require.Equal(t, getSanitizedKernelVersion(x86Kernel), getSanitizedKernelVersion(armKernel)) + + nodes := &corev1.NodeList{} + require.NoError(t, clusterPolicyController.client.List(ctx, nodes)) + require.GreaterOrEqual(t, len(nodes.Items), 2) + + savedLabels := map[string]map[string]string{} + savedMap := clusterPolicyController.kernelVersionMap + savedPolicy := clusterPolicyController.singleton.DeepCopy() + defer func() { + for name, labels := range savedLabels { + node := &corev1.Node{} + if err := clusterPolicyController.client.Get(ctx, types.NamespacedName{Name: name}, node); err != nil { + t.Errorf("restore node %s: %v", name, err) + continue + } + node.Labels = labels + if err := clusterPolicyController.client.Update(ctx, node); err != nil { + t.Errorf("restore node %s labels: %v", name, err) + } + } + clusterPolicyController.kernelVersionMap = savedMap + if err := updateClusterPolicy(&clusterPolicyController, savedPolicy); err != nil { + t.Errorf("restore ClusterPolicy: %v", err) + } + }() + + kernels := []string{x86Kernel, armKernel} + for i := range kernels { + node := nodes.Items[i].DeepCopy() + savedLabels[node.Name] = maps.Clone(node.Labels) + node.Labels[nfdKernelLabelKey] = kernels[i] + require.NoError(t, clusterPolicyController.client.Update(ctx, node)) + } + + kernelVersionMap, err := clusterPolicyController.getKernelVersionsMap() + require.NoError(t, err) + require.Len(t, kernelVersionMap, 2) + clusterPolicyController.kernelVersionMap = kernelVersionMap + + cp := getDriverTestInput("precompiled") + require.NoError(t, updateClusterPolicy(&clusterPolicyController, cp)) + addState(&clusterPolicyController, filepath.Join(cfg.root, driverAssetsPath)) + defer func() { + if err := removeState(&clusterPolicyController, clusterPolicyController.idx-1); err != nil { + t.Errorf("remove driver state: %v", err) + return + } + clusterPolicyController.idx-- + }() + + _, err = clusterPolicyController.step() + require.NoError(t, err) + + list := &appsv1.DaemonSetList{} + require.NoError(t, clusterPolicyController.client.List(ctx, list, client.MatchingLabels{"app": "nvidia-driver-daemonset"})) + require.Len(t, list.Items, 2) + + got := map[string]string{} + for i := range list.Items { + ds := &list.Items[i] + kernel := ds.Spec.Template.Spec.NodeSelector[nfdKernelLabelKey] + got[kernel] = ds.Name + driver := findContainerByName(ds.Spec.Template.Spec.Containers, "nvidia-driver-ctr") + require.NotNil(t, driver) + require.Contains(t, driver.Image, kernel) + } + require.NotEqual(t, got[x86Kernel], got[armKernel]) + require.Contains(t, got[x86Kernel], "x86.64") + require.Contains(t, got[armKernel], "aarch64") +} + +func TestKernelSuffixKeepsDotAndUnderscoreKernelsDistinct(t *testing.T) { + dotted := "5.14.0-427.el9.x86.64" + underscored := "5.14.0-427.el9.x86_64" + require.NotEqual(t, + kernelSuffixForPrecompiledDaemonSet(dotted), + kernelSuffixForPrecompiledDaemonSet(underscored)) + require.Equal(t, "5.4.0-generic", kernelSuffixForPrecompiledDaemonSet("5.4.0-generic")) +} + +func precompiledDriverDS(namespace, name string) *appsv1.DaemonSet { + return &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + Labels: map[string]string{ + precompiledIdentificationLabelKey: precompiledIdentificationLabelValue, + }, + }, + } +} + +func TestDeleteUndesiredPrecompiledDaemonsets(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, appsv1.AddToScheme(scheme)) + + const ( + operatorNS = "gpu-operator" + legacyName = "nvidia-driver-daemonset-5.14.0-427.37.1.el9.4-rhcos" + keepName = "nvidia-driver-daemonset-5.14.0-427.37.1.el9.4.x86.64-rhcos" + otherName = "nvidia-driver-daemonset-5.14.0-427.37.1.el9.4-rhcos" + failName = "nvidia-driver-daemonset-fail" + laterName = "nvidia-driver-daemonset-later" + ) + legacy := precompiledDriverDS(operatorNS, legacyName) + keep := precompiledDriverDS(operatorNS, keepName) + otherNS := precompiledDriverDS("other", otherName) + k8s := fake.NewClientBuilder().WithScheme(scheme).WithObjects(legacy, keep, otherNS).Build() + n := ClusterPolicyController{ + client: k8s, + operatorNamespace: operatorNS, + logger: ctrl.Log.WithName("test"), + } + require.NoError(t, n.deleteUndesiredPrecompiledDaemonsets(ctx, map[string]struct{}{keepName: {}})) + + err := k8s.Get(ctx, types.NamespacedName{Namespace: operatorNS, Name: legacyName}, &appsv1.DaemonSet{}) + require.True(t, apierrors.IsNotFound(err)) + require.NoError(t, k8s.Get(ctx, types.NamespacedName{Namespace: operatorNS, Name: keepName}, &appsv1.DaemonSet{})) + require.NoError(t, k8s.Get(ctx, types.NamespacedName{Namespace: "other", Name: otherName}, &appsv1.DaemonSet{})) + + fail := precompiledDriverDS(operatorNS, failName) + later := precompiledDriverDS(operatorNS, laterName) + failing := fake.NewClientBuilder().WithScheme(scheme).WithObjects(fail, later). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if obj.GetName() == failName { + return fmt.Errorf("injected delete error") + } + return c.Delete(ctx, obj, opts...) + }, + }).Build() + n.client = failing + err = n.deleteUndesiredPrecompiledDaemonsets(ctx, map[string]struct{}{}) + require.ErrorContains(t, err, "injected delete error") + err = failing.Get(ctx, types.NamespacedName{Namespace: operatorNS, Name: laterName}, &appsv1.DaemonSet{}) + require.True(t, apierrors.IsNotFound(err)) +} + func TestServiceMonitor(t *testing.T) { const ( testNamespace = "test-namespace" diff --git a/internal/state/nodepool.go b/internal/state/nodepool.go index 21bde3f9d..ae48b34b6 100644 --- a/internal/state/nodepool.go +++ b/internal/state/nodepool.go @@ -139,9 +139,16 @@ func getNodePools(ctx context.Context, k8sClient client.Client, cr *nvidiav1alph nodePool.name = rhcosVersion } - if _, exists := nodePoolMap[nodePool.name]; !exists { + // Pool names drop arch and _64k. The node selector and the + // precompiled image tag keep the raw kernel, so x86_64 and + // aarch64 (or 4k and 64k) stay separate pools when those names match. + poolKey := nodePool.name + if nodePool.kernel != "" { + poolKey = nodePool.name + "/" + nodePool.kernel + } + if _, exists := nodePoolMap[poolKey]; !exists { logger.Info("Detected new node pool", "NodePool", nodePool) - nodePoolMap[nodePool.name] = nodePool + nodePoolMap[poolKey] = nodePool } } @@ -154,7 +161,10 @@ func sortedNodePools(nodePoolMap map[string]nodePool) []nodePool { nodePools = append(nodePools, nodePool) } sort.Slice(nodePools, func(i, j int) bool { - return nodePools[i].name < nodePools[j].name + if nodePools[i].name != nodePools[j].name { + return nodePools[i].name < nodePools[j].name + } + return nodePools[i].kernel < nodePools[j].kernel }) return nodePools } diff --git a/internal/state/nodepool_test.go b/internal/state/nodepool_test.go index e7e36955b..aea13853d 100644 --- a/internal/state/nodepool_test.go +++ b/internal/state/nodepool_test.go @@ -307,6 +307,57 @@ func TestGetNodePoolsPartitionsPrecompiledNodesByKernel(t *testing.T) { require.Equal(t, "5.15.0-70-generic_x86_64", nodePools[0].nodeSelector[nfdKernelLabelKey]) } +func TestGetNodePoolsKeepsPrecompiledKernelsThatSanitizeTogether(t *testing.T) { + require.NoError(t, corev1.AddToScheme(scheme.Scheme)) + + // x86_64 and aarch64 sanitize to the same string. See TestGetSanitizedKernelVersion. + const ( + x86Kernel = "5.14.0-427.37.1.el9_4.x86_64" + armKernel = "5.14.0-427.37.1.el9_4.aarch64" + ) + node := func(name, kernel string) *corev1.Node { + return &corev1.Node{ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: map[string]string{ + consts.GPUPresentLabel: "true", + consts.NVIDIADriverOwnerLabel: "driver-a", + nfdOSReleaseIDLabelKey: "rhel", + nfdOSVersionIDLabelKey: "9.4", + nfdOSVersionIDMajorLabelKey: "9", + nfdKernelLabelKey: kernel, + }, + }} + } + k8sClient := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects( + node("x86-node", x86Kernel), + node("arm-node", armKernel), + node("second-x86-node", x86Kernel), + ). + Build() + driver := &nvidiav1alpha1.NVIDIADriver{ + ObjectMeta: metav1.ObjectMeta{Name: "driver-a"}, + Spec: nvidiav1alpha1.NVIDIADriverSpec{ + UsePrecompiled: new(true), + }, + } + + nodePools, err := getNodePools(context.Background(), k8sClient, driver, false) + + require.NoError(t, err) + require.Len(t, nodePools, 2) + got := map[string]string{} + for _, pool := range nodePools { + require.Equal(t, "rhel9-5.14.0-427.37.1.el9.4", pool.name) + got[pool.kernel] = pool.nodeSelector[nfdKernelLabelKey] + } + require.Equal(t, map[string]string{ + x86Kernel: x86Kernel, + armKernel: armKernel, + }, got) +} + func TestGetNodePoolsPartitionsOpenShiftNodesByRHCOSVersion(t *testing.T) { require.NoError(t, corev1.AddToScheme(scheme.Scheme))