[lxc-devel] [lxd/master] make Storage{Start, Stop}() return bool and error

brauner on Github lxc-bot at linuxcontainers.org
Fri Mar 17 11:41:49 UTC 2017


A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 665 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20170317/176f2388/attachment.bin>
-------------- next part --------------
From fab708aa330672f2ddeb5665ecf040cf64adc170 Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Fri, 17 Mar 2017 12:38:42 +0100
Subject: [PATCH] make Storage{Start,Stop}() return bool and error

This way we can have Storage{Start,Stop}() indicate whether the operation that
is taking place has been initialized by us and if so we can make an informed
decision about stopping it or not. This should e.g. avoid unnecessary
{u,m}ounts and also should improve writing out the Backup.yaml file.

Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
 lxd/container.go          |  20 ++++++-
 lxd/container_lxc.go      | 139 ++++++++++++++++++++++++++++------------------
 lxd/container_snapshot.go |  13 +----
 lxd/migrate.go            |   6 +-
 lxd/storage.go            |   4 +-
 lxd/storage_btrfs.go      |  38 +++++++------
 lxd/storage_dir.go        |  18 ++++--
 lxd/storage_lvm.go        |  30 +++++-----
 lxd/storage_migration.go  |  14 +++--
 lxd/storage_mock.go       |   8 +--
 lxd/storage_zfs.go        |  18 +++---
 11 files changed, 184 insertions(+), 124 deletions(-)

diff --git a/lxd/container.go b/lxd/container.go
index ca4356d..230fe86 100644
--- a/lxd/container.go
+++ b/lxd/container.go
@@ -484,8 +484,8 @@ type container interface {
 
 	// FIXME: Those should be internal functions
 	// Needed for migration for now.
-	StorageStart() error
-	StorageStop() error
+	StorageStart() (bool, error)
+	StorageStop() (bool, error)
 	Storage() storage
 	IdmapSet() (*shared.IdmapSet, error)
 	LastIdmapSet() (*shared.IdmapSet, error)
@@ -646,6 +646,14 @@ func containerCreateAsSnapshot(d *Daemon, args containerArgs, sourceContainer co
 		return nil, err
 	}
 
+	ourStart, err := c.StorageStart()
+	if err != nil {
+		return nil, err
+	}
+	if ourStart {
+		defer c.StorageStop()
+	}
+
 	err = writeBackupFile(sourceContainer)
 	if err != nil {
 		c.Delete()
@@ -779,6 +787,14 @@ func containerConfigureInternal(c container) error {
 		}
 	}
 
+	ourStart, err := c.StorageStart()
+	if err != nil {
+		return err
+	}
+	if ourStart {
+		defer c.StorageStop()
+	}
+
 	err = writeBackupFile(c)
 	if err != nil {
 		return err
diff --git a/lxd/container_lxc.go b/lxd/container_lxc.go
index 4299395..1acd959 100644
--- a/lxd/container_lxc.go
+++ b/lxd/container_lxc.go
@@ -1553,7 +1553,7 @@ func (c *containerLXC) startCommon() (string, error) {
 	if !reflect.DeepEqual(idmap, lastIdmap) {
 		shared.LogDebugf("Container idmap changed, remapping")
 
-		err := c.StorageStart()
+		ourStart, err := c.StorageStart()
 		if err != nil {
 			return "", err
 		}
@@ -1561,7 +1561,9 @@ func (c *containerLXC) startCommon() (string, error) {
 		if lastIdmap != nil {
 			err = lastIdmap.UnshiftRootfs(c.RootfsPath())
 			if err != nil {
-				c.StorageStop()
+				if ourStart {
+					c.StorageStop()
+				}
 				return "", err
 			}
 		}
@@ -1569,7 +1571,9 @@ func (c *containerLXC) startCommon() (string, error) {
 		if idmap != nil {
 			err = idmap.ShiftRootfs(c.RootfsPath())
 			if err != nil {
-				c.StorageStop()
+				if ourStart {
+					c.StorageStop()
+				}
 				return "", err
 			}
 		}
@@ -1597,9 +1601,11 @@ func (c *containerLXC) startCommon() (string, error) {
 			return "", err
 		}
 
-		err = c.StorageStop()
-		if err != nil {
-			return "", err
+		if ourStart {
+			_, err = c.StorageStop()
+			if err != nil {
+				return "", err
+			}
 		}
 	}
 
@@ -1842,7 +1848,7 @@ func (c *containerLXC) startCommon() (string, error) {
 	}
 
 	// Storage is guaranteed to be mountable now.
-	err = c.StorageStart()
+	ourStart, err := c.StorageStart()
 	if err != nil {
 		return "", err
 	}
@@ -1858,11 +1864,13 @@ func (c *containerLXC) startCommon() (string, error) {
 	// Update the backup.yaml file
 	err = writeBackupFile(c)
 	if err != nil {
-		c.StorageStop()
+		if ourStart {
+			c.StorageStop()
+		}
 		return "", err
 	}
 
-	err = c.StorageStop()
+	_, err = c.StorageStop()
 	if err != nil {
 		return "", err
 	}
@@ -1898,7 +1906,7 @@ func (c *containerLXC) Start(stateful bool) error {
 	}
 
 	// Ensure that the container storage volume is mounted.
-	err = c.StorageStart()
+	_, err = c.StorageStart()
 	if err != nil {
 		return err
 	}
@@ -2008,7 +2016,7 @@ func (c *containerLXC) OnStart() error {
 	c.fromHook = true
 
 	// Start the storage for this container
-	err := c.StorageStart()
+	ourStart, err := c.StorageStart()
 	if err != nil {
 		return err
 	}
@@ -2016,7 +2024,9 @@ func (c *containerLXC) OnStart() error {
 	// Load the container AppArmor profile
 	err = AALoadProfile(c)
 	if err != nil {
-		c.StorageStop()
+		if ourStart {
+			c.StorageStop()
+		}
 		return err
 	}
 
@@ -2027,7 +2037,9 @@ func (c *containerLXC) OnStart() error {
 		err = c.templateApplyNow(c.localConfig[key])
 		if err != nil {
 			AADestroy(c)
-			c.StorageStop()
+			if ourStart {
+				c.StorageStop()
+			}
 			return err
 		}
 
@@ -2035,7 +2047,9 @@ func (c *containerLXC) OnStart() error {
 		err := dbContainerConfigRemove(c.daemon.db, c.id, key)
 		if err != nil {
 			AADestroy(c)
-			c.StorageStop()
+			if ourStart {
+				c.StorageStop()
+			}
 			return err
 		}
 	}
@@ -2043,7 +2057,9 @@ func (c *containerLXC) OnStart() error {
 	err = c.templateApplyNow("start")
 	if err != nil {
 		AADestroy(c)
-		c.StorageStop()
+		if ourStart {
+			c.StorageStop()
+		}
 		return err
 	}
 
@@ -2250,7 +2266,7 @@ func (c *containerLXC) OnStop(target string) error {
 	c.fromHook = true
 
 	// Stop the storage for this container
-	err := c.StorageStop()
+	_, err := c.StorageStop()
 	if err != nil {
 		if op != nil {
 			op.Done(err)
@@ -3803,8 +3819,11 @@ func (c *containerLXC) Update(args containerArgs, userRequested bool) error {
 	 * yet before container creation; this is okay, because at the end of
 	 * container creation we write the backup file, so let's not worry about
 	 * ENOENT. */
-	if err := writeBackupFile(c); err != nil && !os.IsNotExist(err) {
-		return err
+	if c.storage.ContainerStorageReady(c.Name()) {
+		err := writeBackupFile(c)
+		if err != nil && !os.IsNotExist(err) {
+			return err
+		}
 	}
 
 	// Update network leases
@@ -3839,12 +3858,14 @@ func (c *containerLXC) Export(w io.Writer, properties map[string]string) error {
 	shared.LogInfo("Exporting container", ctxMap)
 
 	// Start the storage
-	err := c.StorageStart()
+	ourStart, err := c.StorageStart()
 	if err != nil {
 		shared.LogError("Failed exporting container", ctxMap)
 		return err
 	}
-	defer c.StorageStop()
+	if ourStart {
+		defer c.StorageStop()
+	}
 
 	// Unshift the container
 	idmap, err := c.LastIdmapSet()
@@ -4147,19 +4168,21 @@ func (c *containerLXC) Migrate(cmd uint, stateDir string, function string, stop
 				return err
 			}
 
-			err = c.StorageStart()
+			ourStart, err := c.StorageStart()
 			if err != nil {
 				return err
 			}
 
 			err = idmapset.ShiftRootfs(stateDir)
-			err2 := c.StorageStop()
-			if err != nil {
-				return err
-			}
+			if ourStart {
+				_, err2 := c.StorageStop()
+				if err != nil {
+					return err
+				}
 
-			if err2 != nil {
-				return err2
+				if err2 != nil {
+					return err2
+				}
 			}
 		}
 
@@ -4389,8 +4412,10 @@ func (c *containerLXC) templateApplyNow(trigger string) error {
 
 func (c *containerLXC) FileExists(path string) error {
 	// Setup container storage if needed
+	var ourStart bool
+	var err error
 	if !c.IsRunning() {
-		err := c.StorageStart()
+		ourStart, err = c.StorageStart()
 		if err != nil {
 			return err
 		}
@@ -4406,8 +4431,8 @@ func (c *containerLXC) FileExists(path string) error {
 	)
 
 	// Tear down container storage if needed
-	if !c.IsRunning() {
-		err := c.StorageStop()
+	if !c.IsRunning() && ourStart {
+		_, err := c.StorageStop()
 		if err != nil {
 			return err
 		}
@@ -4432,9 +4457,11 @@ func (c *containerLXC) FileExists(path string) error {
 }
 
 func (c *containerLXC) FilePull(srcpath string, dstpath string) (int64, int64, os.FileMode, string, []string, error) {
+	var ourStart bool
+	var err error
 	// Setup container storage if needed
 	if !c.IsRunning() {
-		err := c.StorageStart()
+		ourStart, err = c.StorageStart()
 		if err != nil {
 			return -1, -1, 0, "", nil, err
 		}
@@ -4451,8 +4478,8 @@ func (c *containerLXC) FilePull(srcpath string, dstpath string) (int64, int64, o
 	)
 
 	// Tear down container storage if needed
-	if !c.IsRunning() {
-		err := c.StorageStop()
+	if !c.IsRunning() && ourStart {
+		_, err := c.StorageStop()
 		if err != nil {
 			return -1, -1, 0, "", nil, err
 		}
@@ -4568,9 +4595,11 @@ func (c *containerLXC) FilePush(srcpath string, dstpath string, uid int64, gid i
 		}
 	}
 
+	var ourStart bool
+	var err error
 	// Setup container storage if needed
 	if !c.IsRunning() {
-		err := c.StorageStart()
+		ourStart, err = c.StorageStart()
 		if err != nil {
 			return err
 		}
@@ -4594,8 +4623,8 @@ func (c *containerLXC) FilePush(srcpath string, dstpath string, uid int64, gid i
 	)
 
 	// Tear down container storage if needed
-	if !c.IsRunning() {
-		err := c.StorageStop()
+	if !c.IsRunning() && ourStart {
+		_, err := c.StorageStop()
 		if err != nil {
 			return err
 		}
@@ -4632,10 +4661,12 @@ func (c *containerLXC) FilePush(srcpath string, dstpath string, uid int64, gid i
 
 func (c *containerLXC) FileRemove(path string) error {
 	var errStr string
+	var ourStart bool
+	var err error
 
 	// Setup container storage if needed
 	if !c.IsRunning() {
-		err := c.StorageStart()
+		ourStart, err = c.StorageStart()
 		if err != nil {
 			return err
 		}
@@ -4651,8 +4682,8 @@ func (c *containerLXC) FileRemove(path string) error {
 	)
 
 	// Tear down container storage if needed
-	if !c.IsRunning() {
-		err := c.StorageStop()
+	if !c.IsRunning() && ourStart {
+		_, err := c.StorageStop()
 		if err != nil {
 			return err
 		}
@@ -5024,40 +5055,38 @@ func (c *containerLXC) Storage() storage {
 	return c.storage
 }
 
-func (c *containerLXC) StorageStart() error {
+func (c *containerLXC) StorageStart() (bool, error) {
 	// Initialize storage interface for the container.
 	err := c.initStorage()
 	if err != nil {
-		return err
+		return false, err
 	}
 
+	isOurOperation := false
 	if c.IsSnapshot() {
-		return c.storage.ContainerSnapshotStart(c)
+		isOurOperation, err = c.storage.ContainerSnapshotStart(c)
+	} else {
+		isOurOperation, err = c.storage.ContainerMount(c.Name(), c.Path())
 	}
 
-	_, err = c.storage.ContainerMount(c.Name(), c.Path())
-	if err != nil {
-		return err
-	}
-	return nil
+	return isOurOperation, err
 }
 
-func (c *containerLXC) StorageStop() error {
+func (c *containerLXC) StorageStop() (bool, error) {
 	// Initialize storage interface for the container.
 	err := c.initStorage()
 	if err != nil {
-		return err
+		return false, err
 	}
 
+	isOurOperation := false
 	if c.IsSnapshot() {
-		return c.storage.ContainerSnapshotStop(c)
+		isOurOperation, err = c.storage.ContainerSnapshotStop(c)
+	} else {
+		isOurOperation, err = c.storage.ContainerUmount(c.Name(), c.Path())
 	}
 
-	_, err = c.storage.ContainerUmount(c.Name(), c.Path())
-	if err != nil {
-		return err
-	}
-	return err
+	return isOurOperation, err
 }
 
 // Mount handling
diff --git a/lxd/container_snapshot.go b/lxd/container_snapshot.go
index 5d8b6ea..097b233 100644
--- a/lxd/container_snapshot.go
+++ b/lxd/container_snapshot.go
@@ -107,20 +107,11 @@ func containerSnapshotsPost(d *Daemon, r *http.Request) Response {
 		return SmartError(err)
 	}
 
-	// Check whether the container will be mounted exclusively by us or if
-	// it already was by someone else. In the latter case don't unmount it.
-	storagePool, err := c.StoragePool()
-	if err != nil {
-		return SmartError(err)
-	}
-
-	containerPoolVolumeMntPoint := getContainerMountPoint(storagePool, name)
-	mountedBefore := shared.IsMountPoint(containerPoolVolumeMntPoint)
-	err = c.StorageStart()
+	ourStart, err := c.StorageStart()
 	if err != nil {
 		return InternalError(err)
 	}
-	if !mountedBefore || c.IsSnapshot() {
+	if ourStart {
 		defer c.StorageStop()
 	}
 
diff --git a/lxd/migrate.go b/lxd/migrate.go
index 15f722c..93f3525 100644
--- a/lxd/migrate.go
+++ b/lxd/migrate.go
@@ -296,11 +296,13 @@ func (s *migrationSourceWs) Do(migrateOp *operation) error {
 
 	// Storage needs to start unconditionally now, since we need to
 	// initialize a new storage interface.
-	err := s.container.StorageStart()
+	ourStart, err := s.container.StorageStart()
 	if err != nil {
 		return err
 	}
-	defer s.container.StorageStop()
+	if ourStart {
+		defer s.container.StorageStop()
+	}
 
 	idmaps := make([]*IDMapType, 0)
 
diff --git a/lxd/storage.go b/lxd/storage.go
index 79ce072..b19612c 100644
--- a/lxd/storage.go
+++ b/lxd/storage.go
@@ -235,8 +235,8 @@ type storage interface {
 	ContainerSnapshotCreate(snapshotContainer container, sourceContainer container) error
 	ContainerSnapshotDelete(snapshotContainer container) error
 	ContainerSnapshotRename(snapshotContainer container, newName string) error
-	ContainerSnapshotStart(container container) error
-	ContainerSnapshotStop(container container) error
+	ContainerSnapshotStart(container container) (bool, error)
+	ContainerSnapshotStop(container container) (bool, error)
 
 	// For use in migrating snapshots.
 	ContainerSnapshotCreateEmpty(snapshotContainer container) error
diff --git a/lxd/storage_btrfs.go b/lxd/storage_btrfs.go
index 12dd233..6a21038 100644
--- a/lxd/storage_btrfs.go
+++ b/lxd/storage_btrfs.go
@@ -754,11 +754,13 @@ func (s *storageBtrfs) ContainerCopy(container container, sourceContainer contai
 		return err
 	}
 
-	err = sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
-	defer sourceContainer.StorageStop()
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	_, sourcePool := sourceContainer.Storage().GetContainerPoolInfo()
 	sourceContainerSubvolumeName := ""
@@ -923,11 +925,13 @@ func (s *storageBtrfs) ContainerRestore(container container, sourceContainer con
 		}
 	}()
 
-	err = sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
-	defer sourceContainer.StorageStop()
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	// Mount the source container.
 	srcContainerStorage := sourceContainer.Storage()
@@ -1086,60 +1090,62 @@ func (s *storageBtrfs) ContainerSnapshotDelete(snapshotContainer container) erro
 	return nil
 }
 
-func (s *storageBtrfs) ContainerSnapshotStart(container container) error {
+func (s *storageBtrfs) ContainerSnapshotStart(container container) (bool, error) {
 	shared.LogDebugf("Initializing BTRFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	_, err := s.StoragePoolMount()
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	snapshotSubvolumeName := getSnapshotMountPoint(s.pool.Name, container.Name())
 	roSnapshotSubvolumeName := fmt.Sprintf("%s.ro", snapshotSubvolumeName)
 	if shared.PathExists(roSnapshotSubvolumeName) {
-		return fmt.Errorf("The snapshot is already mounted read-write.")
+		shared.LogDebugf("The BTRFS snapshot is already mounted read-write.")
+		return false, nil
 	}
 
 	err = os.Rename(snapshotSubvolumeName, roSnapshotSubvolumeName)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	err = s.btrfsPoolVolumesSnapshot(roSnapshotSubvolumeName, snapshotSubvolumeName, false)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	shared.LogDebugf("Initialized BTRFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
-	return nil
+	return true, nil
 }
 
-func (s *storageBtrfs) ContainerSnapshotStop(container container) error {
+func (s *storageBtrfs) ContainerSnapshotStop(container container) (bool, error) {
 	shared.LogDebugf("Stopping BTRFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	_, err := s.StoragePoolMount()
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	snapshotSubvolumeName := getSnapshotMountPoint(s.pool.Name, container.Name())
 	roSnapshotSubvolumeName := fmt.Sprintf("%s.ro", snapshotSubvolumeName)
 	if !shared.PathExists(roSnapshotSubvolumeName) {
-		return fmt.Errorf("The snapshot isn't currently mounted read-write.")
+		shared.LogDebugf("The BTRFS snapshot is currently not mounted read-write.")
+		return false, nil
 	}
 
 	err = btrfsSubVolumesDelete(snapshotSubvolumeName)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	err = os.Rename(roSnapshotSubvolumeName, snapshotSubvolumeName)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	shared.LogDebugf("Stopped BTRFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
-	return nil
+	return true, nil
 }
 
 // ContainerSnapshotRename renames a snapshot of a container.
diff --git a/lxd/storage_dir.go b/lxd/storage_dir.go
index 074c661..05b717a 100644
--- a/lxd/storage_dir.go
+++ b/lxd/storage_dir.go
@@ -357,10 +357,13 @@ func (s *storageDir) ContainerDelete(container container) error {
 func (s *storageDir) ContainerCopy(container container, sourceContainer container) error {
 	shared.LogDebugf("Copying DIR container storage %s -> %s.", sourceContainer.Name(), container.Name())
 
-	err := sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	// Deal with the source container.
 	_, sourcePool := sourceContainer.Storage().GetContainerPoolInfo()
@@ -526,10 +529,13 @@ func (s *storageDir) ContainerSnapshotCreate(snapshotContainer container, source
 		return nil
 	}
 
-	err = sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	_, sourcePool := sourceContainer.Storage().GetContainerPoolInfo()
 	sourceContainerName := sourceContainer.Name()
@@ -675,12 +681,12 @@ func (s *storageDir) ContainerSnapshotRename(snapshotContainer container, newNam
 	return nil
 }
 
-func (s *storageDir) ContainerSnapshotStart(container container) error {
-	return nil
+func (s *storageDir) ContainerSnapshotStart(container container) (bool, error) {
+	return true, nil
 }
 
-func (s *storageDir) ContainerSnapshotStop(container container) error {
-	return nil
+func (s *storageDir) ContainerSnapshotStop(container container) (bool, error) {
+	return true, nil
 }
 
 func (s *storageDir) ImageCreate(fingerprint string) error {
diff --git a/lxd/storage_lvm.go b/lxd/storage_lvm.go
index 8c2339d..b66bcb8 100644
--- a/lxd/storage_lvm.go
+++ b/lxd/storage_lvm.go
@@ -1263,11 +1263,13 @@ func (s *storageLvm) ContainerCopy(container container, sourceContainer containe
 
 	tryUndo := true
 
-	err := sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
-	defer sourceContainer.StorageStop()
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	if sourceContainer.Storage().GetStorageType() == storageTypeLvm {
 		err := s.createSnapshotContainer(container, sourceContainer, false)
@@ -1511,11 +1513,13 @@ func (s *storageLvm) ContainerRename(container container, newContainerName strin
 func (s *storageLvm) ContainerRestore(container container, sourceContainer container) error {
 	shared.LogDebugf("Restoring LVM storage volume for container \"%s\" from %s -> %s.", s.volume.Name, sourceContainer.Name(), container.Name())
 
-	err := sourceContainer.StorageStart()
+	ourStart, err := sourceContainer.StorageStart()
 	if err != nil {
 		return err
 	}
-	defer sourceContainer.StorageStop()
+	if ourStart {
+		defer sourceContainer.StorageStop()
+	}
 
 	_, sourcePool := sourceContainer.Storage().GetContainerPoolInfo()
 	if s.pool.Name != sourcePool {
@@ -1659,7 +1663,7 @@ func (s *storageLvm) ContainerSnapshotRename(snapshotContainer container, newCon
 	return nil
 }
 
-func (s *storageLvm) ContainerSnapshotStart(container container) error {
+func (s *storageLvm) ContainerSnapshotStart(container container) (bool, error) {
 	shared.LogDebugf("Initializing LVM storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	tryUndo := true
@@ -1676,7 +1680,7 @@ func (s *storageLvm) ContainerSnapshotStart(container container) error {
 	poolName := s.getOnDiskPoolName()
 	lvpath, err := s.createSnapshotLV(poolName, sourceLvmName, storagePoolVolumeApiEndpointContainers, tmpTargetLvmName, storagePoolVolumeApiEndpointContainers, false)
 	if err != nil {
-		return fmt.Errorf("Error creating snapshot LV: %s", err)
+		return false, fmt.Errorf("Error creating snapshot LV: %s", err)
 	}
 	defer func() {
 		if tryUndo {
@@ -1693,24 +1697,24 @@ func (s *storageLvm) ContainerSnapshotStart(container container) error {
 	if lvFsType == "xfs" {
 		err := xfsGenerateNewUUID(lvpath)
 		if err != nil {
-			return err
+			return false, err
 		}
 	}
 
 	if !shared.IsMountPoint(containerMntPoint) {
 		err = tryMount(containerLvmPath, containerMntPoint, lvFsType, 0, mountOptions)
 		if err != nil {
-			return fmt.Errorf("Error mounting snapshot LV path='%s': %s", containerMntPoint, err)
+			return false, fmt.Errorf("Error mounting snapshot LV path='%s': %s", containerMntPoint, err)
 		}
 	}
 
 	tryUndo = false
 
 	shared.LogDebugf("Initialized LVM storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
-	return nil
+	return true, nil
 }
 
-func (s *storageLvm) ContainerSnapshotStop(container container) error {
+func (s *storageLvm) ContainerSnapshotStop(container container) (bool, error) {
 	shared.LogDebugf("Stopping LVM storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	name := container.Name()
@@ -1722,18 +1726,18 @@ func (s *storageLvm) ContainerSnapshotStop(container container) error {
 	if shared.IsMountPoint(snapshotMntPoint) {
 		err := tryUnmount(snapshotMntPoint, 0)
 		if err != nil {
-			return err
+			return false, err
 		}
 	}
 
 	tmpLvName := getTmpSnapshotName(lvName)
 	err := s.removeLV(poolName, storagePoolVolumeApiEndpointContainers, tmpLvName)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	shared.LogDebugf("Stopped LVM storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
-	return nil
+	return true, nil
 }
 
 func (s *storageLvm) ContainerSnapshotCreateEmpty(snapshotContainer container) error {
diff --git a/lxd/storage_migration.go b/lxd/storage_migration.go
index b565ac3..858c628 100644
--- a/lxd/storage_migration.go
+++ b/lxd/storage_migration.go
@@ -42,10 +42,13 @@ func (s rsyncStorageSourceDriver) Snapshots() []container {
 
 func (s rsyncStorageSourceDriver) SendWhileRunning(conn *websocket.Conn, op *operation) error {
 	for _, send := range s.snapshots {
-		if err := send.StorageStart(); err != nil {
+		ourStart, err := send.StorageStart()
+		if err != nil {
 			return err
 		}
-		defer send.StorageStop()
+		if ourStart {
+			defer send.StorageStop()
+		}
 
 		path := send.Path()
 		wrapper := StorageProgressReader(op, "fs_progress", send.Name())
@@ -107,10 +110,13 @@ func snapshotProtobufToContainerArgs(containerName string, snap *Snapshot) conta
 }
 
 func rsyncMigrationSink(live bool, container container, snapshots []*Snapshot, conn *websocket.Conn, srcIdmap *shared.IdmapSet, op *operation) error {
-	if err := container.StorageStart(); err != nil {
+	ourStart, err := container.StorageStart()
+	if err != nil {
 		return err
 	}
-	defer container.StorageStop()
+	if ourStart {
+		defer container.StorageStop()
+	}
 
 	// At this point we have already figured out the parent
 	// container's root disk device so we can simply
diff --git a/lxd/storage_mock.go b/lxd/storage_mock.go
index f0ab7e6..7f2f990 100644
--- a/lxd/storage_mock.go
+++ b/lxd/storage_mock.go
@@ -177,12 +177,12 @@ func (s *storageMock) ContainerSnapshotRename(
 	return nil
 }
 
-func (s *storageMock) ContainerSnapshotStart(container container) error {
-	return nil
+func (s *storageMock) ContainerSnapshotStart(container container) (bool, error) {
+	return true, nil
 }
 
-func (s *storageMock) ContainerSnapshotStop(container container) error {
-	return nil
+func (s *storageMock) ContainerSnapshotStop(container container) (bool, error) {
+	return true, nil
 }
 
 func (s *storageMock) ContainerSnapshotCreateEmpty(snapshotContainer container) error {
diff --git a/lxd/storage_zfs.go b/lxd/storage_zfs.go
index e507caa..3e80bee 100644
--- a/lxd/storage_zfs.go
+++ b/lxd/storage_zfs.go
@@ -1195,12 +1195,12 @@ func (s *storageZfs) ContainerSnapshotRename(snapshotContainer container, newNam
 	return nil
 }
 
-func (s *storageZfs) ContainerSnapshotStart(container container) error {
+func (s *storageZfs) ContainerSnapshotStart(container container) (bool, error) {
 	shared.LogDebugf("Initializing ZFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	fields := strings.SplitN(container.Name(), shared.SnapshotDelimiter, 2)
 	if len(fields) < 2 {
-		return fmt.Errorf("Invalid snapshot name: %s", container.Name())
+		return false, fmt.Errorf("Invalid snapshot name: %s", container.Name())
 	}
 
 	cName := fields[0]
@@ -1212,19 +1212,19 @@ func (s *storageZfs) ContainerSnapshotStart(container container) error {
 	snapshotMntPoint := getSnapshotMountPoint(s.pool.Name, container.Name())
 	err := s.zfsPoolVolumeClone(sourceFs, sourceSnap, destFs, snapshotMntPoint)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	shared.LogDebugf("Initialized ZFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
-	return nil
+	return true, nil
 }
 
-func (s *storageZfs) ContainerSnapshotStop(container container) error {
+func (s *storageZfs) ContainerSnapshotStop(container container) (bool, error) {
 	shared.LogDebugf("Stopping ZFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 
 	fields := strings.SplitN(container.Name(), shared.SnapshotDelimiter, 2)
 	if len(fields) < 2 {
-		return fmt.Errorf("Invalid snapshot name: %s", container.Name())
+		return false, fmt.Errorf("Invalid snapshot name: %s", container.Name())
 	}
 	cName := fields[0]
 	sName := fields[1]
@@ -1232,7 +1232,7 @@ func (s *storageZfs) ContainerSnapshotStop(container container) error {
 
 	err := s.zfsPoolVolumeDestroy(destFs)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	/* zfs creates this directory on clone (start), so we need to clean it
@@ -1240,10 +1240,10 @@ func (s *storageZfs) ContainerSnapshotStop(container container) error {
 	shared.LogDebugf("Stopped ZFS storage volume for snapshot \"%s\" on storage pool \"%s\".", s.volume.Name, s.pool.Name)
 	err = os.RemoveAll(container.Path())
 	if err != nil {
-		return err
+		return false, err
 	}
 
-	return nil
+	return true, nil
 }
 
 func (s *storageZfs) ContainerSnapshotCreateEmpty(snapshotContainer container) error {


More information about the lxc-devel mailing list