Repository: metal3-io/baremetal-operator
Pull request #1264 (2023-04-28 05:34:57)

:sparkles: Support /dev/disk/by-path root device hints
Allow paths of the form `/dev/disk/by-path` in the root device hints field.

By-path support is needed because it is the only method of identifying a disk that is stable and doesn't require knowledge of the components of a particular server (e.g. the drive serial number). In particular, HCTL (which you might think would do that) is not stable because the H part is numbered in the order the SCSI hosts are discovered by the kernel.

Support for by-path was previously discussed in #317, but in that case it was proposed to add a separate field. This PR reuses the same field, so you can specify either device name. This avoids the weirdness of two separate fields, each containing a Linux device path, at the possible cost of confusing people into thinking that _any_ Linux device path is valid. To mitigate this, the validating webhook rejects other path types.

When available, the stable by-path name is now reported in the HardwareDetails. Existing consumers that rely on round-tripping this name back through root device hints should continue to work and in fact become more reliable, as the name will now refer to the same device on every boot.


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index b63df035..c92e92a2 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -52,8 +52,9 @@ const (
 // RootDeviceHints holds the hints for specifying the storage location
 // for the root filesystem for the image.
 type RootDeviceHints struct {
-	// A Linux device name like "/dev/vda". The hint must match the
-	// actual value exactly.
+	// A Linux device name like "/dev/vda", or a by-path link to it like
+	// "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0". The hint must match
+	// the actual value exactly.
 	DeviceName string `json:"deviceName,omitempty"`
 
 	// A SCSI bus address like 0:0:0:0. The hint must match the actual
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 7ea9590a..c2039c96 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -5,6 +5,7 @@ import (
 	"net"
 	"net/url"
 	"regexp"
+	"strings"
 
 	"github.com/google/uuid"
 	"github.com/pkg/errors"
@@ -44,6 +45,10 @@ func (host *BareMetalHost) validateHost() []error {
 		errs = append(errs, err)
 	}
 
+	if err := validateRootDeviceHints(host.Spec.RootDeviceHints); err != nil {
+		errs = append(errs, err)
+	}
+
 	if host.Spec.Image != nil {
 		if err := validateImageURL(host.Spec.Image.URL); err != nil {
 			errs = append(errs, err)
@@ -179,3 +184,20 @@ func validateImageURL(imageURL string) error {
 
 	return nil
 }
+
+func validateRootDeviceHints(rdh *RootDeviceHints) error {
+	if rdh == nil || rdh.DeviceName == "" {
+		return nil
+	}
+
+	subpath := strings.TrimPrefix(rdh.DeviceName, "/dev/")
+	if rdh.DeviceName == subpath {
+		return fmt.Errorf("Device Name of root device hint must be a /dev/ path, not \"%s\"", rdh.DeviceName)
+	}
+
+	subpath = strings.TrimPrefix(subpath, "disk/by-path/")
+	if strings.Contains(subpath, "/") {
+		return fmt.Errorf("Device Name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"%s\"", rdh.DeviceName)
+	}
+	return nil
+}
diff --git a/config/crd/bases/metal3.io_baremetalhosts.yaml b/config/crd/bases/metal3.io_baremetalhosts.yaml
index 34692f15..065ccd97 100644
--- a/config/crd/bases/metal3.io_baremetalhosts.yaml
+++ b/config/crd/bases/metal3.io_baremetalhosts.yaml
@@ -374,7 +374,8 @@ spec:
                               image.
                             properties:
                               deviceName:
-                                description: A Linux device name like "/dev/vda".
+                                description: A Linux device name like "/dev/vda",
+                                  or a by-path link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
                                   The hint must match the actual value exactly.
                                 type: string
                               hctl:
@@ -436,8 +437,9 @@ spec:
                   image being provisioned.
                 properties:
                   deviceName:
-                    description: A Linux device name like "/dev/vda". The hint must
-                      match the actual value exactly.
+                    description: A Linux device name like "/dev/vda", or a by-path
+                      link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
+                      The hint must match the actual value exactly.
                     type: string
                   hctl:
                     description: A SCSI bus address like 0:0:0:0. The hint must match
@@ -977,7 +979,8 @@ spec:
                                   the image.
                                 properties:
                                   deviceName:
-                                    description: A Linux device name like "/dev/vda".
+                                    description: A Linux device name like "/dev/vda",
+                                      or a by-path link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
                                       The hint must match the actual value exactly.
                                     type: string
                                   hctl:
@@ -1039,8 +1042,9 @@ spec:
                     description: The RootDevicehints set by the user
                     properties:
                       deviceName:
-                        description: A Linux device name like "/dev/vda". The hint
-                          must match the actual value exactly.
+                        description: A Linux device name like "/dev/vda", or a by-path
+                          link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
+                          The hint must match the actual value exactly.
                         type: string
                       hctl:
                         description: A SCSI bus address like 0:0:0:0. The hint must
diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
index 0adb1a6d..47b41837 100644
--- a/config/render/capm3.yaml
+++ b/config/render/capm3.yaml
@@ -373,7 +373,8 @@ spec:
                               image.
                             properties:
                               deviceName:
-                                description: A Linux device name like "/dev/vda".
+                                description: A Linux device name like "/dev/vda",
+                                  or a by-path link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
                                   The hint must match the actual value exactly.
                                 type: string
                               hctl:
@@ -435,8 +436,9 @@ spec:
                   image being provisioned.
                 properties:
                   deviceName:
-                    description: A Linux device name like "/dev/vda". The hint must
-                      match the actual value exactly.
+                    description: A Linux device name like "/dev/vda", or a by-path
+                      link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
+                      The hint must match the actual value exactly.
                     type: string
                   hctl:
                     description: A SCSI bus address like 0:0:0:0. The hint must match
@@ -976,7 +978,8 @@ spec:
                                   the image.
                                 properties:
                                   deviceName:
-                                    description: A Linux device name like "/dev/vda".
+                                    description: A Linux device name like "/dev/vda",
+                                      or a by-path link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
                                       The hint must match the actual value exactly.
                                     type: string
                                   hctl:
@@ -1038,8 +1041,9 @@ spec:
                     description: The RootDevicehints set by the user
                     properties:
                       deviceName:
-                        description: A Linux device name like "/dev/vda". The hint
-                          must match the actual value exactly.
+                        description: A Linux device name like "/dev/vda", or a by-path
+                          link to it like "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0".
+                          The hint must match the actual value exactly.
                         type: string
                       hctl:
                         description: A SCSI bus address like 0:0:0:0. The hint must
diff --git a/pkg/provisioner/ironic/devicehints/devicehints.go b/pkg/provisioner/ironic/devicehints/devicehints.go
index e5c00785..b5daf67d 100644
--- a/pkg/provisioner/ironic/devicehints/devicehints.go
+++ b/pkg/provisioner/ironic/devicehints/devicehints.go
@@ -2,6 +2,7 @@ package devicehints
 
 import (
 	"fmt"
+	"strings"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
@@ -16,7 +17,11 @@ func MakeHintMap(source *metal3v1alpha1.RootDeviceHints) map[string]string {
 	}
 
 	if source.DeviceName != "" {
-		hints["name"] = fmt.Sprintf("s== %s", source.DeviceName)
+		if strings.HasPrefix(source.DeviceName, "/dev/disk/by-path/") {
+			hints["by_path"] = fmt.Sprintf("s== %s", source.DeviceName)
+		} else {
+			hints["name"] = fmt.Sprintf("s== %s", source.DeviceName)
+		}
 	}
 	if source.HCTL != "" {
 		hints["hctl"] = fmt.Sprintf("s== %s", source.HCTL)
diff --git a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
index c3e63cd9..aa3c2cca 100644
--- a/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
+++ b/pkg/provisioner/ironic/hardwaredetails/hardwaredetails.go
@@ -110,8 +110,12 @@ func getDiskType(diskdata introspection.RootDiskType) metal3v1alpha1.DiskType {
 func getStorageDetails(diskdata []introspection.RootDiskType) []metal3v1alpha1.Storage {
 	storage := make([]metal3v1alpha1.Storage, len(diskdata))
 	for i, disk := range diskdata {
+		device := disk.Name
+		if disk.ByPath != "" {
+			device = disk.ByPath
+		}
 		storage[i] = metal3v1alpha1.Storage{
-			Name:               disk.Name,
+			Name:               device,
 			Rotational:         disk.Rotational,
 			Type:               getDiskType(disk),
 			SizeBytes:          metal3v1alpha1.Capacity(disk.Size),


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 747af983..b8132f8a 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -437,6 +437,62 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "BMO validation: failed to parse BMC address information: BMC address hostname/IP : [fe80::fc33:62ff:fe33:8xff] is invalid",
 		},
+		{
+			name: "validRootDeviceHint",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RootDeviceHints: &RootDeviceHints{
+						DeviceName: "/dev/sda",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "validRootDeviceHintByPath",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RootDeviceHints: &RootDeviceHints{
+						DeviceName: "/dev/disk/by-path/pci-0000:01:00.0-scsi-0:2:0:0",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "invalidRootDeviceHintByUUID",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RootDeviceHints: &RootDeviceHints{
+						DeviceName: "/dev/disk/by-uuid/cdaacd50-3a4c-421c-91c0-fe9ba7b8b2f1",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "Device Name of root device hint must be path in /dev/ or /dev/disk/by-path/, not \"/dev/disk/by-uuid/cdaacd50-3a4c-421c-91c0-fe9ba7b8b2f1\"",
+		},
+		{
+			name: "invalidRootDeviceHintNoPath",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RootDeviceHints: &RootDeviceHints{
+						DeviceName: "sda",
+					},
+				},
+			},
+			oldBMH:    nil,
+			wantedErr: "Device Name of root device hint must be a /dev/ path, not \"sda\"",
+		},
 		{
 			name: "invalidImageURL",
 			newBMH: &BareMetalHost{