Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/explanation/architecture/components.rst
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,9 @@ used for temporary storage during workshop rebuild operations.
Storage backends
~~~~~~~~~~~~~~~~

|ws_markup| uses ZFS for storage on Linux,
with automatic Btrfs fallback on Windows Subsystem for Linux (WSL).
|ws_markup| uses ZFS for storage where it detects that ZFS is available,
with automatic Btrfs fallback otherwise,
for example on Windows Subsystem for Linux (WSL).
Storage is managed via LXD and requires a minimum pool size of 5 GiB.

The storage backend manages container root filesystems, workshop-specific data volumes,
Expand Down
5 changes: 3 additions & 2 deletions docs/reference/workshops.rst
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ Storage pools and drivers
-------------------------

|ws_markup| stores its containers and data on a storage pool.
On Linux, |ws_markup| uses ZFS,
while on Windows Subsystem for Linux it automatically uses Btrfs.
It uses ZFS on systems where |ws_markup| detects that ZFS is available,
and otherwise falls back to Btrfs
(for example on Windows Subsystem for Linux).
This approach consolidates container images, :program:`apt` caches, SDKs
and other workshop content under a single system.

Expand Down
5 changes: 3 additions & 2 deletions docs/tutorial/part-1-get-started.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ Prerequisites

|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.

|ws_markup| relies on
`LXD 6.8+ <https://canonical.com/lxd>`_
Expand Down
2 changes: 2 additions & 0 deletions internal/workshop/lxd/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ var (
HandleImageUpdate = handleImageUpdate
CheckServerVersion = checkVersion
GenerateCNAME = generateCNAME
PreferredDriver = preferredDriver
DriverSupported = driverSupported
)

func MockFirewallChecker(f func(string) string) func() {
Expand Down
99 changes: 63 additions & 36 deletions internal/workshop/lxd/lxd_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package lxdbackend

import (
"bytes"
"cmp"
"context"
"embed"
Expand Down Expand Up @@ -58,6 +57,11 @@ const (
storagePool = "workshop"
storagePoolMinimalGiB = 5

// ZFS is workshop's preferred storage driver; Btrfs is the fallback used
// on hosts where LXD reports ZFS as unavailable.
storageDriverZFS = "zfs"
storageDriverBtrfs = "btrfs"

Comment on lines +57 to +61

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what we gain from these constants

networkName = "workshopbr0"
networkType = "bridge"

Expand All @@ -73,31 +77,12 @@ const (

var (
startCommandTimeout = 1 * time.Minute
storagePoolDriver = "zfs"
)

//go:embed start_command.sh
var startCommand string

// isWSL checks if we're running on Windows Subsystem for Linux
func isWSL() bool {
var utsname unix.Utsname
if err := unix.Uname(&utsname); err != nil {
return false
}
data := utsname.Release[:]
if idx := bytes.IndexByte(data, 0); idx >= 0 {
data = data[:idx]
}
version := strings.ToLower(string(data))
return strings.Contains(version, "microsoft") || strings.Contains(version, "wsl2")
}

func init() {
if isWSL() {
storagePoolDriver = "btrfs"
}

// Order matters: capabilities (version and storage) must be validated
// before ensureBackendReady attempts to create the storage pool and
// network. Registering it as a check also lets the daemon recover after
Expand Down Expand Up @@ -179,19 +164,56 @@ func checkVersion(version string) error {
return nil
}

func checkStorageDriver(drivers []api.ServerStorageDriverInfo) error {
hasDriver := func(driver api.ServerStorageDriverInfo) bool {
return driver.Name == storagePoolDriver
// driverSupported reports whether LXD lists the named storage driver as
// supported. LXD builds this list by attempting to load each driver's kernel
// module (e.g. modprobe zfs), so it reflects what the host can actually back.
func driverSupported(supported []api.ServerStorageDriverInfo, name string) bool {
return slices.ContainsFunc(supported, func(d api.ServerStorageDriverInfo) bool {
return d.Name == name
})
}

// preferredDriver picks the driver for a new workshop pool: ZFS when LXD
// reports it as supported, otherwise Btrfs.
func preferredDriver(supported []api.ServerStorageDriverInfo) string {
if driverSupported(supported, storageDriverZFS) {
return storageDriverZFS
}
return storageDriverBtrfs
}

// poolUsesZFS reports whether the workshop storage pool is backed by ZFS. The
// driver is read from the pool itself, since it is fixed when the pool is
// created and can differ from what the host would pick today.
func poolUsesZFS(conn lxd.InstanceServer) (bool, error) {
pool, _, err := conn.GetStoragePool(storagePool)
if err != nil {
return false, err
}
if slices.ContainsFunc(drivers, hasDriver) {
return pool.Driver == storageDriverZFS, nil
}

// checkStoragePool verifies the workshop storage pool is usable. When the pool
// exists but cannot be loaded, most likely because its storage driver's kernel
// module is gone (e.g. a ZFS pool after booting a kernel without the ZFS
// module), it returns an actionable error that puts the daemon into degraded
// mode. When the pool does not exist yet, it ensures a supported driver is
// available to create it with.
func checkStoragePool(conn lxd.InstanceServer, supported []api.ServerStorageDriverInfo) error {
_, _, err := conn.GetStoragePool(storagePool)
if err == nil {
return nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to check if the storage pool driver is supported here?

}
if !api.StatusErrorCheck(err, http.StatusNotFound) {
return fmt.Errorf(`cannot use the %q storage pool, its storage driver may be unavailable (for example a ZFS pool after booting a kernel without the ZFS module): %w
Boot into a kernel that provides the pool's storage driver, or reinstall Workshop to recreate the pool on an available driver`, storagePool, err)
Comment on lines +206 to +207

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the error returned from GetStoragePool related to this explanation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from my tests, this is what you get:

$ 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

So the error returned by GetStoragePool is:

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)

Perhaps the explanation should be less verbose? Since the error is already quite informative.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting and a bit unexpected. I'd rather not rely on specific LXD error messages if possible, it's likely not something they have tests for. How about checking info.Environment.StorageSupportedDrivers like you did below?

The error message is useful though, does it appear in the LXD logs? Maybe we can direct users there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not rely on specific LXD error messages if possible

Good point, I'll keep my explanation rather than trimming it in favor of LXD's message.

How about checking info.Environment.StorageSupportedDrivers like you did below?

On using info.Environment.StorageSupportedDrivers for this check, though, I ran into a wall: to know whether the existing workshop pool is affected, we need its driver, and every call that returns it loads that driver's module. GetStoragePool (and GetStoragePools with recursion) goes through LXD's LoadByName -> drivers.Load -> modprobe, which is the exact call that fails once the module is gone (that's why my test showed GetStoragePool erroring). GetStoragePoolNames avoids the load, but only returns names, not drivers.

So StorageSupportedDrivers works for creation (picking a driver we already know is available, as below), but on its own it can't tell us whether the already-created pool's driver is still supported, since we can't read that pool's driver without triggering the failing load.

On the logs: LXD only logs the "driver not available" line at Debug level (in SupportedDrivers), so it won't appear in default logs, directing users that there wouldn't be reliability unless they first raise LXD's log level.

A couple of options that I could think of:

  1. Keep the status-based detection. We branch on the 404 vs. non-404 status from GetStoragePool, not on the message text; the LXD error is only surfaced through %w. I'd just keep our wording neutral.

  2. Persist the pool's driver at creation, then the health check compares that stored value against StorageSupportedDrivers without ever calling GetStoragePool. This would also address your caching comment on the snapshot path.

I prefer (1) for this PR as the smaller change, but (2) is cleaner if we want to avoid depending on the LXD error at all. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely keep branching on 404. I would do the following:

  1. Call GetStoragePool("workshop") (pseudocode). If that fails for any reason other than 404, report the error and give up.
  2. If it succeeds, record its driver.
  3. If it 404s, get the info and pick a driver from info.Environment.StorageSupportedDrivers.

}

// The LXD error message when creating a pool is:
// Error: Error loading "zfs" module: Failed to run: modprobe -b zfs:
// exit status 1 (modprobe: FATAL: Module zfs not found ...)
// We keep the first part for consistency, the rest doesn't add much.
return fmt.Errorf(`suitable storage backend not found: error loading %q module`, storagePoolDriver)
// The pool doesn't exist yet; make sure it can be created.
if !driverSupported(supported, preferredDriver(supported)) {
return fmt.Errorf(`suitable storage backend not found: neither %q nor %q is available`, storageDriverZFS, storageDriverBtrfs)
}
return nil
}

// checkStorageSpace puts the daemon into degraded mode when the workshop
Expand Down Expand Up @@ -253,7 +275,7 @@ func checkServerCapabilities() error {
return err
}

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

// New constructs the LXD backend and attempts to prepare the required LXD
Expand Down Expand Up @@ -284,15 +306,22 @@ func ensureBackendReady() error {
}
defer conn.Disconnect()

// Create LXD storage pool if it doesn't exist.
pools, err := conn.GetStoragePools()
// Create LXD storage pool if it doesn't exist. GetStoragePoolNames avoids
// loading each pool's driver, so an existing but currently-unloadable pool
// (e.g. a ZFS pool whose module is gone) is reported by checkStoragePool
// rather than failing here.
names, err := conn.GetStoragePoolNames()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to list all the storage pools here? what stops us querying workshop directly?

if err != nil {
return err
}
if idx := slices.IndexFunc(pools, func(p api.StoragePool) bool { return p.Name == storagePool }); idx < 0 {
if !slices.Contains(names, storagePool) {
info, _, err := conn.GetServer()
if err != nil {
return err
}
req := api.StoragePoolsPost{
Name: storagePool,
Driver: storagePoolDriver,
Driver: preferredDriver(info.Environment.StorageSupportedDrivers),
}
op, err := conn.CreateStoragePool(req)
if err != nil {
Expand Down Expand Up @@ -331,8 +360,6 @@ func ensureBackendReady() error {

logger.Noticef("On ensureBackendReady: set storage pool to the minimal size: %dGiB", storagePoolMinimalGiB)
}
} else if pools[idx].Driver != storagePoolDriver {
return fmt.Errorf("storage pool %q already exists with a different driver: %q (expected %q)", storagePool, pools[idx].Driver, storagePoolDriver)
}

network, etag, err := conn.GetNetwork(networkName)
Expand Down
16 changes: 12 additions & 4 deletions internal/workshop/lxd/lxd_backend_snapshots.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,11 @@ func (s *Backend) TakeSnapshot(ctx context.Context, name string, snapshot worksh
if inst.Devices == nil {
inst.Devices = map[string]map[string]string{}
}
if err := mergeDevices(inst.Devices, snapshot.Sdks, name); err != nil {
usesZFS, err := poolUsesZFS(conn)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return err
}
if err := mergeDevices(inst.Devices, snapshot.Sdks, name, usesZFS); err != nil {
return err
}

Expand Down Expand Up @@ -604,12 +608,12 @@ func mergeConfig(source, target, config map[string]string) {
maps.Copy(source, config)
}

func mergeDevices(source map[string]map[string]string, sdks []sdk.ContentID, w string) error {
func mergeDevices(source map[string]map[string]string, sdks []sdk.ContentID, w string, usesZFS bool) error {
maps.DeleteFunc(source, func(k string, v map[string]string) bool {
return k != "root"
})

if storagePoolDriver == "zfs" {
if usesZFS {
root := maps.Clone(source["root"])
if source == nil || root == nil {
return fmt.Errorf("internal error: %q workshop has no rootfs", w)
Expand Down Expand Up @@ -740,7 +744,11 @@ func (s *Backend) copyInstance(src, dst lxd.InstanceServer, srcName, dstName str

req := *srcInst

if storagePoolDriver == "zfs" {
usesZFS, err := poolUsesZFS(dst)
if err != nil {
return err
}
if usesZFS {
req.Devices = maps.Clone(req.Devices)
root := maps.Clone(req.Devices["root"])
if req.Devices == nil || root == nil {
Expand Down
29 changes: 29 additions & 0 deletions internal/workshop/lxd/lxd_backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package lxdbackend_test
import (
"testing"

"github.com/canonical/lxd/shared/api"
"gopkg.in/check.v1"

"github.com/canonical/workshop/internal/testutil"
Expand Down Expand Up @@ -134,3 +135,31 @@ func (f *LxdBeTests) TestCheckLxdVersion(c *check.C) {
err = lxdbackend.CheckServerVersion("6.7.9")
c.Assert(err, check.ErrorMatches, `(?s).*LXD server version.*is not supported.*`)
}

func driverInfo(names ...string) []api.ServerStorageDriverInfo {
infos := make([]api.ServerStorageDriverInfo, 0, len(names))
for _, n := range names {
infos = append(infos, api.ServerStorageDriverInfo{Name: n})
}
return infos
}

// preferredDriver keeps ZFS whenever LXD reports it, and falls back to Btrfs
// otherwise. Detection now trusts LXD's supported-drivers list (which LXD
// builds by trying to load each driver's module) rather than probing modules.
func (f *LxdBeTests) TestPreferredDriver(c *check.C) {
c.Check(lxdbackend.PreferredDriver(driverInfo("zfs", "btrfs", "dir")), check.Equals, "zfs")
c.Check(lxdbackend.PreferredDriver(driverInfo("btrfs", "zfs")), check.Equals, "zfs")

c.Check(lxdbackend.PreferredDriver(driverInfo("btrfs", "dir")), check.Equals, "btrfs")
c.Check(lxdbackend.PreferredDriver(driverInfo("dir")), check.Equals, "btrfs")
c.Check(lxdbackend.PreferredDriver(nil), check.Equals, "btrfs")
}

func (f *LxdBeTests) TestDriverSupported(c *check.C) {
supported := driverInfo("zfs", "btrfs")
c.Check(lxdbackend.DriverSupported(supported, "zfs"), check.Equals, true)
c.Check(lxdbackend.DriverSupported(supported, "btrfs"), check.Equals, true)
c.Check(lxdbackend.DriverSupported(supported, "lvm"), check.Equals, false)
c.Check(lxdbackend.DriverSupported(nil, "zfs"), check.Equals, false)
}
Loading