diff --git a/config/manager/controller_config.yaml b/config/manager/controller_config.yaml index e42e92f5c..fc8ac09cf 100644 --- a/config/manager/controller_config.yaml +++ b/config/manager/controller_config.yaml @@ -13,3 +13,5 @@ metrics: worker: runAsUser: 0 seLinuxType: spc_t + #FIXME: rename + setFirmwareClassPath: /var/lib/firmware diff --git a/docs/mkdocs/documentation/deploy_kmod.md b/docs/mkdocs/documentation/deploy_kmod.md index 54d3f5038..bfd5785b5 100644 --- a/docs/mkdocs/documentation/deploy_kmod.md +++ b/docs/mkdocs/documentation/deploy_kmod.md @@ -124,7 +124,9 @@ spec: dirName: /opt # Optional - # Optional. Will copy /firmware/* into /var/lib/firmware/ on the node. + # Optional. Will copy /firmware/* on the node into the path specified + # in the `kmm-operator-manager-config` at `worker.setFirmwareClassPath` + # before `modprobe` is called to insert the kernel module.. firmwarePath: /firmware parameters: # Optional diff --git a/docs/mkdocs/documentation/firmwares.md b/docs/mkdocs/documentation/firmwares.md index 43a522a6f..7313c11ed 100644 --- a/docs/mkdocs/documentation/firmwares.md +++ b/docs/mkdocs/documentation/firmwares.md @@ -3,8 +3,9 @@ Kernel modules sometimes need to load firmware files from the filesystem. KMM supports copying firmware files from the [kmod image](kmod_image.md) to the node's filesystem. -The contents of `.spec.moduleLoader.container.modprobe.firmwarePath` are copied into `/var/lib/firmware` on the node -before `modprobe` is called to insert the kernel module. +The contents of `.spec.moduleLoader.container.modprobe.firmwarePath` are copied +on the node into the path specified in the `kmm-operator-manager-config` configMap +at `worker.setFirmwareClassPath` before `modprobe` is called to insert the kernel module. All files and empty directories are removed from that location before `modprobe -r` is called to unload the kernel module, when the pod is terminated. @@ -42,7 +43,9 @@ spec: modprobe: moduleName: my-kmod # Required - # Optional. Will copy /firmware/* into /var/lib/firmware/ on the node. + # Optional. Will copy /firmware/* on the node into the path specified + # in the `kmm-operator-manager-config` at `worker.setFirmwareClassPath` + # before `modprobe` is called to insert the kernel module.. firmwarePath: /firmware # Add kernel mappings @@ -55,5 +58,6 @@ spec: The Linux kernel accepts the `firmware_class.path` parameter as a [search path for firmwares](https://www.kernel.org/doc/html/latest/driver-api/firmware/fw_search_path.html). Since version 2.0.0, KMM workers can set that value on nodes by writing to sysfs, before attempting to load kmods. -To enable that feature, set `worker.setFirmwareClassPath` to `/var/lib/firmware` in the +To enable that feature, set `worker.setFirmwareClassPath` in the [operator configuration](configure.md#workersetfirmwareclasspath). +The default value is `/var/lib/firmware` diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 5c87862fb..15cdbca21 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -839,12 +839,28 @@ func (p *podManagerImpl) LoaderPodTemplate(ctx context.Context, nmc client.Objec return nil, fmt.Errorf("could not create the base Pod: %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) + } + } + args := []string{"kmod", "load", configFullPath} privileged := false + if nms.Config.Modprobe.FirmwarePath != "" { - if p.workerCfg.SetFirmwareClassPath != nil { - args = append(args, "--"+worker.FlagFirmwareClassPath, *p.workerCfg.SetFirmwareClassPath) + firmwareClassPath := p.workerCfg.SetFirmwareClassPath + if firmwareClassPath == nil { + return nil, fmt.Errorf("firmwarePath was set but firmwareClassPath wasn't set") + } + + args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareClassPath) + if err = setFirmwareVolume(pod, firmwareClassPath); err != nil { + return nil, fmt.Errorf("could not map host volume needed for firmware loading: %v", err) + } + + args = append(args, "--"+worker.FlagFirmwareClassPath, *firmwareClassPath) privileged = true } @@ -856,19 +872,6 @@ func (p *podManagerImpl) LoaderPodTemplate(ctx context.Context, nmc client.Objec return nil, fmt.Errorf("could not set the worker Pod as privileged: %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) - } - } - - if nms.Config.Modprobe.FirmwarePath != "" { - args = append(args, "--"+worker.FlagFirmwareMountPath, worker.FirmwareMountPath) - if err = setFirmwareVolume(pod, p.workerCfg.SetFirmwareClassPath); err != nil { - return nil, fmt.Errorf("could not map host volume needed for firmware loading: %v", err) - } - } - if err = setWorkerContainerArgs(pod, args); err != nil { return nil, fmt.Errorf("could not set worker container args: %v", err) } @@ -901,8 +904,12 @@ func (p *podManagerImpl) UnloaderPodTemplate(ctx context.Context, nmc client.Obj } if nms.Config.Modprobe.FirmwarePath != "" { - args = append(args, "--"+worker.FlagFirmwareMountPath, worker.FirmwareMountPath) - if err = setFirmwareVolume(pod, p.workerCfg.SetFirmwareClassPath); err != nil { + firmwareClassPath := p.workerCfg.SetFirmwareClassPath + if firmwareClassPath == nil { + return nil, fmt.Errorf("firmwarePath was set but firmwareClassPath wasn't set") + } + args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareClassPath) + if err = setFirmwareVolume(pod, firmwareClassPath); err != nil { return nil, fmt.Errorf("could not map host volume needed for firmware loading: %v", err) } } @@ -1199,14 +1206,13 @@ func setFirmwareVolume(pod *v1.Pod, hostFirmwarePath *string) error { return errors.New("could not find the worker container") } - firmwareVolumeMount := v1.VolumeMount{ - Name: volNameVarLibFirmware, - MountPath: worker.FirmwareMountPath, + if hostFirmwarePath == nil { + return errors.New("hostFirmwarePath must be set") } - hostMountPath := "/var/lib/firmware" - if hostFirmwarePath != nil { - hostMountPath = *hostFirmwarePath + firmwareVolumeMount := v1.VolumeMount{ + Name: volNameVarLibFirmware, + MountPath: *hostFirmwarePath, } hostPathDirectoryOrCreate := v1.HostPathDirectoryOrCreate @@ -1214,7 +1220,7 @@ func setFirmwareVolume(pod *v1.Pod, hostFirmwarePath *string) error { Name: volNameVarLibFirmware, VolumeSource: v1.VolumeSource{ HostPath: &v1.HostPathVolumeSource{ - Path: hostMountPath, + Path: *hostFirmwarePath, Type: &hostPathDirectoryOrCreate, }, }, diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index e335fa83c..eca5608e6 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -4,12 +4,13 @@ import ( "context" "errors" "fmt" - "k8s.io/apimachinery/pkg/util/sets" "path/filepath" "reflect" "strings" "time" + "k8s.io/apimachinery/pkg/util/sets" + "github.com/budougumi0617/cmpmock" "github.com/mitchellh/hashstructure/v2" . "github.com/onsi/ginkgo/v2" @@ -1575,6 +1576,75 @@ var ( ) var _ = Describe("podManagerImpl_CreateLoaderPod", func() { + + const irsName = "some-secret" + + var ( + ctrl *gomock.Controller + client *testclient.MockClient + psh *MockpullSecretHelper + + nmc *kmmv1beta1.NodeModulesConfig + mi kmmv1beta1.ModuleItem + + ctx context.Context + moduleConfigToUse kmmv1beta1.ModuleConfig + caHelper *ca.MockHelper + ) + + BeforeEach(func() { + + ctrl = gomock.NewController(GinkgoT()) + client = testclient.NewMockClient(ctrl) + psh = NewMockpullSecretHelper(ctrl) + caHelper = ca.NewMockHelper(ctrl) + + nmc = &kmmv1beta1.NodeModulesConfig{ + ObjectMeta: metav1.ObjectMeta{Name: nmcName}, + } + + mi = kmmv1beta1.ModuleItem{ + ImageRepoSecret: &v1.LocalObjectReference{Name: irsName}, + Name: moduleName, + Namespace: namespace, + ServiceAccountName: serviceAccountName, + } + + moduleConfigToUse = moduleConfig + ctx = context.TODO() + }) + + It("it should fail if firmwareClassPath was not set but firmware loading was", func() { + + moduleConfigToUse.Modprobe.FirmwarePath = "/firmware-path" + + nms := &kmmv1beta1.NodeModuleSpec{ + ModuleItem: mi, + Config: moduleConfigToUse, + } + + pm := &podManagerImpl{ + client: client, + psh: psh, + scheme: scheme, + workerImage: workerImage, + workerCfg: workerCfg, + caHelper: caHelper, + } + + gomock.InOrder( + psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), + caHelper.EXPECT().GetClusterCA(ctx, namespace).Return(clusterCACM, nil), + caHelper.EXPECT().GetServiceCA(ctx, namespace).Return(serviceCACM, nil), + ) + + Expect( + pm.CreateLoaderPod(ctx, nmc, nms), + ).To( + HaveOccurred(), + ) + }) + DescribeTable( "should work as expected", func(firmwareClassPath *string, withFirmwareLoading bool) { @@ -1583,20 +1653,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { psh := NewMockpullSecretHelper(ctrl) caHelper := ca.NewMockHelper(ctrl) - nmc := &kmmv1beta1.NodeModulesConfig{ - ObjectMeta: metav1.ObjectMeta{Name: nmcName}, - } - - const irsName = "some-secret" - - mi := kmmv1beta1.ModuleItem{ - ImageRepoSecret: &v1.LocalObjectReference{Name: irsName}, - Name: moduleName, - Namespace: namespace, - ServiceAccountName: serviceAccountName, - } - - moduleConfigToUse := moduleConfig if withFirmwareLoading { moduleConfigToUse.Modprobe.FirmwarePath = "/firmware-path" } @@ -1606,7 +1662,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { Config: moduleConfigToUse, } - expected := getBaseWorkerPod("load", WorkerActionLoad, nmc, firmwareClassPath, withFirmwareLoading) + expected := getBaseWorkerPod("load", WorkerActionLoad, nmc, firmwareClassPath, withFirmwareLoading, true) Expect( controllerutil.SetControllerReference(nmc, expected, scheme), @@ -1619,7 +1675,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { container, _ := podcmd.FindContainerByName(expected, "worker") Expect(container).NotTo(BeNil()) - if firmwareClassPath != nil { + if withFirmwareLoading && firmwareClassPath != nil { container.SecurityContext = &v1.SecurityContext{ Privileged: ptr.To(true), } @@ -1638,8 +1694,6 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { expected.Annotations[hashAnnotationKey] = fmt.Sprintf("%d", hash) - ctx := context.TODO() - gomock.InOrder( psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), caHelper.EXPECT().GetClusterCA(ctx, namespace).Return(clusterCACM, nil), @@ -1669,35 +1723,75 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() { Entry("pod with empty firmwareClassPath, without firmware loading", ptr.To(""), false), Entry("pod with firmwareClassPath, without firmware loading", ptr.To("some-path"), false), Entry("pod with firmwareClassPath, with firmware loading", ptr.To("some-path"), true), - Entry("pod without firmwareClassPath, with firmware loading", nil, true), ) }) var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { - It("should work as expected", func() { - ctrl := gomock.NewController(GinkgoT()) - client := testclient.NewMockClient(ctrl) - psh := NewMockpullSecretHelper(ctrl) - caHelper := ca.NewMockHelper(ctrl) - nmc := &kmmv1beta1.NodeModulesConfig{ + const irsName = "some-secret" + + var ( + ctrl *gomock.Controller + client *testclient.MockClient + psh *MockpullSecretHelper + + nmc *kmmv1beta1.NodeModulesConfig + mi kmmv1beta1.ModuleItem + + ctx context.Context + moduleConfigToUse kmmv1beta1.ModuleConfig + status *kmmv1beta1.NodeModuleStatus + caHelper *ca.MockHelper + ) + + BeforeEach(func() { + + ctrl = gomock.NewController(GinkgoT()) + client = testclient.NewMockClient(ctrl) + psh = NewMockpullSecretHelper(ctrl) + caHelper = ca.NewMockHelper(ctrl) + + nmc = &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, } - mi := kmmv1beta1.ModuleItem{ + mi = kmmv1beta1.ModuleItem{ + ImageRepoSecret: &v1.LocalObjectReference{Name: irsName}, Name: moduleName, Namespace: namespace, ServiceAccountName: serviceAccountName, } - moduleConfigToUse := moduleConfig + moduleConfigToUse = moduleConfig moduleConfigToUse.Modprobe.FirmwarePath = "/firmware-path" - status := &kmmv1beta1.NodeModuleStatus{ + status = &kmmv1beta1.NodeModuleStatus{ ModuleItem: mi, Config: moduleConfigToUse, } + ctx = context.TODO() + }) - expected := getBaseWorkerPod("unload", WorkerActionUnload, nmc, nil, true) + It("it should fail if firmwareClassPath was not set but firmware loading was", func() { + + pm := newPodManager(client, workerImage, scheme, caHelper, workerCfg) + pm.(*podManagerImpl).psh = psh + + gomock.InOrder( + psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), + caHelper.EXPECT().GetClusterCA(ctx, namespace).Return(clusterCACM, nil), + caHelper.EXPECT().GetServiceCA(ctx, namespace).Return(serviceCACM, nil), + ) + + Expect( + pm.CreateUnloaderPod(ctx, nmc, status), + ).To( + HaveOccurred(), + ) + }) + + It("should work as expected", func() { + + expected := getBaseWorkerPod("unload", WorkerActionUnload, nmc, ptr.To("some-path"), true, false) container, _ := podcmd.FindContainerByName(expected, "worker") Expect(container).NotTo(BeNil()) @@ -1715,8 +1809,6 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { expected.Annotations[hashAnnotationKey] = fmt.Sprintf("%d", hash) - ctx := context.TODO() - gomock.InOrder( psh.EXPECT().VolumesAndVolumeMounts(ctx, &mi), caHelper.EXPECT().GetClusterCA(ctx, namespace).Return(clusterCACM, nil), @@ -1724,7 +1816,9 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() { client.EXPECT().Create(ctx, cmpmock.DiffEq(expected)), ) - pm := newPodManager(client, workerImage, scheme, caHelper, workerCfg) + workerCfg := *workerCfg + workerCfg.SetFirmwareClassPath = ptr.To("some-path") + pm := newPodManager(client, workerImage, scheme, caHelper, &workerCfg) pm.(*podManagerImpl).psh = psh Expect( @@ -1825,7 +1919,8 @@ var _ = Describe("podManagerImpl_ListWorkerPodsOnNode", func() { }) }) -func getBaseWorkerPod(subcommand string, action WorkerAction, owner ctrlclient.Object, firmwareClassPath *string, withFirmware bool) *v1.Pod { +func getBaseWorkerPod(subcommand string, action WorkerAction, owner ctrlclient.Object, firmwareClassPath *string, + withFirmware, isLoaderPod bool) *v1.Pod { GinkgoHelper() const ( @@ -1864,13 +1959,13 @@ softdep b pre: c ` args := []string{"kmod", subcommand, "/etc/kmm-worker/config.yaml"} - if firmwareClassPath != nil { - args = append(args, "--set-firmware-class-path", *firmwareClassPath) - } - if !withFirmware { - configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n ", "") + if withFirmware { + args = append(args, "--set-firmware-mount-path", *firmwareClassPath) + if isLoaderPod && firmwareClassPath != nil { + args = append(args, "--set-firmware-class-path", *firmwareClassPath) + } } else { - args = append(args, "--set-firmware-mount-path", "/var/lib/firmware") + configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n ", "") } pod := v1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -1898,6 +1993,7 @@ softdep b pre: c Limits: limits, Requests: requests, }, + SecurityContext: &v1.SecurityContext{}, VolumeMounts: []v1.VolumeMount{ { Name: volNameConfig, @@ -2041,20 +2137,16 @@ softdep b pre: c } if withFirmware { - hostPath := "/var/lib/firmware" - if firmwareClassPath != nil { - hostPath = *firmwareClassPath - } fwVolMount := v1.VolumeMount{ Name: volNameVarLibFirmware, - MountPath: "/var/lib/firmware", + MountPath: *firmwareClassPath, } pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, fwVolMount) fwVol := v1.Volume{ Name: volNameVarLibFirmware, VolumeSource: v1.VolumeSource{ HostPath: &v1.HostPathVolumeSource{ - Path: hostPath, + Path: *firmwareClassPath, Type: &hostPathDirectoryOrCreate, }, }, diff --git a/internal/worker/constants.go b/internal/worker/constants.go index 2cf929693..602c607c3 100644 --- a/internal/worker/constants.go +++ b/internal/worker/constants.go @@ -8,5 +8,4 @@ const ( ImagesDir = "/var/run/kmm/images" PullSecretsDir = "/var/run/kmm/pull-secrets" GlobalPullSecretPath = "/var/lib/kubelet/config.json" - FirmwareMountPath = "/var/lib/firmware" )