[lxc-devel] [lxd/master] storage: rework storage pool updating
brauner on Github
lxc-bot at linuxcontainers.org
Fri Sep 22 10:19:13 UTC 2017
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 398 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170922/b2952758/attachment.bin>
-------------- next part --------------
From e57a3041cd3711887c9484930006116f8b6676e6 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Fri, 22 Sep 2017 12:12:44 +0200
Subject: [PATCH 1/3] storage: rework storage pool updating
Closes #3834.
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
lxd/storage_btrfs.go | 21 +++++++++++++----
lxd/storage_ceph.go | 22 ++++++++++++++++-
lxd/storage_dir.go | 19 ++++++++++++---
lxd/storage_lvm.go | 57 +++++++++++++++++++--------------------------
lxd/storage_pools_config.go | 32 +++++++++++++++++++++++++
lxd/storage_zfs.go | 42 ++++++++++-----------------------
6 files changed, 122 insertions(+), 71 deletions(-)
diff --git a/lxd/storage_btrfs.go b/lxd/storage_btrfs.go
index b78b415ad..410cd01ab 100644
--- a/lxd/storage_btrfs.go
+++ b/lxd/storage_btrfs.go
@@ -466,10 +466,23 @@ func (s *storageBtrfs) StoragePoolUmount() (bool, error) {
return true, nil
}
-func (s *storageBtrfs) StoragePoolUpdate(writable *api.StoragePoolPut, changedConfig []string) error {
- logger.Infof("Updating BTRFS storage pool \"%s\".", s.pool.Name)
+func (s *storageBtrfs) StoragePoolUpdate(writable *api.StoragePoolPut,
+ changedConfig []string) error {
+ logger.Infof(`Updating BTRFS storage pool "%s"`, s.pool.Name)
- // rsync.bwlimit does not require any on-disk changes
+ changeable := changeableStoragePoolProperties["btrfs"]
+ unchangeable := []string{}
+ for _, change := range changedConfig {
+ if !shared.StringInSlice(change, changeable) {
+ unchangeable = append(unchangeable, change)
+ }
+ }
+
+ if len(unchangeable) > 0 {
+ return updateError(unchangeable, "btrfs")
+ }
+
+ // "rsync.bwlimit" requires no on-disk modifications.
if shared.StringInSlice("btrfs.mount_options", changedConfig) {
s.setBtrfsMountOptions(writable.Config["btrfs.mount_options"])
@@ -480,7 +493,7 @@ func (s *storageBtrfs) StoragePoolUpdate(writable *api.StoragePoolPut, changedCo
}
}
- logger.Infof("Updated BTRFS storage pool \"%s\".", s.pool.Name)
+ logger.Infof(`Updated BTRFS storage pool "%s"`, s.pool.Name)
return nil
}
diff --git a/lxd/storage_ceph.go b/lxd/storage_ceph.go
index c73a4b4d8..4aedab882 100644
--- a/lxd/storage_ceph.go
+++ b/lxd/storage_ceph.go
@@ -695,7 +695,27 @@ func (s *storageCeph) StoragePoolVolumeUpdate(writable *api.StorageVolumePut, ch
}
func (s *storageCeph) StoragePoolUpdate(writable *api.StoragePoolPut, changedConfig []string) error {
- return fmt.Errorf("ODS storage pool properties cannot be changed")
+ logger.Infof(`Updating CEPH storage pool "%s"`, s.pool.Name)
+
+ changeable := changeableStoragePoolProperties["ceph"]
+ unchangeable := []string{}
+ for _, change := range changedConfig {
+ if !shared.StringInSlice(change, changeable) {
+ unchangeable = append(unchangeable, change)
+ }
+ }
+
+ if len(unchangeable) > 0 {
+ return updateError(unchangeable, "ceph")
+ }
+
+ // "rsync.bwlimit" requires no on-disk modifications.
+ // "volume.block.filesystem" requires no on-disk modifications.
+ // "volume.block.mount_options" requires no on-disk modifications.
+ // "volume.size" requires no on-disk modifications.
+
+ logger.Infof(`Updated CEPH storage pool "%s"`, s.pool.Name)
+ return nil
}
func (s *storageCeph) ContainerStorageReady(name string) bool {
diff --git a/lxd/storage_dir.go b/lxd/storage_dir.go
index acdbca417..07e47e174 100644
--- a/lxd/storage_dir.go
+++ b/lxd/storage_dir.go
@@ -287,11 +287,24 @@ func (s *storageDir) GetContainerPoolInfo() (int64, string) {
}
func (s *storageDir) StoragePoolUpdate(writable *api.StoragePoolPut, changedConfig []string) error {
- if shared.StringInSlice("rsync.bwlimit", changedConfig) {
- return nil
+ logger.Infof(`Updating DIR storage pool "%s"`, s.pool.Name)
+
+ changeable := changeableStoragePoolProperties["dir"]
+ unchangeable := []string{}
+ for _, change := range changedConfig {
+ if !shared.StringInSlice(change, changeable) {
+ unchangeable = append(unchangeable, change)
+ }
}
- return fmt.Errorf("storage property cannot be changed")
+ if len(unchangeable) > 0 {
+ return updateError(unchangeable, "dir")
+ }
+
+ // "rsync.bwlimit" requires no on-disk modifications.
+
+ logger.Infof(`Updated DIR storage pool "%s"`, s.pool.Name)
+ return nil
}
// Functions dealing with storage pools.
diff --git a/lxd/storage_lvm.go b/lxd/storage_lvm.go
index ece80e331..139f4b25f 100644
--- a/lxd/storage_lvm.go
+++ b/lxd/storage_lvm.go
@@ -673,26 +673,18 @@ func (s *storageLvm) GetContainerPoolInfo() (int64, string) {
}
func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConfig []string) error {
- logger.Infof("Updating LVM storage pool \"%s\".", s.pool.Name)
+ logger.Infof(`Updating LVM storage pool "%s"`, s.pool.Name)
- if shared.StringInSlice("size", changedConfig) {
- return fmt.Errorf("the \"size\" property cannot be changed")
- }
-
- if shared.StringInSlice("source", changedConfig) {
- return fmt.Errorf("the \"source\" property cannot be changed")
- }
-
- if shared.StringInSlice("volume.zfs.use_refquota", changedConfig) {
- return fmt.Errorf("the \"volume.zfs.use_refquota\" property does not apply to LVM drivers")
- }
-
- if shared.StringInSlice("volume.zfs.remove_snapshots", changedConfig) {
- return fmt.Errorf("the \"volume.zfs.remove_snapshots\" property does not apply to LVM drivers")
+ changeable := changeableStoragePoolProperties["lvm"]
+ unchangeable := []string{}
+ for _, change := range changedConfig {
+ if !shared.StringInSlice(change, changeable) {
+ unchangeable = append(unchangeable, change)
+ }
}
- if shared.StringInSlice("zfs.pool_name", changedConfig) {
- return fmt.Errorf("the \"zfs.pool_name\" property does not apply to LVM drivers")
+ if len(unchangeable) > 0 {
+ return updateError(unchangeable, "lvm")
}
// "volume.block.mount_options" requires no on-disk modifications.
@@ -700,23 +692,20 @@ func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConf
// "volume.size" requires no on-disk modifications.
// "rsync.bwlimit" requires no on-disk modifications.
- // Given a set of changeable pool properties the change should be
- // "transactional": either the whole update succeeds or none. So try to
- // revert on error.
revert := true
- if shared.StringInSlice("lvm.use_thinpool", changedConfig) {
- return fmt.Errorf("the \"lvm.use_thinpool\" property cannot be changed")
- }
if shared.StringInSlice("lvm.thinpool_name", changedConfig) {
if !s.useThinpool {
- return fmt.Errorf("the LVM storage pool \"%s\" does not use thin pools. The \"lvm.thinpool_name\" porperty cannot be set", s.pool.Name)
+ return fmt.Errorf(`The LVM storage pool "%s" does `+
+ `not use thin pools. The "lvm.thinpool_name" `+
+ `property cannot be set`, s.pool.Name)
}
newThinpoolName := writable.Config["lvm.thinpool_name"]
// Paranoia check
if newThinpoolName == "" {
- return fmt.Errorf("could not rename volume group: No new name provided")
+ return fmt.Errorf(`Could not rename volume group: No ` +
+ `new name provided`)
}
poolName := s.getOnDiskPoolName()
@@ -737,10 +726,10 @@ func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConf
err = lvmLVRename(poolName, newThinpoolName, oldThinpoolName)
if err != nil {
- logger.Warnf("Failed to rename LVM thinpool from \"%s\" to \"%s\": %s. Manual intervention needed.",
- newThinpoolName,
- oldThinpoolName,
- err)
+ logger.Warnf(`Failed to rename LVM thinpool `+
+ `from "%s" to "%s": %s. Manual `+
+ `intervention needed`, newThinpoolName,
+ oldThinpoolName, err)
}
s.setLvmThinpoolName(oldThinpoolName)
}()
@@ -750,7 +739,8 @@ func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConf
newName := writable.Config["lvm.vg_name"]
// Paranoia check
if newName == "" {
- return fmt.Errorf("could not rename volume group: No new name provided")
+ return fmt.Errorf(`Could not rename volume group: No ` +
+ `new name provided`)
}
writable.Config["source"] = newName
@@ -771,8 +761,9 @@ func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConf
err := lvmVGRename(newName, oldPoolName)
if err != nil {
- logger.Warnf("Failed to rename LVM volume group from \"%s\" to \"%s\": %s. Manual intervention needed.",
- newName,
+ logger.Warnf(`Failed to rename LVM volume `+
+ `group from "%s" to "%s": %s. Manual `+
+ `intervention needed`, newName,
oldPoolName)
}
s.setOnDiskPoolName(oldPoolName)
@@ -782,7 +773,7 @@ func (s *storageLvm) StoragePoolUpdate(writable *api.StoragePoolPut, changedConf
// Update succeeded.
revert = false
- logger.Infof("Updated LVM storage pool \"%s\".", s.pool.Name)
+ logger.Infof(`Updated LVM storage pool "%s"`, s.pool.Name)
return nil
}
diff --git a/lxd/storage_pools_config.go b/lxd/storage_pools_config.go
index 293b2062a..e73fc1813 100644
--- a/lxd/storage_pools_config.go
+++ b/lxd/storage_pools_config.go
@@ -9,6 +9,38 @@ import (
"github.com/lxc/lxd/shared"
)
+func updateError(unchangeable []string, driverName string) error {
+ return fmt.Errorf(`The %v properties cannot be changed for "%s" `+
+ `storage pools`, unchangeable, driverName)
+}
+
+var changeableStoragePoolProperties = map[string][]string{
+ "btrfs": []string{
+ "rsync.bwlimit",
+ "btrfs.mount_options"},
+
+ "ceph": []string{
+ "volume.block.filesystem",
+ "volume.block.mount_options",
+ "volume.size"},
+
+ "dir": []string{
+ "rsync.bwlimit"},
+
+ "lvm": []string{
+ "lvm.thinpool_name",
+ "lvm.vg_name",
+ "volume.block.filesystem",
+ "volume.block.mount_options",
+ "volume.size"},
+
+ "zfs": []string{
+ "rsync_bwlimit",
+ "volume.zfs.remove_snapshots",
+ "volume.zfs.use_refquota",
+ },
+}
+
var storagePoolConfigKeys = map[string]func(value string) error{
// valid drivers: btrfs
// (Note, that we can't be smart in detecting mount options since a lot
diff --git a/lxd/storage_zfs.go b/lxd/storage_zfs.go
index a2f6445f3..280ad083f 100644
--- a/lxd/storage_zfs.go
+++ b/lxd/storage_zfs.go
@@ -537,43 +537,25 @@ func (s *storageZfs) GetContainerPoolInfo() (int64, string) {
}
func (s *storageZfs) StoragePoolUpdate(writable *api.StoragePoolPut, changedConfig []string) error {
- logger.Infof("Updating ZFS storage pool \"%s\".", s.pool.Name)
+ logger.Infof(`Updating ZFS storage pool "%s"`, s.pool.Name)
- if shared.StringInSlice("size", changedConfig) {
- return fmt.Errorf("the \"size\" property cannot be changed")
- }
-
- if shared.StringInSlice("source", changedConfig) {
- return fmt.Errorf("the \"source\" property cannot be changed")
- }
-
- if shared.StringInSlice("volume.size", changedConfig) {
- return fmt.Errorf("the \"volume.size\" property cannot be changed")
- }
-
- if shared.StringInSlice("volume.block.mount_options", changedConfig) {
- return fmt.Errorf("the \"volume.block.mount_options\" property cannot be changed")
- }
-
- if shared.StringInSlice("volume.block.filesystem", changedConfig) {
- return fmt.Errorf("the \"volume.block.filesystem\" property cannot be changed")
- }
-
- if shared.StringInSlice("lvm.thinpool_name", changedConfig) {
- return fmt.Errorf("the \"lvm.thinpool_name\" property cannot be changed")
- }
-
- if shared.StringInSlice("lvm.vg_name", changedConfig) {
- return fmt.Errorf("the \"lvm.vg_name\" property cannot be changed")
+ changeable := changeableStoragePoolProperties["zfs"]
+ unchangeable := []string{}
+ for _, change := range changedConfig {
+ if !shared.StringInSlice(change, changeable) {
+ unchangeable = append(unchangeable, change)
+ }
}
- if shared.StringInSlice("zfs.pool_name", changedConfig) {
- return fmt.Errorf("the \"zfs.pool_name\" property cannot be changed")
+ if len(unchangeable) > 0 {
+ return updateError(unchangeable, "zfs")
}
// "rsync.bwlimit" requires no on-disk modifications.
+ // "volume.zfs.remove_snapshots" requires no on-disk modifications.
+ // "volume.zfs.use_refquota" requires no on-disk modifications.
- logger.Infof("Updated ZFS storage pool \"%s\".", s.pool.Name)
+ logger.Infof(`Updated ZFS storage pool "%s"`, s.pool.Name)
return nil
}
From c5ad529a003af98ee00d84e84632c5db45bc3d85 Mon Sep 17 00:00:00 2001
From: Alberto Donato <alberto.donato at canonical.com>
Date: Wed, 20 Sep 2017 13:33:10 +0200
Subject: [PATCH 2/3] storage/lvm: re-import image on thinpool-based pools if
volume filesystem has changed
Signed-off-by: Alberto Donato <alberto.donato at canonical.com>
---
lxd/storage_lvm_utils.go | 15 +++++++++++++++
test/suites/storage.sh | 5 +++++
2 files changed, 20 insertions(+)
diff --git a/lxd/storage_lvm_utils.go b/lxd/storage_lvm_utils.go
index 1383e0c6e..e86f9b88c 100644
--- a/lxd/storage_lvm_utils.go
+++ b/lxd/storage_lvm_utils.go
@@ -538,6 +538,21 @@ func (s *storageLvm) containerCreateFromImageThinLv(c container, fp string) erro
var imgerr error
ok, _ := storageLVExists(imageLvmDevPath)
+ if ok {
+ _, volume, err := db.StoragePoolVolumeGetType(s.s.DB, fp, db.StoragePoolVolumeTypeImage, s.poolID)
+ if err != nil {
+ return err
+ }
+ if volume.Config["block.filesystem"] != s.getLvmFilesystem() {
+ // The storage pool volume.blockfilesystem property has changed, re-import the image
+ err := s.ImageDelete(fp)
+ if err != nil {
+ return err
+ }
+ ok = false
+ }
+ }
+
if !ok {
imgerr = s.ImageCreate(fp)
}
diff --git a/test/suites/storage.sh b/test/suites/storage.sh
index 4342d83b0..d3cc21bb7 100644
--- a/test/suites/storage.sh
+++ b/test/suites/storage.sh
@@ -373,6 +373,10 @@ test_storage() {
lxc launch testimage c12pool15 -s "lxdtest-$(basename "${LXD_DIR}")-non-thinpool-pool15"
lxc list -c b c12pool15 | grep "lxdtest-$(basename "${LXD_DIR}")-non-thinpool-pool15"
+ # Test that changing block filesystem works
+ lxc storage set "lxdtest-$(basename "${LXD_DIR}")-pool6" volume.block.filesystem xfs
+ lxc init testimage c1pool6 -s "lxdtest-$(basename "${LXD_DIR}")-pool6"
+
lxc storage volume create "lxdtest-$(basename "${LXD_DIR}")-pool6" c10pool6
lxc storage volume attach "lxdtest-$(basename "${LXD_DIR}")-pool6" c10pool6 c10pool6 testDevice /opt
! lxc storage volume attach "lxdtest-$(basename "${LXD_DIR}")-pool6" c10pool6 c10pool6 testDevice2 /opt
@@ -562,6 +566,7 @@ test_storage() {
lxc storage volume delete "lxdtest-$(basename "${LXD_DIR}")-pool5" c11pool5
if [ "$lxd_backend" = "lvm" ]; then
+ lxc delete -f c1pool6
lxc delete -f c10pool6
lxc delete -f c12pool6
From 0d0ab0b0fb038a8b7edf4d113b7dcd6e74182c7f Mon Sep 17 00:00:00 2001
From: Alberto Donato <alberto.donato at canonical.com>
Date: Wed, 20 Sep 2017 13:41:06 +0200
Subject: [PATCH 3/3] storage/storage: re-import image if volume filesystem has
changed
Signed-off-by: Alberto Donato <alberto.donato at canonical.com>
---
lxd/storage_ceph.go | 21 +++++++++++++++++++--
test/suites/storage_driver_ceph.sh | 4 ++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/lxd/storage_ceph.go b/lxd/storage_ceph.go
index 4aedab882..eaa32507a 100644
--- a/lxd/storage_ceph.go
+++ b/lxd/storage_ceph.go
@@ -898,8 +898,25 @@ func (s *storageCeph) ContainerCreateFromImage(container container, fingerprint
lxdStorageMapLock.Unlock()
var imgerr error
- if !cephRBDVolumeExists(s.ClusterName, s.OSDPoolName,
- fingerprint, storagePoolVolumeTypeNameImage, s.UserName) {
+ ok := cephRBDVolumeExists(s.ClusterName, s.OSDPoolName,
+ fingerprint, storagePoolVolumeTypeNameImage, s.UserName)
+
+ if ok {
+ _, volume, err := db.StoragePoolVolumeGetType(s.s.DB, fingerprint, db.StoragePoolVolumeTypeImage, s.poolID)
+ if err != nil {
+ return err
+ }
+ if volume.Config["block.filesystem"] != s.getRBDFilesystem() {
+ // The storage pool volume.blockfilesystem property has changed, re-import the image
+ err := s.ImageDelete(fingerprint)
+ if err != nil {
+ return err
+ }
+ ok = false
+ }
+ }
+
+ if !ok {
imgerr = s.ImageCreate(fingerprint)
}
diff --git a/test/suites/storage_driver_ceph.sh b/test/suites/storage_driver_ceph.sh
index f8a31b9c2..0fa97c08f 100644
--- a/test/suites/storage_driver_ceph.sh
+++ b/test/suites/storage_driver_ceph.sh
@@ -55,6 +55,9 @@ test_storage_driver_ceph() {
lxc launch testimage c4pool2 -s "lxdtest-$(basename "${LXD_DIR}")-pool2"
lxc list -c b c4pool2 | grep "lxdtest-$(basename "${LXD_DIR}")-pool2"
+ lxc storage set "lxdtest-$(basename "${LXD_DIR}")-pool1" volume.block.filesystem xfs
+ lxc init testimage c5pool1 -s "lxdtest-$(basename "${LXD_DIR}")-pool1"
+
# Test whether dependency tracking is working correctly. We should be able
# to create a container, copy it, which leads to a dependency relation
# between the source container's storage volume and the copied container's
@@ -107,6 +110,7 @@ test_storage_driver_ceph() {
lxc delete -f c1pool1
lxc delete -f c3pool1
+ lxc delete -f c5pool1
lxc delete -f c4pool2
lxc delete -f c2pool2
More information about the lxc-devel
mailing list