Skip to content
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

Storage: Avoid filling volume config on VolumeDBCreate #14923

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
114c56a
lxd/storage/drivers/btrfs: Remove unused parameters
hamistao Jan 16, 2025
90e46ac
lxd/storage/drivers/common: Remove stale return value from `[F|f]illV…
hamistao Feb 4, 2025
07db4a6
lxd/storage/drivers: Remove stale return value from `FillVolumeConfig`
hamistao Feb 4, 2025
1b1caed
lxd/storage/backend_lxd: Update `FillVolumeConfig` usage
hamistao Feb 4, 2025
97b9d2a
lxd/storage/utils: Update `FillVolumeConfig` usage
hamistao Feb 4, 2025
47d2d13
lxd/storage/drivers/ceph: Remove stale return value from `FillVolumeC…
hamistao Feb 4, 2025
251a065
lxd/storage/drivers/dir: Remove stale return value from `FillVolumeCo…
hamistao Feb 4, 2025
da25e20
lxd/storage/drivers/lvm: Remove stale return value from `FillVolumeCo…
hamistao Feb 4, 2025
256429a
lxd/storage/drivers/powerflex: Remove stale return value from `FillVo…
hamistao Feb 4, 2025
1c7d95c
lxd/storage/drivers/zfs: Remove stale return value from `FillVolumeCo…
hamistao Feb 4, 2025
c55a39f
lxd/storage/drivers/pure: Remove stale return value from `FillVolumeC…
hamistao Feb 5, 2025
97c038c
lxd/storage/utils: Don't fill config on `VolumeDBCreate`
hamistao Feb 4, 2025
4dbd651
lxd/storage/backend_lxd: Fill volume configuration on `GetNewVolume`
hamistao Feb 4, 2025
0e0ccc9
lxd/storage/utils: Simplify `VolumeDBCreate`'s parameters
hamistao Feb 24, 2025
3806675
lxd/storage/backend_lxd: Update `VolumeDBCreate` usage
hamistao Feb 24, 2025
3510e02
lxd/storage/backend_lxd_patches: Update `VolumeDBCreate` usage
hamistao Feb 25, 2025
4e44263
lxd/storage: Add needed `FillVolumeConfig` calls
hamistao Feb 24, 2025
147ac84
lxd/storage: Drop `FillVolumeConfig` calls
hamistao Feb 24, 2025
72f39e7
lxd/storage/backend_lxd: Update comments
hamistao Feb 5, 2025
4875aca
lxd/storage/drivers/ceph: Pre-allocate `ret` slice (prealloc)
hamistao Feb 5, 2025
9871358
lxd/storage/drivers/ceph: Avoid deprecated `RunCommand` (staticcheck)
hamistao Feb 5, 2025
59bcb5f
lxd/storage/drivers/common: Avoid deprecated `RunCommand` (staticcheck)
hamistao Feb 5, 2025
d8df58a
WIP
hamistao Feb 5, 2025
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
114 changes: 50 additions & 64 deletions lxd/storage/backend_lxd.go

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion lxd/storage/backend_lxd_patches.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ func patchMissingSnapshotRecords(b *lxdBackend) error {

if !foundVolumeSnapshot {
b.logger.Info("Creating missing volume snapshot record", logger.Ctx{"project": snapshots[i].Project, "instance": snapshots[i].Name})
err = VolumeDBCreate(b, snapshots[i].Project, snapshots[i].Name, "Auto repaired", volType, true, dbVol.Config, snapshots[i].CreationDate, time.Time{}, contentType, false, true)
sanpVol := b.GetNewVolume(volType, contentType, snapshots[i].Name, dbVol.Config)
err = VolumeDBCreate(b, snapshots[i].Project, snapshots[i].Name, sanpVol, "Auto repaired", true, snapshots[i].CreationDate, time.Time{}, false, true)
if err != nil {
return err
}
Expand Down
16 changes: 8 additions & 8 deletions lxd/storage/drivers/driver_btrfs_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ func (d *btrfs) CreateVolumeFromBackup(vol VolumeCopy, srcBackup backup.Info, sr

// createVolumeFromCopy creates a volume from copy by snapshotting the parent volume.
// It also copies the source volume's snapshots and supports refreshing an already existing volume.
func (d *btrfs) createVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, allowInconsistent bool, refresh bool, op *operations.Operation) error {
func (d *btrfs) createVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, refresh bool, op *operations.Operation) error {
revert := revert.New()
defer revert.Fail()

Expand Down Expand Up @@ -540,7 +540,7 @@ func (d *btrfs) createVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, allowInc

// CreateVolumeFromCopy provides same-pool volume copying functionality.
func (d *btrfs) CreateVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, allowInconsistent bool, op *operations.Operation) error {
return d.createVolumeFromCopy(vol, srcVol, allowInconsistent, false, op)
return d.createVolumeFromCopy(vol, srcVol, false, op)
}

// CreateVolumeFromMigration creates a volume being sent via a migration.
Expand Down Expand Up @@ -589,7 +589,7 @@ func (d *btrfs) CreateVolumeFromMigration(vol VolumeCopy, conn io.ReadWriteClose
}

if volTargetArgs.Refresh && shared.ValueInSlice(migration.BTRFSFeatureSubvolumeUUIDs, volTargetArgs.MigrationType.Features) {
snapshots, err := d.volumeSnapshotsSorted(vol.Volume, op)
snapshots, err := d.volumeSnapshotsSorted(vol.Volume)
if err != nil {
return err
}
Expand Down Expand Up @@ -648,10 +648,10 @@ func (d *btrfs) CreateVolumeFromMigration(vol VolumeCopy, conn io.ReadWriteClose
syncSubvolumes = migrationHeader.Subvolumes
}

return d.createVolumeFromMigrationOptimized(vol.Volume, conn, volTargetArgs, preFiller, syncSubvolumes, op)
return d.createVolumeFromMigrationOptimized(vol.Volume, conn, volTargetArgs, syncSubvolumes, op)
}

func (d *btrfs) createVolumeFromMigrationOptimized(vol Volume, conn io.ReadWriteCloser, volTargetArgs migration.VolumeTargetArgs, preFiller *VolumeFiller, subvolumes []BTRFSSubVolume, op *operations.Operation) error {
func (d *btrfs) createVolumeFromMigrationOptimized(vol Volume, conn io.ReadWriteCloser, volTargetArgs migration.VolumeTargetArgs, subvolumes []BTRFSSubVolume, op *operations.Operation) error {
revert := revert.New()
defer revert.Fail()

Expand Down Expand Up @@ -826,7 +826,7 @@ func (d *btrfs) createVolumeFromMigrationOptimized(vol Volume, conn io.ReadWrite

// RefreshVolume provides same-pool volume and specific snapshots syncing functionality.
func (d *btrfs) RefreshVolume(vol VolumeCopy, srcVol VolumeCopy, refreshSnapshots []string, allowInconsistent bool, op *operations.Operation) error {
return d.createVolumeFromCopy(vol, srcVol, allowInconsistent, true, op)
return d.createVolumeFromCopy(vol, srcVol, true, op)
}

// DeleteVolume deletes a volume of the storage device. If any snapshots of the volume remain then
Expand Down Expand Up @@ -1190,7 +1190,7 @@ func (d *btrfs) MigrateVolume(vol VolumeCopy, conn io.ReadWriteCloser, volSrcArg

if !volSrcArgs.VolumeOnly {
// Generate restoration header, containing info on the subvolumes and how they should be restored.
snapshots, err = d.volumeSnapshotsSorted(vol.Volume, op)
snapshots, err = d.volumeSnapshotsSorted(vol.Volume)
if err != nil {
return err
}
Expand Down Expand Up @@ -1785,7 +1785,7 @@ func (d *btrfs) VolumeSnapshots(vol Volume, op *operations.Operation) ([]string,

// volumeSnapshotsSorted returns a list of snapshots for the volume (ordered by subvolume ID).
// Since the subvolume ID is incremental, this also represents the order of creation.
func (d *btrfs) volumeSnapshotsSorted(vol Volume, op *operations.Operation) ([]string, error) {
func (d *btrfs) volumeSnapshotsSorted(vol Volume) ([]string, error) {
stdout := bytes.Buffer{}

err := shared.RunCommandWithFds(d.state.ShutdownCtx, nil, &stdout, "btrfs", "subvolume", "list", GetPoolMountPath(vol.pool))
Expand Down
26 changes: 13 additions & 13 deletions lxd/storage/drivers/driver_ceph_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ import (
func (d *ceph) CreateVolume(vol Volume, filler *VolumeFiller, op *operations.Operation) error {
// Function to rename an RBD volume.
renameVolume := func(oldName string, newName string) error {
_, err := shared.RunCommand(
_, err := shared.RunCommandContext(
context.Background(),
"rbd",
"--id", d.config["ceph.user.name"],
"--cluster", d.config["ceph.cluster_name"],
Expand Down Expand Up @@ -390,7 +391,8 @@ func (d *ceph) CreateVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, allowInco
if len(vol.Snapshots) == 0 || len(snapshots) == 0 {
// If lightweight clone mode isn't enabled, perform a full copy of the volume.
if shared.IsFalse(d.config["ceph.rbd.clone_copy"]) {
_, err = shared.RunCommand(
_, err = shared.RunCommandContext(
context.Background(),
"rbd",
"--id", d.config["ceph.user.name"],
"--cluster", d.config["ceph.cluster_name"],
Expand Down Expand Up @@ -1022,7 +1024,8 @@ func (d *ceph) DeleteVolume(vol Volume, op *operations.Operation) error {
}

// Delete snapshots.
_, err := shared.RunCommand(
_, err := shared.RunCommandContext(
context.Background(),
"rbd",
"--id", d.config["ceph.user.name"],
"--cluster", d.config["ceph.cluster_name"],
Expand Down Expand Up @@ -1116,14 +1119,11 @@ func (d *ceph) HasVolume(vol Volume) (bool, error) {
}

// FillVolumeConfig populate volume with default config.
func (d *ceph) FillVolumeConfig(vol Volume) error {
func (d *ceph) FillVolumeConfig(vol Volume) {
// Copy volume.* configuration options from pool.
// Exclude 'block.filesystem' and 'block.mount_options'
// as this ones are handled below in this function and depends from volume type
err := d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")
if err != nil {
return err
}
d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")

// Only validate filesystem config keys for filesystem volumes or VM block volumes (which have an
// associated filesystem volume).
Expand Down Expand Up @@ -1155,8 +1155,6 @@ func (d *ceph) FillVolumeConfig(vol Volume) error {
vol.config["block.mount_options"] = "discard"
}
}

return nil
}

// commonVolumeRules returns validation rules which are common for pool and volume.
Expand Down Expand Up @@ -1926,7 +1924,8 @@ func (d *ceph) CreateVolumeSnapshot(snapVol Volume, op *operations.Operation) er
// DeleteVolumeSnapshot removes a snapshot from the storage device.
func (d *ceph) DeleteVolumeSnapshot(snapVol Volume, op *operations.Operation) error {
// Check if snapshot exists, and return if not.
_, err := shared.RunCommand(
_, err := shared.RunCommandContext(
context.Background(),
"rbd",
"--id", d.config["ceph.user.name"],
"--cluster", d.config["ceph.cluster_name"],
Expand Down Expand Up @@ -2167,7 +2166,7 @@ func (d *ceph) VolumeSnapshots(vol Volume, op *operations.Operation) ([]string,
return nil, err
}

var ret []string
ret := make([]string, 0, len(snapshots))

for _, snap := range snapshots {
// Ignore zombie snapshots as these are only used internally and
Expand Down Expand Up @@ -2196,7 +2195,8 @@ func (d *ceph) restoreVolume(vol Volume, snapVol Volume, op *operations.Operatio

_, snapshotName, _ := api.GetParentAndSnapshotName(snapVol.name)

_, err = shared.RunCommand(
_, err = shared.RunCommandContext(
context.Background(),
"rbd",
"--id", d.config["ceph.user.name"],
"--cluster", d.config["ceph.cluster_name"],
Expand Down
15 changes: 7 additions & 8 deletions lxd/storage/drivers/driver_common.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package drivers

import (
"context"
"fmt"
"io"
"net/url"
Expand Down Expand Up @@ -105,7 +106,7 @@ func (d *common) validatePool(config map[string]string, driverRules map[string]f
// excludeKeys allow exclude some keys from copying to volume config.
// Sometimes that can be useful when copying is dependant from specific conditions
// and shouldn't be done in generic way.
func (d *common) fillVolumeConfig(vol *Volume, excludedKeys ...string) error {
func (d *common) fillVolumeConfig(vol *Volume, excludedKeys ...string) {
for k := range d.config {
if !strings.HasPrefix(k, "volume.") {
continue
Expand Down Expand Up @@ -144,13 +145,11 @@ func (d *common) fillVolumeConfig(vol *Volume, excludedKeys ...string) error {
vol.config[volKey] = d.config[k]
}
}

return nil
}

// FillVolumeConfig populate volume with default config.
func (d *common) FillVolumeConfig(vol Volume) error {
return d.fillVolumeConfig(&vol)
func (d *common) FillVolumeConfig(vol Volume) {
d.fillVolumeConfig(&vol)
}

// validateVolume validates a volume config against common rules and optional driver specific rules.
Expand Down Expand Up @@ -291,7 +290,7 @@ func (d *common) moveGPTAltHeader(devPath string) error {
return nil
}

_, err = shared.RunCommand(path, "--move-second-header", devPath)
_, err = shared.RunCommandContext(context.Background(), path, "--move-second-header", devPath)
if err == nil {
d.logger.Debug("Moved GPT alternative header to end of disk", logger.Ctx{"dev": devPath})
return nil
Expand Down Expand Up @@ -584,15 +583,15 @@ func (d *common) filesystemFreeze(path string) (func() error, error) {
return nil, fmt.Errorf("Failed syncing filesystem %q: %w", path, err)
}

_, err = shared.RunCommand("fsfreeze", "--freeze", path)
_, err = shared.RunCommandContext(context.Background(), "fsfreeze", "--freeze", path)
if err != nil {
return nil, fmt.Errorf("Failed freezing filesystem %q: %w", path, err)
}

d.logger.Info("Filesystem frozen", logger.Ctx{"path": path})

unfreezeFS := func() error {
_, err := shared.RunCommand("fsfreeze", "--unfreeze", path)
_, err := shared.RunCommandContext(context.Background(), "fsfreeze", "--unfreeze", path)
if err != nil {
return fmt.Errorf("Failed unfreezing filesystem %q: %w", path, err)
}
Expand Down
9 changes: 2 additions & 7 deletions lxd/storage/drivers/driver_dir_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,21 +226,16 @@ func (d *dir) HasVolume(vol Volume) (bool, error) {
}

// FillVolumeConfig populate volume with default config.
func (d *dir) FillVolumeConfig(vol Volume) error {
func (d *dir) FillVolumeConfig(vol Volume) {
initialSize := vol.config["size"]

err := d.fillVolumeConfig(&vol)
if err != nil {
return err
}
d.fillVolumeConfig(&vol)

// Buckets do not support default volume size.
// If size is specified manually, do not remove, so it triggers validation failure and an error to user.
if vol.volType == VolumeTypeBucket && initialSize == "" {
delete(vol.config, "size")
}

return nil
}

// ValidateVolume validates the supplied volume config. Optionally removes invalid keys from the volume's config.
Expand Down
9 changes: 2 additions & 7 deletions lxd/storage/drivers/driver_lvm_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -265,14 +265,11 @@ func (d *lvm) HasVolume(vol Volume) (bool, error) {
}

// FillVolumeConfig populate volume with default config.
func (d *lvm) FillVolumeConfig(vol Volume) error {
func (d *lvm) FillVolumeConfig(vol Volume) {
// Copy volume.* configuration options from pool.
// Exclude "block.filesystem" and "block.mount_options" as they depend on volume type (handled below).
// Exclude "lvm.stripes", "lvm.stripes.size" as they only work on non-thin storage pools (handled below).
err := d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options", "lvm.stripes", "lvm.stripes.size")
if err != nil {
return err
}
d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options", "lvm.stripes", "lvm.stripes.size")

// Only validate filesystem config keys for filesystem volumes or VM block volumes (which have an
// associated filesystem volume).
Expand Down Expand Up @@ -315,8 +312,6 @@ func (d *lvm) FillVolumeConfig(vol Volume) error {
vol.config["lvm.stripes.size"] = d.config["lvm.stripes.size"]
}
}

return nil
}

// commonVolumeRules returns validation rules which are common for pool and volume.
Expand Down
9 changes: 2 additions & 7 deletions lxd/storage/drivers/driver_powerflex_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,14 +376,11 @@ func (d *powerflex) HasVolume(vol Volume) (bool, error) {
}

// FillVolumeConfig populate volume with default config.
func (d *powerflex) FillVolumeConfig(vol Volume) error {
func (d *powerflex) FillVolumeConfig(vol Volume) {
// Copy volume.* configuration options from pool.
// Exclude 'block.filesystem' and 'block.mount_options'
// as these ones are handled below in this function and depend on the volume's type.
err := d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")
if err != nil {
return err
}
d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")

// Only validate filesystem config keys for filesystem volumes or VM block volumes (which have an
// associated filesystem volume).
Expand Down Expand Up @@ -415,8 +412,6 @@ func (d *powerflex) FillVolumeConfig(vol Volume) error {
vol.config["block.mount_options"] = "discard"
}
}

return nil
}

// commonVolumeRules returns validation rules which are common for pool and volume.
Expand Down
9 changes: 2 additions & 7 deletions lxd/storage/drivers/driver_pure_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -671,14 +671,11 @@ func (d *pure) HasVolume(vol Volume) (bool, error) {
}

// FillVolumeConfig populate volume with default config.
func (d *pure) FillVolumeConfig(vol Volume) error {
func (d *pure) FillVolumeConfig(vol Volume) {
// Copy volume.* configuration options from pool.
// Exclude 'block.filesystem' and 'block.mount_options'
// as these ones are handled below in this function and depend on the volume's type.
err := d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")
if err != nil {
return err
}
d.fillVolumeConfig(&vol, "block.filesystem", "block.mount_options")

// Only validate filesystem config keys for filesystem volumes or VM block volumes (which have an
// associated filesystem volume).
Expand Down Expand Up @@ -710,8 +707,6 @@ func (d *pure) FillVolumeConfig(vol Volume) error {
vol.config["block.mount_options"] = "discard"
}
}

return nil
}

// ValidateVolume validates the supplied volume config.
Expand Down
9 changes: 2 additions & 7 deletions lxd/storage/drivers/driver_zfs_volumes.go
Original file line number Diff line number Diff line change
Expand Up @@ -3508,7 +3508,7 @@ func (d *zfs) RenameVolumeSnapshot(vol Volume, newSnapshotName string, op *opera
}

// FillVolumeConfig populate volume with default config.
func (d *zfs) FillVolumeConfig(vol Volume) error {
func (d *zfs) FillVolumeConfig(vol Volume) {
var excludedKeys []string

// Copy volume.* configuration options from pool.
Expand All @@ -3519,10 +3519,7 @@ func (d *zfs) FillVolumeConfig(vol Volume) error {
excludedKeys = []string{"block.filesystem", "block.mount_options"}
}

err := d.fillVolumeConfig(&vol, excludedKeys...)
if err != nil {
return err
}
d.fillVolumeConfig(&vol, excludedKeys...)

// Only validate filesystem config keys for filesystem volumes.
if d.isBlockBacked(vol) && vol.ContentType() == ContentTypeFS {
Expand Down Expand Up @@ -3553,8 +3550,6 @@ func (d *zfs) FillVolumeConfig(vol Volume) error {
vol.config["block.mount_options"] = "discard"
}
}

return nil
}

func (d *zfs) isBlockBacked(vol Volume) bool {
Expand Down
2 changes: 1 addition & 1 deletion lxd/storage/drivers/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ type Driver interface {
DeleteBucketKey(bucket Volume, keyName string, op *operations.Operation) error

// Volumes.
FillVolumeConfig(vol Volume) error
FillVolumeConfig(vol Volume)
ValidateVolume(vol Volume, removeUnknownKeys bool) error
CreateVolume(vol Volume, filler *VolumeFiller, op *operations.Operation) error
CreateVolumeFromCopy(vol VolumeCopy, srcVol VolumeCopy, allowInconsistent bool, op *operations.Operation) error
Expand Down
Loading
Loading