Merge lp:~wallyworld/juju-core/constraints-validation-merge into lp:~go-bot/juju-core/trunk

[Ian Booth]

New constraints validation and merge

A new constraints Validator implementation
is introduced, which provides validation
and merging support for constraints. It
allows for conflicting attributes and
unsupported attributes to be registered,
and does the right thing with these.
To illustrate the point, a new constraints
attribute instance-type is also added, which
will be used in a followup branch.

https://codereview.appspot.com/87970043/

New constraints validation and merge

A new constraints Validator implementation
is introduced, which provides validation
and merging support for constraints. It
allows for conflicting attributes and
unsupported attributes to be registered,
and does the right thing with these.
To illustrate the point, a new constraints
attribute instance-type is also added, which
will be used in a followup branch.

https://codereview.appspot.com/87970043/

[Ian Booth (wallyworld)]

Reviewers: mp+215807_code.launchpad.net,

Message:
Please take a look.

Description:
New constraints validation and merge

A new constraints Validator implementation
is introduced, which provides validation
and merging support for constraints. It
allows for conflicting attributes and
unsupported attributes to be registered,
and does the right thing with these.
To illustrate the point, a new constraints
attribute instance-type is also added, which
will be used in a followup branch.

https://code.launchpad.net/~wallyworld/juju-core/constraints-validation-merge/+merge/215807

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/87970043/

Affected files (+757, -184 lines):
   A [revision details]
   M constraints/constraints.go
   M constraints/constraints_test.go
   A constraints/export_test.go
   A constraints/validation.go
   A constraints/validation_test.go
   M state/addmachine.go
   M state/prechecker_test.go
   M state/service.go

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/87970043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/87970043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/87970043/

[William Reade (fwereade)]

LGTM apart from the overly-broad interface.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode17
constraints/validation.go:17: type Validator interface {
I'm not sure why this is an interface when it's impossible for other
people to implement it (unexported methods...). In general, I'm all for
interfaces at judiciously chosen boundaries, but I'm not 100% sure this
is one. Something like:

type Validator interface {
     Validate(cons Value) error
     Merge(consA, consB Value) (Value, error)
}

...would feel neater to me. Although "consA, consB" is suboptimal...
"fallbacks, primary" doesn't work either, but I'd really like something
about the names to indicate which is overriding which.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode25
constraints/validation.go:25: RegisterConflicts(reds, blues []string)
I really appreciate blank lines before comments.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode137
constraints/validation.go:137: return v.checkUnsupported(cons)
Callers will indeed probably want to ignore them. I wonder how we can
cleanly feed this stuff back to CLI users (or, well, any users)?

https://codereview.appspot.com/87970043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode17
constraints/validation.go:17: type Validator interface {
On 2014/04/18 07:48:20, fwereade wrote:
> I'm not sure why this is an interface when it's impossible for other
people to
> implement it (unexported methods...). In general, I'm all for
interfaces at
> judiciously chosen boundaries, but I'm not 100% sure this is one.
Something
> like:

> type Validator interface {
> Validate(cons Value) error
> Merge(consA, consB Value) (Value, error)
> }

> ...would feel neater to me. Although "consA, consB" is suboptimal...
"fallbacks,
> primary" doesn't work either, but I'd really like something about the
names to
> indicate which is overriding which.

I renamed consA to consFallbacks, and consB to cons
I kept only the exported methods on the interface. This includes both
the Register methods and Validate and Merge.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode25
constraints/validation.go:25: RegisterConflicts(reds, blues []string)
On 2014/04/18 07:48:20, fwereade wrote:
> I really appreciate blank lines before comments.

Done.

https://codereview.appspot.com/87970043/diff/60001/constraints/validation.go#newcode137
constraints/validation.go:137: return v.checkUnsupported(cons)
On 2014/04/18 07:48:20, fwereade wrote:
> Callers will indeed probably want to ignore them. I wonder how we can
cleanly
> feed this stuff back to CLI users (or, well, any users)?

I reworked the Validate interface - it now returns ([]string, error).
[]string is the unsupported attributes, error is the true validation
error. The NotSUpportedError is gone. So if error is nil, the caller can
then see if the unsupported attrs is populated and do as it wishes, or
just call _, err := validator.Validate(....)

https://codereview.appspot.com/87970043/

[William Reade (fwereade)]

Still LGTM, with a vague feeling that an explicit UnsupportedConstraints
error, with a method that itself returns the []string, would be even
nicer -- and then it's a similar story, you try to validate and can
either ignore or handle the specific error as you please. I'll leave it
to your judgment, though.

https://codereview.appspot.com/87970043/

=== modified file 'constraints/constraints.go'
--- constraints/constraints.go	2014-03-17 05:04:51 +0000
+++ constraints/constraints.go	2014-04-18 13:10:25 +0000
@@ -1,4 +1,4 @@
-// Copyright 2013 Canonical Ltd.
+// Copyright 2013, 2014 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
 package constraints
@@ -6,6 +6,7 @@
 import (
 	"fmt"
 	"math"
+	"reflect"
 	"strconv"
 	"strings"
 
@@ -15,6 +16,19 @@
 	"launchpad.net/juju-core/juju/arch"
 )
 
+// The following constants list the supported constraint attribute names, as defined
+// by the fields in the Value struct.
+const (
+	Arch         = "arch"
+	Container    = "container"
+	CpuCores     = "cpu-cores"
+	CpuPower     = "cpu-power"
+	Mem          = "mem"
+	RootDisk     = "root-disk"
+	Tags         = "tags"
+	InstanceType = "instance-type"
+)
+
 // Value describes a user's requirements of the hardware on which units
 // of a service will run. Constraints are used to choose an existing machine
 // onto which a unit will be deployed, or to provision a new machine if no
@@ -52,18 +66,45 @@
 	// An empty list is treated the same as a nil (unspecified) list, except an
 	// empty list will override any default tags, where a nil list will not.
 	Tags *[]string `json:"tags,omitempty" yaml:"tags,omitempty"`
+
+	// InstanceType, if not nil, indicates that the specified cloud instance type
+	// be used. Only valid for clouds which support instance types.
+	InstanceType *string `json:"instance-type,omitempty" yaml:"instance-type,omitempty"`
+}
+
+// fieldNames records a mapping from the constraint tag to struct field name.
+// eg "root-disk" maps to RootDisk.
+var fieldNames map[string]string
+
+func init() {
+	// Create the fieldNames map by inspecting the json tags for each of
+	// the Value struct fields.
+	fieldNames = make(map[string]string)
+	typ := reflect.TypeOf(Value{})
+	for i := 0; i < typ.NumField(); i++ {
+		field := typ.Field(i)
+		if tag := field.Tag.Get("json"); tag != "" {
+			if i := strings.Index(tag, ","); i >= 0 {
+				tag = tag[0:i]
+			}
+			if tag == "-" {
+				continue
+			}
+			if tag != "" {
+				fieldNames[tag] = field.Name
+			}
+		}
+	}
 }
 
 // IsEmpty returns if the given constraints value has no constraints set
 func IsEmpty(v *Value) bool {
-	return v == nil ||
-		v.Arch == nil &&
-			v.Container == nil &&
-			v.CpuCores == nil &&
-			v.CpuPower == nil &&
-			v.Mem == nil &&
-			v.RootDisk == nil &&
-			v.Tags == nil
+	return v.String() == ""
+}
+
+// HasInstanceType returns true if the constraints.Value specifies an instance type.
+func (v *Value) HasInstanceType() bool {
+	return v.InstanceType != nil && *v.InstanceType != ""
 }
 
 // String expresses a constraints.Value in the language in which it was specified.
@@ -81,6 +122,9 @@
 	if v.CpuPower != nil {
 		strs = append(strs, "cpu-power="+uintStr(*v.CpuPower))
 	}
+	if v.InstanceType != nil {
+		strs = append(strs, "instance-type="+string(*v.InstanceType))
+	}
 	if v.Mem != nil {
 		s := uintStr(*v.Mem)
 		if s != "" {
@@ -102,33 +146,6 @@
 	return strings.Join(strs, " ")
 }
 
-// WithFallbacks returns a copy of v with nil values taken from v0.
-func (v Value) WithFallbacks(v0 Value) Value {
-	v1 := v0
-	if v.Arch != nil {
-		v1.Arch = v.Arch
-	}
-	if v.Container != nil {
-		v1.Container = v.Container
-	}
-	if v.CpuCores != nil {
-		v1.CpuCores = v.CpuCores
-	}
-	if v.CpuPower != nil {
-		v1.CpuPower = v.CpuPower
-	}
-	if v.Mem != nil {
-		v1.Mem = v.Mem
-	}
-	if v.RootDisk != nil {
-		v1.RootDisk = v.RootDisk
-	}
-	if v.Tags != nil {
-		v1.Tags = v.Tags
-	}
-	return v1
-}
-
 func uintStr(i uint64) string {
 	if i == 0 {
 		return ""
@@ -183,6 +200,58 @@
 	return v.Target.String()
 }
 
+func (v *Value) fieldFromTag(tagName string) (reflect.Value, bool) {
+	fieldName := fieldNames[tagName]
+	val := reflect.ValueOf(v).Elem().FieldByName(fieldName)
+	return val, val.IsValid()
+}
+
+// attributesWithValues returns the non-zero attribute tags from the constraint.
+func (v *Value) attributesWithValues() []string {
+	var result []string = []string{}
+	for fieldTag, fieldName := range fieldNames {
+		val := reflect.ValueOf(v).Elem().FieldByName(fieldName)
+		if !val.IsNil() {
+			result = append(result, fieldTag)
+		}
+	}
+	return result
+}
+
+// hasAny returns any attrTags for which the constraint has a non-nil value.
+func (v *Value) hasAny(attrTags ...string) []string {
+	withValues := v.attributesWithValues()
+	containsFunc := func(values []string, value string) bool {
+		for _, v := range values {
+			if v == value {
+				return true
+			}
+		}
+		return false
+	}
+	var result []string = []string{}
+	for _, tag := range attrTags {
+		if containsFunc(withValues, tag) {
+			result = append(result, tag)
+		}
+	}
+	return result
+}
+
+// without returns a copy of the constraint without values for
+// the specified attributes.
+func (v *Value) without(attrTags ...string) (Value, error) {
+	result := *v
+	for _, tag := range attrTags {
+		val, ok := result.fieldFromTag(tag)
+		if !ok {
+			return Value{}, fmt.Errorf("unknown constraint %q", tag)
+		}
+		val.Set(reflect.Zero(val.Type()))
+	}
+	return result, nil
+}
+
 // setRaw interprets a name=value string and sets the supplied value.
 func (v *Value) setRaw(raw string) error {
 	eq := strings.Index(raw, "=")
@@ -192,20 +261,22 @@
 	name, str := raw[:eq], raw[eq+1:]
 	var err error
 	switch name {
-	case "arch":
+	case Arch:
 		err = v.setArch(str)
-	case "container":
+	case Container:
 		err = v.setContainer(str)
-	case "cpu-cores":
+	case CpuCores:
 		err = v.setCpuCores(str)
-	case "cpu-power":
+	case CpuPower:
 		err = v.setCpuPower(str)
-	case "mem":
+	case Mem:
 		err = v.setMem(str)
-	case "root-disk":
+	case RootDisk:
 		err = v.setRootDisk(str)
-	case "tags":
+	case Tags:
 		err = v.setTags(str)
+	case InstanceType:
+		err = v.setInstanceType(str)
 	default:
 		return fmt.Errorf("unknown constraint %q", name)
 	}
@@ -229,20 +300,22 @@
 		vstr := fmt.Sprintf("%v", val)
 		var err error
 		switch k {
-		case "arch":
+		case Arch:
 			v.Arch = &vstr
-		case "container":
+		case Container:
 			ctype := instance.ContainerType(vstr)
 			v.Container = &ctype
-		case "cpu-cores":
+		case InstanceType:
+			v.InstanceType = &vstr
+		case CpuCores:
 			v.CpuCores, err = parseUint64(vstr)
-		case "cpu-power":
+		case CpuPower:
 			v.CpuPower, err = parseUint64(vstr)
-		case "mem":
+		case Mem:
 			v.Mem, err = parseUint64(vstr)
-		case "root-disk":
+		case RootDisk:
 			v.RootDisk, err = parseUint64(vstr)
-		case "tags":
+		case Tags:
 			v.Tags, err = parseYamlTags(val)
 		default:
 			return false
@@ -303,6 +376,14 @@
 	return
 }
 
+func (v *Value) setInstanceType(str string) error {
+	if v.InstanceType != nil {
+		return fmt.Errorf("already set")
+	}
+	v.InstanceType = &str
+	return nil
+}
+
 func (v *Value) setMem(str string) (err error) {
 	if v.Mem != nil {
 		return fmt.Errorf("already set")

=== modified file 'constraints/constraints_test.go'
--- constraints/constraints_test.go	2014-04-09 16:36:12 +0000
+++ constraints/constraints_test.go	2014-04-18 13:10:25 +0000
@@ -263,13 +263,26 @@
 		args:    []string{"tags="},
 	},
 
+	// instance type
+	{
+		summary: "set instance type",
+		args:    []string{"instance-type=foo"},
+	}, {
+		summary: "instance type empty",
+		args:    []string{"instance-type="},
+	},
+
 	// Everything at once.
 	{
 		summary: "kitchen sink together",
-		args:    []string{" root-disk=8G mem=2T  arch=i386  cpu-cores=4096 cpu-power=9001 container=lxc tags=foo,bar"},
+		args: []string{
+			"root-disk=8G mem=2T  arch=i386  cpu-cores=4096 cpu-power=9001 container=lxc " +
+				"tags=foo,bar instance-type=foo"},
 	}, {
 		summary: "kitchen sink separately",
-		args:    []string{"root-disk=8G", "mem=2T", "cpu-cores=4096", "cpu-power=9001", "arch=armhf", "container=lxc", "tags=foo,bar"},
+		args: []string{
+			"root-disk=8G", "mem=2T", "cpu-cores=4096", "cpu-power=9001", "arch=armhf",
+			"container=lxc", "tags=foo,bar", "instance-type=foo"},
 	},
 }
 
@@ -321,6 +334,8 @@
 	c.Check(&con, gc.Not(jc.Satisfies), constraints.IsEmpty)
 	con = constraints.MustParse("container=")
 	c.Check(&con, gc.Not(jc.Satisfies), constraints.IsEmpty)
+	con = constraints.MustParse("instance-type=")
+	c.Check(&con, gc.Not(jc.Satisfies), constraints.IsEmpty)
 }
 
 func uint64p(i uint64) *uint64 {
@@ -363,14 +378,17 @@
 	{"Tags1", constraints.Value{Tags: nil}},
 	{"Tags2", constraints.Value{Tags: &[]string{}}},
 	{"Tags3", constraints.Value{Tags: &[]string{"foo", "bar"}}},
+	{"InstanceType1", constraints.Value{InstanceType: strp("")}},
+	{"InstanceType2", constraints.Value{InstanceType: strp("foo")}},
 	{"All", constraints.Value{
-		Arch:      strp("i386"),
-		Container: ctypep("lxc"),
-		CpuCores:  uint64p(4096),
-		CpuPower:  uint64p(9001),
-		Mem:       uint64p(18000000000),
-		RootDisk:  uint64p(24000000000),
-		Tags:      &[]string{"foo", "bar"},
+		Arch:         strp("i386"),
+		Container:    ctypep("lxc"),
+		CpuCores:     uint64p(4096),
+		CpuPower:     uint64p(9001),
+		Mem:          uint64p(18000000000),
+		RootDisk:     uint64p(24000000000),
+		Tags:         &[]string{"foo", "bar"},
+		InstanceType: strp("foo"),
 	}},
 }
 
@@ -418,128 +436,6 @@
 	}
 }
 
-var withFallbacksTests = []struct {
-	desc      string
-	initial   string
-	fallbacks string
-	final     string
-}{
-	{
-		desc: "empty all round",
-	}, {
-		desc:    "container with empty fallback",
-		initial: "container=lxc",
-		final:   "container=lxc",
-	}, {
-		desc:      "container from fallback",
-		fallbacks: "container=lxc",
-		final:     "container=lxc",
-	}, {
-		desc:    "arch with empty fallback",
-		initial: "arch=amd64",
-		final:   "arch=amd64",
-	}, {
-		desc:      "arch with ignored fallback",
-		initial:   "arch=amd64",
-		fallbacks: "arch=i386",
-		final:     "arch=amd64",
-	}, {
-		desc:      "arch from fallback",
-		fallbacks: "arch=i386",
-		final:     "arch=i386",
-	}, {
-		desc:    "cpu-cores with empty fallback",
-		initial: "cpu-cores=2",
-		final:   "cpu-cores=2",
-	}, {
-		desc:      "cpu-cores with ignored fallback",
-		initial:   "cpu-cores=4",
-		fallbacks: "cpu-cores=8",
-		final:     "cpu-cores=4",
-	}, {
-		desc:      "cpu-cores from fallback",
-		fallbacks: "cpu-cores=8",
-		final:     "cpu-cores=8",
-	}, {
-		desc:    "cpu-power with empty fallback",
-		initial: "cpu-power=100",
-		final:   "cpu-power=100",
-	}, {
-		desc:      "cpu-power with ignored fallback",
-		initial:   "cpu-power=100",
-		fallbacks: "cpu-power=200",
-		final:     "cpu-power=100",
-	}, {
-		desc:      "cpu-power from fallback",
-		fallbacks: "cpu-power=200",
-		final:     "cpu-power=200",
-	}, {
-		desc:    "tags with empty fallback",
-		initial: "tags=foo,bar",
-		final:   "tags=foo,bar",
-	}, {
-		desc:      "tags with ignored fallback",
-		initial:   "tags=foo,bar",
-		fallbacks: "tags=baz",
-		final:     "tags=foo,bar",
-	}, {
-		desc:      "tags from fallback",
-		fallbacks: "tags=foo,bar",
-		final:     "tags=foo,bar",
-	}, {
-		desc:      "tags inital empty",
-		initial:   "tags=",
-		fallbacks: "tags=foo,bar",
-		final:     "tags=",
-	}, {
-		desc:    "mem with empty fallback",
-		initial: "mem=4G",
-		final:   "mem=4G",
-	}, {
-		desc:      "mem with ignored fallback",
-		initial:   "mem=4G",
-		fallbacks: "mem=8G",
-		final:     "mem=4G",
-	}, {
-		desc:      "mem from fallback",
-		fallbacks: "mem=8G",
-		final:     "mem=8G",
-	}, {
-		desc:    "root-disk with empty fallback",
-		initial: "root-disk=4G",
-		final:   "root-disk=4G",
-	}, {
-		desc:      "root-disk with ignored fallback",
-		initial:   "root-disk=4G",
-		fallbacks: "root-disk=8G",
-		final:     "root-disk=4G",
-	}, {
-		desc:      "root-disk from fallback",
-		fallbacks: "root-disk=8G",
-		final:     "root-disk=8G",
-	}, {
-		desc:      "non-overlapping mix",
-		initial:   "root-disk=8G mem=4G arch=amd64",
-		fallbacks: "cpu-power=1000 cpu-cores=4",
-		final:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
-	}, {
-		desc:      "overlapping mix",
-		initial:   "root-disk=8G mem=4G arch=amd64",
-		fallbacks: "cpu-power=1000 cpu-cores=4 mem=8G",
-		final:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
-	},
-}
-
-func (s *ConstraintsSuite) TestWithFallbacks(c *gc.C) {
-	for i, t := range withFallbacksTests {
-		c.Logf("test %d: %s", i, t.desc)
-		initial := constraints.MustParse(t.initial)
-		fallbacks := constraints.MustParse(t.fallbacks)
-		final := constraints.MustParse(t.final)
-		c.Check(initial.WithFallbacks(fallbacks), gc.DeepEquals, final)
-	}
-}
-
 var hasContainerTests = []struct {
 	constraints  string
 	hasContainer bool
@@ -562,3 +458,128 @@
 		c.Check(cons.HasContainer(), gc.Equals, t.hasContainer)
 	}
 }
+
+func (s *ConstraintsSuite) TestHasInstanceType(c *gc.C) {
+	cons := constraints.MustParse("arch=amd64")
+	c.Check(cons.HasInstanceType(), jc.IsFalse)
+	cons = constraints.MustParse("arch=amd64 instance-type=foo")
+	c.Check(cons.HasInstanceType(), jc.IsTrue)
+}
+
+var withoutTests = []struct {
+	initial string
+	without []string
+	final   string
+}{
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"root-disk"},
+		final:   "mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"mem"},
+		final:   "root-disk=8G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"arch"},
+		final:   "root-disk=8G mem=4G cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"cpu-power"},
+		final:   "root-disk=8G mem=4G arch=amd64 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"cpu-cores"},
+		final:   "root-disk=8G mem=4G arch=amd64 cpu-power=1000 tags=foo container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"tags"},
+		final:   "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 container=lxc instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"container"},
+		final:   "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo instance-type=bar",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"instance-type"},
+		final:   "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc",
+	},
+	{
+		initial: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+		without: []string{"root-disk", "mem", "arch"},
+		final:   "cpu-power=1000 cpu-cores=4 tags=foo container=lxc instance-type=bar",
+	},
+}
+
+func (s *ConstraintsSuite) TestWithout(c *gc.C) {
+	for i, t := range withoutTests {
+		c.Logf("test %d", i)
+		initial := constraints.MustParse(t.initial)
+		final, err := constraints.Without(initial, t.without...)
+		c.Assert(err, gc.IsNil)
+		c.Check(final, gc.DeepEquals, constraints.MustParse(t.final))
+	}
+}
+
+func (s *ConstraintsSuite) TestWithoutError(c *gc.C) {
+	cons := constraints.MustParse("mem=4G")
+	_, err := constraints.Without(cons, "foo")
+	c.Assert(err, gc.ErrorMatches, `unknown constraint "foo"`)
+}
+
+var attributesWithValuesTests = []struct {
+	cons     string
+	attrs    []string
+	expected []string
+}{
+	{
+		cons:     "",
+		expected: []string{},
+	},
+	{
+		cons:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+		expected: []string{"arch", "cpu-cores", "cpu-power", "mem", "root-disk"},
+	},
+}
+
+func (s *ConstraintsSuite) TestAttributesWithValues(c *gc.C) {
+	for i, t := range attributesWithValuesTests {
+		c.Logf("test %d", i)
+		cons := constraints.MustParse(t.cons)
+		obtained := constraints.AttributesWithValues(cons)
+		c.Check(obtained, gc.DeepEquals, t.expected)
+	}
+}
+
+var hasAnyTests = []struct {
+	cons     string
+	attrs    []string
+	expected []string
+}{
+	{
+		cons:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+		attrs:    []string{"root-disk", "tags", "mem"},
+		expected: []string{"root-disk", "mem"},
+	},
+	{
+		cons:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+		attrs:    []string{"tags"},
+		expected: []string{},
+	},
+}
+
+func (s *ConstraintsSuite) TestHasAny(c *gc.C) {
+	for i, t := range hasAnyTests {
+		c.Logf("test %d", i)
+		cons := constraints.MustParse(t.cons)
+		obtained := constraints.HasAny(cons, t.attrs...)
+		c.Check(obtained, gc.DeepEquals, t.expected)
+	}
+}

=== added file 'constraints/export_test.go'
--- constraints/export_test.go	1970-01-01 00:00:00 +0000
+++ constraints/export_test.go	2014-04-18 13:10:25 +0000
@@ -0,0 +1,18 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package constraints
+
+var WithFallbacks = withFallbacks
+
+func Without(cons Value, attrTags ...string) (Value, error) {
+	return cons.without(attrTags...)
+}
+
+func HasAny(cons Value, attrTags ...string) []string {
+	return cons.hasAny(attrTags...)
+}
+
+func AttributesWithValues(cons Value) []string {
+	return cons.attributesWithValues()
+}

=== added file 'constraints/validation.go'
--- constraints/validation.go	1970-01-01 00:00:00 +0000
+++ constraints/validation.go	2014-04-18 13:10:25 +0000
@@ -0,0 +1,134 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package constraints
+
+import (
+	"fmt"
+	"reflect"
+
+	"launchpad.net/juju-core/utils/set"
+)
+
+// Validator defines operations on constraints attributes which are
+// used to ensure a constraints value is valid, as well as being able
+// to handle overridden attributes.
+type Validator interface {
+
+	// RegisterConflicts is used to define cross-constraint override behaviour.
+	// The red and blue attribute lists contain attribute names which conflict
+	// with those in the other list.
+	// When two constraints conflict:
+	//  it is an error to set both constraints in the same constraints Value.
+	//  when a constraints Value overrides another which specifies a conflicting
+	//   attribute, the attribute in the overridden Value is cleared.
+	RegisterConflicts(reds, blues []string)
+
+	// RegisterUnsupported records attributes which are not supported by a constraints Value.
+	RegisterUnsupported(unsupported []string)
+
+	// Validate returns an error if the given constraints are not valid, and also
+	// any unsupported attributes.
+	Validate(cons Value) ([]string, error)
+
+	// Merge merges cons into consFallback, with any conflicting attributes from cons
+	// overriding those from consFallback.
+	Merge(consFallback, cons Value) (Value, error)
+}
+
+// NewValidator returns a new constraints Validator instance.
+func NewValidator() Validator {
+	c := validator{}
+	c.conflicts = make(map[string]set.Strings)
+	return &c
+}
+
+type validator struct {
+	unsupported set.Strings
+	conflicts   map[string]set.Strings
+}
+
+// RegisterConflicts is defined on Validator.
+func (v *validator) RegisterConflicts(reds, blues []string) {
+	for _, red := range reds {
+		v.conflicts[red] = set.NewStrings(blues...)
+	}
+	for _, blue := range blues {
+		v.conflicts[blue] = set.NewStrings(reds...)
+	}
+}
+
+// RegisterUnsupported is defined on Validator.
+func (v *validator) RegisterUnsupported(unsupported []string) {
+	v.unsupported = set.NewStrings(unsupported...)
+}
+
+// checkConflicts returns an error if the constraints Value contains conflicting attributes.
+func (v *validator) checkConflicts(cons Value) error {
+	attrNames := cons.attributesWithValues()
+	attrSet := set.NewStrings(attrNames...)
+	for _, attr := range attrNames {
+		conflicts, ok := v.conflicts[attr]
+		if !ok {
+			continue
+		}
+		for _, conflict := range conflicts.Values() {
+			if attrSet.Contains(conflict) {
+				return fmt.Errorf("ambiguous constraints: %q overlaps with %q", attr, conflict)
+			}
+		}
+	}
+	return nil
+}
+
+// checkUnsupported returns any unsupported attributes.
+func (v *validator) checkUnsupported(cons Value) []string {
+	return cons.hasAny(v.unsupported.Values()...)
+}
+
+// withFallbacks returns a copy of v with nil values taken from vFallback.
+func withFallbacks(v Value, vFallback Value) Value {
+	result := vFallback
+	for _, fieldName := range fieldNames {
+		resultVal := reflect.ValueOf(&result).Elem().FieldByName(fieldName)
+		val := reflect.ValueOf(&v).Elem().FieldByName(fieldName)
+		if !val.IsNil() {
+			resultVal.Set(val)
+		}
+	}
+	return result
+}
+
+// Validate is defined on Validator.
+func (v *validator) Validate(cons Value) ([]string, error) {
+	unsupported := v.checkUnsupported(cons)
+	if err := v.checkConflicts(cons); err != nil {
+		return unsupported, err
+	}
+	return unsupported, nil
+}
+
+// Merge is defined on Validator.
+func (v *validator) Merge(consFallback, cons Value) (Value, error) {
+	// First ensure both constraints are valid. We don't care if there
+	// are constraint attributes that are unsupported.
+	if _, err := v.Validate(consFallback); err != nil {
+		return Value{}, err
+	}
+	if _, err := v.Validate(cons); err != nil {
+		return Value{}, err
+	}
+	// Gather any attributes from consFallback which conflict with those on cons.
+	attrs := cons.attributesWithValues()
+	var fallbackConflicts []string
+	for _, attr := range attrs {
+		fallbackConflicts = append(fallbackConflicts, v.conflicts[attr].Values()...)
+	}
+	// Null out the conflicting consFallback attribute values because
+	// cons takes priority. We can't error here because we
+	// know that aConflicts contains valid attr names.
+	consFallbackMinusConflicts, _ := consFallback.without(fallbackConflicts...)
+	// The result is cons with fallbacks coming from any
+	// non conflicting consFallback attributes.
+	return withFallbacks(cons, consFallbackMinusConflicts), nil
+}

=== added file 'constraints/validation_test.go'
--- constraints/validation_test.go	1970-01-01 00:00:00 +0000
+++ constraints/validation_test.go	2014-04-18 13:10:25 +0000
@@ -0,0 +1,296 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package constraints_test
+
+import (
+	jc "github.com/juju/testing/checkers"
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/constraints"
+)
+
+type validationSuite struct{}
+
+var _ = gc.Suite(&validationSuite{})
+
+var validationTests = []struct {
+	cons        string
+	unsupported []string
+	reds        []string
+	blues       []string
+	err         string
+}{
+	{
+		cons: "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+	},
+	{
+		cons:        "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 tags=foo",
+		unsupported: []string{"tags"},
+	},
+	{
+		cons:        "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4 instance-type=foo",
+		unsupported: []string{"cpu-power", "instance-type"},
+	},
+	{
+		// Ambiguous constraint errors take precedence over unsupported errors.
+		cons:        "root-disk=8G mem=4G cpu-cores=4 instance-type=foo",
+		reds:        []string{"mem", "arch"},
+		blues:       []string{"instance-type"},
+		unsupported: []string{"cpu-cores"},
+		err:         `ambiguous constraints: "mem" overlaps with "instance-type"`,
+	},
+	{
+		cons: "root-disk=8G mem=4G arch=amd64 cpu-cores=4 instance-type=foo",
+		reds: []string{"mem", "arch"},
+		err:  "",
+	},
+	{
+		cons:  "root-disk=8G mem=4G arch=amd64 cpu-cores=4 instance-type=foo",
+		blues: []string{"mem", "arch"},
+		err:   "",
+	},
+	{
+		cons:  "root-disk=8G mem=4G arch=amd64 cpu-cores=4 instance-type=foo",
+		reds:  []string{"mem", "arch"},
+		blues: []string{"instance-type"},
+		err:   `ambiguous constraints: "arch" overlaps with "instance-type"`,
+	},
+	{
+		cons:  "root-disk=8G mem=4G arch=amd64 cpu-cores=4 instance-type=foo",
+		reds:  []string{"instance-type"},
+		blues: []string{"mem", "arch"},
+		err:   `ambiguous constraints: "arch" overlaps with "instance-type"`,
+	},
+	{
+		cons:  "root-disk=8G mem=4G cpu-cores=4 instance-type=foo",
+		reds:  []string{"mem", "arch"},
+		blues: []string{"instance-type"},
+		err:   `ambiguous constraints: "mem" overlaps with "instance-type"`,
+	},
+}
+
+func (s *validationSuite) TestValidation(c *gc.C) {
+	for i, t := range validationTests {
+		c.Logf("test %d", i)
+		validator := constraints.NewValidator()
+		validator.RegisterUnsupported(t.unsupported)
+		validator.RegisterConflicts(t.reds, t.blues)
+		cons := constraints.MustParse(t.cons)
+		unsupported, err := validator.Validate(cons)
+		if t.err == "" {
+			c.Assert(err, gc.IsNil)
+			c.Assert(unsupported, jc.DeepEquals, t.unsupported)
+		} else {
+			c.Assert(err, gc.ErrorMatches, t.err)
+		}
+	}
+}
+
+var mergeTests = []struct {
+	desc         string
+	consFallback string
+	cons         string
+	unsupported  []string
+	reds         []string
+	blues        []string
+	expected     string
+}{
+	{
+		desc: "empty all round",
+	}, {
+		desc:     "container with empty fallback",
+		cons:     "container=lxc",
+		expected: "container=lxc",
+	}, {
+		desc:         "container from fallback",
+		consFallback: "container=lxc",
+		expected:     "container=lxc",
+	}, {
+		desc:     "arch with empty fallback",
+		cons:     "arch=amd64",
+		expected: "arch=amd64",
+	}, {
+		desc:         "arch with ignored fallback",
+		cons:         "arch=amd64",
+		consFallback: "arch=i386",
+		expected:     "arch=amd64",
+	}, {
+		desc:         "arch from fallback",
+		consFallback: "arch=i386",
+		expected:     "arch=i386",
+	}, {
+		desc:     "instance type with empty fallback",
+		cons:     "instance-type=foo",
+		expected: "instance-type=foo",
+	}, {
+		desc:         "instance type with ignored fallback",
+		cons:         "instance-type=foo",
+		consFallback: "instance-type=bar",
+		expected:     "instance-type=foo",
+	}, {
+		desc:         "instance type from fallback",
+		consFallback: "instance-type=foo",
+		expected:     "instance-type=foo",
+	}, {
+		desc:     "cpu-cores with empty fallback",
+		cons:     "cpu-cores=2",
+		expected: "cpu-cores=2",
+	}, {
+		desc:         "cpu-cores with ignored fallback",
+		cons:         "cpu-cores=4",
+		consFallback: "cpu-cores=8",
+		expected:     "cpu-cores=4",
+	}, {
+		desc:         "cpu-cores from fallback",
+		consFallback: "cpu-cores=8",
+		expected:     "cpu-cores=8",
+	}, {
+		desc:     "cpu-power with empty fallback",
+		cons:     "cpu-power=100",
+		expected: "cpu-power=100",
+	}, {
+		desc:         "cpu-power with ignored fallback",
+		cons:         "cpu-power=100",
+		consFallback: "cpu-power=200",
+		expected:     "cpu-power=100",
+	}, {
+		desc:         "cpu-power from fallback",
+		consFallback: "cpu-power=200",
+		expected:     "cpu-power=200",
+	}, {
+		desc:     "tags with empty fallback",
+		cons:     "tags=foo,bar",
+		expected: "tags=foo,bar",
+	}, {
+		desc:         "tags with ignored fallback",
+		cons:         "tags=foo,bar",
+		consFallback: "tags=baz",
+		expected:     "tags=foo,bar",
+	}, {
+		desc:         "tags from fallback",
+		consFallback: "tags=foo,bar",
+		expected:     "tags=foo,bar",
+	}, {
+		desc:         "tags inital empty",
+		cons:         "tags=",
+		consFallback: "tags=foo,bar",
+		expected:     "tags=",
+	}, {
+		desc:     "mem with empty fallback",
+		cons:     "mem=4G",
+		expected: "mem=4G",
+	}, {
+		desc:         "mem with ignored fallback",
+		cons:         "mem=4G",
+		consFallback: "mem=8G",
+		expected:     "mem=4G",
+	}, {
+		desc:         "mem from fallback",
+		consFallback: "mem=8G",
+		expected:     "mem=8G",
+	}, {
+		desc:     "root-disk with empty fallback",
+		cons:     "root-disk=4G",
+		expected: "root-disk=4G",
+	}, {
+		desc:         "root-disk with ignored fallback",
+		cons:         "root-disk=4G",
+		consFallback: "root-disk=8G",
+		expected:     "root-disk=4G",
+	}, {
+		desc:         "root-disk from fallback",
+		consFallback: "root-disk=8G",
+		expected:     "root-disk=8G",
+	}, {
+		desc:         "non-overlapping mix",
+		cons:         "root-disk=8G mem=4G arch=amd64",
+		consFallback: "cpu-power=1000 cpu-cores=4",
+		expected:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+	}, {
+		desc:         "overlapping mix",
+		cons:         "root-disk=8G mem=4G arch=amd64",
+		consFallback: "cpu-power=1000 cpu-cores=4 mem=8G",
+		expected:     "root-disk=8G mem=4G arch=amd64 cpu-power=1000 cpu-cores=4",
+	}, {
+		desc:         "fallback only, no conflicts",
+		consFallback: "root-disk=8G cpu-cores=4 instance-type=foo",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 instance-type=foo",
+	}, {
+		desc:     "no fallback, no conflicts",
+		cons:     "root-disk=8G cpu-cores=4 instance-type=foo",
+		reds:     []string{"mem", "arch"},
+		blues:    []string{"instance-type"},
+		expected: "root-disk=8G cpu-cores=4 instance-type=foo",
+	}, {
+		desc:         "conflict value from override",
+		consFallback: "root-disk=8G instance-type=foo",
+		cons:         "root-disk=8G cpu-cores=4 instance-type=bar",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 instance-type=bar",
+	}, {
+		desc:         "unsupported attributes ignored",
+		consFallback: "root-disk=8G instance-type=foo",
+		cons:         "root-disk=8G cpu-cores=4 instance-type=bar",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		unsupported:  []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 instance-type=bar",
+	}, {
+		desc:         "red conflict masked from fallback",
+		consFallback: "root-disk=8G mem=4G",
+		cons:         "root-disk=8G cpu-cores=4 instance-type=bar",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 instance-type=bar",
+	}, {
+		desc:         "second red conflict masked from fallback",
+		consFallback: "root-disk=8G arch=amd64",
+		cons:         "root-disk=8G cpu-cores=4 instance-type=bar",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 instance-type=bar",
+	}, {
+		desc:         "blue conflict masked from fallback",
+		consFallback: "root-disk=8G cpu-cores=4 instance-type=bar",
+		cons:         "root-disk=8G mem=4G",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 mem=4G",
+	}, {
+		desc:         "both red conflicts used, blue mased from fallback",
+		consFallback: "root-disk=8G cpu-cores=4 instance-type=bar",
+		cons:         "root-disk=8G arch=amd64 mem=4G",
+		reds:         []string{"mem", "arch"},
+		blues:        []string{"instance-type"},
+		expected:     "root-disk=8G cpu-cores=4 arch=amd64 mem=4G",
+	},
+}
+
+func (s *validationSuite) TestMerge(c *gc.C) {
+	for i, t := range mergeTests {
+		c.Logf("test %d: %s", i, t.desc)
+		validator := constraints.NewValidator()
+		validator.RegisterConflicts(t.reds, t.blues)
+		consFallback := constraints.MustParse(t.consFallback)
+		cons := constraints.MustParse(t.cons)
+		merged, err := validator.Merge(consFallback, cons)
+		c.Assert(err, gc.IsNil)
+		expected := constraints.MustParse(t.expected)
+		c.Check(merged, gc.DeepEquals, expected)
+	}
+}
+
+func (s *validationSuite) TestMergeError(c *gc.C) {
+	validator := constraints.NewValidator()
+	validator.RegisterConflicts([]string{"instance-type"}, []string{"mem"})
+	consFallback := constraints.MustParse("instance-type=foo mem=4G")
+	cons := constraints.MustParse("cpu-cores=2")
+	_, err := validator.Merge(consFallback, cons)
+	c.Assert(err, gc.ErrorMatches, `ambiguous constraints: "mem" overlaps with "instance-type"`)
+	_, err = validator.Merge(cons, consFallback)
+	c.Assert(err, gc.ErrorMatches, `ambiguous constraints: "mem" overlaps with "instance-type"`)
+}

=== modified file 'state/addmachine.go'
--- state/addmachine.go	2014-04-17 12:53:23 +0000
+++ state/addmachine.go	2014-04-18 13:10:25 +0000
@@ -191,7 +191,11 @@
 	if err != nil {
 		return MachineTemplate{}, err
 	}
-	p.Constraints = p.Constraints.WithFallbacks(cons)
+	validator := constraints.NewValidator()
+	p.Constraints, err = validator.Merge(cons, p.Constraints)
+	if err != nil {
+		return MachineTemplate{}, err
+	}
 	// Machine constraints do not use a container constraint value.
 	// Both provisioning and deployment constraints use the same
 	// constraints.Value struct so here we clear the container

=== modified file 'state/prechecker_test.go'
--- state/prechecker_test.go	2014-04-14 12:36:13 +0000
+++ state/prechecker_test.go	2014-04-18 13:10:25 +0000
@@ -51,7 +51,9 @@
 	template, err := s.addOneMachine(c, envCons)
 	c.Assert(err, gc.IsNil)
 	c.Assert(s.prechecker.precheckInstanceSeries, gc.Equals, template.Series)
-	cons := template.Constraints.WithFallbacks(envCons)
+	validator := constraints.NewValidator()
+	cons, err := validator.Merge(envCons, template.Constraints)
+	c.Assert(err, gc.IsNil)
 	c.Assert(s.prechecker.precheckInstanceConstraints, gc.DeepEquals, cons)
 }
 

=== modified file 'state/service.go'
--- state/service.go	2014-04-17 12:47:50 +0000
+++ state/service.go	2014-04-18 13:10:25 +0000
@@ -598,7 +598,11 @@
 		if err != nil {
 			return "", nil, err
 		}
-		cons := scons.WithFallbacks(econs)
+		validator := constraints.NewValidator()
+		cons, err := validator.Merge(econs, scons)
+		if err != nil {
+			return "", nil, err
+		}
 		ops = append(ops, createConstraintsOp(s.st, globalKey, cons))
 	}
 	return name, ops, nil