Changing the FirmwarePath config flag (#1172)

1) changing the setFirmwareClassPath flag to firmwareHostPath flag. This
   flag is used to:
   a) define the firmware path on the host
   b) define the mount point on the worker pod
   c) define the new firmware search path in the Linux kernel
2) updating NMC controller code to use the new flag for setting firmware
   volume for worker pods and passing parameters to worker pod
   entrypoint
3) updating unit-test

config/manager/controller_config.yaml

@@ -13,5 +13,4 @@ metrics:
 worker:
   runAsUser: 0
   seLinuxType: spc_t
-  #FIXME: rename
-  setFirmwareClassPath: /var/lib/firmware
+  firmwareHostPath: /var/lib/firmware

internal/config/config.go

@@ -25,9 +25,9 @@ type Webhook struct {
 }
 
 type Worker struct {
-	RunAsUser            *int64  `yaml:"runAsUser"`
-	SELinuxType          string  `yaml:"seLinuxType"`
-	SetFirmwareClassPath *string `yaml:"setFirmwareClassPath,omitempty"`
+	RunAsUser        *int64  `yaml:"runAsUser"`
+	SELinuxType      string  `yaml:"seLinuxType"`
+	FirmwareHostPath *string `yaml:"firmwareHostPath,omitempty"`
 }
 
 type LeaderElection struct {

internal/config/config_test.go

@@ -36,9 +36,9 @@ var _ = Describe("ParseFile", func() {
 				SecureServing:    true,
 			},
 			Worker: Worker{
-				RunAsUser:            ptr.To[int64](1234),
-				SELinuxType:          "mySELinuxType",
-				SetFirmwareClassPath: ptr.To("/some/path"),
+				RunAsUser:        ptr.To[int64](1234),
+				SELinuxType:      "mySELinuxType",
+				FirmwareHostPath: ptr.To("/some/path"),
 			},
 		}
 

internal/config/testdata/config.yaml

@@ -16,5 +16,5 @@ metrics:
 worker:
   runAsUser: 1234
   seLinuxType: mySELinuxType
-  setFirmwareClassPath: /some/path
+  firmwareHostPath: /some/path
 

internal/controllers/nmc_reconciler.go

@@ -850,17 +850,17 @@ func (p *podManagerImpl) LoaderPodTemplate(ctx context.Context, nmc client.Objec
 	privileged := false
 	if nms.Config.Modprobe.FirmwarePath != "" {
 
-		firmwareClassPath := p.workerCfg.SetFirmwareClassPath
-		if firmwareClassPath == nil {
-			return nil, fmt.Errorf("firmwarePath was set but firmwareClassPath wasn't set")
+		firmwareHostPath := p.workerCfg.FirmwareHostPath
+		if firmwareHostPath == nil {
+			return nil, fmt.Errorf("firmwareHostPath wasn't set, while the Module requires firmware loading")
 		}
 
-		args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareClassPath)
-		if err = setFirmwareVolume(pod, firmwareClassPath); err != nil {
+		args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareHostPath)
+		if err = setFirmwareVolume(pod, firmwareHostPath); err != nil {
 			return nil, fmt.Errorf("could not map host volume needed for firmware loading: %v", err)
 		}
 
-		args = append(args, "--"+worker.FlagFirmwareClassPath, *firmwareClassPath)
+		args = append(args, "--"+worker.FlagFirmwareClassPath, *firmwareHostPath)
 		privileged = true
 	}
 
@@ -904,13 +904,13 @@ func (p *podManagerImpl) UnloaderPodTemplate(ctx context.Context, nmc client.Obj
 	}
 
 	if nms.Config.Modprobe.FirmwarePath != "" {
-		firmwareClassPath := p.workerCfg.SetFirmwareClassPath
-		if firmwareClassPath == nil {
-			return nil, fmt.Errorf("firmwarePath was set but firmwareClassPath wasn't set")
+		firmwareHostPath := p.workerCfg.FirmwareHostPath
+		if firmwareHostPath == nil {
+			return nil, fmt.Errorf("firmwareHostPath was not set while the Module requires firmware unloading")
 		}
-		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.FlagFirmwareMountPath, *firmwareHostPath)
+		if err = setFirmwareVolume(pod, firmwareHostPath); err != nil {
+			return nil, fmt.Errorf("could not map host volume needed for firmware unloading: %v", err)
 		}
 	}
 
@@ -1199,28 +1199,28 @@ func setWorkerSofdepConfig(pod *v1.Pod, modulesLoadingOrder []string) error {
 	return nil
 }
 
-func setFirmwareVolume(pod *v1.Pod, hostFirmwarePath *string) error {
+func setFirmwareVolume(pod *v1.Pod, firmwareHostPath *string) error {
 	const volNameVarLibFirmware = "var-lib-firmware"
 	container, _ := podcmd.FindContainerByName(pod, workerContainerName)
 	if container == nil {
 		return errors.New("could not find the worker container")
 	}
 
-	if hostFirmwarePath == nil {
+	if firmwareHostPath == nil {
 		return errors.New("hostFirmwarePath must be set")
 	}
 
 	firmwareVolumeMount := v1.VolumeMount{
 		Name:      volNameVarLibFirmware,
-		MountPath: *hostFirmwarePath,
+		MountPath: *firmwareHostPath,
 	}
 
 	hostPathDirectoryOrCreate := v1.HostPathDirectoryOrCreate
 	firmwareVolume := v1.Volume{
 		Name: volNameVarLibFirmware,
 		VolumeSource: v1.VolumeSource{
 			HostPath: &v1.HostPathVolumeSource{
-				Path: *hostFirmwarePath,
+				Path: *firmwareHostPath,
 				Type: &hostPathDirectoryOrCreate,
 			},
 		},

internal/controllers/nmc_reconciler_test.go

@@ -1614,7 +1614,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 		ctx = context.TODO()
 	})
 
-	It("it should fail if firmwareClassPath was not set but firmware loading was", func() {
+	It("it should fail if firmwareHostPath was not set but firmware loading was", func() {
 
 		moduleConfigToUse.Modprobe.FirmwarePath = "/firmware-path"
 
@@ -1647,7 +1647,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 
 	DescribeTable(
 		"should work as expected",
-		func(firmwareClassPath *string, withFirmwareLoading bool) {
+		func(firmwareHostPath *string, withFirmwareLoading bool) {
 			ctrl := gomock.NewController(GinkgoT())
 			client := testclient.NewMockClient(ctrl)
 			psh := NewMockpullSecretHelper(ctrl)
@@ -1662,7 +1662,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 				Config:     moduleConfigToUse,
 			}
 
-			expected := getBaseWorkerPod("load", WorkerActionLoad, nmc, firmwareClassPath, withFirmwareLoading, true)
+			expected := getBaseWorkerPod("load", nmc, firmwareHostPath, withFirmwareLoading, true)
 
 			Expect(
 				controllerutil.SetControllerReference(nmc, expected, scheme),
@@ -1675,7 +1675,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 			container, _ := podcmd.FindContainerByName(expected, "worker")
 			Expect(container).NotTo(BeNil())
 
-			if withFirmwareLoading && firmwareClassPath != nil {
+			if withFirmwareLoading && firmwareHostPath != nil {
 				container.SecurityContext = &v1.SecurityContext{
 					Privileged: ptr.To(true),
 				}
@@ -1702,7 +1702,7 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 			)
 
 			workerCfg := *workerCfg
-			workerCfg.SetFirmwareClassPath = firmwareClassPath
+			workerCfg.FirmwareHostPath = firmwareHostPath
 
 			pm := &podManagerImpl{
 				caHelper:    caHelper,
@@ -1719,10 +1719,10 @@ var _ = Describe("podManagerImpl_CreateLoaderPod", func() {
 				HaveOccurred(),
 			)
 		},
-		Entry("pod without firmwareClassPath, without firmware loading", nil, false),
-		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("firmwareHostPath not set, firmware loading not requested", nil, false),
+		Entry("firmwareHostPath set to empty string, firmware loading not requested", ptr.To(""), false),
+		Entry("firmwareHostPath set, firmware loading not requested", ptr.To("some-path"), false),
+		Entry("firmwareHostPath set , firmware loading requested", ptr.To("some-path"), true),
 	)
 })
 
@@ -1791,7 +1791,7 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() {
 
 	It("should work as expected", func() {
 
-		expected := getBaseWorkerPod("unload", WorkerActionUnload, nmc, ptr.To("some-path"), true, false)
+		expected := getBaseWorkerPod("unload", nmc, ptr.To("some-path"), true, false)
 
 		container, _ := podcmd.FindContainerByName(expected, "worker")
 		Expect(container).NotTo(BeNil())
@@ -1817,7 +1817,7 @@ var _ = Describe("podManagerImpl_CreateUnloaderPod", func() {
 		)
 
 		workerCfg := *workerCfg
-		workerCfg.SetFirmwareClassPath = ptr.To("some-path")
+		workerCfg.FirmwareHostPath = ptr.To("some-path")
 		pm := newPodManager(client, workerImage, scheme, caHelper, &workerCfg)
 		pm.(*podManagerImpl).psh = psh
 
@@ -1919,7 +1919,7 @@ var _ = Describe("podManagerImpl_ListWorkerPodsOnNode", func() {
 	})
 })
 
-func getBaseWorkerPod(subcommand string, action WorkerAction, owner ctrlclient.Object, firmwareClassPath *string,
+func getBaseWorkerPod(subcommand string, owner ctrlclient.Object, firmwareHostPath *string,
 	withFirmware, isLoaderPod bool) *v1.Pod {
 	GinkgoHelper()
 
@@ -1932,6 +1932,11 @@ func getBaseWorkerPod(subcommand string, action WorkerAction, owner ctrlclient.O
 		volNameModulesOrder   = "modules-order"
 	)
 
+	action := WorkerActionLoad
+	if !isLoaderPod {
+		action = WorkerActionUnload
+	}
+
 	hostPathFile := v1.HostPathFile
 	hostPathDirectory := v1.HostPathDirectory
 	hostPathDirectoryOrCreate := v1.HostPathDirectoryOrCreate
@@ -1960,9 +1965,9 @@ softdep b pre: c
 
 	args := []string{"kmod", subcommand, "/etc/kmm-worker/config.yaml"}
 	if withFirmware {
-		args = append(args, "--set-firmware-mount-path", *firmwareClassPath)
-		if isLoaderPod && firmwareClassPath != nil {
-			args = append(args, "--set-firmware-class-path", *firmwareClassPath)
+		args = append(args, "--set-firmware-mount-path", *firmwareHostPath)
+		if isLoaderPod && firmwareHostPath != nil {
+			args = append(args, "--set-firmware-class-path", *firmwareHostPath)
 		}
 	} else {
 		configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n  ", "")
@@ -2139,14 +2144,14 @@ softdep b pre: c
 	if withFirmware {
 		fwVolMount := v1.VolumeMount{
 			Name:      volNameVarLibFirmware,
-			MountPath: *firmwareClassPath,
+			MountPath: *firmwareHostPath,
 		}
 		pod.Spec.Containers[0].VolumeMounts = append(pod.Spec.Containers[0].VolumeMounts, fwVolMount)
 		fwVol := v1.Volume{
 			Name: volNameVarLibFirmware,
 			VolumeSource: v1.VolumeSource{
 				HostPath: &v1.HostPathVolumeSource{
-					Path: *firmwareClassPath,
+					Path: *firmwareHostPath,
 					Type: &hostPathDirectoryOrCreate,
 				},
 			},