Merge pull request #7646 from axw/import-storage-review-fixes

Import storage review fixes

## Description of change

Address review comments on #7644.

- Rename State.ImportFilesystem to State.AddExistingFilesystem. Separate validation into another function.
- Rename api/storage's Client.ImportStorage to Client.Import, and add unit tests.

## QA steps

Non-functional change. Check it builds and tests pass.

## Documentation changes

None.

## Bug reference

None.

api/storage/client.go

@@ -247,7 +247,7 @@ func (c *Client) Detach(storageIds []string) ([]params.ErrorResult, error) {
 }
 
 // Import imports storage into the model.
-func (c *Client) ImportStorage(
+func (c *Client) Import(
 	kind storage.StorageKind,
 	storagePool string,
 	storageProviderId string,

api/storage/client_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/juju/juju/api/storage"
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/params"
+	jujustorage "github.com/juju/juju/storage"
 	"github.com/juju/juju/testing"
 )
 
@@ -771,3 +772,63 @@ func (s *storageMockSuite) TestAttachArityMismatch(c *gc.C) {
 	_, err := client.Attach("foo/0", []string{"bar/1", "baz/2"})
 	c.Check(err, gc.ErrorMatches, `expected 2 result\(s\), got 3`)
 }
+
+func (s *storageMockSuite) TestImport(c *gc.C) {
+	apiCaller := basetesting.APICallerFunc(
+		func(objType string,
+			version int,
+			id, request string,
+			a, result interface{},
+		) error {
+			c.Check(objType, gc.Equals, "Storage")
+			c.Check(id, gc.Equals, "")
+			c.Check(request, gc.Equals, "Import")
+			c.Check(a, jc.DeepEquals, params.BulkImportStorageParams{[]params.ImportStorageParams{{
+				Kind:        params.StorageKindBlock,
+				Pool:        "foo",
+				ProviderId:  "bar",
+				StorageName: "baz",
+			}}})
+			c.Assert(result, gc.FitsTypeOf, &params.ImportStorageResults{})
+			results := result.(*params.ImportStorageResults)
+			results.Results = []params.ImportStorageResult{{
+				Result: &params.ImportStorageDetails{
+					StorageTag: "storage-qux-0",
+				},
+			}}
+			return nil
+		},
+	)
+	client := storage.NewClient(apiCaller)
+	storageTag, err := client.Import(jujustorage.StorageKindBlock, "foo", "bar", "baz")
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(storageTag, gc.Equals, names.NewStorageTag("qux/0"))
+}
+
+func (s *storageMockSuite) TestImportError(c *gc.C) {
+	apiCaller := basetesting.APICallerFunc(
+		func(objType string, version int, id, request string, a, result interface{}) error {
+			results := result.(*params.ImportStorageResults)
+			results.Results = []params.ImportStorageResult{{
+				Error: &params.Error{Message: "qux"},
+			}}
+			return nil
+		},
+	)
+	client := storage.NewClient(apiCaller)
+	_, err := client.Import(jujustorage.StorageKindBlock, "foo", "bar", "baz")
+	c.Check(err, gc.ErrorMatches, "qux")
+}
+
+func (s *storageMockSuite) TestImportArityMismatch(c *gc.C) {
+	apiCaller := basetesting.APICallerFunc(
+		func(objType string, version int, id, request string, a, result interface{}) error {
+			results := result.(*params.ImportStorageResults)
+			results.Results = []params.ImportStorageResult{{}, {}}
+			return nil
+		},
+	)
+	client := storage.NewClient(apiCaller)
+	_, err := client.Import(jujustorage.StorageKindBlock, "foo", "bar", "baz")
+	c.Check(err, gc.ErrorMatches, `expected 1 result, got 2`)
+}

apiserver/facades/client/storage/shim.go

@@ -143,8 +143,8 @@ type storageAccess interface {
 	// identified unit.
 	UnitStorageAttachments(names.UnitTag) ([]state.StorageAttachment, error)
 
-	// ImportFilesystem imports an existing filesystem into the model.
-	ImportFilesystem(f state.FilesystemInfo, v *state.VolumeInfo, storageName string) (names.StorageTag, error)
+	// AddExistingFilesystem imports an existing filesystem into the model.
+	AddExistingFilesystem(f state.FilesystemInfo, v *state.VolumeInfo, storageName string) (names.StorageTag, error)
 }
 
 var getState = func(st *state.State) storageAccess {

apiserver/facades/client/storage/storage.go

@@ -1005,7 +1005,7 @@ func (a *APIv4) importFilesystem(
 		filesystemInfo.Size = info.Size
 	}
 
-	storageTag, err := a.storage.ImportFilesystem(filesystemInfo, volumeInfo, arg.StorageName)
+	storageTag, err := a.storage.AddExistingFilesystem(filesystemInfo, volumeInfo, arg.StorageName)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}

cmd/juju/storage/import.go

@@ -11,6 +11,7 @@ import (
 	"github.com/juju/cmd"
 	"github.com/juju/errors"
 
+	apistorage "github.com/juju/juju/api/storage"
 	"github.com/juju/juju/cmd/modelcmd"
 	"github.com/juju/juju/jujuclient"
 	"github.com/juju/juju/storage"
@@ -43,7 +44,8 @@ type NewStorageImporterFunc func(*StorageCommandBase) (StorageImporter, error)
 // NewStorageImporter returns a new StorageImporter,
 // given a StorageCommandBase.
 func NewStorageImporter(cmd *StorageCommandBase) (StorageImporter, error) {
-	return cmd.NewStorageAPI()
+	api, err := cmd.NewStorageAPI()
+	return apiStorageImporter{api}, err
 }
 
 const (
@@ -152,3 +154,13 @@ type StorageImporter interface {
 		storagePool, storageProviderId, storageName string,
 	) (names.StorageTag, error)
 }
+
+type apiStorageImporter struct {
+	*apistorage.Client
+}
+
+func (a apiStorageImporter) ImportStorage(
+	kind storage.StorageKind, storagePool, storageProviderId, storageName string,
+) (names.StorageTag, error) {
+	return a.Import(kind, storagePool, storageProviderId, storageName)
+}

state/filesystem.go

@@ -6,6 +6,7 @@ package state
 import (
 	"fmt"
 	"path"
+	"regexp"
 	"strings"
 	"time"
 
@@ -780,43 +781,19 @@ func removeFilesystemOps(st *State, filesystem Filesystem, assert interface{}) (
 	return ops, nil
 }
 
-// ImportFilesystem imports an existing, already-provisioned
+// AddExistingFilesystem imports an existing, already-provisioned
 // filesystem into the model. The model will start out with
 // the status "detached". The filesystem and associated backing
 // volume (if any) will be associated with the given storage
 // name, with the allocated storage tag being returned.
-func (st *State) ImportFilesystem(
+func (st *State) AddExistingFilesystem(
 	info FilesystemInfo,
 	backingVolume *VolumeInfo,
 	storageName string,
 ) (_ names.StorageTag, err error) {
-	defer errors.DeferredAnnotatef(&err, "cannot import filesystem")
-	if info.Pool == "" {
-		return names.StorageTag{}, errors.NotValidf("empty pool name")
-	}
-	if backingVolume == nil {
-		if info.FilesystemId == "" {
-			return names.StorageTag{}, errors.NotValidf("empty filesystem ID")
-		}
-	} else {
-		if info.FilesystemId != "" {
-			return names.StorageTag{}, errors.NotValidf("non-empty filesystem ID with backing volume")
-		}
-		if backingVolume.VolumeId == "" {
-			return names.StorageTag{}, errors.NotValidf("empty backing volume ID")
-		}
-		if backingVolume.Pool != info.Pool {
-			return names.StorageTag{}, errors.Errorf(
-				"volume pool %q does not match filesystem pool %q",
-				backingVolume.Pool, info.Pool,
-			)
-		}
-		if backingVolume.Size != info.Size {
-			return names.StorageTag{}, errors.Errorf(
-				"volume size %d does not match filesystem size %d",
-				backingVolume.Size, info.Size,
-			)
-		}
+	defer errors.DeferredAnnotatef(&err, "cannot add existing filesystem")
+	if err := validateAddExistingFilesystem(st, info, backingVolume, storageName); err != nil {
+		return names.StorageTag{}, errors.Trace(err)
 	}
 	storageId, err := newStorageInstanceId(st, storageName)
 	if err != nil {
@@ -860,6 +837,60 @@ func (st *State) ImportFilesystem(
 	return storageTag, nil
 }
 
+var storageNameRE = regexp.MustCompile(names.StorageNameSnippet)
+
+func validateAddExistingFilesystem(
+	st *State,
+	info FilesystemInfo,
+	backingVolume *VolumeInfo,
+	storageName string,
+) error {
+	if !storage.IsValidPoolName(info.Pool) {
+		return errors.NotValidf("pool name %q", info.Pool)
+	}
+	if !storageNameRE.MatchString(storageName) {
+		return errors.NotValidf("storage name %q", storageName)
+	}
+	if backingVolume == nil {
+		if info.FilesystemId == "" {
+			return errors.NotValidf("empty filesystem ID")
+		}
+	} else {
+		if info.FilesystemId != "" {
+			return errors.NotValidf("non-empty filesystem ID with backing volume")
+		}
+		if backingVolume.VolumeId == "" {
+			return errors.NotValidf("empty backing volume ID")
+		}
+		if backingVolume.Pool != info.Pool {
+			return errors.Errorf(
+				"volume pool %q does not match filesystem pool %q",
+				backingVolume.Pool, info.Pool,
+			)
+		}
+		if backingVolume.Size != info.Size {
+			return errors.Errorf(
+				"volume size %d does not match filesystem size %d",
+				backingVolume.Size, info.Size,
+			)
+		}
+	}
+	_, provider, err := poolStorageProvider(st, info.Pool)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	if !provider.Supports(storage.StorageKindFilesystem) {
+		if backingVolume == nil {
+			return errors.New("backing volume info missing")
+		}
+	} else {
+		if backingVolume != nil {
+			return errors.New("unexpected volume info")
+		}
+	}
+	return nil
+}
+
 // filesystemAttachmentId returns a filesystem attachment document ID,
 // given the corresponding filesystem name and machine ID.
 func filesystemAttachmentId(machineId, filesystemId string) string {
@@ -946,10 +977,6 @@ func (st *State) addFilesystemOps(params FilesystemParams, machineId string) ([]
 		}
 		volumeId = volumeTag.Id()
 		ops = append(ops, volumeOps...)
-	} else {
-		if params.volumeInfo != nil {
-			return nil, names.FilesystemTag{}, names.VolumeTag{}, errors.Errorf("unexpected volume info")
-		}
 	}
 
 	statusDoc := statusDoc{

state/filesystem_test.go

@@ -1124,13 +1124,13 @@ func (s *FilesystemStateSuite) TestFilesystemAttachmentLocationConflict(c *gc.C)
 			`mount point "/srv/within" for "data" storage`)
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystem(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystem(c *gc.C) {
 	fsInfoIn := state.FilesystemInfo{
 		Pool:         "modelscoped",
 		Size:         123,
 		FilesystemId: "foo",
 	}
-	storageTag, err := s.State.ImportFilesystem(fsInfoIn, nil, "pgdata")
+	storageTag, err := s.State.AddExistingFilesystem(fsInfoIn, nil, "pgdata")
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(storageTag, gc.Equals, names.NewStorageTag("pgdata/0"))
 
@@ -1145,16 +1145,16 @@ func (s *FilesystemStateSuite) TestImportFilesystem(c *gc.C) {
 	c.Assert(fsStatus.Status, gc.Equals, status.Detached)
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystemEmptyFilesystemId(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystemEmptyFilesystemId(c *gc.C) {
 	fsInfoIn := state.FilesystemInfo{
 		Pool: "modelscoped",
 		Size: 123,
 	}
-	_, err := s.State.ImportFilesystem(fsInfoIn, nil, "pgdata")
-	c.Assert(err, gc.ErrorMatches, "cannot import filesystem: empty filesystem ID not valid")
+	_, err := s.State.AddExistingFilesystem(fsInfoIn, nil, "pgdata")
+	c.Assert(err, gc.ErrorMatches, "cannot add existing filesystem: empty filesystem ID not valid")
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystemVolumeBacked(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystemVolumeBacked(c *gc.C) {
 	fsInfoIn := state.FilesystemInfo{
 		Pool: "modelscoped-block",
 		Size: 123,
@@ -1164,15 +1164,15 @@ func (s *FilesystemStateSuite) TestImportFilesystemVolumeBacked(c *gc.C) {
 		Size:     123,
 		VolumeId: "foo",
 	}
-	storageTag, err := s.State.ImportFilesystem(fsInfoIn, &volInfoIn, "pgdata")
+	storageTag, err := s.State.AddExistingFilesystem(fsInfoIn, &volInfoIn, "pgdata")
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(storageTag, gc.Equals, names.NewStorageTag("pgdata/0"))
 
 	filesystem, err := s.State.StorageInstanceFilesystem(storageTag)
 	c.Assert(err, jc.ErrorIsNil)
 	fsInfoOut, err := filesystem.Info()
 	c.Assert(err, jc.ErrorIsNil)
-	fsInfoIn.FilesystemId = "filesystem-0" // set by ImportFilesystem
+	fsInfoIn.FilesystemId = "filesystem-0" // set by AddExistingFilesystem
 	c.Assert(fsInfoOut, jc.DeepEquals, fsInfoIn)
 
 	fsStatus, err := filesystem.Status()
@@ -1190,17 +1190,17 @@ func (s *FilesystemStateSuite) TestImportFilesystemVolumeBacked(c *gc.C) {
 	c.Assert(volStatus.Status, gc.Equals, status.Detached)
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystemVolumeBackedVolumeInfoMissing(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystemVolumeBackedVolumeInfoMissing(c *gc.C) {
 	fsInfo := state.FilesystemInfo{
 		Pool:         "modelscoped-block",
 		Size:         123,
 		FilesystemId: "foo",
 	}
-	_, err := s.State.ImportFilesystem(fsInfo, nil, "pgdata")
-	c.Assert(err, gc.ErrorMatches, "cannot import filesystem: backing volume info missing")
+	_, err := s.State.AddExistingFilesystem(fsInfo, nil, "pgdata")
+	c.Assert(err, gc.ErrorMatches, "cannot add existing filesystem: backing volume info missing")
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystemVolumeBackedFilesystemIdSupplied(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystemVolumeBackedFilesystemIdSupplied(c *gc.C) {
 	fsInfo := state.FilesystemInfo{
 		Pool:         "modelscoped-block",
 		Size:         123,
@@ -1211,11 +1211,11 @@ func (s *FilesystemStateSuite) TestImportFilesystemVolumeBackedFilesystemIdSuppl
 		Size:     123,
 		VolumeId: "foo",
 	}
-	_, err := s.State.ImportFilesystem(fsInfo, &volInfo, "pgdata")
-	c.Assert(err, gc.ErrorMatches, "cannot import filesystem: non-empty filesystem ID with backing volume not valid")
+	_, err := s.State.AddExistingFilesystem(fsInfo, &volInfo, "pgdata")
+	c.Assert(err, gc.ErrorMatches, "cannot add existing filesystem: non-empty filesystem ID with backing volume not valid")
 }
 
-func (s *FilesystemStateSuite) TestImportFilesystemVolumeBackedEmptyVolumeId(c *gc.C) {
+func (s *FilesystemStateSuite) TestAddExistingFilesystemVolumeBackedEmptyVolumeId(c *gc.C) {
 	fsInfo := state.FilesystemInfo{
 		Pool: "modelscoped-block",
 		Size: 123,
@@ -1224,8 +1224,8 @@ func (s *FilesystemStateSuite) TestImportFilesystemVolumeBackedEmptyVolumeId(c *
 		Pool: "modelscoped-block",
 		Size: 123,
 	}
-	_, err := s.State.ImportFilesystem(fsInfo, &volInfo, "pgdata")
-	c.Assert(err, gc.ErrorMatches, "cannot import filesystem: empty backing volume ID not valid")
+	_, err := s.State.AddExistingFilesystem(fsInfo, &volInfo, "pgdata")
+	c.Assert(err, gc.ErrorMatches, "cannot add existing filesystem: empty backing volume ID not valid")
 }
 
 func (s *FilesystemStateSuite) setupFilesystemAttachment(c *gc.C, pool string) (state.Filesystem, *state.Machine) {