From e8aa4262e77aaf069fbdc2442bb2bc5451b535d9 Mon Sep 17 00:00:00 2001 From: Yoni Bettan Date: Sun, 11 Aug 2024 23:19:54 +0300 Subject: [PATCH] Set a default value for `setFirmwareClassPath` in the worker config. (#1170) When firmware is added to the driver-cotnainer there are 2 actions that were taken: 1. Copy the firmware files onto the node at `/var/lib/firmware` 2. Add `/var/lib/firmware` to `/sys/module/firmware_class/parameters/path` in order to update the kernel lookup directories for firmware files. In order to make this feature configurable, we will allow the user to choose the path on the nodes to write the firmware files to. In order to make it work with no additional changes, when firmware files are added to the driver-container, we added a default directory to add the firmware files to on the nodes. To keep it simple, we will also always add this path to the kernel lookup directories. Signed-off-by: Yoni Bettan --- config/manager/controller_config.yaml | 2 + docs/mkdocs/documentation/deploy_kmod.md | 4 +- docs/mkdocs/documentation/firmwares.md | 12 +- internal/controllers/nmc_reconciler.go | 54 +++--- internal/controllers/nmc_reconciler_test.go | 184 +++++++++++++++----- internal/worker/constants.go | 1 - 6 files changed, 181 insertions(+), 76 deletions(-) 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" )