Add: Allow reusing existing local storage pools by krishvsoni · Pull Request #1641 · canonical/microcloud

krishvsoni · 2026-10-28T17:27:26Z

Summary

microcloud add (and microcloud join, whose disk questions the initiator answers) can now reuse an existing local storage pool on a disk. A rebuilt member can rejoin without wiping its data.

Disk classification, following the issue and the maintainer's comment on it:

IsPristineDisk(disk)
  return disk.device_fs_uuid == "" AND disk.partitions is empty

Disk	`device_fs_uuid`	`partitions`	Result
empty	`""`	`[]`	pristine, no prompt
ZFS pool	`""`	set	non-pristine, prompt
btrfs / LVM	set	`[]`	non-pristine, prompt

Local storage flow when adding systems (askLocalPool):

 collect disks per system
-  skip every disk with partitions
+  disks with partitions that are not mounted -> ReusableDisks
+  (offered for local storage only, never for Ceph)
 select one disk per system
+for each selected non-pristine disk
+  ask "Disk X on Y is not empty. Reuse its existing local storage pool?" (default: yes)
+  yes -> source.recover=true, left out of the wipe table
 ask which disks to wipe
-wipe every selected disk (the wipe answer was ignored)
+wipe only the disks selected in the wipe table
+error if a partitioned disk is neither reused nor wiped

Notes:

The LXD key is source.recover (API extension storage_source_recover), not source.reuse as written in the issue. It is a member-specific pool key, so it works in the join config. With it, LXD mounts/imports the existing pool instead of running zpool create.
Reusable disks are only offered when adding systems (!bootstrap) and when LXD has storage_source_recover.
Bug fix: askLocalPool ended with if wipeable { toWipe = selectedDisks }, so every selected local disk was wiped regardless of the "Select which disks to wipe" answer (regression from 346d179). Without this fix, a disk selected for reuse would have been wiped.
Preseed: new systems[].storage.local.recover field. Validation rejects it combined with wipe, without path, on Ceph disks, and during init. Parse checks for the LXD extension before using it.

Commits:

e8f19dc cmd/microcloud: Respect disk wipe selection for local storage
cd315da service: Collect unmounted disks with partitions as reusable
1416071 service: Support source.recover in ZFS storage pool helpers
b966b2f cmd/microcloud: Allow reusing existing local storage pools when adding systems
d1a848f cmd/microcloud: Add recover option for local storage disks in preseed
88fdfeb doc: Document reusing existing local storage pools

Evidence

Unit tests, run on Linux (arm64, Go 1.27.1, dqlite built as in make deps). go vet ./service/ ./cmd/microcloud/ is clean.

Before: IsPristineDisk with the issue's original rule (device_fs_uuid == "" only):

--- FAIL: TestDiskSuite/Test_IsPristineDisk
    case 2: Disk with partitions but no filesystem (zfs) -> Not equal

With today's rule (len(partitions) == 0 only):

--- FAIL: TestDiskSuite/Test_IsPristineDisk
    case 1: Disk with a filesystem (btrfs, lvm) -> Not equal

Without the preseed recover validation:

--- FAIL: TestPreseedSuite/Test_preseedValidateRecover
    An error is expected but got nil (x3)
    expected: "Missing the local storage disk path to recover on system \"n2\""
    actual  : "Some systems are missing local storage disks"

After:

--- PASS: TestDiskSuite/Test_IsPristineDisk
--- PASS: TestPreseedSuite/Test_preseedValidateRecover
--- PASS: TestPreseedSuite/Test_preseedValidateInvalid
ok  github.com/canonical/microcloud/microcloud/service
ok  github.com/canonical/microcloud/microcloud/cmd/microcloud

Not covered: the interactive flow has no automated test, and the change has not been exercised end to end on a real cluster. A system test in test/suites (create a ZFS pool on a disk, remove the member, add it back with reuse, check the pool survives) can be added if wanted.

Merge Danger

Door: two-way

Reuse never destroys data (default answer is yes; the wipe table excludes reused disks). The code reverts cleanly.

Blast Radius: local-storage

The wipe fix changes behavior. Disks not selected in the wipe table are no longer wiped. A user who relied on the old implicit wipe with a dirty disk now gets an error from LXD (or the new partitions error) instead of a wiped disk.
microcloud add can show a new question. Scripted input (TEST_CONSOLE) only sees it when a selected disk is non-pristine. System test disks are wiped by reset_systems, so existing suites should be unaffected.
DefaultPendingZFSStoragePool and DefaultZFSStoragePoolJoinConfig take a new recoverSource argument. All callers are updated.

Checklist

I have read the contributing guidelines and attest that all commits in this PR are signed off, cryptographically signed, and follow this project's commit structure.
I have checked and added or updated relevant documentation.

The answer to "Select which disks to wipe" was overwritten afterwards, so every selected local storage disk was always wiped. Signed-off-by: krish <krish@lyzr.com>

Disks with partitions were dropped entirely, which hides disks holding an existing local storage pool. Keep the unmounted ones separately as ReusableDisks and add IsPristineDisk which treats a disk as pristine only if it has neither a filesystem UUID nor partitions. Signed-off-by: krish <krish@lyzr.com>

Signed-off-by: krish <krish@lyzr.com>

…g systems When adding systems and LXD supports storage_source_recover, offer reusable disks for local storage and ask whether to reuse the existing storage pool on any selected non-pristine disk. Reused disks are set up with source.recover and are never wiped or offered for remote storage. Signed-off-by: krish <krish@lyzr.com>

Signed-off-by: krish <krish@lyzr.com>

roosterfish

Hi @krishvsoni, thanks for taking a look at this. Please see my comments.

roosterfish · 2026-10-29T19:09:17Z

 				continue
 			}

+			// Exclude non-pristine disks with partitions.


This comments needs updating. Non-pristine disks are only excluded in case they are mounted.

you are right, the comment is outdated now, Non-pristine disks are only dropped if they are mounted, the unmounted ones go to ReusableDisks.

roosterfish · 2026-10-29T19:32:47Z

+			// Exclude non-pristine disks with partitions.
+			// Disks already used for local storage (zfs) contain a partition and are therefore excluded by this check.
+			// Unmounted ones are kept separately as they might contain an existing storage pool that can be reused.
+			if len(disk.Partitions) != 0 {


Why not using the IsPristineDisk func here?

good point; I kept the plain partitions check on purpose so that disks with a filesystem but no partitions (e.g. a whole-disk btrfs/LVM) keep showing up in the Ceph disk list like today.

but that is inconsistent, and the docs already say storage disks should be free of partitions and filesystems.

I'll switch to IsPristineDisk so the split becomes:

if IsPristineDisk(disk) -> AvailableDisks else if not mounted -> ReusableDisks (local storage only)

one side effect worth flagging: disks with only a filesystem will no longer be offered for distributed storage
(they can still be picked for local storage and reused or wiped there).

Is that fine with you @roosterfish ?

roosterfish · 2026-10-29T19:39:25Z


+// IsPristineDisk returns true if the disk has neither partitions nor a filesystem.
+func IsPristineDisk(disk api.ResourcesStorageDisk) bool {
+	return disk.DeviceFSUUID == "" && len(disk.Partitions) == 0


why not including isDiskMounted also in here?

makes sense.

a mounted disk should never count as pristine, so I'll add it there. In practice a mounted disk already has a filesystem or partitions, but this makes it explicit and safer.

I'll still need isDiskMounted separately in the filter to decide whether a non-pristine disk is offered for reuse or skipped completely.

roosterfish · 2026-10-29T19:40:41Z

+	for i, c := range cases {
+		s.T().Logf("%d: %s", i, c.desc)
+
+		s.Equal(c.pristine, IsPristineDisk(c.disk))


As mentioned in the earlier comment, if isDiskMounted is included inside IsPristineDisk, then we can also check for the cases in case the disk or any of its partitions is mounted.

got it, will add test cases for a mounted disk and for a disk with a mounted partition.

roosterfish · 2026-10-29T19:41:59Z

+		Entity: "storage-pool",
+		Name:   DefaultZFSPool,
+		Key:    "source.recover",
+		Value:  "true",


Doesn't this value need to be based on recoverSource? Otherwise it's always true.

The key is only appended when go recoverSource is true:

if recoverSource { joinConfig = append(joinConfig, recoverDisk) }

So it can never end up in the config as true unless recovery was explicitly requested. This follows the same pattern as the existing wipeDisk key just above it.

If the logic is confusing, I'm happy to build the key inside the if block instead to make the conditional behavior more obvious.

roosterfish · 2026-10-29T19:43:52Z

+					continue
+				}
+
+				reuse, err := c.asker.AskBool(fmt.Sprintf("Disk %q on %q is not empty. Reuse its existing local storage pool?", selected[target], target), true)


Instead of empty, let's use the same terminology "non-pristine" instead of "not empty".

will do: Disk %q on %q is non-pristine. Reuse its existing local storage pool?

roosterfish · 2026-10-29T19:45:00Z


+		for _, disk := range system.Storage.Ceph {
+			if disk.Recover {
+				return fmt.Errorf("Cannot recover Ceph disks of system %q", system.Name)


Can this path be actually triggered?

yes; DirectStorage is shared by both the local and Ceph entries, so the following configuration parses successfully:

storage: ceph: - path: /dev/sdc recover: true

without the check, recover would simply be silently ignored for Ceph disks.

that said, I noticed that encrypt is also silently ignored for local disks today, so silently ignoring unsupported fields would at least be consistent with the existing behavior.

would you prefer to keep the explicit validation error or remove it for consistency with encrypt? alternatively, I could introduce a separate struct for local storage so that recover is only available there.

roosterfish · 2026-10-29T19:45:31Z

+				return fmt.Errorf("Missing the local storage disk path to recover on system %q", system.Name)
+			}
+
+			if system.Storage.Local.Wipe {


What about not asking to wipe if the user already selected to recover the disk?

The interactive flow already excludes reused disks from the wipe selection. In preseed, both flags can be set explicitly, so I added an error to prevent conflicting actions and unintended data loss. Alternatively, I can make recover take precedence over wipe.

roosterfish · 2026-10-29T19:48:59Z

+		}
+
+		if !recoverable {
+			return nil, errors.New("Cannot recover local storage disks: LXD is missing the storage_source_recover API extension")


This should never be surfaced to a user. The existence of the API extension should define whether or not a user can even recover non-pristine disks. This means if the API extension is absent, we should not even display non-pristine disks and continue as today with pristine disks only.

roosterfish · 2026-10-29T19:49:40Z


 The default file system type for local storage is ZFS.

+When adding a cluster member, MicroCloud also offers unmounted disks that contain an existing local storage pool, for example after reinstalling a machine.


Let's use the "non-pristine" terminology here too.

got it, will do ..

A mounted disk is never pristine. Only pristine disks are available for storage, unmounted non-pristine disks are kept as reusable. Signed-off-by: krish <krish@lyzr.com>

The extension was added in LXD 6.7 and MicroCloud already requires at least LXD 6.10, so it is always present. Signed-off-by: krish <krish@lyzr.com>

Recovering an existing local storage pool is also useful when the whole cluster is set up again, so allow it both interactively and in the preseed when initializing MicroCloud. Signed-off-by: krish <krish@lyzr.com>

…rompt Signed-off-by: krish <krish@lyzr.com>

Signed-off-by: krish <krish@lyzr.com>

krishvsoni · 2026-10-30T17:53:16Z

@roosterfish thanks for the detailed review. I pushed the changes, here is a summary per comment:

Disk classification (service/system_information.go)

Comment updated and the filter now uses IsPristineDisk: pristine disks go to AvailableDisks, unmounted non-pristine disks go to ReusableDisks, mounted ones are skipped.
IsPristineDisk now also checks isDiskMounted, with test cases for a mounted disk and a disk with a mounted partition.
Side effect: disks with only a filesystem are no longer offered for distributed storage. This matches the documented requirement that storage disks are free of partitions and filesystems.

→ 1d15a2f

API extension check

Removed it in both the interactive flow and the preseed. storage_source_recover was added in LXD 6.7 and MicroCloud already requires LXD 6.10 or newer (lxdMinVersion), so the extension is always present and the error can no longer reach the user.

→ bd90b8d

Reuse during init

Non-pristine disks are now offered and the reuse question is asked during init as well, both interactively and in the preseed.

→ 0b0a83f

Terminology

The prompt is now Disk %q on %q is non-pristine. Reuse its existing local storage pool? and the docs use "non-pristine" too.

→ bb814c0, 59ff196

source.recover in DefaultZFSStoragePoolJoinConfig

Unchanged, the key is only appended when recoverSource is true (same pattern as wipeDisk). Happy to restructure it if you think it reads better.

Open questions, waiting for your preference

recover on Ceph disks in the preseed: keep the explicit error, or silently ignore it like encrypt on local disks?
wipe + recover on the same disk in the preseed: keep the error, or let recover take precedence?

krishvsoni added 6 commits October 28, 2026 22:51

cmd/microcloud: Respect disk wipe selection for local storage

e8f19dc

The answer to "Select which disks to wipe" was overwritten afterwards, so every selected local storage disk was always wiped. Signed-off-by: krish <krish@lyzr.com>

service: Support source.recover in ZFS storage pool helpers

1416071

Signed-off-by: krish <krish@lyzr.com>

cmd/microcloud: Add recover option for local storage disks in preseed

d1a848f

Signed-off-by: krish <krish@lyzr.com>

doc: Document reusing existing local storage pools

88fdfeb

Signed-off-by: krish <krish@lyzr.com>

github-actions Bot added the Documentation Documentation needs updating label Oct 28, 2026

roosterfish requested changes Oct 29, 2026

View reviewed changes

krishvsoni added 5 commits October 30, 2026 23:15

service: Use IsPristineDisk to separate available and reusable disks

1d15a2f

A mounted disk is never pristine. Only pristine disks are available for storage, unmounted non-pristine disks are kept as reusable. Signed-off-by: krish <krish@lyzr.com>

cmd/microcloud: Remove storage_source_recover extension checks

bd90b8d

The extension was added in LXD 6.7 and MicroCloud already requires at least LXD 6.10, so it is always present. Signed-off-by: krish <krish@lyzr.com>

cmd/microcloud: Allow reusing local storage pools during init

0b0a83f

Recovering an existing local storage pool is also useful when the whole cluster is set up again, so allow it both interactively and in the preseed when initializing MicroCloud. Signed-off-by: krish <krish@lyzr.com>

cmd/microcloud: Use non-pristine terminology in local storage reuse p…

bb814c0

…rompt Signed-off-by: krish <krish@lyzr.com>

doc: Use non-pristine terminology for local storage pool reuse

59ff196

Signed-off-by: krish <krish@lyzr.com>

krishvsoni requested a review from roosterfish October 30, 2026 17:55


		The default file system type for local storage is ZFS.

		When adding a cluster member, MicroCloud also offers unmounted disks that contain an existing local storage pool, for example after reinstalling a machine.

Conversation

krishvsoni commented Oct 28, 2026

Summary

Evidence

Merge Danger

Checklist

Uh oh!

roosterfish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krishvsoni commented Oct 30, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants