Skip to content

Commit f17d16d

Browse files
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
1 parent 36567c5 commit f17d16d

4 files changed

Lines changed: 59 additions & 31 deletions

File tree

internal/pod/workerpodmanager.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ func (wpmi *workerPodManagerImpl) LoaderPodTemplate(ctx context.Context, nmc cli
166166
return nil, fmt.Errorf("could not create the base Pod: %v", err)
167167
}
168168

169-
if nms.Config.Modprobe.ModulesLoadingOrder != nil {
169+
if len(nms.Config.Modprobe.ModulesLoadingOrder) > 0 {
170170
if err = setWorkerSofdepConfig(pod, nms.Config.Modprobe.ModulesLoadingOrder); err != nil {
171171
return nil, fmt.Errorf("could not set software dependency for mulitple modules: %v", err)
172172
}
@@ -238,11 +238,7 @@ func (wpmi *workerPodManagerImpl) UnloaderPodTemplate(ctx context.Context, nmc c
238238
return nil, fmt.Errorf("could not set the worker Pod's security context: %v", err)
239239
}
240240

241-
if nms.Config.Modprobe.ModulesLoadingOrder != nil {
242-
if err = setWorkerSofdepConfig(pod, nms.Config.Modprobe.ModulesLoadingOrder); err != nil {
243-
return nil, fmt.Errorf("could not set software dependency for mulitple modules: %v", err)
244-
}
245-
}
241+
// we skip the softdep configuration, since in unload we will do it with one "modprobe -r" call
246242

247243
if nms.Config.Modprobe.FirmwarePath != "" {
248244
firmwareHostPath := wpmi.workerCfg.FirmwareHostPath

internal/pod/workerpodmanager_test.go

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,13 @@ cp -R /firmware-path/* /tmp/firmware-path;
484484
configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n ", "")
485485
}
486486
tolerationAnotationValue := "- effect: NoExecute\n key: test-key\n value: test-value\n"
487+
annotations := map[string]string{
488+
configAnnotationKey: configAnnotationValue,
489+
tolerationsAnnotationKey: tolerationAnotationValue,
490+
}
491+
if isLoaderPod {
492+
annotations[modulesOrderKey] = modulesOrderValue
493+
}
487494
pod := v1.Pod{
488495
ObjectMeta: metav1.ObjectMeta{
489496
Name: WorkerPodName(nmcName, moduleName),
@@ -495,11 +502,7 @@ cp -R /firmware-path/* /tmp/firmware-path;
495502
actionLabelKey: string(action),
496503
constants.ModuleNameLabel: moduleName,
497504
},
498-
Annotations: map[string]string{
499-
configAnnotationKey: configAnnotationValue,
500-
modulesOrderKey: modulesOrderValue,
501-
tolerationsAnnotationKey: tolerationAnotationValue,
502-
},
505+
Annotations: annotations,
503506
},
504507
Spec: v1.PodSpec{
505508
Tolerations: []v1.Toleration{
@@ -559,11 +562,6 @@ cp -R /firmware-path/* /tmp/firmware-path;
559562
MountPath: sharedFilesDir,
560563
ReadOnly: true,
561564
},
562-
{
563-
Name: "modules-order",
564-
ReadOnly: true,
565-
MountPath: "/etc/modprobe.d",
566-
},
567565
},
568566
},
569567
},
@@ -601,21 +599,31 @@ cp -R /firmware-path/* /tmp/firmware-path;
601599
EmptyDir: &v1.EmptyDirVolumeSource{},
602600
},
603601
},
604-
{
605-
Name: "modules-order",
606-
VolumeSource: v1.VolumeSource{
607-
DownwardAPI: &v1.DownwardAPIVolumeSource{
608-
Items: []v1.DownwardAPIVolumeFile{
609-
{
610-
Path: "softdep.conf",
611-
FieldRef: &v1.ObjectFieldSelector{FieldPath: fmt.Sprintf("metadata.annotations['%s']", modulesOrderKey)},
612-
},
613-
},
602+
},
603+
},
604+
}
605+
606+
if isLoaderPod {
607+
softDepVolumeMount := v1.VolumeMount{
608+
Name: "modules-order",
609+
ReadOnly: true,
610+
MountPath: "/etc/modprobe.d",
611+
}
612+
pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, softDepVolumeMount)
613+
softdepVolume := v1.Volume{
614+
Name: "modules-order",
615+
VolumeSource: v1.VolumeSource{
616+
DownwardAPI: &v1.DownwardAPIVolumeSource{
617+
Items: []v1.DownwardAPIVolumeFile{
618+
{
619+
Path: "softdep.conf",
620+
FieldRef: &v1.ObjectFieldSelector{FieldPath: fmt.Sprintf("metadata.annotations['%s']", modulesOrderKey)},
614621
},
615622
},
616623
},
617624
},
618-
},
625+
}
626+
pod.Spec.Volumes = append(pod.Spec.Volumes, softdepVolume)
619627
}
620628

621629
if withFirmware {

internal/worker/worker.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,14 +146,19 @@ func (w *worker) UnloadKmod(ctx context.Context, cfg *kmmv1beta1.ModuleConfig, f
146146
if cfg.Modprobe.Args != nil {
147147
args = append(args, cfg.Modprobe.Args.Unload...)
148148
}
149-
150-
args = append(args, moduleName)
149+
if len(cfg.Modprobe.ModulesLoadingOrder) > 0 {
150+
w.logger.Info("Preparing to unload multiple modules", "modules", cfg.Modprobe.ModulesLoadingOrder)
151+
args = append(args, cfg.Modprobe.ModulesLoadingOrder...)
152+
} else {
153+
w.logger.Info("Preparing to unload single module", "moduleName", moduleName)
154+
args = append(args, moduleName)
155+
}
151156
}
152157

153-
w.logger.Info("Unloading module", "name", moduleName)
158+
w.logger.Info("Starting unloading", "args", args)
154159

155160
if err := w.mr.Run(ctx, args...); err != nil {
156-
return fmt.Errorf("could not unload module %s: %v", moduleName, err)
161+
return fmt.Errorf("failed to unload module %q: %v", args, err)
157162
}
158163

159164
//remove firmware files only (no directories)

internal/worker/worker_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,25 @@ var _ = Describe("worker_UnloadKmod", func() {
332332
)
333333
})
334334

335+
It("should use ModulesLoadingOrder when set", func() {
336+
cfg := v1beta1.ModuleConfig{
337+
ContainerImage: imageName,
338+
Modprobe: v1beta1.ModprobeSpec{
339+
ModuleName: moduleName,
340+
DirName: dirName,
341+
ModulesLoadingOrder: []string{"a", "b", "c"},
342+
},
343+
}
344+
345+
mr.EXPECT().Run(ctx, "-rvd", filepath.Join(sharedFilesDir, dirName), "a", "b", "c")
346+
347+
Expect(
348+
w.UnloadKmod(ctx, &cfg, ""),
349+
).NotTo(
350+
HaveOccurred(),
351+
)
352+
})
353+
335354
It("should remove all firmware file only", func() {
336355
cfg := v1beta1.ModuleConfig{
337356
ContainerImage: imageName,

0 commit comments

Comments
 (0)