Add provider validation on pool creation; add disallow persistent att…

…ribute for unsupported providers
bnhassin · Mar 17, 2015 · 687afda · 687afda
1 parent 2d63649
commit 687afda
Show file tree

Hide file tree

Showing 13 changed files with 62 additions and 12 deletions.
diff --git a/provider/dummy/init.go b/provider/dummy/init.go
@@ -4,9 +4,11 @@
 package dummy
 
 import (
+	dummystorage "github.com/juju/juju/storage/provider/dummy"
 	"github.com/juju/juju/storage/provider/registry"
 )
 
 func init() {
-	registry.RegisterEnvironStorageProviders("dummy")
+	registry.RegisterEnvironStorageProviders("dummy", "dummy")
+	registry.RegisterProvider("dummy", &dummystorage.StorageProvider{})
 }
diff --git a/provider/ec2/ebs.go b/provider/ec2/ebs.go
@@ -46,6 +46,7 @@ type ebsProvider struct{}
 var _ storage.Provider = (*ebsProvider)(nil)
 
 var validConfigOptions = set.NewStrings(
+	storage.Persistent,
 	EBS_VolumeType,
 	EBS_IOPS,
 	EBS_Encrypted,

diff --git a/storage/poolmanager/defaultpool_test.go b/storage/poolmanager/defaultpool_test.go
@@ -26,8 +26,8 @@ func (s *defaultStoragePoolsSuite) TestDefaultStoragePools(c *gc.C) {
 	s.SetFeatureFlags(feature.Storage)
 	featureflag.SetFlagsFromEnvironment(osenv.JujuFeatureFlagEnvKey)
 
-	p1, err := storage.NewConfig("pool1", storage.ProviderType("foo"), map[string]interface{}{"1": "2"})
-	p2, err := storage.NewConfig("pool2", storage.ProviderType("bar"), map[string]interface{}{"3": "4"})
+	p1, err := storage.NewConfig("pool1", storage.ProviderType("loop"), map[string]interface{}{"1": "2"})
+	p2, err := storage.NewConfig("pool2", storage.ProviderType("tmpfs"), map[string]interface{}{"3": "4"})
 	c.Assert(err, jc.ErrorIsNil)
 	defaultPools := []*storage.Config{p1, p2}
 	poolmanager.RegisterDefaultStoragePools(defaultPools)

diff --git a/storage/poolmanager/poolmanager.go b/storage/poolmanager/poolmanager.go
@@ -7,6 +7,7 @@ import (
 	"github.com/juju/errors"
 
 	"github.com/juju/juju/storage"
+	"github.com/juju/juju/storage/provider/registry"
 )
 
 const (
@@ -55,6 +56,15 @@ func (pm *poolManager) Create(name string, providerType storage.ProviderType, at
 	for k, v := range attrs {
 		poolAttrs[k] = v
 	}
+	// Instantiate the provider to validate config.
+	p, err := registry.StorageProvider(providerType)
+	if err != nil {
+		return nil, err
+	}
+	if err := p.ValidateConfig(cfg); err != nil {
+		return nil, err
+	}
+
 	poolAttrs[Name] = name
 	poolAttrs[Type] = string(providerType)
 	if err := pm.settings.CreateSettings(globalKey(name), poolAttrs); err != nil {

diff --git a/storage/poolmanager/poolmanager_test.go b/storage/poolmanager/poolmanager_test.go
@@ -113,6 +113,11 @@ func (s *poolSuite) TestCreateMissingType(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, "provider type is missing")
 }
 
+func (s *poolSuite) TestCreateInvalidConfig(c *gc.C) {
+	_, err := s.poolManager.Create("testpool", storage.ProviderType("loop"), map[string]interface{}{"persistent": true})
+	c.Assert(err, gc.ErrorMatches, `machine scoped storage provider "testpool" does not support persistent storage`)
+}
+
 func (s *poolSuite) TestDelete(c *gc.C) {
 	s.createSettings(c)
 	err := s.poolManager.Delete("testpool")

diff --git a/storage/provider/common.go b/storage/provider/common.go
@@ -3,7 +3,11 @@
 
 package provider
 
-import "github.com/juju/juju/storage"
+import (
+	"github.com/juju/errors"
+
+	"github.com/juju/juju/storage"
+)
 
 // CommonProviders returns the storage providers used by all environments.
 func CommonProviders() map[storage.ProviderType]storage.Provider {
@@ -13,3 +17,11 @@ func CommonProviders() map[storage.ProviderType]storage.Provider {
 		TmpfsProviderType:  &tmpfsProvider{logAndExec},
 	}
 }
+
+// ValidateConfig performs common provider config validation.
+func ValidateConfig(p storage.Provider, cfg *storage.Config) error {
+	if p.Scope() == storage.ScopeMachine && cfg.IsPersistent() {
+		return errors.Errorf("machine scoped storage provider %q does not support persistent storage", cfg.Name())
+	}
+	return nil
+}
diff --git a/storage/provider/loop.go b/storage/provider/loop.go
@@ -33,8 +33,7 @@ var _ storage.Provider = (*loopProvider)(nil)
 
 // ValidateConfig is defined on the Provider interface.
 func (lp *loopProvider) ValidateConfig(cfg *storage.Config) error {
-	// Loop provider has no configuration.
-	return nil
+	return ValidateConfig(lp, cfg)
 }
 
 // validateFullConfig validates a fully-constructed storage config,

diff --git a/storage/provider/loop_test.go b/storage/provider/loop_test.go
@@ -66,6 +66,14 @@ func (s *loopSuite) TestValidateConfig(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *loopSuite) TestPersistentNotAllowed(c *gc.C) {
+	p := s.loopProvider(c)
+	cfg, err := storage.NewConfig("name", provider.LoopProviderType, map[string]interface{}{"persistent": true})
+	c.Assert(err, jc.ErrorIsNil)
+	err = p.ValidateConfig(cfg)
+	c.Assert(err, gc.ErrorMatches, `machine scoped storage provider "name" does not support persistent storage`)
+}
+
 func (s *loopSuite) TestSupports(c *gc.C) {
 	p := s.loopProvider(c)
 	c.Assert(p.Supports(storage.StorageKindBlock), jc.IsTrue)

diff --git a/storage/provider/rootfs.go b/storage/provider/rootfs.go
@@ -31,8 +31,7 @@ var (
 
 // ValidateConfig is defined on the Provider interface.
 func (p *rootfsProvider) ValidateConfig(cfg *storage.Config) error {
-	// Rootfs provider has no configuration.
-	return nil
+	return ValidateConfig(p, cfg)
 }
 
 // validateFullConfig validates a fully-constructed storage config,

diff --git a/storage/provider/rootfs_test.go b/storage/provider/rootfs_test.go
@@ -61,6 +61,14 @@ func (s *rootfsSuite) TestValidateConfig(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *rootfsSuite) TestPersistentNotAllowed(c *gc.C) {
+	p := s.rootfsProvider(c)
+	cfg, err := storage.NewConfig("name", provider.RootfsProviderType, map[string]interface{}{"persistent": true})
+	c.Assert(err, jc.ErrorIsNil)
+	err = p.ValidateConfig(cfg)
+	c.Assert(err, gc.ErrorMatches, `machine scoped storage provider "name" does not support persistent storage`)
+}
+
 func (s *rootfsSuite) TestSupports(c *gc.C) {
 	p := s.rootfsProvider(c)
 	c.Assert(p.Supports(storage.StorageKindBlock), jc.IsFalse)

diff --git a/storage/provider/tmpfs.go b/storage/provider/tmpfs.go
@@ -29,8 +29,7 @@ var (
 
 // ValidateConfig is defined on the Provider interface.
 func (p *tmpfsProvider) ValidateConfig(cfg *storage.Config) error {
-	// Tmpfs provider has no configuration.
-	return nil
+	return ValidateConfig(p, cfg)
 }
 
 // validateFullConfig validates a fully-constructed storage config,

diff --git a/storage/provider/tmpfs_test.go b/storage/provider/tmpfs_test.go
@@ -61,6 +61,14 @@ func (s *tmpfsSuite) TestValidateConfig(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
+func (s *tmpfsSuite) TestPersistentNotAllowed(c *gc.C) {
+	p := s.tmpfsProvider(c)
+	cfg, err := storage.NewConfig("name", provider.TmpfsProviderType, map[string]interface{}{"persistent": true})
+	c.Assert(err, jc.ErrorIsNil)
+	err = p.ValidateConfig(cfg)
+	c.Assert(err, gc.ErrorMatches, `machine scoped storage provider "name" does not support persistent storage`)
+}
+
 func (s *tmpfsSuite) TestSupports(c *gc.C) {
 	p := s.tmpfsProvider(c)
 	c.Assert(p.Supports(storage.StorageKindBlock), jc.IsFalse)

diff --git a/worker/provisioner/provisioner_test.go b/worker/provisioner/provisioner_test.go
@@ -38,7 +38,6 @@ import (
 	"github.com/juju/juju/state/multiwatcher"
 	"github.com/juju/juju/storage"
 	"github.com/juju/juju/storage/poolmanager"
-	"github.com/juju/juju/storage/provider"
 	coretesting "github.com/juju/juju/testing"
 	coretools "github.com/juju/juju/tools"
 	"github.com/juju/juju/version"
@@ -831,7 +830,7 @@ func (s *CommonProvisionerSuite) addMachineWithRequestedVolumes(volumes []state.
 func (s *ProvisionerSuite) TestProvisioningMachinesWithRequestedVolumes(c *gc.C) {
 	// Set up a persistent pool.
 	poolManager := poolmanager.New(state.NewStateSettings(s.State))
-	_, err := poolManager.Create("persistent-pool", provider.LoopProviderType, map[string]interface{}{"persistent": true})
+	_, err := poolManager.Create("persistent-pool", "dummy", map[string]interface{}{"persistent": true})
 	c.Assert(err, jc.ErrorIsNil)
 
 	p := s.newEnvironProvisioner(c)