-
Notifications
You must be signed in to change notification settings - Fork 16
Add Btrfs fallback for other Linux distros #956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1a6a633
4d2d9cb
54bf6dd
9ce2502
d75fc17
949bc5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ import ( | |
| "slices" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "text/template" | ||
| "time" | ||
|
|
||
|
|
@@ -69,7 +70,6 @@ const ( | |
|
|
||
| var ( | ||
| startCommandTimeout = 1 * time.Minute | ||
| storagePoolDriver = "zfs" | ||
|
|
||
| workshopFormatsChecked = false | ||
| ) | ||
|
|
@@ -78,10 +78,6 @@ var ( | |
| var startCommand string | ||
|
|
||
| func init() { | ||
| if osutil.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 | ||
|
|
@@ -164,19 +160,77 @@ 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, "zfs") { | ||
| return "zfs" | ||
| } | ||
| return "btrfs" | ||
| } | ||
|
|
||
| // poolDriverCache stores the driver of the workshop storage pool once known. | ||
| // ensureBackendReady records it, and pool operations reuse it instead of | ||
| // 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 | ||
| value string | ||
| } | ||
|
|
||
| func setPoolDriver(driver string) { | ||
| poolDriverCache.Lock() | ||
| poolDriverCache.value = driver | ||
| poolDriverCache.Unlock() | ||
| } | ||
|
|
||
| func getPoolDriver() string { | ||
| poolDriverCache.RLock() | ||
| defer poolDriverCache.RUnlock() | ||
| return poolDriverCache.value | ||
| } | ||
|
|
||
| // poolUsesZFS reports whether the workshop storage pool is backed by ZFS. It | ||
| // reuses the driver recorded by ensureBackendReady, falling back to querying | ||
| // LXD if it hasn't been recorded yet. | ||
| func poolUsesZFS(conn lxd.InstanceServer) (bool, error) { | ||
| driver := getPoolDriver() | ||
| if driver == "" { | ||
| pool, _, err := conn.GetStoragePool(storagePool) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| driver = pool.Driver | ||
| setPoolDriver(driver) | ||
| } | ||
| if slices.ContainsFunc(drivers, hasDriver) { | ||
| return driver == "zfs", 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. A missing pool is not an error here: ensureBackendReady creates it. | ||
| func checkStoragePool(conn lxd.InstanceServer) error { | ||
| _, _, err := conn.GetStoragePool(storagePool) | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| 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
+229
to
+230
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the error returned from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Perhaps the explanation should be less verbose? Since the error is already quite informative.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The error message is useful though, does it appear in the LXD logs? Maybe we can direct users there
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good point, I'll keep my explanation rather than trimming it in favor of LXD's message.
On using So On the logs: LXD only logs the "driver not available" line at A couple of options that I could think of:
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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should definitely keep branching on 404. I would do the following:
|
||
| } | ||
|
|
||
| // 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) | ||
| return nil | ||
| } | ||
|
|
||
| // checkStorageSpace puts the daemon into degraded mode when the workshop | ||
|
|
@@ -238,7 +292,7 @@ func checkServerCapabilities() error { | |
| return err | ||
| } | ||
|
|
||
| return checkStorageDriver(info.Environment.StorageSupportedDrivers) | ||
| return checkStoragePool(conn) | ||
| } | ||
|
|
||
| func checkWorkshopFormats() error { | ||
|
|
@@ -296,15 +350,20 @@ func ensureBackendReady() error { | |
| } | ||
| defer conn.Disconnect() | ||
|
|
||
| // Create LXD storage pool if it doesn't exist. | ||
| pools, err := conn.GetStoragePools() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if idx := slices.IndexFunc(pools, func(p api.StoragePool) bool { return p.Name == storagePool }); idx < 0 { | ||
| // Create the LXD storage pool if it doesn't exist, and record its driver so | ||
| // pool operations can reuse it. A missing pool returns a 404; any other | ||
| // error (e.g. a ZFS pool whose module is gone) is left for checkStoragePool | ||
| // to report as a degraded state. | ||
| existingPool, _, err := conn.GetStoragePool(storagePool) | ||
| if api.StatusErrorCheck(err, http.StatusNotFound) { | ||
| info, _, err := conn.GetServer() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| driver := preferredDriver(info.Environment.StorageSupportedDrivers) | ||
| req := api.StoragePoolsPost{ | ||
| Name: storagePool, | ||
| Driver: storagePoolDriver, | ||
| Driver: driver, | ||
| } | ||
| op, err := conn.CreateStoragePool(req) | ||
| if err != nil { | ||
|
|
@@ -313,6 +372,7 @@ func ensureBackendReady() error { | |
| if err := op.Wait(); err != nil { | ||
| return err | ||
| } | ||
| setPoolDriver(driver) | ||
|
|
||
| // Ensure the new pool has enough total space available. | ||
| pool, etag, err := conn.GetStoragePool(storagePool) | ||
|
|
@@ -343,8 +403,10 @@ 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) | ||
| } else if err != nil { | ||
| return err | ||
| } else { | ||
| setPoolDriver(existingPool.Driver) | ||
| } | ||
|
|
||
| network, etag, err := conn.GetNetwork(networkName) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this change cbe0b61 |
||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := mergeDevices(inst.Devices, snapshot.Sdks, name, usesZFS); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -598,12 +602,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) | ||
|
|
@@ -734,7 +738,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 { | ||
|
|
||
There was a problem hiding this comment.
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?