Repository: metal3-io/baremetal-operator
Pull request #1109 (2022-04-12 13:31:36)

Allow image provider to override kernel and add kernel params
Currently, we assume that the same kernel can be used for all images.
This is not the case in multi-arch scenarios. Add a new optional field
to pass a kernel from the provider.

Also add support for extra kernel parameters. These are required by
CoreOS to link to the correct rootfs.

Requires: https://review.opendev.org/c/openstack/ironic/+/837379


metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **dtantsur** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dtantsur"]} -->
dtantsur: /test-v1b1-centos-integration
/test-v1b1-ubuntu-integration
dtantsur: /test-v1b1-centos-integration
/test-v1b1-ubuntu-integration
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1109#pullrequestreview-941514574" title="Approved">zaneb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)~~ [zaneb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/apis/metal3.io/v1alpha1/preprovisioningimage_types.go b/apis/metal3.io/v1alpha1/preprovisioningimage_types.go
index 8853a5e3..7345d77b 100644
--- a/apis/metal3.io/v1alpha1/preprovisioningimage_types.go
+++ b/apis/metal3.io/v1alpha1/preprovisioningimage_types.go
@@ -65,6 +65,16 @@ type PreprovisioningImageStatus struct {
 	// imageUrl is the URL from which the built image can be downloaded.
 	ImageUrl string `json:"imageUrl,omitempty"`
 
+	// kernelUrl is the URL from which the kernel of the image can be downloaded.
+	// Only makes sense for initrd images.
+	// +optional
+	KernelUrl string `json:"kernelUrl,omitempty"`
+
+	// extraKernelParams is a string with extra parameters to pass to the
+	// kernel when booting the image over network. Only makes sense for initrd images.
+	// +optional
+	ExtraKernelParams string `json:"extraKernelParams,omitempty"`
+
 	// format is the type of image that is available at the download url:
 	// either iso or initrd.
 	// +optional
diff --git a/config/crd/bases/metal3.io_preprovisioningimages.yaml b/config/crd/bases/metal3.io_preprovisioningimages.yaml
index 5447d179..c465235a 100644
--- a/config/crd/bases/metal3.io_preprovisioningimages.yaml
+++ b/config/crd/bases/metal3.io_preprovisioningimages.yaml
@@ -147,6 +147,11 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              extraKernelParams:
+                description: extraKernelParams is a string with extra parameters to
+                  pass to the kernel when booting the image over network. Only makes
+                  sense for initrd images.
+                type: string
               format:
                 description: 'format is the type of image that is available at the
                   download url: either iso or initrd.'
@@ -158,6 +163,10 @@ spec:
                 description: imageUrl is the URL from which the built image can be
                   downloaded.
                 type: string
+              kernelUrl:
+                description: kernelUrl is the URL from which the kernel of the image
+                  can be downloaded. Only makes sense for initrd images.
+                type: string
               networkData:
                 description: networkData is a reference to the version of the Secret
                   containing the network data used to build the image.
diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
index a9cb8131..09159869 100644
--- a/config/render/capm3.yaml
+++ b/config/render/capm3.yaml
@@ -1621,6 +1621,11 @@ spec:
                 x-kubernetes-list-map-keys:
                 - type
                 x-kubernetes-list-type: map
+              extraKernelParams:
+                description: extraKernelParams is a string with extra parameters to
+                  pass to the kernel when booting the image over network. Only makes
+                  sense for initrd images.
+                type: string
               format:
                 description: 'format is the type of image that is available at the
                   download url: either iso or initrd.'
@@ -1632,6 +1637,10 @@ spec:
                 description: imageUrl is the URL from which the built image can be
                   downloaded.
                 type: string
+              kernelUrl:
+                description: kernelUrl is the URL from which the kernel of the image
+                  can be downloaded. Only makes sense for initrd images.
+                type: string
               networkData:
                 description: networkData is a reference to the version of the Secret
                   containing the network data used to build the image.
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index a6d95d5f..24eabe80 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -45,6 +45,7 @@ import (
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/hardware"
 	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/imageprovider"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 	"github.com/metal3-io/baremetal-operator/pkg/utils"
@@ -753,8 +754,12 @@ func (r *BareMetalHostReconciler) getPreprovImage(info *reconcileInfo, formats [
 	}
 
 	image := provisioner.PreprovisioningImage{
-		ImageURL: preprovImage.Status.ImageUrl,
-		Format:   preprovImage.Status.Format,
+		GeneratedImage: imageprovider.GeneratedImage{
+			ImageURL:          preprovImage.Status.ImageUrl,
+			KernelURL:         preprovImage.Status.KernelUrl,
+			ExtraKernelParams: preprovImage.Status.ExtraKernelParams,
+		},
+		Format: preprovImage.Status.Format,
 	}
 	info.log.Info("using PreprovisioningImage")
 	return &image, nil
diff --git a/controllers/metal3.io/preprovisioningimage_controller.go b/controllers/metal3.io/preprovisioningimage_controller.go
index 5faf9ff2..d9c6406e 100644
--- a/controllers/metal3.io/preprovisioningimage_controller.go
+++ b/controllers/metal3.io/preprovisioningimage_controller.go
@@ -170,8 +170,8 @@ func (r *PreprovisioningImageReconciler) update(img *metal3.PreprovisioningImage
 			// Set up all the data before building the image and adding the URL,
 			// so that even if we fail to write the built image status and the
 			// config subsequently changes, the image cache cannot leak.
-			setImage(generation, &img.Status, "", format,
-				secretStatus, img.Spec.Architecture,
+			setImage(generation, &img.Status, imageprovider.GeneratedImage{},
+				format, secretStatus, img.Spec.Architecture,
 				reason)
 		}
 		return true, nil
@@ -181,7 +181,7 @@ func (r *PreprovisioningImageReconciler) update(img *metal3.PreprovisioningImage
 	if networkData != nil {
 		networkDataContent = networkData.Data
 	}
-	url, err := r.ImageProvider.BuildImage(imageprovider.ImageData{
+	image, err := r.ImageProvider.BuildImage(imageprovider.ImageData{
 		ImageMetadata:     img.ObjectMeta.DeepCopy(),
 		Format:            format,
 		Architecture:      img.Spec.Architecture,
@@ -195,9 +195,9 @@ func (r *PreprovisioningImageReconciler) update(img *metal3.PreprovisioningImage
 		}
 		return false, err
 	}
-	log.Info("image URL available", "url", url, "format", format)
+	log.Info("image URL available", "url", image, "format", format)
 
-	return setImage(generation, &img.Status, url, format,
+	return setImage(generation, &img.Status, image, format,
 		secretStatus, img.Spec.Architecture,
 		"Generated image"), nil
 }
@@ -279,12 +279,14 @@ func setImageCondition(generation int64, status *metal3.PreprovisioningImageStat
 	meta.SetStatusCondition(&status.Conditions, newCondition)
 }
 
-func setImage(generation int64, status *metal3.PreprovisioningImageStatus, url string,
+func setImage(generation int64, status *metal3.PreprovisioningImageStatus, image imageprovider.GeneratedImage,
 	format metal3.ImageFormat, networkData metal3.SecretStatus, arch string,
 	message string) bool {
 
 	newStatus := status.DeepCopy()
-	newStatus.ImageUrl = url
+	newStatus.ImageUrl = image.ImageURL
+	newStatus.KernelUrl = image.KernelURL
+	newStatus.ExtraKernelParams = image.ExtraKernelParams
 	newStatus.Format = format
 	newStatus.Architecture = arch
 	newStatus.NetworkData = networkData
@@ -292,7 +294,7 @@ func setImage(generation int64, status *metal3.PreprovisioningImageStatus, url s
 	time := metav1.Now()
 	reason := reasonImageSuccess
 	ready := metav1.ConditionFalse
-	if url != "" {
+	if newStatus.ImageUrl != "" {
 		ready = metav1.ConditionTrue
 	}
 	setImageCondition(generation, newStatus,
diff --git a/pkg/imageprovider/default.go b/pkg/imageprovider/default.go
index 37127ff2..44099b74 100644
--- a/pkg/imageprovider/default.go
+++ b/pkg/imageprovider/default.go
@@ -41,14 +41,13 @@ func (eip envImageProvider) SupportsFormat(format metal3.ImageFormat) bool {
 	}
 }
 
-func (eip envImageProvider) BuildImage(data ImageData, networkData NetworkData, log logr.Logger) (url string, err error) {
+func (eip envImageProvider) BuildImage(data ImageData, networkData NetworkData, log logr.Logger) (image GeneratedImage, err error) {
 	switch data.Format {
 	case metal3.ImageFormatISO:
-		url = eip.isoURL
+		image.ImageURL = eip.isoURL
 	case metal3.ImageFormatInitRD:
-		url = eip.initrdURL
-	}
-	if url == "" {
+		image.ImageURL = eip.initrdURL
+	default:
 		err = BuildInvalidError(fmt.Errorf("Unsupported image format \"%s\"", data.Format))
 	}
 	return
diff --git a/pkg/imageprovider/imageprovider.go b/pkg/imageprovider/imageprovider.go
index ab47ba64..0dd85e94 100644
--- a/pkg/imageprovider/imageprovider.go
+++ b/pkg/imageprovider/imageprovider.go
@@ -17,6 +17,14 @@ type ImageData struct {
 	NetworkDataStatus metal3.SecretStatus
 }
 
+// GeneratedImage contains information about the generated image. At least the
+// URL must be populated.
+type GeneratedImage struct {
+	ImageURL          string
+	KernelURL         string
+	ExtraKernelParams string
+}
+
 type NetworkData map[string][]byte
 
 type ImageProvider interface {
@@ -30,7 +38,7 @@ type ImageProvider interface {
 
 	// BuildImage requests the ImageProvider to build an image with the
 	// supplied network data and return a URL where it can be accessed.
-	BuildImage(ImageData, NetworkData, logr.Logger) (string, error)
+	BuildImage(ImageData, NetworkData, logr.Logger) (GeneratedImage, error)
 
 	// DiscardImage notifies the ImageProvider that a previously built image
 	// is no longer required.
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 55505766..0205dd24 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -45,6 +45,7 @@ const (
 	deployKernelKey  = "deploy_kernel"
 	deployRamdiskKey = "deploy_ramdisk"
 	deployISOKey     = "deploy_iso"
+	kernelParamsKey  = "kernel_append_params"
 )
 
 var bootModeCapabilities = map[metal3v1alpha1.BootMode]string{
@@ -567,6 +568,7 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 		deployKernelKey:  nil,
 		deployRamdiskKey: nil,
 		deployISOKey:     nil,
+		kernelParamsKey:  nil,
 	}
 
 	defer func() {
@@ -580,7 +582,6 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 	}()
 
 	allowISO := accessDetails.SupportsISOPreprovisioningImage()
-	allowInitRD := config.deployKernelURL != ""
 
 	if hostImage != nil {
 		switch hostImage.Format {
@@ -590,11 +591,19 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 				return deployImageInfo
 			}
 		case metal3v1alpha1.ImageFormatInitRD:
-			if allowInitRD {
+			if hostImage.KernelURL != "" {
+				deployImageInfo[deployKernelKey] = hostImage.KernelURL
+			} else if config.deployKernelURL == "" {
+				return nil
+			} else {
 				deployImageInfo[deployKernelKey] = config.deployKernelURL
-				deployImageInfo[deployRamdiskKey] = hostImage.ImageURL
-				return deployImageInfo
 			}
+			deployImageInfo[deployRamdiskKey] = hostImage.ImageURL
+			if hostImage.ExtraKernelParams != "" {
+				// Using %default% prevents overriding the config in ironic-image
+				deployImageInfo[kernelParamsKey] = fmt.Sprintf("%%default%% %s", hostImage.ExtraKernelParams)
+			}
+			return deployImageInfo
 		}
 	}
 
@@ -603,7 +612,7 @@ func setDeployImage(driverInfo map[string]interface{}, config ironicConfig, acce
 			deployImageInfo[deployISOKey] = config.deployISOURL
 			return deployImageInfo
 		}
-		if allowInitRD && config.deployRamdiskURL != "" {
+		if config.deployKernelURL != "" && config.deployRamdiskURL != "" {
 			deployImageInfo[deployKernelKey] = config.deployKernelURL
 			deployImageInfo[deployRamdiskKey] = config.deployRamdiskURL
 			return deployImageInfo
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index c3cf9a74..74b671bd 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -9,6 +9,7 @@ import (
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/hardware"
 	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/imageprovider"
 )
 
 /*
@@ -68,8 +69,8 @@ type HostConfigData interface {
 }
 
 type PreprovisioningImage struct {
-	ImageURL string
-	Format   metal3v1alpha1.ImageFormat
+	imageprovider.GeneratedImage
+	Format metal3v1alpha1.ImageFormat
 }
 
 type ManagementAccessData struct {


Test patch:
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index d4e5924e..205718cc 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -11,6 +11,7 @@ import (
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/imageprovider"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
@@ -867,19 +868,24 @@ func TestSetDeployImage(t *testing.T) {
 		localRamdisk = "http://local.test/ipa.initrd"
 		localIso     = "http://local.test/ipa.iso"
 
-		buildKernel  = localKernel
+		buildKernel  = "http://build.test/ipa.kernel"
 		buildRamdisk = "http://build.test/ipa.initrd"
 		buildIso     = "http://build.test/ipa.iso"
+		kernelParams = "cat meow"
+
+		expectedKernelParams = "%default% cat meow"
 	)
 
 	testCases := []struct {
-		Scenario    string
-		Config      ironicConfig
-		Driver      bmc.AccessDetails
-		Image       *provisioner.PreprovisioningImage
-		ExpectBuild bool
-		ExpectISO   bool
-		ExpectPXE   bool
+		Scenario           string
+		Config             ironicConfig
+		Driver             bmc.AccessDetails
+		Image              *provisioner.PreprovisioningImage
+		ExpectBuild        bool
+		ExpectISO          bool
+		ExpectPXE          bool
+		ExpectNewKernel    bool
+		ExpectKernelParams bool
 	}{
 		{
 			Scenario: "iso no imgbuilder",
@@ -941,8 +947,10 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: isoDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildIso,
-				Format:   metal3v1alpha1.ImageFormatISO,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildIso,
+				},
+				Format: metal3v1alpha1.ImageFormatISO,
 			},
 			ExpectBuild: true,
 			ExpectISO:   true,
@@ -958,13 +966,38 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: pxeDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildRamdisk,
-				Format:   metal3v1alpha1.ImageFormatInitRD,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildRamdisk,
+				},
+				Format: metal3v1alpha1.ImageFormatInitRD,
 			},
 			ExpectBuild: true,
 			ExpectISO:   false,
 			ExpectPXE:   true,
 		},
+		{
+			Scenario: "pxe build with new kernel and kernel params",
+			Config: ironicConfig{
+				havePreprovImgBuilder: true,
+				deployKernelURL:       localKernel,
+				deployRamdiskURL:      localRamdisk,
+				deployISOURL:          localIso,
+			},
+			Driver: pxeDriver,
+			Image: &provisioner.PreprovisioningImage{
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL:          buildRamdisk,
+					KernelURL:         buildKernel,
+					ExtraKernelParams: kernelParams,
+				},
+				Format: metal3v1alpha1.ImageFormatInitRD,
+			},
+			ExpectBuild:        true,
+			ExpectISO:          false,
+			ExpectPXE:          true,
+			ExpectNewKernel:    true,
+			ExpectKernelParams: true,
+		},
 		{
 			Scenario: "pxe iso build",
 			Config: ironicConfig{
@@ -975,8 +1008,10 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: pxeDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildIso,
-				Format:   metal3v1alpha1.ImageFormatISO,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildIso,
+				},
+				Format: metal3v1alpha1.ImageFormatISO,
 			},
 			ExpectBuild: false,
 			ExpectISO:   false,
@@ -990,8 +1025,10 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: pxeDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildRamdisk,
-				Format:   metal3v1alpha1.ImageFormatInitRD,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildRamdisk,
+				},
+				Format: metal3v1alpha1.ImageFormatInitRD,
 			},
 			ExpectISO: false,
 			ExpectPXE: false,
@@ -1003,8 +1040,10 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: pxeDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildRamdisk,
-				Format:   metal3v1alpha1.ImageFormatISO,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildRamdisk,
+				},
+				Format: metal3v1alpha1.ImageFormatISO,
 			},
 			ExpectISO: false,
 			ExpectPXE: false,
@@ -1017,8 +1056,10 @@ func TestSetDeployImage(t *testing.T) {
 			},
 			Driver: pxeDriver,
 			Image: &provisioner.PreprovisioningImage{
-				ImageURL: buildRamdisk,
-				Format:   metal3v1alpha1.ImageFormatISO,
+				GeneratedImage: imageprovider.GeneratedImage{
+					ImageURL: buildRamdisk,
+				},
+				Format: metal3v1alpha1.ImageFormatISO,
 			},
 			ExpectISO: false,
 			ExpectPXE: false,
@@ -1072,13 +1113,20 @@ func TestSetDeployImage(t *testing.T) {
 				assert.Nil(t, opts["deploy_ramdisk"])
 				assert.Nil(t, driverInfo["deploy_kernel"])
 				assert.Nil(t, driverInfo["deploy_ramdisk"])
+				assert.Nil(t, opts["kernel_append_params"])
+				assert.Nil(t, driverInfo["kernel_append_params"])
 			case tc.ExpectPXE:
 				assert.Nil(t, opts["deploy_iso"])
 				assert.Nil(t, driverInfo["deploy_iso"])
 				if tc.ExpectBuild {
-					assert.Equal(t, buildKernel, opts["deploy_kernel"])
+					if tc.ExpectNewKernel {
+						assert.Equal(t, buildKernel, opts["deploy_kernel"])
+						assert.Equal(t, buildKernel, driverInfo["deploy_kernel"])
+					} else {
+						assert.Equal(t, localKernel, opts["deploy_kernel"])
+						assert.Equal(t, localKernel, driverInfo["deploy_kernel"])
+					}
 					assert.Equal(t, buildRamdisk, opts["deploy_ramdisk"])
-					assert.Equal(t, buildKernel, driverInfo["deploy_kernel"])
 					assert.Equal(t, buildRamdisk, driverInfo["deploy_ramdisk"])
 				} else {
 					assert.Equal(t, localKernel, opts["deploy_kernel"])
@@ -1086,6 +1134,13 @@ func TestSetDeployImage(t *testing.T) {
 					assert.Equal(t, localKernel, driverInfo["deploy_kernel"])
 					assert.Equal(t, localRamdisk, driverInfo["deploy_ramdisk"])
 				}
+				if tc.ExpectKernelParams {
+					assert.Equal(t, expectedKernelParams, opts["kernel_append_params"])
+					assert.Equal(t, expectedKernelParams, driverInfo["kernel_append_params"])
+				} else {
+					assert.Nil(t, opts["kernel_append_params"])
+					assert.Nil(t, driverInfo["kernel_append_params"])
+				}
 			default:
 				assert.Nil(t, opts)
 				assert.Empty(t, driverInfo)