Repository: metal3-io/baremetal-operator
Pull request #907 (2021-06-02 14:20:22)

Support deploy_iso in addition to deploy_kernel/ramdisk
Redfish virtual media supports pre-generated ISO ramdisks in addition
to a kernel/initramfs pair. Other drivers will catch up as well once
https://storyboard.openstack.org/#!/story/2008880 is fixed.

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/master/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-integration 
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/907#pullrequestreview-676003386" title="Approved">elfosardo</a>*
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/master/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"]} -->
zaneb: Q: do we expect the ISO-only option to work with _all_ drivers (in the foreseeable future), or just virtualmedia ones?
dtantsur: > Q: do we expect the ISO-only option to work with all drivers (in the foreseeable future), or just virtualmedia ones?

Only virtual media for the foreseeable future. So we'll need to keep kernel/ramdisk support for machines that don't have virtual media.

Patch:
diff --git a/docs/configuration.md b/docs/configuration.md
index 553c5abf..4cce1185 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -10,6 +10,9 @@ containing the Ironic agent.
 `DEPLOY_KERNEL_URL` -- The URL for the kernel to go with the deploy
 ramdisk.
 
+`DEPLOY_ISO_URL` -- The URL for the ISO containing the Ironic agent for
+drivers that support ISO boot. Optional if kernel/ramdisk are set.
+
 `IRONIC_ENDPOINT` -- The URL for the operator to use when talking to
 Ironic.
 
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index c5ce111a..eb18ab57 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -35,6 +35,7 @@ var (
 	softPowerOffTimeout       = time.Second * 180
 	deployKernelURL           string
 	deployRamdiskURL          string
+	deployISOURL              string
 	ironicEndpoint            string
 	inspectorEndpoint         string
 	ironicTrustedCAFile       string
@@ -82,36 +83,23 @@ func NewMacAddressConflictError(address, node string) error {
 	return macAddressConflictError{Address: address, ExistingNode: node}
 }
 
-func init() {
-	// NOTE(dhellmann): Use Fprintf() to report errors instead of
-	// logging, because logging is not configured yet in init().
-
-	var authErr error
-	ironicAuth, inspectorAuth, authErr = clients.LoadAuth()
-	if authErr != nil {
-		fmt.Fprintf(os.Stderr, "Cannot start: %s\n", authErr)
-		os.Exit(1)
-	}
-
+func loadConfigFromEnv() error {
 	deployKernelURL = os.Getenv("DEPLOY_KERNEL_URL")
-	if deployKernelURL == "" {
-		fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_KERNEL_URL variable set\n")
-		os.Exit(1)
-	}
 	deployRamdiskURL = os.Getenv("DEPLOY_RAMDISK_URL")
-	if deployRamdiskURL == "" {
-		fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_RAMDISK_URL variable set\n")
-		os.Exit(1)
+	deployISOURL = os.Getenv("DEPLOY_ISO_URL")
+	if deployISOURL == "" && (deployKernelURL == "" || deployRamdiskURL == "") {
+		return errors.New("Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set")
+	}
+	if (deployKernelURL == "" && deployRamdiskURL != "") || (deployKernelURL != "" && deployRamdiskURL == "") {
+		return errors.New("DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together")
 	}
 	ironicEndpoint = os.Getenv("IRONIC_ENDPOINT")
 	if ironicEndpoint == "" {
-		fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_ENDPOINT variable set\n")
-		os.Exit(1)
+		return errors.New("No IRONIC_ENDPOINT variable set")
 	}
 	inspectorEndpoint = os.Getenv("IRONIC_INSPECTOR_ENDPOINT")
 	if inspectorEndpoint == "" {
-		fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_INSPECTOR_ENDPOINT variable set\n")
-		os.Exit(1)
+		return errors.New("No IRONIC_INSPECTOR_ENDPOINT variable set")
 	}
 	ironicTrustedCAFile = os.Getenv("IRONIC_CACERT_FILE")
 	if ironicTrustedCAFile == "" {
@@ -137,11 +125,30 @@ func init() {
 	if maxHostsStr := os.Getenv("PROVISIONING_LIMIT"); maxHostsStr != "" {
 		value, err := strconv.Atoi(maxHostsStr)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "Cannot start: Invalid value set for variable PROVISIONING_LIMIT=%s", maxHostsStr)
-			os.Exit(1)
+			return errors.Errorf("Invalid value set for variable PROVISIONING_LIMIT=%s", maxHostsStr)
 		}
 		maxBusyHosts = value
 	}
+
+	return nil
+}
+
+func init() {
+	// NOTE(dhellmann): Use Fprintf() to report errors instead of
+	// logging, because logging is not configured yet in init().
+
+	var authErr error
+	ironicAuth, inspectorAuth, authErr = clients.LoadAuth()
+	if authErr != nil {
+		fmt.Fprintf(os.Stderr, "Cannot start: %s\n", authErr)
+		os.Exit(1)
+	}
+
+	err := loadConfigFromEnv()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Cannot start: %s\n", err)
+		os.Exit(1)
+	}
 }
 
 // Provisioner implements the provisioning.Provisioner interface
@@ -181,6 +188,7 @@ func LogStartup() {
 		"inspectorAuthType", inspectorAuth.Type,
 		"deployKernelURL", deployKernelURL,
 		"deployRamdiskURL", deployRamdiskURL,
+		"deployISOURL", deployISOURL,
 	)
 }
 
@@ -510,8 +518,13 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
 	driverInfo := bmcAccess.DriverInfo(p.bmcCreds)
 	// FIXME(dhellmann): We need to get our IP on the
 	// provisioning network from somewhere.
-	driverInfo["deploy_kernel"] = deployKernelURL
-	driverInfo["deploy_ramdisk"] = deployRamdiskURL
+	if deployKernelURL != "" && deployRamdiskURL != "" {
+		driverInfo["deploy_kernel"] = deployKernelURL
+		driverInfo["deploy_ramdisk"] = deployRamdiskURL
+	}
+	if deployISOURL != "" {
+		driverInfo["deploy_iso"] = deployISOURL
+	}
 
 	// If we have not found a node yet, we need to create one
 	if ironicNode == nil {
diff --git a/tools/run_local_ironic.sh b/tools/run_local_ironic.sh
index 95c0d951..ff7b3713 100755
--- a/tools/run_local_ironic.sh
+++ b/tools/run_local_ironic.sh
@@ -50,6 +50,7 @@ fi
 
 DEPLOY_KERNEL_URL="${DEPLOY_KERNEL_URL:-"http://${CLUSTER_PROVISIONING_IP}:${HTTP_PORT}/images/ironic-python-agent.kernel"}"
 DEPLOY_RAMDISK_URL="${DEPLOY_RAMDISK_URL:-"http://${CLUSTER_PROVISIONING_IP}:${HTTP_PORT}/images/ironic-python-agent.initramfs"}"
+DEPLOY_ISO_URL=${DEPLOY_ISO_URL:-}
 IRONIC_ENDPOINT="${IRONIC_ENDPOINT:-"${IRONIC_BASE_URL}:6385/v1/"}"
 IRONIC_INSPECTOR_ENDPOINT="${IRONIC_INSPECTOR_ENDPOINT:-"${IRONIC_BASE_URL}:5050/v1/"}"
 CACHEURL="${CACHEURL:-"http://${PROVISIONING_IP}/images"}"
@@ -71,6 +72,7 @@ PROVISIONING_INTERFACE=${PROVISIONING_INTERFACE}
 DHCP_RANGE=${CLUSTER_DHCP_RANGE}
 DEPLOY_KERNEL_URL=${DEPLOY_KERNEL_URL}
 DEPLOY_RAMDISK_URL=${DEPLOY_RAMDISK_URL}
+DEPLOY_ISO_URL=${DEPLOY_ISO_URL}
 IRONIC_ENDPOINT=${IRONIC_ENDPOINT}
 IRONIC_INSPECTOR_ENDPOINT=${IRONIC_INSPECTOR_ENDPOINT}
 CACHEURL=${CACHEURL}


Test patch:
diff --git a/pkg/provisioner/ironic/init_test.go b/pkg/provisioner/ironic/init_test.go
new file mode 100644
index 00000000..e15fdbe3
--- /dev/null
+++ b/pkg/provisioner/ironic/init_test.go
@@ -0,0 +1,152 @@
+package ironic
+
+import (
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type EnvFixture struct {
+	ironicEndpoint    string
+	inspectorEndpoint string
+	kernelURL         string
+	ramdiskURL        string
+	isoURL            string
+}
+
+func tearDown() {
+	for _, e := range os.Environ() {
+		if !strings.HasPrefix(e, "IRONIC_") && !strings.HasPrefix(e, "DEPLOY_") {
+			continue
+		}
+		parts := strings.SplitN(e, "=", 2)
+		os.Unsetenv(parts[0])
+	}
+}
+
+func (f EnvFixture) SetUp() {
+	tearDown()
+
+	if f.ironicEndpoint != "" {
+		os.Setenv("IRONIC_ENDPOINT", f.ironicEndpoint)
+	}
+	if f.inspectorEndpoint != "" {
+		os.Setenv("IRONIC_INSPECTOR_ENDPOINT", f.inspectorEndpoint)
+	}
+	if f.kernelURL != "" {
+		os.Setenv("DEPLOY_KERNEL_URL", f.kernelURL)
+	}
+	if f.ramdiskURL != "" {
+		os.Setenv("DEPLOY_RAMDISK_URL", f.ramdiskURL)
+	}
+	if f.isoURL != "" {
+		os.Setenv("DEPLOY_ISO_URL", f.isoURL)
+	}
+}
+
+func (f EnvFixture) Verify(t *testing.T) {
+	assert.Equal(t, f.ironicEndpoint, ironicEndpoint)
+	assert.Equal(t, f.inspectorEndpoint, inspectorEndpoint)
+	assert.Equal(t, f.kernelURL, deployKernelURL)
+	assert.Equal(t, f.ramdiskURL, deployRamdiskURL)
+	assert.Equal(t, f.isoURL, deployISOURL)
+}
+
+func TestLoadFromEnv(t *testing.T) {
+	defer tearDown()
+
+	cases := []struct {
+		name          string
+		env           EnvFixture
+		expectedError string
+	}{
+		{
+			name: "kernel and ramdisk",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				kernelURL:         "http://kernel",
+				ramdiskURL:        "http://ramdisk",
+			},
+		},
+		{
+			name: "ISO only",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				isoURL:            "http://iso",
+			},
+		},
+		{
+			name: "ISO and kernel/ramdisk",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				isoURL:            "http://iso",
+				kernelURL:         "http://kernel",
+				ramdiskURL:        "http://ramdisk",
+			},
+		},
+		{
+			name: "no deploy info",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+			},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "only kernel",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				kernelURL:         "http://kernel",
+			},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "only ramdisk",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				ramdiskURL:        "http://ramdisk",
+			},
+			expectedError: "Either DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL or DEPLOY_ISO_URL must be set",
+		},
+		{
+			name: "ISO and kernel",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				kernelURL:         "http://kernel",
+				isoURL:            "http://iso",
+			},
+			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
+		},
+		{
+			name: "ISO and ramdisk",
+			env: EnvFixture{
+				ironicEndpoint:    "http://ironic",
+				inspectorEndpoint: "http://inspector",
+				ramdiskURL:        "http://ramdisk",
+				isoURL:            "http://iso",
+			},
+			expectedError: "DEPLOY_KERNEL_URL and DEPLOY_RAMDISK_URL can only be set together",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.env.SetUp()
+			err := loadConfigFromEnv()
+			if tc.expectedError != "" {
+				assert.Regexp(t, tc.expectedError, err, "error message")
+			} else {
+				assert.Nil(t, err)
+				tc.env.Verify(t)
+			}
+		})
+	}
+}