WIP: sfdisk implementation

config/shared/errors/errors.go

@@ -93,6 +93,7 @@ var (
 	ErrPathConflictsSystemd             = errors.New("path conflicts with systemd unit or dropin")
 	ErrCexWithClevis                    = errors.New("cannot use cex with clevis")
 	ErrCexWithKeyFile                   = errors.New("cannot use key file with cex")
+	ErrBadSfdiskPretend                 = errors.New("sfdisk had unexpected output while pretending partition configuration on device")
 
 	// Systemd section errors
 	ErrInvalidSystemdExt       = errors.New("invalid systemd unit extension")

internal/distro/distro.go

@@ -17,6 +17,7 @@ package distro
 import (
 	"fmt"
 	"os"
+	"strings"
 )
 
 // Distro-specific settings that can be overridden at link time with e.g.
@@ -42,6 +43,7 @@ var (
 	mountCmd     = "mount"
 	partxCmd     = "partx"
 	sgdiskCmd    = "sgdisk"
+	sfdiskCmd    = "sfdisk"
 	modprobeCmd  = "modprobe"
 	udevadmCmd   = "udevadm"
 	usermodCmd   = "usermod"
@@ -113,6 +115,7 @@ func MdadmCmd() string     { return mdadmCmd }
 func MountCmd() string     { return mountCmd }
 func PartxCmd() string     { return partxCmd }
 func SgdiskCmd() string    { return sgdiskCmd }
+func SfdiskCmd() string    { return sfdiskCmd }
 func ModprobeCmd() string  { return modprobeCmd }
 func UdevadmCmd() string   { return udevadmCmd }
 func UsermodCmd() string   { return usermodCmd }
@@ -149,6 +152,36 @@ func WriteAuthorizedKeysFragment() bool {
 	return bakedStringToBool(fromEnv("WRITE_AUTHORIZED_KEYS_FRAGMENT", writeAuthorizedKeysFragment))
 }
 
+var partitionerBackend string
+
+func PartitionerBackend() string {
+	if partitionerBackend == "" {
+		partitionerBackend = readPartitionerFromCmdline()
+	}
+	return partitionerBackend
+}
+
+func readPartitionerFromCmdline() string {
+	// Allow override via environment variable for testing
+	if env := os.Getenv("IGNITION_PARTITIONER"); env == "sfdisk" || env == "sgdisk" {
+		return env
+	}
+	cmdline, err := os.ReadFile(kernelCmdlinePath)
+	if err != nil {
+		return "sfdisk"
+	}
+	for _, arg := range strings.Split(strings.TrimSpace(string(cmdline)), " ") {
+		parts := strings.SplitN(arg, "=", 2)
+		if parts[0] == "ignition.partitioner" && len(parts) == 2 {
+			switch parts[1] {
+			case "sfdisk", "sgdisk":
+				return parts[1]
+			}
+		}
+	}
+	return "sfdisk"
+}
+
 func fromEnv(nameSuffix, defaultValue string) string {
 	value := os.Getenv("IGNITION_" + nameSuffix)
 	if value != "" {

internal/exec/stages/disks/partitions.go

@@ -20,12 +20,10 @@ package disks
 
 import (
 	"bufio"
-	"errors"
 	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
-	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -34,13 +32,21 @@ import (
 	"github.com/coreos/ignition/v2/config/v3_7_experimental/types"
 	"github.com/coreos/ignition/v2/internal/distro"
 	"github.com/coreos/ignition/v2/internal/exec/util"
-	"github.com/coreos/ignition/v2/internal/sgdisk"
+	"github.com/coreos/ignition/v2/internal/log"
+	"github.com/coreos/ignition/v2/internal/partitioners"
+	"github.com/coreos/ignition/v2/internal/partitioners/sfdisk"
+	"github.com/coreos/ignition/v2/internal/partitioners/sgdisk"
 	iutil "github.com/coreos/ignition/v2/internal/util"
 )
 
-var (
-	ErrBadSgdiskOutput = errors.New("sgdisk had unexpected output")
-)
+func getDeviceManager(logger *log.Logger, dev string) partitioners.DeviceManager {
+	switch distro.PartitionerBackend() {
+	case "sfdisk":
+		return sfdisk.Begin(logger, dev)
+	default:
+		return sgdisk.Begin(logger, dev)
+	}
+}
 
 // createPartitions creates the partitions described in config.Storage.Disks.
 func (s stage) createPartitions(config types.Config) error {
@@ -75,7 +81,7 @@ func (s stage) createPartitions(config types.Config) error {
 
 // partitionMatches determines if the existing partition matches the spec given. See doc/operator notes for what
 // what it means for an existing partition to match the spec. spec must have non-zero Start and Size.
-func partitionMatches(existing util.PartitionInfo, spec sgdisk.Partition) error {
+func partitionMatches(existing util.PartitionInfo, spec partitioners.Partition) error {
 	if err := partitionMatchesCommon(existing, spec); err != nil {
 		return err
 	}
@@ -87,13 +93,13 @@ func partitionMatches(existing util.PartitionInfo, spec sgdisk.Partition) error
 
 // partitionMatchesResize returns if the existing partition should be resized by evaluating if
 // `resize`field is true and partition matches in all respects except size.
-func partitionMatchesResize(existing util.PartitionInfo, spec sgdisk.Partition) bool {
+func partitionMatchesResize(existing util.PartitionInfo, spec partitioners.Partition) bool {
 	return cutil.IsTrue(spec.Resize) && partitionMatchesCommon(existing, spec) == nil
 }
 
 // partitionMatchesCommon handles the common tests (excluding the partition size) to determine
 // if the existing partition matches the spec given.
-func partitionMatchesCommon(existing util.PartitionInfo, spec sgdisk.Partition) error {
+func partitionMatchesCommon(existing util.PartitionInfo, spec partitioners.Partition) error {
 	if spec.Number != existing.Number {
 		return fmt.Errorf("partition numbers did not match (specified %d, got %d). This should not happen, please file a bug", spec.Number, existing.Number)
 	}
@@ -113,7 +119,7 @@ func partitionMatchesCommon(existing util.PartitionInfo, spec sgdisk.Partition)
 }
 
 // partitionShouldBeInspected returns if the partition has zeroes that need to be resolved to sectors.
-func partitionShouldBeInspected(part sgdisk.Partition) bool {
+func partitionShouldBeInspected(part partitioners.Partition) bool {
 	if part.Number == 0 {
 		return false
 	}
@@ -133,17 +139,17 @@ func convertMiBToSectors(mib *int, sectorSize int) *int64 {
 // getRealStartAndSize returns a map of partition numbers to a struct that contains what their real start
 // and end sector should be. It runs sgdisk --pretend to determine what the partitions would look like if
 // everything specified were to be (re)created.
-func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo util.DiskInfo) ([]sgdisk.Partition, error) {
-	partitions := []sgdisk.Partition{}
+func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo util.DiskInfo) ([]partitioners.Partition, error) {
+	partitions := []partitioners.Partition{}
 	for _, cpart := range dev.Partitions {
-		partitions = append(partitions, sgdisk.Partition{
+		partitions = append(partitions, partitioners.Partition{
 			Partition:     cpart,
 			StartSector:   convertMiBToSectors(cpart.StartMiB, diskInfo.LogicalSectorSize),
 			SizeInSectors: convertMiBToSectors(cpart.SizeMiB, diskInfo.LogicalSectorSize),
 		})
 	}
 
-	op := sgdisk.Begin(s.Logger, devAlias)
+	op := getDeviceManager(s.Logger, devAlias)
 	for _, part := range partitions {
 		if info, exists := diskInfo.GetPartition(part.Number); exists {
 			// delete all existing partitions
@@ -177,116 +183,29 @@ func (s stage) getRealStartAndSize(dev types.Disk, devAlias string, diskInfo uti
 		return nil, err
 	}
 
-	realDimensions, err := parseSgdiskPretend(output, partitionsToInspect)
+	realDimensions, err := op.ParseOutput(output, partitionsToInspect)
 	if err != nil {
 		return nil, err
 	}
 
-	result := []sgdisk.Partition{}
+	result := []partitioners.Partition{}
 	for _, part := range partitions {
 		if dims, ok := realDimensions[part.Number]; ok {
 			if part.StartSector != nil {
-				part.StartSector = &dims.start
+				part.StartSector = &dims.Start
 			}
 			if part.SizeInSectors != nil {
-				part.SizeInSectors = &dims.size
+				part.SizeInSectors = &dims.Size
 			}
 		}
 		result = append(result, part)
 	}
 	return result, nil
 }
 
-type sgdiskOutput struct {
-	start int64
-	size  int64
-}
-
-// parseLine takes a regexp that captures an int64 and a string to match on. On success it returns
-// the captured int64 and nil. If the regexp does not match it returns -1 and nil. If it encountered
-// an error it returns 0 and the error.
-func parseLine(r *regexp.Regexp, line string) (int64, error) {
-	matches := r.FindStringSubmatch(line)
-	switch len(matches) {
-	case 0:
-		return -1, nil
-	case 2:
-		return strconv.ParseInt(matches[1], 10, 64)
-	default:
-		return 0, ErrBadSgdiskOutput
-	}
-}
-
-// parseSgdiskPretend parses the output of running sgdisk pretend with --info specified for each partition
-// number specified in partitionNumbers. E.g. if paritionNumbers is [1,4,5], it is expected that the sgdisk
-// output was from running `sgdisk --pretend <commands> --info=1 --info=4 --info=5`. It assumes the the
-// partition labels are well behaved (i.e. contain no control characters). It returns a list of partitions
-// matching the partition numbers specified, but with the start and size information as determined by sgdisk.
-// The partition numbers need to passed in because sgdisk includes them in its output.
-func parseSgdiskPretend(sgdiskOut string, partitionNumbers []int) (map[int]sgdiskOutput, error) {
-	if len(partitionNumbers) == 0 {
-		return nil, nil
-	}
-	startRegex := regexp.MustCompile(`^First sector: (\d*) \(.*\)$`)
-	endRegex := regexp.MustCompile(`^Last sector: (\d*) \(.*\)$`)
-	const (
-		START             = iota
-		END               = iota
-		FAIL_ON_START_END = iota
-	)
-
-	output := map[int]sgdiskOutput{}
-	state := START
-	current := sgdiskOutput{}
-	i := 0
-
-	lines := strings.Split(sgdiskOut, "\n")
-	for _, line := range lines {
-		switch state {
-		case START:
-			start, err := parseLine(startRegex, line)
-			if err != nil {
-				return nil, err
-			}
-			if start != -1 {
-				current.start = start
-				state = END
-			}
-		case END:
-			end, err := parseLine(endRegex, line)
-			if err != nil {
-				return nil, err
-			}
-			if end != -1 {
-				current.size = 1 + end - current.start
-				output[partitionNumbers[i]] = current
-				i++
-				if i == len(partitionNumbers) {
-					state = FAIL_ON_START_END
-				} else {
-					current = sgdiskOutput{}
-					state = START
-				}
-			}
-		case FAIL_ON_START_END:
-			if len(startRegex.FindStringSubmatch(line)) != 0 ||
-				len(endRegex.FindStringSubmatch(line)) != 0 {
-				return nil, ErrBadSgdiskOutput
-			}
-		}
-	}
-
-	if state != FAIL_ON_START_END {
-		// We stopped parsing in the middle of a info block. Something is wrong
-		return nil, ErrBadSgdiskOutput
-	}
-
-	return output, nil
-}
-
 // partitionShouldExist returns whether a bool is indicating if a partition should exist or not.
 // nil (unspecified in json) is treated the same as true.
-func partitionShouldExist(part sgdisk.Partition) bool {
+func partitionShouldExist(part partitioners.Partition) bool {
 	return !cutil.IsFalse(part.ShouldExist)
 }
 
@@ -450,7 +369,7 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error {
 		return fmt.Errorf("refusing to operate on directly active disk %q", devAlias)
 	}
 	if cutil.IsTrue(dev.WipeTable) {
-		op := sgdisk.Begin(s.Logger, devAlias)
+		op := getDeviceManager(s.Logger, devAlias)
 		s.Info("wiping partition table requested on %q", devAlias)
 		if len(activeParts) > 0 {
 			return fmt.Errorf("refusing to wipe active disk %q", devAlias)
@@ -464,12 +383,15 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error {
 				return err
 			}
 		}
+		if err := s.waitForUdev(blockDevResolved); err != nil {
+			return fmt.Errorf("failed to wait for udev after wipe on %q: %v", blockDevResolved, err)
+		}
 	}
 
 	// Ensure all partitions with number 0 are last
 	sort.Stable(PartitionList(dev.Partitions))
 
-	op := sgdisk.Begin(s.Logger, devAlias)
+	op := getDeviceManager(s.Logger, devAlias)
 
 	diskInfo, err := s.getPartitionMap(devAlias)
 	if err != nil {
@@ -519,6 +441,14 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error {
 			partxDelete = append(partxDelete, uint64(part.Number))
 		case exists && shouldExist && matches:
 			s.Info("partition %d found with correct specifications", part.Number)
+			if op.WritesCompleteTable() {
+				part.StartSector = &info.StartSector
+				part.SizeInSectors = &info.SizeInSectors
+				part.TypeGUID = &info.TypeGUID
+				part.GUID = &info.GUID
+				part.Label = &info.Label
+				op.CreatePartition(part)
+			}
 		case exists && shouldExist && !wipeEntry && !matches:
 			if partitionMatchesResize(info, part) {
 				s.Info("resizing partition %d", part.Number)
@@ -554,10 +484,10 @@ func (s stage) partitionDisk(dev types.Disk, devAlias string) error {
 		return fmt.Errorf("commit failure: %v", err)
 	}
 
-	// In contrast to similar tools, sgdisk does not trigger the update of the
-	// kernel partition table with BLKPG but only uses BLKRRPART which fails
-	// as soon as one partition of the disk is mounted
-	if len(activeParts) > 0 {
+	// sgdisk does not trigger the update of the kernel partition table with
+	// BLKPG but only uses BLKRRPART which fails as soon as one partition of
+	// the disk is mounted. sfdisk handles this natively.
+	if op.NeedsPartx() && len(activeParts) > 0 {
 		runPartxCommand := func(op string, partitions []uint64) error {
 			for _, partNr := range partitions {
 				cmd := exec.Command(distro.PartxCmd(), "--"+op, "--nr", strconv.FormatUint(partNr, 10), blockDevResolved)

internal/partitioners/partitioners.go

@@ -0,0 +1,47 @@
+// Copyright 2024 Red Hat, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package partitioners
+
+import (
+	"github.com/coreos/ignition/v2/config/v3_7_experimental/types"
+)
+
+type DeviceManager interface {
+	CreatePartition(p Partition)
+	DeletePartition(num int)
+	Info(num int)
+	WipeTable(wipe bool)
+	Pretend() (string, error)
+	Commit() error
+	ParseOutput(string, []int) (map[int]Output, error)
+	NeedsPartx() bool
+	WritesCompleteTable() bool
+}
+
+// Partition wraps config types with sector-level positioning.
+// StartMiB/SizeMiB shadow the config fields to prevent accidental use;
+// callers should use StartSector/SizeInSectors instead.
+type Partition struct {
+	types.Partition
+	StartSector   *int64
+	SizeInSectors *int64
+	StartMiB      string
+	SizeMiB       string
+}
+
+type Output struct {
+	Start int64
+	Size  int64
+}