Add Btrfs fallback for other Linux distros by locnnil · Pull Request #956 · canonical/workshop

locnnil · 2026-07-21T22:29:50Z

Description

Enables Btrfs fallback for other Linux distributions.

ZFS is the preferred storage driver, but it isn't available everywhere: WSL, SUSE, Arch, Fedora, and other distributions may ship without it. Keying the fallback off the distribution name is too coarse; it assumes every non-Ubuntu host lacks ZFS and every Ubuntu-derived one has it, neither of which holds for Debian and its derivatives.

Instead, detect whether the host provides a usable ZFS kernel module, already loaded (present at /dev/zfs AND /sys/module/zfs ) or loadable for the running kernel per modules.dep(5) (/lib/modules/$(uname -r)/modules.dep) or modules.builtin (/lib/modules/$(uname -r)/modules.builtin) and keep ZFS only then, falling back to btrfs otherwise. Only the kernel module is checked: LXD ships as a snap that bundles the zpool/zfs userspace tools, so a loaded or loadable module is what actually determines usability from the host side, tested on Arch Linux and openSUSE tumbleweed.

Closes: workshop does not work on non-Ubuntu systems #742

Self-review quick check

Make decisions that cost a lot to reverse explicit in the PR description.
Avoid nested conditions.
Delete dead code and redundant comments. TODO: check if it is okay to delete isWSL()
Normalise symmetries by sticking to doing identical things identically.

// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...

Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
Put variable declaration and initialisation together.
Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
Put a blank line between two logically different chunks of code.
Follow the style guide for new error messages.

Docs

Procedure:

I have checked and added or updated relevant documentation.
I have checked and added or updated relevant release notes.
I have included the technical author in the review.

Content:

Headings and titles accurately describe the content.
New and updated pages include correct metadata.
Documentation tests are added or updated where applicable (for tutorial/ and how-to/ sections).
Documentation follows the style guide.
If needed, docs/.coverage.yaml updated, coverage tags added (.. artefact).

Or:

I confirm the PR has no implications for documentation.

ZFS is the preferred storage driver, but it isn't available everywhere: WSL, SUSE, Arch, Fedora and other distributions may ship without it. Keying the fallback off the distribution name is too coarse -- it assumes every non-Ubuntu host lacks ZFS and every Ubuntu-derived one has it, neither of which holds for Debian and its derivatives. Instead, detect whether the host provides a usable ZFS kernel module (already loaded, or loadable for the running kernel per modules.dep / modules.builtin) and keep ZFS only then, falling back to btrfs otherwise. Only the kernel module is checked: LXD ships as a snap that bundles the zpool/zfs userspace tools, so a loaded or loadable module is what actually determines usability from the host side. Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

The storage docs described the Btrfs fallback as WSL-specific. Reword the three affected passages to reflect that Workshop uses ZFS wherever it detects ZFS is available and otherwise falls back to Btrfs, keeping WSL as an example rather than the sole case. Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

jonathan-conder · 2026-07-21T22:35:16Z

It looks to me like the way LXD does it is just trying to load the zfs module; maybe we should follow their lead on that

jonathan-conder · 2026-07-21T22:37:40Z

I think we also need to be more careful about where we use the storagePoolDriver variable. Currently we can assume it's accurate, since whether the host is WSL or not is unlikely to change. The availability of the ZFS module can easily change, though; we need a separate mechanism to indicate "what's the current driver of the workshop pool."

locnnil · 2026-07-22T00:23:10Z

I think we also need to be more careful about where we use the storagePoolDriver variable. Currently we can assume it's accurate, since whether the host is WSL or not is unlikely to change. The availability of the ZFS module can easily change, though; we need a separate mechanism to indicate "what's the current driver of the workshop pool."

So, considering the case where ZFS was available when the pool was created but later stops being present, most likely because the user installed a different kernel that doesn't ship the ZFS module. What should we do with the now-unusable ZFS pool?

I can think of those options:

Put the daemon into a degraded state and add an actionable error message like "restore the ZFS-capable kernel, or remove the pool to recreate it as Btrfs"
Warn the user with an actionable message like "Old ZFS pool detected on system without support for ZFS on path <path-to-zfs-pool>" but create a new Btrfs pool under a different name.

I believe that the first option is better, right?

jonathan-conder · 2026-07-22T01:37:09Z

I believe that the first option is better, right?

I agree, but we should not suggest removing the pool, it will probably break the workshops using it (if it's even possible to remove it). Instead maybe it's best to reinstall the Workshop snap.

We just need to test what actually happens in that scenario, maybe LXD will fail to start and we probably handle that already.

ZFS is the preferred storage driver, but it isn't available everywhere (Debian, SUSE, Fedora and WSL, among others, may ship without it). Use ZFS only when LXD reports it as a supported storage driver; which LXD determines by trying to load the driver's kernel module, and fall back to Btrfs otherwise. Keying the choice off actual availability rather than the host distribution makes the fallback apply to any host lacking ZFS, while still using ZFS wherever it works. Derive the pool's driver from LXD and the pool itself rather than from a fixed value: pick it from LXD's supported drivers when creating the pool, and read it back from the pool wherever behaviour depends on it. A pool's driver is fixed at creation and ZFS availability can change, so a single cached value can't be trusted. When the workshop pool exists but its storage driver is no longer usable (for example a ZFS pool after booting a kernel without the ZFS module), put the daemon into degraded mode with an actionable message instead of failing obscurely. Closes: canonical#742 Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

locnnil · 2026-08-14T01:50:14Z

@jonathan-conder I've implemented the requested change in 54bf6dd, which stops assuming a fixed state for ZFS availability. I then tested the degraded-state behavior for the scenario where ZFS is present at first but later vanishes.

I ran this on an Ubuntu Resolute LXD VM, reproducing a workshop pool created on ZFS whose kernel module later becomes unavailable after a kernel change.

Reproduction

Started with ZFS available, so the workshop pool was created on the zfs driver.
Built and installed a .deb for a newer kernel (7.2.0-rc7) that doesn't ship the ZFS module.
Removed the previous kernel and its ZFS module package (linux-main-modules-zfs-7.0.0-29-generic).
Rebooted into the new kernel and ran workshop list.

Result

Instead of surfacing a raw LXD error, the daemon goes into degraded mode with an actionable message:

$ workshop list
error: system is not healthy: cannot use the "workshop" storage pool, its storage driver may be unavailable (for example a ZFS pool after booting a kernel without the ZFS module): Error loading "zfs" module: Failed running: modprobe -b zfs
: exit status 1 (modprobe: FATAL: Module zfs not found in directory /lib/modules/7.2.0-rc7)
Boot into a kernel that provides the pool's storage driver, or reinstall Workshop to recreate the pool on an available driver

Recovery

The ZFS pool can't be deleted through LXD while the module is missing (lxc storage delete loads the driver, which fails), so the recovery is to reinstall Workshop: this recreates the workshop pool on an available driver (Btrfs) for a host that no longer provides ZFS.

On adding a Spread test

Let me know if you think we should add a Spread test covering this case. Two caveats:

For my manual test I had to build and install an upstream kernel, since simply uninstalling linux-main-modules-zfs-7.0.0-xx-generic isn't enough to reproduce a realistic scenario. For an automated test we could instead force the broken state, making it reproducible by just forcefully removing a package.
The bigger drawback is that this would require Spread to run on LXD VMs instead of containers. That would be a significant change to the current testing infrastructure.

jonathan-conder · 2026-08-16T22:17:22Z

 		inst.Devices = map[string]map[string]string{}
 	}
-	if err := mergeDevices(inst.Devices, snapshot.Sdks, name); err != nil {
+	usesZFS, err := poolUsesZFS(conn)


this adds an additional API call to these operations, do we have any info on the impact it has on latency? we could alternatively cache the storage pool driver at launch

With this change cbe0b61 poolUsesZFS reads from a cached value.

jonathan-conder · 2026-08-16T23:15:41Z

The ZFS pool can't be deleted through LXD while the module is missing (lxc storage delete loads the driver, which fails), so the recovery is to reinstall Workshop: this recreates the workshop pool on an available driver (Btrfs) for a host that no longer provides ZFS.

I don't see how reinstalling Workshop will help. If LXD is unable to delete the pool then it will still be there after the reinstall.

For my manual test I had to build and install an upstream kernel, since simply uninstalling linux-main-modules-zfs-7.0.0-xx-generic isn't enough to reproduce a realistic scenario. For an automated test we could instead force the broken state, making it reproducible by just forcefully removing a package.

It's not reasonable to do that in a test. It's a rare enough case that I think some manual recovery steps are OK.

The bigger drawback is that this would require Spread to run on LXD VMs instead of containers. That would be a significant change to the current testing infrastructure.

we already do this, it's not a change at all

Refine how the LXD backend determines the workshop pool's storage driver: - Query the workshop pool directly with GetStoragePool and branch on the 404 status instead of listing every pool. This matches checkStoragePool and checkStorageSpace and avoids loading unrelated pools' drivers. - Record the pool's driver once it is known (picked when the pool is created, or read back from an existing pool) and reuse it for snapshot and copy operations, instead of querying LXD on every call. The driver is fixed at creation, so the cached value stays accurate for the daemon run. - Drop the redundant "is the preferred driver supported" check: it only re-tested the choice preferredDriver already made from the supported list, and pool creation surfaces an unusable driver anyway. - Inline the "zfs"/"btrfs" literals; the named constants added nothing over LXD's fixed driver identifiers. Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

locnnil · 2026-08-29T01:13:18Z

I don't see how reinstalling Workshop will help. If LXD is unable to delete the pool then it will still be there after the reinstall.

Yeah, I think in this case you would need to reinstall LXD and Workshop, but this seems too drastic. Anyway, I've tested, and when the ZFS-capable kernel is restored, the workshop will be working again.

jonathan-conder · 2026-09-04T00:10:27Z

+// re-querying LXD (which reloads the driver's module). The pool's driver is
+// fixed at creation, so the cached value stays accurate for the daemon's run.
+var poolDriverCache struct {
+	sync.RWMutex


do we need a mutex here? I think anything that looks at the storage driver should run after the value has been initialised.

- doc: lowercase btrfs spelling - cleanup: refact preferredDrivers function - feat: Record workshop pool driver without a mutex The pool driver is recorded once during the initial system check, before the daemon serves requests, so pool operations can read it directly. Drop the RWMutex and lazy initialisation, which are no longer needed. - chore: improve comment Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Co-authored-by: Jonathan Conder <jono.conder@gmail.com>

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

jonathan-conder

Ok I think we're nearly there, just a couple of minor comments below, and I still think we should check for storagePoolDriver != existingPool.Driver in ensureBackendReady()

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot

🟡 Changes recommended

Active critical issues remain, including an unused import that prevents compilation and insufficient validation of existing or unknown storage drivers, which can cause incorrect snapshot behavior.

5 open issues

_{Review effort: Lite}

Copilot

🔵 Needs a closer look

Approval is blocked by an active moderate backend issue that accepts unsupported existing pool drivers, along with documentation inconsistencies about filesystem support and capitalization.

4 open issues

1 resolved since last review

Remove the unused text/template import

_{Review effort: Lite}

jonathan-conder · 2026-11-05T04:06:35Z

-	return checkStorageDriver(info.Environment.StorageSupportedDrivers)
+	return checkStoragePool(conn)


this looks like a behaviour change. If the storage pool doesn't exist we should still check that at least one driver is available

jonathan-conder · 2026-11-05T04:16:45Z

 		if err := op.Wait(); err != nil {
 			return err
 		}
+		setPoolDriver(driver)


I'm not sure an atomic is enough, here. Since, apparently, several tasks might call New() concurrently, if two of them attempt to create the storage pool, one will fail with pool already exists or something like that. Maybe use sync.Once or a mutex?

I've decided to go with a mutex on ensureBackendReady(), since ensureBackendReady() needs to run again after failures and to recreate a pool deleted at runtime.

jonathan-conder · 2026-11-05T04:20:51Z

+		// The pool was recreated on another driver while the daemon was
+		// running. Adopting the new driver would change snapshot behavior
+		// underneath running operations, so refuse instead.
+		return fmt.Errorf("storage pool %q changed driver: %q (expected %q)", storagePool, existingPool.Driver, driver)


I think in this case we might as well just adopt the pool's driver (as long as it's zfs or btrfs). It's still not really supported but if you change it while the daemon is completely idle it seems fine, now that we can handle concurrent reads/writes.

Co-authored-by: Jonathan Conder <jono.conder@gmail.com>

Copilot

🔵 Needs a closer look

The change still has an unhandled unsupported-pool-driver path and documentation that incorrectly presents ZFS behavior as universal, leaving active correctness and user-facing accuracy issues.

2 open issues

2 resolved since last review

_{Review effort: Lite}

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot

🟡 Changes recommended

The change leaves two active documentation inconsistencies: an unconditional ZFS/LZ4 capability claim on the Btrfs path and a contradictory developer prerequisite that still requires ZFS.

3 open issues

1 resolved since last review

Reject existing pools with unsupported drivers

_{Review effort: Lite}

+with automatic btrfs fallback otherwise,
+for example on Windows Subsystem for Linux (WSL).


 |ws_markup| is supported on Ubuntu
 and other :program:`snap`-enabled Linux distributions;
-it is also compatible with Windows Subsystem for Linux (WSL2),
-where it uses Btrfs instead of ZFS for storage.
+it is also compatible with Windows Subsystem for Linux (WSL2).
+For storage, it uses ZFS where |ws_markup| detects that ZFS is available,
+and otherwise falls back to btrfs.


Copilot

🟡 Changes recommended

Active documentation inconsistency and stale storage-driver caching remain unresolved, including a moderate correctness risk when recreating a deleted pool.

3 open issues

1 resolved since last review

Synchronize storage driver recovery with backend goroutines

_{Review effort: Lite}

+		// Reuse the driver already in use, if any.
+		driver := poolDriver()
+		if driver == "" {
+			info, _, err := conn.GetServer()
+			if err != nil {
+				return err
+			}
+			driver, err = preferredDriver(info.Environment.StorageSupportedDrivers)
+			if err != nil {
+				return err
+			}
+		}


Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot

🔵 Needs a closer look

The documentation remains internally contradictory because several sections still present ZFS as universal despite newly documented Btrfs support, leaving an active correctness issue to resolve.

3 open issues

_{Review effort: Lite}

Copilot

🟡 Changes recommended

Active documentation inconsistencies and a moderate recovery bug that can retain unavailable ZFS instead of selecting supported Btrfs require changes.

5 open issues

_{Review effort: Lite}

locnnil added 2 commits July 21, 2026 17:34

jonathan-conder reviewed Aug 16, 2026

View reviewed changes

locnnil added 2 commits August 19, 2026 22:59

Merge branch 'main' into add-btrfs-fallback

9ce2502

Merge branch 'main' into add-btrfs-fallback

d75fc17

locnnil force-pushed the add-btrfs-fallback branch from cbe0b61 to 2fcccdc Compare August 28, 2026 22:42

locnnil force-pushed the add-btrfs-fallback branch from 2fcccdc to 949bc5e Compare August 28, 2026 22:48

locnnil requested a review from jonathan-conder August 29, 2026 01:13

jonathan-conder requested changes Sep 4, 2026

View reviewed changes

jonathan-conder requested changes Sep 27, 2026

View reviewed changes

locnnil and others added 3 commits September 28, 2026 10:11

Merge branch 'main' into add-btrfs-fallback

bacaa53

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Apply batched suggestions from code review

4655600

Co-authored-by: Jonathan Conder <jono.conder@gmail.com>

address comments

5958238

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

locnnil force-pushed the add-btrfs-fallback branch from e2fffd4 to 5958238 Compare October 5, 2026 14:48

locnnil requested a review from jonathan-conder October 5, 2026 14:56

jonathan-conder reviewed Oct 5, 2026

View reviewed changes

Comment thread docs/reference/workshops.rst Resolved

Comment thread internal/workshop/lxd/lxd_backend.go Resolved

address comments

f314d84

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

locnnil requested review from jonathan-conder and a lite review from Copilot October 6, 2026 13:42

Copilot started reviewing on behalf of locnnil October 6, 2026 13:43

View session

Merge branch 'main' into add-btrfs-fallback

8374a20

Copilot AI lite review requested due to automatic review settings November 4, 2026 12:47

Copilot started reviewing on behalf of locnnil November 4, 2026 12:47

View session

fix imported and not used

7ba83d0

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot AI reviewed Nov 4, 2026

View reviewed changes

Comment thread internal/workshop/lxd/lxd_backend.go Resolved

Comment thread internal/workshop/lxd/lxd_backend.go Resolved

Comment thread internal/workshop/lxd/lxd_backend_snapshots.go Resolved

Comment thread docs/explanation/architecture/components.rst Resolved

Copilot AI lite review requested due to automatic review settings November 4, 2026 12:53

Copilot started reviewing on behalf of locnnil November 4, 2026 12:54

View session

Copilot AI reviewed Nov 4, 2026

View reviewed changes

jonathan-conder reviewed Nov 5, 2026

View reviewed changes

Apply batched suggestions from code review

be2a601

Co-authored-by: Jonathan Conder <jono.conder@gmail.com>

Copilot AI lite review requested due to automatic review settings November 5, 2026 12:41

Copilot started reviewing on behalf of locnnil November 5, 2026 12:41

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

locnnil added 3 commits November 5, 2026 12:42

Check if zfs or btrfs is available once there is no storage pool

6af071b

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

lxd: serialize ensureBackendReady

da7253d

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

adopt the storage pool's driver if it is ZFS or Btrfs

df57b99

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot AI lite review requested due to automatic review settings November 5, 2026 15:52

Copilot started reviewing on behalf of locnnil November 5, 2026 15:52

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

locnnil requested a lite review from Copilot November 5, 2026 17:03

Copilot started reviewing on behalf of locnnil November 5, 2026 17:04

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

Doc: mention about btrfs storage driver support on contributing

275e977

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>

Copilot AI lite review requested due to automatic review settings November 5, 2026 17:12

Copilot started reviewing on behalf of locnnil November 5, 2026 17:13

View session

locnnil requested a review from jonathan-conder November 5, 2026 17:16

Copilot AI reviewed Nov 5, 2026

View reviewed changes

locnnil requested a lite review from Copilot November 9, 2026 16:12

Copilot started reviewing on behalf of locnnil November 9, 2026 16:13

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

Comment thread docs/contributing/development.rst Resolved

Comment thread docs/tutorial/part-1-get-started.rst Resolved

		return checkStorageDriver(info.Environment.StorageSupportedDrivers)
		return checkStoragePool(conn)

		with automatic btrfs fallback otherwise,
		for example on Windows Subsystem for Linux (WSL).

Conversation

locnnil commented Jul 21, 2026

Description

Self-review quick check

Docs

Uh oh!

jonathan-conder commented Jul 21, 2026

Uh oh!

jonathan-conder commented Jul 21, 2026

Uh oh!

locnnil commented Jul 22, 2026

Uh oh!

jonathan-conder commented Jul 22, 2026

Uh oh!

locnnil commented Aug 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathan-conder Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

locnnil Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jonathan-conder commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

locnnil commented Aug 29, 2026

Uh oh!

Uh oh!

Uh oh!

jonathan-conder Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathan-conder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathan-conder Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonathan-conder Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

locnnil Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

jonathan-conder Nov 5, 2026

Choose a reason for hiding this comment

jonathan-conder commented Aug 16, 2026 •

edited

Loading