combine extra kernel params from different sources

This commit:

 - Combines the extra kernel params from both BMH and the PreprovisioningImage
   CRs.

Signed-off-by: Adam Rozman <adam.rozman@est.tech>

Author: Rozzii

internal/controller/metal3.io/baremetalhost_controller.go

@@ -84,11 +84,30 @@ func (info *reconcileInfo) publishEvent(reason, message string) {
 }
 
 // return the PreprovisioningExtraKernelParams from the reconciliation info.
-func retrievePreprovisioningExtraKernelParamsSpec(info *reconcileInfo) string {
+func (r *BareMetalHostReconciler) retrievePreprovisioningExtraKernelParamsSpec(info *reconcileInfo, prov provisioner.Provisioner) string {
 	if info == nil || info.host == nil {
 		return ""
 	}
-	return info.host.Spec.PreprovisioningExtraKernelParams
+	kernelExtraPreprovParams := info.host.Spec.PreprovisioningExtraKernelParams
+	preprovImgFormats, err := prov.PreprovisioningImageFormats()
+	if err != nil {
+		return kernelExtraPreprovParams
+	}
+	preprovImg, err := r.getPreprovImage(info, preprovImgFormats)
+	if err != nil {
+		return kernelExtraPreprovParams
+	}
+	// Make sure kernel extra params coming from dynamically generater preprov
+	// Image are also represented in every life cycle operation
+	if preprovImg != nil {
+		trimmedParams := strings.TrimSpace(kernelExtraPreprovParams)
+		if trimmedParams == "" {
+			kernelExtraPreprovParams = preprovImg.GeneratedImage.ExtraKernelParams
+		} else {
+			kernelExtraPreprovParams += " " + preprovImg.GeneratedImage.ExtraKernelParams
+		}
+	}
+	return kernelExtraPreprovParams
 }
 
 // +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts,verbs=get;list;watch;create;update;patch;delete
@@ -853,7 +872,7 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 			CurrentImage:                     getCurrentImage(info.host),
 			PreprovisioningImage:             preprovImg,
 			PreprovisioningNetworkData:       preprovisioningNetworkData,
-			PreprovisioningExtraKernelParams: retrievePreprovisioningExtraKernelParamsSpec(info),
+			PreprovisioningExtraKernelParams: r.retrievePreprovisioningExtraKernelParamsSpec(info, prov),
 			HasCustomDeploy:                  hasCustomDeploy(info.host),
 			DisablePowerOff:                  info.host.Spec.DisablePowerOff,
 			CPUArchitecture:                  getHostArchitecture(info.host),
@@ -997,7 +1016,7 @@ func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner,
 	provResult, started, details, err := prov.InspectHardware(
 		provisioner.InspectData{
 			BootMode:                         info.host.Status.Provisioning.BootMode,
-			PreprovisioningExtraKernelParams: retrievePreprovisioningExtraKernelParamsSpec(info),
+			PreprovisioningExtraKernelParams: r.retrievePreprovisioningExtraKernelParamsSpec(info, prov),
 		},
 		info.host.Status.ErrorType == metal3api.InspectionError,
 		refresh,
@@ -1171,7 +1190,7 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 		ActualRAIDConfig:                 info.host.Status.Provisioning.RAID.DeepCopy(),
 		RootDeviceHints:                  newStatus.Provisioning.RootDeviceHints.DeepCopy(),
 		FirmwareConfig:                   newStatus.Provisioning.Firmware.DeepCopy(),
-		PreprovisioningExtraKernelParams: retrievePreprovisioningExtraKernelParamsSpec(info),
+		PreprovisioningExtraKernelParams: r.retrievePreprovisioningExtraKernelParamsSpec(info, prov),
 	}
 	// When manual cleaning fails, we think that the existing RAID configuration
 	// is invalid and needs to be reconfigured.
@@ -1299,7 +1318,7 @@ func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisione
 		BootMode:                         info.host.Status.Provisioning.BootMode,
 		HardwareProfile:                  hwProf,
 		RootDeviceHints:                  info.host.Status.Provisioning.RootDeviceHints.DeepCopy(),
-		PreprovisioningExtraKernelParams: retrievePreprovisioningExtraKernelParamsSpec(info),
+		PreprovisioningExtraKernelParams: r.retrievePreprovisioningExtraKernelParamsSpec(info, prov),
 	}, forceReboot)
 	if err != nil {
 		return actionError{fmt.Errorf("failed to provision: %w", err)}
@@ -1415,7 +1434,7 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 
 func (r *BareMetalHostReconciler) doServiceIfNeeded(prov provisioner.Provisioner, info *reconcileInfo, hup *metal3api.HostUpdatePolicy) (result actionResult) {
 	servicingData := provisioner.ServicingData{}
-	servicingData.PreprovisioningExtraKernelParams = retrievePreprovisioningExtraKernelParamsSpec(info)
+	servicingData.PreprovisioningExtraKernelParams = r.retrievePreprovisioningExtraKernelParamsSpec(info, prov)
 
 	// (NOTE)janders: since Servicing is an opt-in feature that requires HostUpdatePolicy to be created and set to onReboot
 	// set below booleans to false by default and change to true based on policy settings

pkg/provisioner/ironic/ironic.go

@@ -331,7 +331,7 @@ func fmtPreprovExtraKernParams(params string) string {
 func (p *ironicProvisioner) configureNode(data provisioner.ManagementAccessData, ironicNode *nodes.Node, bmcAccess bmc.AccessDetails) (result provisioner.Result, err error) {
 	updater := clients.UpdateOptsBuilder(p.log)
 
-	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage)
+	deployImageInfo := setDeployImage(p.config, bmcAccess, data.PreprovisioningImage, data)
 	updater.SetDriverInfoOpts(deployImageInfo, ironicNode)
 
 	updater.SetTopLevelOpt("automated_clean",
@@ -445,7 +445,7 @@ func setExternalURL(p *ironicProvisioner, driverInfo map[string]interface{}) map
 
 // setDeployImage configures the IPA ramdisk parameters in the Node's DriverInfo.
 // It can use either the provided PreprovisioningImage or the global configuration from ironicConfig.
-func setDeployImage(config ironicConfig, accessDetails bmc.AccessDetails, hostImage *provisioner.PreprovisioningImage) clients.UpdateOptsData {
+func setDeployImage(config ironicConfig, accessDetails bmc.AccessDetails, hostImage *provisioner.PreprovisioningImage, data provisioner.ManagementAccessData) clients.UpdateOptsData {
 	deployImageInfo := clients.UpdateOptsData{
 		deployKernelKey:  nil,
 		deployRamdiskKey: nil,
@@ -471,9 +471,22 @@ func setDeployImage(config ironicConfig, accessDetails bmc.AccessDetails, hostIm
 				deployImageInfo[deployKernelKey] = config.deployKernelURL
 			}
 			deployImageInfo[deployRamdiskKey] = hostImage.ImageURL
+			// Extra params from the kernel have been already posted to
+			// Ironic node API in the Register function, so extra kernel args in driver info need
+			// update only if the dynamically generated preprov image requires it
 			if hostImage.ExtraKernelParams != "" {
-				// Using %default% prevents overriding the config in ironic-image
-				deployImageInfo[kernelParamsKey] = "%default% " + hostImage.ExtraKernelParams
+				// Combine the default ironic node kernel params with the extra
+				// params from the BMH and the dynamically generated preprov image
+				comExtraKernelParams := defaultKernelParam
+				trimDataKParams := strings.TrimSpace(data.PreprovisioningExtraKernelParams)
+				trimPreprovImgKParams := strings.TrimSpace(hostImage.ExtraKernelParams)
+				if trimDataKParams != "" {
+					comExtraKernelParams += " " + data.PreprovisioningExtraKernelParams
+				}
+				if trimPreprovImgKParams != "" {
+					comExtraKernelParams += " " + hostImage.ExtraKernelParams
+				}
+				deployImageInfo[kernelParamsKey] = comExtraKernelParams
 			}
 			return deployImageInfo
 		}

pkg/provisioner/ironic/register_test.go

@@ -1032,6 +1032,7 @@ func TestSetDeployImage(t *testing.T) {
 		Config             ironicConfig
 		Driver             bmc.AccessDetails
 		Image              *provisioner.PreprovisioningImage
+		Data               provisioner.ManagementAccessData
 		ExpectBuild        bool
 		ExpectISO          bool
 		ExpectPXE          bool
@@ -1248,7 +1249,7 @@ func TestSetDeployImage(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.Scenario, func(t *testing.T) {
-			opts := setDeployImage(tc.Config, tc.Driver, tc.Image)
+			opts := setDeployImage(tc.Config, tc.Driver, tc.Image, tc.Data)
 
 			switch {
 			case tc.ExpectISO: