From 6b41e0415d77f1dbe1c22ab1eff48d946ff01ed9 Mon Sep 17 00:00:00 2001 From: Yevgeny Shnaidman <60741237+yevgeny-shnaidman@users.noreply.github.com> Date: Wed, 28 Aug 2024 20:38:50 +0300 Subject: [PATCH] Simplifying firmware flags between NMC controller and worker pod (#1183) Currently NMC controller passes 2 flag to worker pod: firmware path mount point in pod and firmware search path. Since in our implementation those 2 values are always equal, we can use only one flag to pass these values: --firmware-path, and let worker pod decide when to update the kernel configuration for firmware search --- cmd/worker/funcs_kmod.go | 25 +++++-------- cmd/worker/funcs_kmod_test.go | 40 ++++++++------------- internal/controllers/nmc_reconciler.go | 5 ++- internal/controllers/nmc_reconciler_test.go | 5 +-- internal/worker/constants.go | 3 +- 5 files changed, 27 insertions(+), 51 deletions(-) diff --git a/cmd/worker/funcs_kmod.go b/cmd/worker/funcs_kmod.go index f0d6eb774..82ed06a93 100644 --- a/cmd/worker/funcs_kmod.go +++ b/cmd/worker/funcs_kmod.go @@ -30,16 +30,15 @@ func kmodLoadFunc(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not read config file %s: %v", cfgPath, err) } - if flag := cmd.Flags().Lookup(worker.FlagFirmwareClassPath); flag.Changed { - logger.V(1).Info(worker.FlagFirmwareClassPath + " set, setting firmware_class.path") + mountPathFlag := cmd.Flags().Lookup(worker.FlagFirmwarePath) + if mountPathFlag.Changed { + logger.V(1).Info(worker.FlagFirmwarePath + " set, setting firmware_class.path") - if err := w.SetFirmwareClassPath(flag.Value.String()); err != nil { + if err := w.SetFirmwareClassPath(mountPathFlag.Value.String()); err != nil { return fmt.Errorf("could not set the firmware_class.path parameter: %v", err) } } - mountPathFlag := cmd.Flags().Lookup(worker.FlagFirmwareMountPath) - return w.LoadKmod(cmd.Context(), cfg, mountPathFlag.Value.String()) } @@ -53,22 +52,14 @@ func kmodUnloadFunc(cmd *cobra.Command, args []string) error { return fmt.Errorf("could not read config file %s: %v", cfgPath, err) } - mountPathFlag := cmd.Flags().Lookup(worker.FlagFirmwareMountPath) - - return w.UnloadKmod(cmd.Context(), cfg, mountPathFlag.Value.String()) + return w.UnloadKmod(cmd.Context(), cfg, cmd.Flags().Lookup(worker.FlagFirmwarePath).Value.String()) } func setCommandsFlags() { kmodLoadCmd.Flags().String( - worker.FlagFirmwareClassPath, + worker.FlagFirmwarePath, "", - "if set, this value will be written to "+worker.FirmwareClassPathLocation, - ) - - kmodLoadCmd.Flags().String( - worker.FlagFirmwareMountPath, - "", - "if set, this the value that firmware host path is mounted to") + "if set, this value will be written to "+worker.FirmwareClassPathLocation+" and it is also the value that firmware host path is mounted to") kmodLoadCmd.Flags().Bool( "tarball", @@ -77,7 +68,7 @@ func setCommandsFlags() { ) kmodUnloadCmd.Flags().String( - worker.FlagFirmwareMountPath, + worker.FlagFirmwarePath, "", "if set, this the value that firmware host path is mounted to") diff --git a/cmd/worker/funcs_kmod_test.go b/cmd/worker/funcs_kmod_test.go index 828542169..f49f6bcc1 100644 --- a/cmd/worker/funcs_kmod_test.go +++ b/cmd/worker/funcs_kmod_test.go @@ -47,38 +47,30 @@ var _ = Describe("kmodLoadFunc", func() { DescribeTable( "set firmware class and firmware mount path if defined", - func(flagFirmwareClassPathValue, flagFirmwareMountPath *string) { + func(flagFirmwarePath *string) { cfg := &kmmv1beta1.ModuleConfig{} ctx := context.TODO() cmd := &cobra.Command{} cmd.SetContext(ctx) - cmd.Flags().String(worker.FlagFirmwareClassPath, "", "") - cmd.Flags().String(worker.FlagFirmwareMountPath, "", "") + cmd.Flags().String(worker.FlagFirmwarePath, "", "") - readConfig := ch.EXPECT().ReadConfigFile(configPath).Return(cfg, nil) - var loadKmod *gomock.Call - if flagFirmwareMountPath != nil { + if flagFirmwarePath != nil { Expect( - cmd.Flags().Set(worker.FlagFirmwareMountPath, *flagFirmwareMountPath), + cmd.Flags().Set(worker.FlagFirmwarePath, *flagFirmwarePath), ).NotTo( HaveOccurred(), ) - loadKmod = wo.EXPECT().LoadKmod(ctx, cfg, *flagFirmwareMountPath).After(readConfig) + gomock.InOrder( + ch.EXPECT().ReadConfigFile(configPath).Return(cfg, nil), + wo.EXPECT().SetFirmwareClassPath(*flagFirmwarePath), + wo.EXPECT().LoadKmod(ctx, cfg, *flagFirmwarePath), + ) } else { - loadKmod = wo.EXPECT().LoadKmod(ctx, cfg, "").After(readConfig) - } - - if flagFirmwareClassPathValue != nil { - Expect( - cmd.Flags().Set(worker.FlagFirmwareClassPath, *flagFirmwareClassPathValue), - ).NotTo( - HaveOccurred(), + gomock.InOrder( + ch.EXPECT().ReadConfigFile(configPath).Return(cfg, nil), + wo.EXPECT().LoadKmod(ctx, cfg, ""), ) - - setFirmwarePath := wo.EXPECT().SetFirmwareClassPath(*flagFirmwareClassPathValue) - setFirmwarePath.After(readConfig) - loadKmod.After(setFirmwarePath) } Expect( @@ -87,10 +79,8 @@ var _ = Describe("kmodLoadFunc", func() { HaveOccurred(), ) }, - Entry("flags not defined", nil, nil), - Entry("class path defined and empty, mount path not defined", ptr.To(""), nil), - Entry("class path defined and not empty, mount path not defined", ptr.To("/some/path"), nil), - Entry("class path defined and empty, mount path defined", ptr.To(""), ptr.To("some mount path")), - Entry("class path defined and not empty, mount path defined", ptr.To("/some/path"), ptr.To("some mount path")), + Entry("fimrwarePath not defined", nil), + Entry("fimrwarePath path defined and empty", ptr.To("")), + Entry("firmwarePath defined", ptr.To("/some/path")), ) }) diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 39cdfd301..c20ec20ed 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -855,12 +855,11 @@ func (p *podManagerImpl) LoaderPodTemplate(ctx context.Context, nmc client.Objec return nil, fmt.Errorf("firmwareHostPath wasn't set, while the Module requires firmware loading") } - args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareHostPath) + args = append(args, "--"+worker.FlagFirmwarePath, *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, *firmwareHostPath) privileged = true } @@ -908,7 +907,7 @@ func (p *podManagerImpl) UnloaderPodTemplate(ctx context.Context, nmc client.Obj if firmwareHostPath == nil { return nil, fmt.Errorf("firmwareHostPath was not set while the Module requires firmware unloading") } - args = append(args, "--"+worker.FlagFirmwareMountPath, *firmwareHostPath) + args = append(args, "--"+worker.FlagFirmwarePath, *firmwareHostPath) if err = setFirmwareVolume(pod, firmwareHostPath); err != nil { return nil, fmt.Errorf("could not map host volume needed for firmware unloading: %v", err) } diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index 43ba68298..971d2905a 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -1965,10 +1965,7 @@ softdep b pre: c args := []string{"kmod", subcommand, "/etc/kmm-worker/config.yaml"} if withFirmware { - args = append(args, "--set-firmware-mount-path", *firmwareHostPath) - if isLoaderPod && firmwareHostPath != nil { - args = append(args, "--set-firmware-class-path", *firmwareHostPath) - } + args = append(args, "--firmware-path", *firmwareHostPath) } else { configAnnotationValue = strings.ReplaceAll(configAnnotationValue, "firmwarePath: /firmware-path\n ", "") } diff --git a/internal/worker/constants.go b/internal/worker/constants.go index 602c607c3..b5ed12644 100644 --- a/internal/worker/constants.go +++ b/internal/worker/constants.go @@ -1,8 +1,7 @@ package worker const ( - FlagFirmwareClassPath = "set-firmware-class-path" - FlagFirmwareMountPath = "set-firmware-mount-path" + FlagFirmwarePath = "firmware-path" FirmwareClassPathLocation = "/sys/module/firmware_class/parameters/path" ImagesDir = "/var/run/kmm/images"