fix: wipe system partitions correctly via kernel args

Use `DiscoveredVolumes` instead of `VolumeStatus`, force reboot to avoid confusion in the volume controller. Fixes #9448 Signed-off-by: Andrey Smirnov <[email protected]>
siderolabs · Oct 7, 2024 · 519a483 · 519a483
1 parent 0a2b455
commit 519a483
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 38 deletions.
diff --git a/internal/app/machined/internal/server/v1alpha1/v1alpha1_server.go b/internal/app/machined/internal/server/v1alpha1/v1alpha1_server.go
@@ -569,6 +569,11 @@ func (opt *ResetOptions) GetSystemDiskTargets() []runtime.PartitionTarget {
 	return xslices.Map(opt.systemDiskTargets, func(t *partition.VolumeWipeTarget) runtime.PartitionTarget { return t })
 }
 
+// String implements runtime.ResetOptions interface.
+func (opt *ResetOptions) String() string {
+	return strings.Join(xslices.Map(opt.systemDiskTargets, func(t *partition.VolumeWipeTarget) string { return t.String() }), ", ")
+}
+
 // Reset resets the node.
 //
 //nolint:gocyclo
@@ -635,9 +640,7 @@ func (s *Server) Reset(ctx context.Context, in *machine.ResetRequest) (reply *ma
 				return nil, fmt.Errorf("failed to reset: volume %q is not ready", spec.Label)
 			}
 
-			target := &partition.VolumeWipeTarget{
-				VolumeStatus: volumeStatus,
-			}
+			target := partition.VolumeWipeTargetFromVolumeStatus(volumeStatus)
 
 			if spec.Wipe {
 				opts.systemDiskTargets = append(opts.systemDiskTargets, target)

diff --git a/internal/app/machined/pkg/runtime/v1alpha1/v1alpha1_sequencer_tasks.go b/internal/app/machined/pkg/runtime/v1alpha1/v1alpha1_sequencer_tasks.go
@@ -1583,15 +1583,7 @@ func ResetSystemDiskPartitions(seq runtime.Sequence, _ any) (runtime.TaskExecuti
 
 		logger.Printf("finished resetting system disks %s", diskTargets)
 
-		bootWiped := slices.ContainsFunc(diskTargets, func(t runtime.PartitionTarget) bool {
-			return t.GetLabel() == constants.BootPartitionLabel
-		})
-
-		if bootWiped {
-			return reboot(ctx, logger, r) // only reboot when we wiped boot partition
-		}
-
-		return nil
+		return reboot(ctx, logger, r)
 	}, "wipeSystemDiskPartitions"
 }
 
@@ -1710,29 +1702,45 @@ func (opt targets) GetSystemDiskTargets() []runtime.PartitionTarget {
 	return xslices.Map(opt.systemDiskTargets, func(t *partition.VolumeWipeTarget) runtime.PartitionTarget { return t })
 }
 
-func parseTargets(ctx context.Context, r runtime.Runtime, wipeStr string) (targets, error) {
+func (opt targets) String() string {
+	return strings.Join(xslices.Map(opt.systemDiskTargets, func(t *partition.VolumeWipeTarget) string { return t.String() }), ", ")
+}
+
+func parseTargets(ctx context.Context, r runtime.Runtime, wipeStr string) (SystemDiskTargets, error) {
 	after, found := strings.CutPrefix(wipeStr, "system:")
 	if !found {
 		return targets{}, fmt.Errorf("invalid wipe labels string: %q", wipeStr)
 	}
 
-	var result []*partition.VolumeWipeTarget //nolint:prealloc
+	var result []*partition.VolumeWipeTarget
+
+	// in this early phase, we don't have VolumeStatus resources, so instead we'd use DiscoveredVolumes
+	// to get the volume paths
+	discoveredVolumes, err := safe.StateListAll[*blockres.DiscoveredVolume](ctx, r.State().V1Alpha2().Resources())
+	if err != nil {
+		return targets{}, err
+	}
 
 	for _, label := range strings.Split(after, ",") {
-		volumeStatus, err := safe.ReaderGetByID[*blockres.VolumeStatus](ctx, r.State().V1Alpha2().Resources(), label)
-		if err != nil {
-			return targets{}, fmt.Errorf("failed to get volume status with label %q: %w", label, err)
-		}
+		found := false
 
-		if volumeStatus.TypedSpec().Phase != blockres.VolumePhaseReady {
-			return targets{}, fmt.Errorf("failed to reset: volume %q is not ready", label)
-		}
+		for iter := discoveredVolumes.Iterator(); iter.Next(); {
+			discoveredVolume := iter.Value()
+
+			if discoveredVolume.TypedSpec().PartitionLabel != label {
+				continue
+			}
+
+			result = append(result, partition.VolumeWipeTargetFromDiscoveredVolume(discoveredVolume))
+
+			found = true
 
-		target := &partition.VolumeWipeTarget{
-			VolumeStatus: volumeStatus,
+			break
 		}
 
-		result = append(result, target)
+		if !found {
+			return targets{}, fmt.Errorf("failed to get volume status with label %q", label)
+		}
 	}
 
 	if len(result) == 0 {
@@ -1746,6 +1754,7 @@ func parseTargets(ctx context.Context, r runtime.Runtime, wipeStr string) (targe
 // It's a subset of [runtime.ResetOptions].
 type SystemDiskTargets interface {
 	GetSystemDiskTargets() []runtime.PartitionTarget
+	fmt.Stringer
 }
 
 // ResetSystemDiskSpec represents the task to reset the system disk by spec.

diff --git a/internal/pkg/partition/wipe.go b/internal/pkg/partition/wipe.go
@@ -16,43 +16,74 @@ import (
 
 // VolumeWipeTarget is a target for wiping a volume.
 type VolumeWipeTarget struct {
-	VolumeStatus *blockres.VolumeStatus
+	label string
+
+	parentDevName, devName string
 }
 
-// GetLabel implements runtime.PartitionTarget.
-func (v *VolumeWipeTarget) GetLabel() string {
-	return v.VolumeStatus.Metadata().ID()
+// VolumeWipeTargetFromVolumeStatus creates a new VolumeWipeTarget from a VolumeStatus.
+func VolumeWipeTargetFromVolumeStatus(vs *blockres.VolumeStatus) *VolumeWipeTarget {
+	parentDevName := vs.TypedSpec().ParentLocation
+
+	if parentDevName == "" {
+		parentDevName = vs.TypedSpec().Location
+	}
+
+	return &VolumeWipeTarget{
+		label:         vs.Metadata().ID(),
+		parentDevName: parentDevName,
+		devName:       vs.TypedSpec().Location,
+	}
 }
 
-// Wipe implements runtime.PartitionTarget.
-func (v *VolumeWipeTarget) Wipe(ctx context.Context, log func(string, ...any)) error {
-	parentDevName := v.VolumeStatus.TypedSpec().ParentLocation
+// VolumeWipeTargetFromDiscoveredVolume creates a new VolumeWipeTarget from a DiscoveredVolume.
+func VolumeWipeTargetFromDiscoveredVolume(dv *blockres.DiscoveredVolume) *VolumeWipeTarget {
+	parentDevName := dv.TypedSpec().ParentDevPath
 
 	if parentDevName == "" {
-		parentDevName = v.VolumeStatus.TypedSpec().Location
+		parentDevName = dv.TypedSpec().DevPath
 	}
 
-	parentBd, err := block.NewFromPath(parentDevName)
+	return &VolumeWipeTarget{
+		label:         dv.TypedSpec().PartitionLabel,
+		parentDevName: parentDevName,
+		devName:       dv.TypedSpec().DevPath,
+	}
+}
+
+// GetLabel implements runtime.PartitionTarget.
+func (v *VolumeWipeTarget) GetLabel() string {
+	return v.label
+}
+
+// String implements runtime.PartitionTarget.
+func (v *VolumeWipeTarget) String() string {
+	return fmt.Sprintf("%s:%s", v.label, v.devName)
+}
+
+// Wipe implements runtime.PartitionTarget.
+func (v *VolumeWipeTarget) Wipe(ctx context.Context, log func(string, ...any)) error {
+	parentBd, err := block.NewFromPath(v.parentDevName)
 	if err != nil {
-		return fmt.Errorf("error opening block device %q: %w", parentDevName, err)
+		return fmt.Errorf("error opening block device %q: %w", v.parentDevName, err)
 	}
 
 	defer parentBd.Close() //nolint:errcheck
 
 	if err = parentBd.RetryLockWithTimeout(ctx, true, time.Minute); err != nil {
-		return fmt.Errorf("error locking block device %q: %w", parentDevName, err)
+		return fmt.Errorf("error locking block device %q: %w", v.parentDevName, err)
 	}
 
 	defer parentBd.Unlock() //nolint:errcheck
 
-	bd, err := block.NewFromPath(v.VolumeStatus.TypedSpec().Location, block.OpenForWrite())
+	bd, err := block.NewFromPath(v.devName, block.OpenForWrite())
 	if err != nil {
-		return fmt.Errorf("error opening block device %q: %w", v.VolumeStatus.TypedSpec().Location, err)
+		return fmt.Errorf("error opening block device %q: %w", v.devName, err)
 	}
 
 	defer bd.Close() //nolint:errcheck
 
-	log("wiping the volume %q (%s)", v.GetLabel(), v.VolumeStatus.TypedSpec().Location)
+	log("wiping the volume %q (%s)", v.GetLabel(), v.devName)
 
 	return bd.FastWipe()
 }