From 9dc420df83dbf7129289658a4285a4dd7e8b247a Mon Sep 17 00:00:00 2001 From: Yevgeny Shnaidman Date: Wed, 1 Jul 2026 22:38:23 +0300 Subject: [PATCH] Fix collision between sofdep unloading and hard symbol dependency scenario: 1.KMM Modules needs to load module_a, module_b, module_c 2. module_a has a hard symbol dependency on module_b. 3. modulesLoadingOrder defined as: [module_a, module_b, module_c] There is no problems when loading. When unloading using softdep, the following happens 1. module_a is unloaded first 2. module_b is unloaded automatically, due to hard symbol dependency 3. modprobe looks at softdep, sees that module_b is unloaded, and does not continue to module_c This commit changes the Unload flow: 1. no softdep is created. 2. modprobe -r ocmmand is provided with the list of modules defined in the modulesLoadingOrder This way it will go over every module in the command and try to unload it --- internal/pod/workerpodmanager.go | 8 ++--- internal/pod/workerpodmanager_test.go | 50 ++++++++++++++++----------- internal/worker/worker.go | 13 ++++--- internal/worker/worker_test.go | 19 ++++++++++ 4 files changed, 59 insertions(+), 31 deletions(-) diff --git a/internal/pod/workerpodmanager.go b/internal/pod/workerpodmanager.go index 8f47fcb68..942a7805d 100644 --- a/internal/pod/workerpodmanager.go +++ b/internal/pod/workerpodmanager.go @@ -166,7 +166,7 @@ func (wpmi *workerPodManagerImpl) LoaderPodTemplate(ctx context.Context, nmc cli return nil, fmt.Errorf("could not create the base Pod: %v", err) } - if nms.Config.Modprobe.ModulesLoadingOrder != nil { + if len(nms.Config.Modprobe.ModulesLoadingOrder) > 0 { if err = setWorkerSofdepConfig(pod, nms.Config.Modprobe.ModulesLoadingOrder); err != nil { return nil, fmt.Errorf("could not set software dependency for mulitple modules: %v", err) } @@ -238,11 +238,7 @@ func (wpmi *workerPodManagerImpl) UnloaderPodTemplate(ctx context.Context, nmc c return nil, fmt.Errorf("could not set the worker Pod's security context: %v", err) } - if nms.Config.Modprobe.ModulesLoadingOrder != nil { - if err = setWorkerSofdepConfig(pod, nms.Config.Modprobe.ModulesLoadingOrder); err != nil { - return nil, fmt.Errorf("could not set software dependency for mulitple modules: %v", err) - } - } + // we skip the softdep configuration, since in unload we will do it with one "modprobe -r" call if nms.Config.Modprobe.FirmwarePath != "" { firmwareHostPath := wpmi.workerCfg.FirmwareHostPath diff --git a/internal/pod/workerpodmanager_test.go b/internal/pod/workerpodmanager_test.go index dd0cc7048..e720854a3 100644 --- a/internal/pod/workerpodmanager_test.go +++ b/internal/pod/workerpodmanager_test.go @@ -484,6 +484,13 @@ cp -R /firmware-path/* /tmp/firmware-path; configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n ", "") } tolerationAnotationValue := "- effect: NoExecute\n key: test-key\n value: test-value\n" + annotations := map[string]string{ + configAnnotationKey: configAnnotationValue, + tolerationsAnnotationKey: tolerationAnotationValue, + } + if isLoaderPod { + annotations[modulesOrderKey] = modulesOrderValue + } pod := v1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: WorkerPodName(nmcName, moduleName), @@ -495,11 +502,7 @@ cp -R /firmware-path/* /tmp/firmware-path; actionLabelKey: string(action), constants.ModuleNameLabel: moduleName, }, - Annotations: map[string]string{ - configAnnotationKey: configAnnotationValue, - modulesOrderKey: modulesOrderValue, - tolerationsAnnotationKey: tolerationAnotationValue, - }, + Annotations: annotations, }, Spec: v1.PodSpec{ Tolerations: []v1.Toleration{ @@ -559,11 +562,6 @@ cp -R /firmware-path/* /tmp/firmware-path; MountPath: sharedFilesDir, ReadOnly: true, }, - { - Name: "modules-order", - ReadOnly: true, - MountPath: "/etc/modprobe.d", - }, }, }, }, @@ -601,21 +599,31 @@ cp -R /firmware-path/* /tmp/firmware-path; EmptyDir: &v1.EmptyDirVolumeSource{}, }, }, - { - Name: "modules-order", - VolumeSource: v1.VolumeSource{ - DownwardAPI: &v1.DownwardAPIVolumeSource{ - Items: []v1.DownwardAPIVolumeFile{ - { - Path: "softdep.conf", - FieldRef: &v1.ObjectFieldSelector{FieldPath: fmt.Sprintf("metadata.annotations['%s']", modulesOrderKey)}, - }, - }, + }, + }, + } + + if isLoaderPod { + softDepVolumeMount := v1.VolumeMount{ + Name: "modules-order", + ReadOnly: true, + MountPath: "/etc/modprobe.d", + } + pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, softDepVolumeMount) + softdepVolume := v1.Volume{ + Name: "modules-order", + VolumeSource: v1.VolumeSource{ + DownwardAPI: &v1.DownwardAPIVolumeSource{ + Items: []v1.DownwardAPIVolumeFile{ + { + Path: "softdep.conf", + FieldRef: &v1.ObjectFieldSelector{FieldPath: fmt.Sprintf("metadata.annotations['%s']", modulesOrderKey)}, }, }, }, }, - }, + } + pod.Spec.Volumes = append(pod.Spec.Volumes, softdepVolume) } if withFirmware { diff --git a/internal/worker/worker.go b/internal/worker/worker.go index c11be0872..d9a7f3399 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -146,14 +146,19 @@ func (w *worker) UnloadKmod(ctx context.Context, cfg *kmmv1beta1.ModuleConfig, f if cfg.Modprobe.Args != nil { args = append(args, cfg.Modprobe.Args.Unload...) } - - args = append(args, moduleName) + if len(cfg.Modprobe.ModulesLoadingOrder) > 0 { + w.logger.Info("Preparing to unload multiple modules", "modules", cfg.Modprobe.ModulesLoadingOrder) + args = append(args, cfg.Modprobe.ModulesLoadingOrder...) + } else { + w.logger.Info("Preparing to unload single module", "moduleName", moduleName) + args = append(args, moduleName) + } } - w.logger.Info("Unloading module", "name", moduleName) + w.logger.Info("Starting unloading", "args", args) if err := w.mr.Run(ctx, args...); err != nil { - return fmt.Errorf("could not unload module %s: %v", moduleName, err) + return fmt.Errorf("failed to unload module %q: %v", args, err) } //remove firmware files only (no directories) diff --git a/internal/worker/worker_test.go b/internal/worker/worker_test.go index d9a865cfc..0ed64f400 100644 --- a/internal/worker/worker_test.go +++ b/internal/worker/worker_test.go @@ -332,6 +332,25 @@ var _ = Describe("worker_UnloadKmod", func() { ) }) + It("should use ModulesLoadingOrder when set", func() { + cfg := v1beta1.ModuleConfig{ + ContainerImage: imageName, + Modprobe: v1beta1.ModprobeSpec{ + ModuleName: moduleName, + DirName: dirName, + ModulesLoadingOrder: []string{"a", "b", "c"}, + }, + } + + mr.EXPECT().Run(ctx, "-rvd", filepath.Join(sharedFilesDir, dirName), "a", "b", "c") + + Expect( + w.UnloadKmod(ctx, &cfg, ""), + ).NotTo( + HaveOccurred(), + ) + }) + It("should remove all firmware file only", func() { cfg := v1beta1.ModuleConfig{ ContainerImage: imageName,