environs: add PrecheckInstanceParams

MalloZup · Jun 2, 2017 · 4dba11f · 4dba11f
1 parent 5d9fad9
commit 4dba11f
Show file tree

Hide file tree

Showing 31 changed files with 334 additions and 185 deletions.
diff --git a/apiserver/uniter/storage.go b/apiserver/uniter/storage.go
@@ -315,7 +315,11 @@ func (s *StorageAPI) removeOneStorageAttachment(id params.StorageAttachmentId, c
 	if err != nil {
 		return err
 	}
-	return s.st.RemoveStorageAttachment(storageTag, unitTag)
+	err = s.st.RemoveStorageAttachment(storageTag, unitTag)
+	if errors.IsNotFound(err) {
+		err = nil
+	}
+	return err
 }
 
 // AddUnitStorage validates and creates additional storage instances for units.

diff --git a/environs/interface.go b/environs/interface.go
@@ -336,6 +336,16 @@ type Environ interface {
 	// Provider returns the EnvironProvider that created this Environ.
 	Provider() EnvironProvider
 
+	InstancePrechecker
+
+	// InstanceTypesFetcher represents an environment that can return
+	// information about the available instance types.
+	InstanceTypesFetcher
+}
+
+// InstancePrechecker provides a means of "prechecking" instance
+// arguments before recording them in state.
+type InstancePrechecker interface {
 	// PrecheckInstance performs a preflight check on the specified
 	// series and constraints, ensuring that they are possibly valid for
 	// creating an instance in this model.
@@ -344,16 +354,26 @@ type Environ interface {
 	// all invalid parameters. If PrecheckInstance returns nil, it is not
 	// guaranteed that the constraints are valid; if a non-nil error is
 	// returned, then the constraints are definitely invalid.
-	//
-	// TODO(axw) find a home for state.Prechecker that isn't state and
-	// isn't environs, so both packages can refer to it. Maybe the
-	// constraints package? Can't be instance, because constraints
-	// import instance...
-	PrecheckInstance(series string, cons constraints.Value, placement string) error
+	PrecheckInstance(PrecheckInstanceParams) error
+}
 
-	// InstanceTypesFetcher represents an environment that can return
-	// information about the available instance types.
-	InstanceTypesFetcher
+// PrecheckInstanceParams contains the parameters for
+// InstancePrechecker.PrecheckInstance.
+type PrecheckInstanceParams struct {
+	// Series contains the series of the machine.
+	Series string
+
+	// Constraints contains the machine constraints.
+	Constraints constraints.Value
+
+	// Placement contains the machine placement directive, if any.
+	Placement string
+
+	// VolumeAttachments contains the parameters for attaching existing
+	// volumes to the instance. The PrecheckInstance method should not
+	// expect the attachment's Machine field to be set, as PrecheckInstance
+	// may be called before a machine ID is allocated.
+	VolumeAttachments []storage.VolumeAttachmentParams
 }
 
 // CreateParams contains the parameters for Environ.Create.

diff --git a/environs/jujutest/livetests.go b/environs/jujutest/livetests.go
@@ -233,14 +233,13 @@ func (t *LiveTests) Destroy(c *gc.C) {
 }
 
 func (t *LiveTests) TestPrechecker(c *gc.C) {
-	// Providers may implement Prechecker. If they do, then they should
-	// return nil for empty constraints (excluding the null provider).
-	prechecker, ok := t.Env.(state.Prechecker)
-	if !ok {
-		return
-	}
-	const placement = ""
-	err := prechecker.PrecheckInstance("precise", constraints.Value{}, placement)
+	// All implementations of InstancePrechecker should
+	// return nil for empty constraints (excluding the
+	// manual provider).
+	t.PrepareOnce(c)
+	err := t.Env.PrecheckInstance(environs.PrecheckInstanceParams{
+		Series: "precise",
+	})
 	c.Assert(err, jc.ErrorIsNil)
 }
 

diff --git a/provider/azure/environ.go b/provider/azure/environ.go
@@ -44,7 +44,6 @@ import (
 	"github.com/juju/juju/provider/azure/internal/tracing"
 	"github.com/juju/juju/provider/azure/internal/useragent"
 	"github.com/juju/juju/provider/common"
-	"github.com/juju/juju/state"
 	"github.com/juju/juju/tools"
 )
 
@@ -111,7 +110,6 @@ type azureEnviron struct {
 }
 
 var _ environs.Environ = (*azureEnviron)(nil)
-var _ state.Prechecker = (*azureEnviron)(nil)
 
 // newEnviron creates a new azureEnviron.
 func newEnviron(
@@ -392,12 +390,12 @@ func (env *azureEnviron) ConstraintsValidator() (constraints.Validator, error) {
 	return validator, nil
 }
 
-// PrecheckInstance is defined on the state.Prechecker interface.
-func (env *azureEnviron) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if placement != "" {
-		return fmt.Errorf("unknown placement directive: %s", placement)
+// PrecheckInstance is defined on the environs.InstancePrechecker interface.
+func (env *azureEnviron) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if args.Placement != "" {
+		return fmt.Errorf("unknown placement directive: %s", args.Placement)
 	}
-	if !cons.HasInstanceType() {
+	if !args.Constraints.HasInstanceType() {
 		return nil
 	}
 	// Constraint has an instance-type constraint so let's see if it is valid.
@@ -406,11 +404,11 @@ func (env *azureEnviron) PrecheckInstance(series string, cons constraints.Value,
 		return err
 	}
 	for _, instanceType := range instanceTypes {
-		if instanceType.Name == *cons.InstanceType {
+		if instanceType.Name == *args.Constraints.InstanceType {
 			return nil
 		}
 	}
-	return fmt.Errorf("invalid instance type %q", *cons.InstanceType)
+	return fmt.Errorf("invalid instance type %q", *args.Constraints.InstanceType)
 }
 
 // MaintainInstance is specified in the InstanceBroker interface.

diff --git a/provider/cloudsigma/environ.go b/provider/cloudsigma/environ.go
@@ -10,7 +10,6 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/version"
 
-	"github.com/juju/juju/constraints"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/simplestreams"
@@ -131,7 +130,7 @@ func (env *environ) DestroyController(controllerUUID string) error {
 // all invalid parameters. If PrecheckInstance returns nil, it is not
 // guaranteed that the constraints are valid; if a non-nil error is
 // returned, then the constraints are definitely invalid.
-func (env *environ) PrecheckInstance(series string, cons constraints.Value, placement string) error {
+func (env *environ) PrecheckInstance(environs.PrecheckInstanceParams) error {
 	return nil
 }
 

diff --git a/provider/cloudsigma/environ_test.go b/provider/cloudsigma/environ_test.go
@@ -54,7 +54,7 @@ func (s *environSuite) TestBase(c *gc.C) {
 	c.Assert(cfg, gc.NotNil)
 	c.Check(cfg.Name(), gc.Equals, "testname")
 
-	c.Check(env.PrecheckInstance("", constraints.Value{}, ""), gc.IsNil)
+	c.Check(env.PrecheckInstance(environs.PrecheckInstanceParams{}), gc.IsNil)
 
 	hasRegion, ok := env.(simplestreams.HasRegion)
 	c.Check(ok, gc.Equals, true)

diff --git a/provider/dummy/environs.go b/provider/dummy/environs.go
@@ -658,10 +658,10 @@ func (e *environ) checkBroken(method string) error {
 	return nil
 }
 
-// PrecheckInstance is specified in the state.Prechecker interface.
-func (*environ) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if placement != "" && placement != "valid" {
-		return fmt.Errorf("%s placement is invalid", placement)
+// PrecheckInstance is specified in the environs.InstancePrechecker interface.
+func (*environ) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if args.Placement != "" && args.Placement != "valid" {
+		return fmt.Errorf("%s placement is invalid", args.Placement)
 	}
 	return nil
 }

diff --git a/provider/ec2/environ.go b/provider/ec2/environ.go
@@ -302,14 +302,14 @@ func (e *environ) parsePlacement(placement string) (*ec2Placement, error) {
 	return nil, fmt.Errorf("unknown placement directive: %v", placement)
 }
 
-// PrecheckInstance is defined on the state.Prechecker interface.
-func (e *environ) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if placement != "" {
-		if _, err := e.parsePlacement(placement); err != nil {
+// PrecheckInstance is defined on the environs.InstancePrechecker interface.
+func (e *environ) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if args.Placement != "" {
+		if _, err := e.parsePlacement(args.Placement); err != nil {
 			return err
 		}
 	}
-	if !cons.HasInstanceType() {
+	if !args.Constraints.HasInstanceType() {
 		return nil
 	}
 	// Constraint has an instance-type constraint so let's see if it is valid.
@@ -318,17 +318,17 @@ func (e *environ) PrecheckInstance(series string, cons constraints.Value, placem
 		return errors.Trace(err)
 	}
 	for _, itype := range instanceTypes {
-		if itype.Name != *cons.InstanceType {
+		if itype.Name != *args.Constraints.InstanceType {
 			continue
 		}
-		if archMatches(itype.Arches, cons.Arch) {
+		if archMatches(itype.Arches, args.Constraints.Arch) {
 			return nil
 		}
 	}
-	if cons.Arch == nil {
-		return fmt.Errorf("invalid AWS instance type %q specified", *cons.InstanceType)
+	if args.Constraints.Arch == nil {
+		return fmt.Errorf("invalid AWS instance type %q specified", *args.Constraints.InstanceType)
 	}
-	return fmt.Errorf("invalid AWS instance type %q and arch %q specified", *cons.InstanceType, *cons.Arch)
+	return fmt.Errorf("invalid AWS instance type %q and arch %q specified", *args.Constraints.InstanceType, *args.Constraints.Arch)
 }
 
 // MetadataLookupParams returns parameters which are used to query simplestreams metadata.

diff --git a/provider/ec2/environ_test.go b/provider/ec2/environ_test.go
@@ -15,15 +15,13 @@ import (
 	"github.com/juju/juju/environs/simplestreams"
 	"github.com/juju/juju/instance"
 	"github.com/juju/juju/network"
-	"github.com/juju/juju/state"
 )
 
 // Ensure EC2 provider supports the expected interfaces,
 var (
 	_ environs.NetworkingEnviron = (*environ)(nil)
 	_ config.ConfigSchemaSource  = (*environProvider)(nil)
 	_ simplestreams.HasRegion    = (*environ)(nil)
-	_ state.Prechecker           = (*environ)(nil)
 	_ instance.Distributor       = (*environ)(nil)
 )
 

diff --git a/provider/gce/environ_policy.go b/provider/gce/environ_policy.go
@@ -7,18 +7,19 @@ import (
 	"github.com/juju/errors"
 
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/environs"
 )
 
 // PrecheckInstance verifies that the provided series and constraints
 // are valid for use in creating an instance in this environment.
-func (env *environ) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if _, err := env.parsePlacement(placement); err != nil {
+func (env *environ) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if _, err := env.parsePlacement(args.Placement); err != nil {
 		return errors.Trace(err)
 	}
 
-	if cons.HasInstanceType() {
-		if !checkInstanceType(cons) {
-			return errors.Errorf("invalid GCE instance type %q", *cons.InstanceType)
+	if args.Constraints.HasInstanceType() {
+		if !checkInstanceType(args.Constraints) {
+			return errors.Errorf("invalid GCE instance type %q", *args.Constraints.InstanceType)
 		}
 	}
 

diff --git a/provider/joyent/environ.go b/provider/joyent/environ.go
@@ -12,7 +12,6 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/version"
 
-	"github.com/juju/juju/constraints"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/simplestreams"
@@ -57,12 +56,12 @@ func (*joyentEnviron) Provider() environs.EnvironProvider {
 	return providerInstance
 }
 
-// PrecheckInstance is defined on the state.Prechecker interface.
-func (env *joyentEnviron) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if placement != "" {
-		return fmt.Errorf("unknown placement directive: %s", placement)
+// PrecheckInstance is defined on the environs.InstancePrechecker interface.
+func (env *joyentEnviron) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if args.Placement != "" {
+		return fmt.Errorf("unknown placement directive: %s", args.Placement)
 	}
-	if !cons.HasInstanceType() {
+	if !args.Constraints.HasInstanceType() {
 		return nil
 	}
 	// Constraint has an instance-type constraint so let's see if it is valid.
@@ -71,11 +70,11 @@ func (env *joyentEnviron) PrecheckInstance(series string, cons constraints.Value
 		return err
 	}
 	for _, instanceType := range instanceTypes {
-		if instanceType.Name == *cons.InstanceType {
+		if instanceType.Name == *args.Constraints.InstanceType {
 			return nil
 		}
 	}
-	return fmt.Errorf("invalid Joyent instance %q specified", *cons.InstanceType)
+	return fmt.Errorf("invalid Joyent instance %q specified", *args.Constraints.InstanceType)
 }
 
 func (env *joyentEnviron) SetConfig(cfg *config.Config) error {

diff --git a/provider/lxd/environ_policy.go b/provider/lxd/environ_policy.go
@@ -10,17 +10,18 @@ import (
 	"github.com/juju/utils/arch"
 
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/environs"
 )
 
 // PrecheckInstance verifies that the provided series and constraints
 // are valid for use in creating an instance in this environment.
-func (env *environ) PrecheckInstance(series string, cons constraints.Value, placement string) error {
-	if _, err := env.parsePlacement(placement); err != nil {
+func (env *environ) PrecheckInstance(args environs.PrecheckInstanceParams) error {
+	if _, err := env.parsePlacement(args.Placement); err != nil {
 		return errors.Trace(err)
 	}
 
-	if cons.HasInstanceType() {
-		return errors.Errorf("LXD does not support instance types (got %q)", *cons.InstanceType)
+	if args.Constraints.HasInstanceType() {
+		return errors.Errorf("LXD does not support instance types (got %q)", *args.Constraints.InstanceType)
 	}
 
 	return nil

diff --git a/provider/lxd/environ_policy_test.go b/provider/lxd/environ_policy_test.go
@@ -14,6 +14,7 @@ import (
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/environs"
 	"github.com/juju/juju/provider/lxd"
 )
 
@@ -23,35 +24,23 @@ type environPolSuite struct {
 
 var _ = gc.Suite(&environPolSuite{})
 
-func (s *environPolSuite) TestPrecheckInstanceOkay(c *gc.C) {
-	cons := constraints.Value{}
-	placement := ""
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
-
-	c.Check(err, jc.ErrorIsNil)
-}
-
-func (s *environPolSuite) TestPrecheckInstanceAPI(c *gc.C) {
-	cons := constraints.Value{}
-	placement := ""
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
+func (s *environPolSuite) TestPrecheckInstanceDefaults(c *gc.C) {
+	err := s.Env.PrecheckInstance(environs.PrecheckInstanceParams{Series: series.LatestLts()})
 	c.Assert(err, jc.ErrorIsNil)
 
 	s.CheckNoAPI(c)
 }
 
 func (s *environPolSuite) TestPrecheckInstanceHasInstanceType(c *gc.C) {
 	cons := constraints.MustParse("instance-type=some-instance-type")
-	placement := ""
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
+	err := s.Env.PrecheckInstance(environs.PrecheckInstanceParams{Series: series.LatestLts(), Constraints: cons})
 
 	c.Check(err, gc.ErrorMatches, `LXD does not support instance types.*`)
 }
 
 func (s *environPolSuite) TestPrecheckInstanceDiskSize(c *gc.C) {
 	cons := constraints.MustParse("root-disk=1G")
-	placement := ""
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
+	err := s.Env.PrecheckInstance(environs.PrecheckInstanceParams{Series: series.LatestLts(), Constraints: cons})
 
 	c.Check(err, jc.ErrorIsNil)
 }
@@ -60,16 +49,14 @@ func (s *environPolSuite) TestPrecheckInstanceUnsupportedArch(c *gc.C) {
 	s.PatchValue(&arch.HostArch, func() string { return arch.AMD64 })
 
 	cons := constraints.MustParse("arch=i386")
-	placement := ""
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
+	err := s.Env.PrecheckInstance(environs.PrecheckInstanceParams{Series: series.LatestLts(), Constraints: cons})
 
 	c.Check(err, jc.ErrorIsNil)
 }
 
 func (s *environPolSuite) TestPrecheckInstanceAvailZone(c *gc.C) {
-	cons := constraints.Value{}
 	placement := "zone=a-zone"
-	err := s.Env.PrecheckInstance(series.LatestLts(), cons, placement)
+	err := s.Env.PrecheckInstance(environs.PrecheckInstanceParams{Series: series.LatestLts(), Placement: placement})
 
 	c.Check(err, gc.ErrorMatches, `unknown placement directive: .*`)
 }