Merge pull request juju#8780 from ExternalReality/bug#1771980

juju#8780 ## Description of change Restoring a backup from file twice in success fails. Juju should be able to restore a given backup from file more than once. Why is this change needed? This change fixes the referenced bug. How do we verify that the change works? 1. Create a backup to file. 2. Restore the backup. 3. Restore the backup. ## Documentation changes N/A ## Bug reference https://bugs.launchpad.net/juju/+bug/1771980
sdg0919 · May 31, 2018 · e114fc4 · e114fc4
2 parents 4d6ae83 + cc07d2d
commit e114fc4
Show file tree

Hide file tree

Showing 7 changed files with 219 additions and 5 deletions.
diff --git a/api/backups/client.go b/api/backups/client.go
@@ -20,16 +20,21 @@ type Client struct {
 	client *httprequest.Client
 }
 
+// MakeClient is a direct constructor function for a backups client.
+func MakeClient(frontend base.ClientFacade, backend base.FacadeCaller, client *httprequest.Client) *Client {
+	return &Client{
+		ClientFacade: frontend,
+		facade:       backend,
+		client:       client,
+	}
+}
+
 // NewClient returns a new backups API client.
 func NewClient(st base.APICallCloser) (*Client, error) {
 	frontend, backend := base.NewClientFacade(st, "Backups")
 	client, err := st.HTTPClient()
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	return &Client{
-		ClientFacade: frontend,
-		facade:       backend,
-		client:       client,
-	}, nil
+	return MakeClient(frontend, backend, client), nil
 }
diff --git a/api/backups/restore.go b/api/backups/restore.go
@@ -74,13 +74,27 @@ func (c *Client) RestoreReader(r io.ReadSeeker, meta *params.BackupsMetadataResu
 	}
 	logger.Debugf("Server is now in 'about to restore' mode, proceeding to upload the backup file")
 
+	results, err := c.List()
+	if err != nil {
+		return errors.Trace(err)
+	}
+
+	// Do not upload if backup already exists on controller.
+	list := results.List
+	for _, b := range list {
+		if b.Checksum == meta.Checksum {
+			return c.restore(b.ID, newClient)
+		}
+	}
+
 	// Upload.
 	backupId, err := c.Upload(r, *meta)
 	if err != nil {
 		finishErr := finishRestore(newClient)
 		logger.Errorf("could not clean up after failed backup upload: %v", finishErr)
 		return errors.Annotatef(err, "cannot upload backup file")
 	}
+
 	return c.restore(backupId, newClient)
 }
 

diff --git a/api/backups/restore_test.go b/api/backups/restore_test.go
@@ -0,0 +1,54 @@
+// Copyright 2018 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package backups_test
+
+import (
+	gc "gopkg.in/check.v1"
+
+	"github.com/golang/mock/gomock"
+	"github.com/juju/juju/api/backups"
+
+	"github.com/juju/juju/api/base/mocks"
+	"github.com/juju/juju/apiserver/params"
+)
+
+type restoreSuite struct {
+}
+
+var _ = gc.Suite(&restoreSuite{})
+
+func (s *restoreSuite) TestRestoreFromFileBackupExistsOnController(c *gc.C) {
+	mockController := gomock.NewController(c)
+	mockBackupFacadeCaller := mocks.NewMockFacadeCaller(mockController)
+	mockBackupClientFacade := mocks.NewMockClientFacade(mockController)
+	mockBackupClientFacade.EXPECT().Close().AnyTimes()
+
+	// testBackupResults represents the parameters passed in by the client.
+	testBackupResults := params.BackupsMetadataResult{
+		Checksum: "testCheckSum",
+	}
+
+	// This listBackupResults represents what is passed back from server.
+	testBackupsListResults := params.BackupsListResult{
+		List: []params.BackupsMetadataResult{testBackupResults},
+	}
+
+	// resultBackupList is an out param
+	resultBackupList := &params.BackupsListResult{}
+	args := params.BackupsListArgs{}
+
+	// Upload should never be called. If it is the test will fail.
+	gomock.InOrder(
+		mockBackupFacadeCaller.EXPECT().FacadeCall("PrepareRestore", nil, gomock.Any()),
+		mockBackupFacadeCaller.EXPECT().FacadeCall("List", args, resultBackupList).SetArg(2, testBackupsListResults),
+		mockBackupFacadeCaller.EXPECT().FacadeCall("Restore", gomock.Any(), gomock.Any()).Times(1),
+		mockBackupFacadeCaller.EXPECT().FacadeCall("FinishRestore", gomock.Any(), gomock.Any()).Times(1),
+	)
+
+	connFunc := func() (*backups.Client, error) {
+		return backups.MakeClient(mockBackupClientFacade, mockBackupFacadeCaller, nil), nil
+	}
+	mockBackupsClient, _ := connFunc()
+	mockBackupsClient.RestoreReader(nil, &testBackupResults, connFunc)
+}
diff --git a/api/base/caller.go b/api/base/caller.go
@@ -92,6 +92,7 @@ type Stream interface {
 // FacadeCaller is a wrapper for the common paradigm that a given client just
 // wants to make calls on a facade using the best known version of the API. And
 // without dealing with an id parameter.
+//go:generate mockgen -package mocks -destination mocks/facadecaller_mock.go github.com/juju/juju/api/base FacadeCaller
 type FacadeCaller interface {
 	// FacadeCall will place a request against the API using the requested
 	// Facade and the best version that the API server supports that is

diff --git a/api/base/clientfacade.go b/api/base/clientfacade.go
@@ -17,6 +17,7 @@ type APICallCloser interface {
 // They provide two common methods for writing the client side code.
 // BestAPIVersion() is used to allow for compatibility testing, and Close() is
 // used to indicate when we are done with the connection.
+//go:generate mockgen -package mocks -destination mocks/clientfacade_mock.go github.com/juju/juju/api/base ClientFacade
 type ClientFacade interface {
 	// BestAPIVersion returns the API version that we were able to
 	// determine is supported by both the client and the API Server

diff --git a/api/base/mocks/clientfacade_mock.go b/api/base/mocks/clientfacade_mock.go
diff --git a/api/base/mocks/facadecaller_mock.go b/api/base/mocks/facadecaller_mock.go