[lxc-devel] [lxd/master] Client migration existing volume check bug

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 709 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20191023/b493fbd0/attachment.bin>
-------------- next part --------------
From fa082b7e2395787117fcdafc756727917e8d96fb Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Wed, 23 Oct 2019 12:05:05 +0100
Subject: [PATCH 1/2] client/util: Removes empty error slice check in
 remoteOperationError

remoteOperationError currently checks for an empty slice of errors and if empty then returns a nil error.

However all current uses of remoteOperationError are also wrapped in an `if !success {}` statement leading to two methods of detecting if there has been an error.

1. If success boolean is false
2. If the errors slice is empty

There has been one bug in the migration code that set success to false but did not append to the errors slice, meaning this function did not detect an error.

Either we should remove the success boolean entirely and use an empty errors slice to indicate no error or we should remove the emtpy errors slice check in remoteOperationError, having both just increases the chance of a bug occurring.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 client/util.go | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/client/util.go b/client/util.go
index 6e13123e82..e5c468c8ac 100644
--- a/client/util.go
+++ b/client/util.go
@@ -144,11 +144,6 @@ func (nullReadWriteCloser) Write(p []byte) (int, error) { return len(p), nil }
 func (nullReadWriteCloser) Read(p []byte) (int, error)  { return 0, io.EOF }
 
 func remoteOperationError(msg string, errors map[string]error) error {
-	// Check if empty
-	if len(errors) == 0 {
-		return nil
-	}
-
 	// Check if all identical
 	var err error
 	for _, entry := range errors {

From f0d7aa64faf19fb87acf22944b76bc5b8aa59ebd Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Wed, 23 Oct 2019 12:03:09 +0100
Subject: [PATCH 2/2] client/lxd/storage/volumes: Fixes bug where migration
 errors were ignored

This was a potentially serious bug as if you attempted to move a volume from one machine to another and the volume already existed on the remote side then even if it responded with an error, this error was ignored and the local volume was subsequently deleted without having copied it to the remote machine.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 client/lxd_storage_volumes.go | 1 +
 1 file changed, 1 insertion(+)

diff --git a/client/lxd_storage_volumes.go b/client/lxd_storage_volumes.go
index cd86c39fc9..b10f954ed7 100644
--- a/client/lxd_storage_volumes.go
+++ b/client/lxd_storage_volumes.go
@@ -323,6 +323,7 @@ func (r *ProtocolLXD) tryCreateStoragePoolVolume(pool string, req api.StorageVol
 			path := fmt.Sprintf("/storage-pools/%s/volumes/%s", url.PathEscape(pool), url.PathEscape(req.Type))
 			top, _, err := r.queryOperation("POST", path, req, "")
 			if err != nil {
+				errors[serverURL] = err
 				continue
 			}