From a2c8ed318486ac3a3a2d590c11845f345c29c748 Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 12:03:36 +1200
Subject: [PATCH 1/6] Rework allocate with new storage and interfaces
 constraints.

---
 controller.go      | 121 +++++++++++++++++++++++++++++++++++++--------
 controller_test.go |  29 +++++++----
 errors.go          |   9 +++-
 interfaces.go      |   5 +-
 machine.go         |  10 ++++
 machine_test.go    |   3 ++
 6 files changed, 146 insertions(+), 31 deletions(-)

diff --git a/controller.go b/controller.go
index f2d246d..7c6adcf 100644
--- a/controller.go
+++ b/controller.go
@@ -286,26 +286,56 @@ type AllocateMachineArgs struct {
 	MinMemory int
 	Tags      []string
 	NotTags   []string
-	// Networks - list of networks (defined in MAAS) to which the machine must be
-	// attached. A network can be identified by the name assigned to it in MAAS;
-	// or by an ip: prefix followed by any IP address that falls within the
-	// network; or a vlan: prefix followed by a numeric VLAN tag, e.g. vlan:23
-	// for VLAN number 23. Valid VLAN tags must be in the range of 1 to 4094
-	// inclusive.
-	Networks    []string
-	NotNetworks []string
-	Zone        string
-	NotInZone   []string
-	AgentName   string
-	Comment     string
-	DryRun      bool
+	Zone      string
+	NotInZone []string
+	// Storage is an optional value listing one or more storage constraints.
+	// Format is an optional label, followed by an optional colon, then size
+	// (which is mandatory) followed by an optional comma seperated list of tags
+	// in parentheses.
+	//
+	//   Examples:
+	//
+	//   200(ssd,removable),400(ssd),300
+	//   - 200GB disk with ssd and removable tag
+	//   - 400GB disk with ssd tag
+	//   - 300GB disk
+	//
+	//  root:80(ssd),data:400
+	//  - 80+GB disk with ssd tag, name the constraint "root"
+	//  - 400+GB disk, name the consrtaint "data"
+	Storage string
+	// Interfaces is an optional value listing one or more interface constraints.
+	// Format of the string is semi-colon delimited labelled constraints. Each
+	// labelled constraint has the format:
+	//   <label>:<key>=<value>[,<key2>=<value2>[,...]]'
+	// The label is used in the ConstraintMatch structure returned from
+	// AllocateMachine. Valid keys are:
+	//     'space', 'not_space', 'fabric_class', 'not_fabric_class',
+	//     'subnet_cidr', 'not_subnet_cidr', 'vid', 'not_vid',
+	//     'fabric', 'not_fabric', 'subnet', 'not_subnet', 'mode'
+	Interfaces string
+	AgentName  string
+	Comment    string
+	DryRun     bool
+}
+
+// ConstraintMatches provides a way for the caller of AllocateMachine to determine
+//.how the allocated machine matched the storage and interfaces constraints specified.
+// The labels that were used in the constraints are the keys in the maps.
+type ConstraintMatches struct {
+	// Storage matches will come when we support interfaces for the block devices.
+
+	// Interface is a mapping of the constraint label specified to the Interface
+	// that matches that constraint.
+	Interfaces map[string]Interface
 }
 
 // AllocateMachine implements Controller.
 //
 // Returns an error that satisfies IsNoMatchError if the requested
 // constraints cannot be met.
-func (c *controller) AllocateMachine(args AllocateMachineArgs) (Machine, error) {
+func (c *controller) AllocateMachine(args AllocateMachineArgs) (Machine, ConstraintMatches, error) {
+	var matches ConstraintMatches
 	params := NewURLParams()
 	params.MaybeAdd("name", args.Hostname)
 	params.MaybeAdd("arch", args.Architecture)
@@ -313,8 +343,8 @@ func (c *controller) AllocateMachine(args AllocateMachineArgs) (Machine, error)
 	params.MaybeAddInt("mem", args.MinMemory)
 	params.MaybeAddMany("tags", args.Tags)
 	params.MaybeAddMany("not_tags", args.NotTags)
-	params.MaybeAddMany("networks", args.Networks)
-	params.MaybeAddMany("not_networks", args.NotNetworks)
+	params.MaybeAdd("storage", args.Storage)
+	params.MaybeAdd("interfaces", args.Interfaces)
 	params.MaybeAdd("zone", args.Zone)
 	params.MaybeAddMany("not_in_zone", args.NotInZone)
 	params.MaybeAdd("agent_name", args.AgentName)
@@ -325,19 +355,26 @@ func (c *controller) AllocateMachine(args AllocateMachineArgs) (Machine, error)
 		// A 409 Status code is "No Matching Machines"
 		if svrErr, ok := errors.Cause(err).(ServerError); ok {
 			if svrErr.StatusCode == http.StatusConflict {
-				return nil, errors.Wrap(err, NewNoMatchError(svrErr.BodyMessage))
+				return nil, matches, errors.Wrap(err, NewNoMatchError(svrErr.BodyMessage))
 			}
 		}
 		// Translate http errors.
-		return nil, NewUnexpectedError(err)
+		return nil, matches, NewUnexpectedError(err)
 	}
 
 	machine, err := readMachine(c.apiVersion, result)
 	if err != nil {
-		return nil, errors.Trace(err)
+		return nil, matches, errors.Trace(err)
 	}
 	machine.controller = c
-	return machine, nil
+
+	// Parse the constraint matches.
+	matches, err = parseAllocateConstraintsResponse(result, machine)
+	if err != nil {
+		return nil, matches, errors.Trace(err)
+	}
+
+	return machine, matches, nil
 }
 
 // ReleaseMachinesArgs is an argument struct for passing the machine system IDs
@@ -618,3 +655,47 @@ func (c *controller) readAPIVersion(apiVersion version.Number) (set.Strings, ver
 
 	return capabilities, apiVersion, nil
 }
+
+func parseAllocateConstraintsResponse(source interface{}, machine *machine) (ConstraintMatches, error) {
+	var empty ConstraintMatches
+	// Do the schema check in two passes.
+	fields := schema.Fields{
+		"constraints_by_type": schema.StringMap(schema.Any()),
+	}
+	checker := schema.FieldMap(fields, nil) // no defaults
+	coerced, err := checker.Coerce(source, nil)
+	if err != nil {
+		return empty, WrapWithDeserializationError(err, "allocation constraints response schema check failed")
+	}
+	constraintsMap := coerced.(map[string]interface{})["constraints_by_type"]
+	fields = schema.Fields{
+		"storage":    schema.StringMap(schema.ForceInt()),
+		"interfaces": schema.StringMap(schema.ForceInt()),
+	}
+	defaults := schema.Defaults{
+		"storage":    schema.Omit,
+		"interfaces": schema.Omit,
+	}
+	checker = schema.FieldMap(fields, defaults)
+	coerced, err = checker.Coerce(constraintsMap, nil)
+	if err != nil {
+		return empty, WrapWithDeserializationError(err, "allocation constraints response schema check failed")
+	}
+	valid := coerced.(map[string]interface{})
+	result := ConstraintMatches{
+		// TODO: make storage map
+		Interfaces: make(map[string]Interface),
+	}
+	// TODO: handle storage when we export block devices.
+	if interfaceMatches, found := valid["interfaces"]; found {
+		for label, value := range interfaceMatches.(map[string]interface{}) {
+			id := int(value.(int64))
+			iface := machine.Interface(id)
+			if iface == nil {
+				return empty, NewDeserializationError("constraint match interface %q: %d does not match an interface for the machine", label, id)
+			}
+			result.Interfaces[label] = iface
+		}
+	}
+	return result, nil
+}
diff --git a/controller_test.go b/controller_test.go
index 20999cb..a830502 100644
--- a/controller_test.go
+++ b/controller_test.go
@@ -39,7 +39,7 @@ var _ = gc.Suite(&controllerSuite{})
 
 func (s *controllerSuite) SetUpTest(c *gc.C) {
 	s.LoggingCleanupSuite.SetUpTest(c)
-	loggo.GetLogger("").SetLogLevel(loggo.DEBUG)
+	loggo.GetLogger("").SetLogLevel(loggo.TRACE)
 
 	server := NewSimpleServer()
 	server.AddGetResponse("/api/2.0/boot-resources/", http.StatusOK, bootResourcesResponse)
@@ -264,16 +264,27 @@ func (s *controllerSuite) TestMachinesArgs(c *gc.C) {
 	c.Assert(request.URL.Query(), gc.HasLen, 6)
 }
 
+func (s *controllerSuite) addAllocateReponse(c *gc.C, status int, interfaceMatches map[string]int) {
+	constraints := make(map[string]interface{})
+	if interfaceMatches != nil {
+		constraints["interfaces"] = interfaceMatches
+	}
+	allocateJSON := updateJSONMap(c, machineResponse, map[string]interface{}{
+		"constraints_by_type": constraints,
+	})
+	s.server.AddPostResponse("/api/2.0/machines/?op=allocate", status, allocateJSON)
+}
+
 func (s *controllerSuite) TestAllocateMachine(c *gc.C) {
-	s.server.AddPostResponse("/api/2.0/machines/?op=allocate", http.StatusOK, machineResponse)
+	s.addAllocateReponse(c, http.StatusOK, nil)
 	controller := s.getController(c)
-	machine, err := controller.AllocateMachine(AllocateMachineArgs{})
+	machine, _, err := controller.AllocateMachine(AllocateMachineArgs{})
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(machine.SystemID(), gc.Equals, "4y3ha3")
 }
 
 func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
-	s.server.AddPostResponse("/api/2.0/machines/?op=allocate", http.StatusOK, machineResponse)
+	s.addAllocateReponse(c, http.StatusOK, nil)
 	controller := s.getController(c)
 	// Create an arg structure that sets all the values.
 	args := AllocateMachineArgs{
@@ -283,15 +294,15 @@ func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
 		MinMemory:    20000,
 		Tags:         []string{"good"},
 		NotTags:      []string{"bad"},
-		Networks:     []string{"fast"},
-		NotNetworks:  []string{"slow"},
+		Storage:      "root:200(ssd)",
+		Interfaces:   "default:vid=1;db:space=magic",
 		Zone:         "magic",
 		NotInZone:    []string{"not-magic"},
 		AgentName:    "agent 42",
 		Comment:      "testing",
 		DryRun:       true,
 	}
-	_, err := controller.AllocateMachine(args)
+	_, _, err := controller.AllocateMachine(args)
 	c.Assert(err, jc.ErrorIsNil)
 
 	request := s.server.LastRequest()
@@ -302,14 +313,14 @@ func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
 func (s *controllerSuite) TestAllocateMachineNoMatch(c *gc.C) {
 	s.server.AddPostResponse("/api/2.0/machines/?op=allocate", http.StatusConflict, "boo")
 	controller := s.getController(c)
-	_, err := controller.AllocateMachine(AllocateMachineArgs{})
+	_, _, err := controller.AllocateMachine(AllocateMachineArgs{})
 	c.Assert(err, jc.Satisfies, IsNoMatchError)
 }
 
 func (s *controllerSuite) TestAllocateMachineUnexpected(c *gc.C) {
 	s.server.AddPostResponse("/api/2.0/machines/?op=allocate", http.StatusBadRequest, "boo")
 	controller := s.getController(c)
-	_, err := controller.AllocateMachine(AllocateMachineArgs{})
+	_, _, err := controller.AllocateMachine(AllocateMachineArgs{})
 	c.Assert(err, jc.Satisfies, IsUnexpectedError)
 }
 
diff --git a/errors.go b/errors.go
index 9ede2f8..8931d56 100644
--- a/errors.go
+++ b/errors.go
@@ -87,7 +87,14 @@ func WrapWithDeserializationError(err error, format string, args ...interface{})
 	// previous error, but wrap it in the new type.
 	derr := &DeserializationError{Err: errors.NewErr(message + ": " + err.Error())}
 	derr.SetLocation(1)
-	return errors.Wrap(err, derr)
+	wrapped := errors.Wrap(err, derr)
+	// We want the location of the wrapped error to be the caller of this function,
+	// not the line above.
+	if errType, ok := wrapped.(*errors.Err); ok {
+		// We know it is because that is what Wrap returns.
+		errType.SetLocation(1)
+	}
+	return wrapped
 }
 
 // IsDeserializationError returns true if err is a DeserializationError.
diff --git a/interfaces.go b/interfaces.go
index ef32ac1..649d617 100644
--- a/interfaces.go
+++ b/interfaces.go
@@ -40,7 +40,7 @@ type Controller interface {
 
 	// AllocateMachine will attempt to allocate a machine to the user.
 	// If successful, the allocated machine is returned.
-	AllocateMachine(AllocateMachineArgs) (Machine, error)
+	AllocateMachine(AllocateMachineArgs) (Machine, ConstraintMatches, error)
 
 	// ReleaseMachines will stop the specified machines, and release them
 	// from the user making them available to be allocated again.
@@ -191,6 +191,9 @@ type Machine interface {
 	BootInterface() Interface
 	// InterfaceSet returns all the interfaces for the Machine.
 	InterfaceSet() []Interface
+	// Interface returns the interface for the machine that matches the id
+	// specified. If there is no match, nil is returned.
+	Interface(id int) Interface
 
 	Zone() Zone
 
diff --git a/machine.go b/machine.go
index 383b71e..940ccfa 100644
--- a/machine.go
+++ b/machine.go
@@ -116,6 +116,16 @@ func (m *machine) InterfaceSet() []Interface {
 	return result
 }
 
+// Interface implements Machine.
+func (m *machine) Interface(id int) Interface {
+	for _, iface := range m.interfaceSet {
+		if iface.ID() == id {
+			return iface
+		}
+	}
+	return nil
+}
+
 // OperatingSystem implements Machine.
 func (m *machine) OperatingSystem() string {
 	return m.operatingSystem
diff --git a/machine_test.go b/machine_test.go
index c7cb0f8..a216627 100644
--- a/machine_test.go
+++ b/machine_test.go
@@ -62,6 +62,9 @@ func (*machineSuite) TestReadMachines(c *gc.C) {
 
 	interfaceSet := machine.InterfaceSet()
 	c.Assert(interfaceSet, gc.HasLen, 1)
+	id := interfaceSet[0].ID()
+	c.Assert(machine.Interface(id), jc.DeepEquals, interfaceSet[0])
+	c.Assert(machine.Interface(id+5), gc.IsNil)
 }
 
 func (*machineSuite) TestLowVersion(c *gc.C) {

From fd910bab79c7c136c2ce1f932fd4e3f5aa17fe4d Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 12:12:12 +1200
Subject: [PATCH 2/6] Add tests.

---
 controller.go      |  2 +-
 controller_test.go | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/controller.go b/controller.go
index 7c6adcf..89de282 100644
--- a/controller.go
+++ b/controller.go
@@ -689,7 +689,7 @@ func parseAllocateConstraintsResponse(source interface{}, machine *machine) (Con
 	// TODO: handle storage when we export block devices.
 	if interfaceMatches, found := valid["interfaces"]; found {
 		for label, value := range interfaceMatches.(map[string]interface{}) {
-			id := int(value.(int64))
+			id := value.(int)
 			iface := machine.Interface(id)
 			if iface == nil {
 				return empty, NewDeserializationError("constraint match interface %q: %d does not match an interface for the machine", label, id)
diff --git a/controller_test.go b/controller_test.go
index a830502..ce16dc0 100644
--- a/controller_test.go
+++ b/controller_test.go
@@ -283,6 +283,35 @@ func (s *controllerSuite) TestAllocateMachine(c *gc.C) {
 	c.Assert(machine.SystemID(), gc.Equals, "4y3ha3")
 }
 
+func (s *controllerSuite) TestAllocateMachineInterfacesMatch(c *gc.C) {
+	s.addAllocateReponse(c, http.StatusOK, map[string]int{
+		"database": 35,
+	})
+	controller := s.getController(c)
+	_, match, err := controller.AllocateMachine(AllocateMachineArgs{
+		// This isn't actually used, but here to show how it should be used.
+		Interfaces: "database:space=space-0",
+	})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(match.Interfaces, gc.HasLen, 1)
+	iface := match.Interfaces["database"]
+	c.Assert(iface.ID(), gc.Equals, 35)
+}
+
+func (s *controllerSuite) TestAllocateMachineInterfacesMatchMissing(c *gc.C) {
+	// This should never happen, but if it does it is a clear indication of a
+	// bug somewhere.
+	s.addAllocateReponse(c, http.StatusOK, map[string]int{
+		"database": 40,
+	})
+	controller := s.getController(c)
+	_, _, err := controller.AllocateMachine(AllocateMachineArgs{
+		// This isn't actually used, but here to show how it should be used.
+		Interfaces: "database:space=space-0",
+	})
+	c.Assert(err, jc.Satisfies, IsDeserializationError)
+}
+
 func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
 	s.addAllocateReponse(c, http.StatusOK, nil)
 	controller := s.getController(c)

From e049d74766858537c9323e600f0108d3f8d57e11 Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 12:18:45 +1200
Subject: [PATCH 3/6] Do the schema check in one pass.

---
 controller.go | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/controller.go b/controller.go
index 89de282..94f4b6f 100644
--- a/controller.go
+++ b/controller.go
@@ -658,36 +658,30 @@ func (c *controller) readAPIVersion(apiVersion version.Number) (set.Strings, ver
 
 func parseAllocateConstraintsResponse(source interface{}, machine *machine) (ConstraintMatches, error) {
 	var empty ConstraintMatches
-	// Do the schema check in two passes.
-	fields := schema.Fields{
-		"constraints_by_type": schema.StringMap(schema.Any()),
-	}
-	checker := schema.FieldMap(fields, nil) // no defaults
-	coerced, err := checker.Coerce(source, nil)
-	if err != nil {
-		return empty, WrapWithDeserializationError(err, "allocation constraints response schema check failed")
-	}
-	constraintsMap := coerced.(map[string]interface{})["constraints_by_type"]
-	fields = schema.Fields{
+	matchFields := schema.Fields{
 		"storage":    schema.StringMap(schema.ForceInt()),
 		"interfaces": schema.StringMap(schema.ForceInt()),
 	}
-	defaults := schema.Defaults{
+	matchDefaults := schema.Defaults{
 		"storage":    schema.Omit,
 		"interfaces": schema.Omit,
 	}
-	checker = schema.FieldMap(fields, defaults)
-	coerced, err = checker.Coerce(constraintsMap, nil)
+	fields := schema.Fields{
+		"constraints_by_type": schema.FieldMap(matchFields, matchDefaults),
+	}
+	checker := schema.FieldMap(fields, nil) // no defaults
+	coerced, err := checker.Coerce(source, nil)
 	if err != nil {
 		return empty, WrapWithDeserializationError(err, "allocation constraints response schema check failed")
 	}
 	valid := coerced.(map[string]interface{})
+	constraintsMap := valid["constraints_by_type"].(map[string]interface{})
 	result := ConstraintMatches{
 		// TODO: make storage map
 		Interfaces: make(map[string]Interface),
 	}
 	// TODO: handle storage when we export block devices.
-	if interfaceMatches, found := valid["interfaces"]; found {
+	if interfaceMatches, found := constraintsMap["interfaces"]; found {
 		for label, value := range interfaceMatches.(map[string]interface{}) {
 			id := value.(int)
 			iface := machine.Interface(id)

From 06b63186c364ebeb63888ca80f60e7cc53a64f9a Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 16:46:30 +1200
Subject: [PATCH 4/6] Use structured storage and interface specifications.

---
 controller.go      | 182 ++++++++++++++++++++++++++++++++++++++-------
 controller_test.go | 167 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 315 insertions(+), 34 deletions(-)

diff --git a/controller.go b/controller.go
index 94f4b6f..29d528e 100644
--- a/controller.go
+++ b/controller.go
@@ -5,11 +5,13 @@ package gomaasapi
 
 import (
 	"encoding/json"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"net/url"
 	"path"
+	"strings"
 	"sync/atomic"
 
 	"github.com/juju/errors"
@@ -277,6 +279,106 @@ func (c *controller) Machines(args MachinesArgs) ([]Machine, error) {
 	return result, nil
 }
 
+// StorageSpec represents one element of storage constraints necessary
+// to be satisfied to allocate a machine.
+type StorageSpec struct {
+	// Label is optional and an arbitrary string.
+	// Labels need to be unique across the StorageSpec elements specified
+	// in the AllcoateMachineArgs.
+	Label string
+	// Size is required and refers to the required minimum size in GB.
+	Size int
+	// Zero or more tags assocated to with the disks.
+	Tags []string
+}
+
+// Validate ensures that there is a positive size and that there are no Empty
+// tag values.
+func (s *StorageSpec) Validate() error {
+	if s.Size <= 0 {
+		return errors.NotValidf("Size value %d", s.Size)
+	}
+	for _, v := range s.Tags {
+		if v == "" {
+			return errors.NotValidf("empty tag")
+		}
+	}
+	return nil
+}
+
+// String returns the string representation of the storage spec.
+func (s *StorageSpec) String() string {
+	label := s.Label
+	if label != "" {
+		label += ":"
+	}
+	tags := strings.Join(s.Tags, ",")
+	if tags != "" {
+		tags = "(" + tags + ")"
+	}
+	return fmt.Sprintf("%s%d%s", label, s.Size, tags)
+}
+
+// InterfaceSpec represents one elemenet of network related constraints.
+type InterfaceSpec struct {
+	// Label is required and an arbitrary string. Labels need to be unique
+	// across the InterfaceSpec elements specified in the AllcoateMachineArgs.
+	// The label is returned in the ConstraintMatches response from
+	// AllocateMachine.
+	Label    string
+	Space    []string
+	NotSpace []string
+
+	// NOTE: there are other interface spec values that we are not exposing at
+	// this stage that can be added on an as needed basis. Other possible values are:
+	//     'fabric_class', 'not_fabric_class',
+	//     'subnet_cidr', 'not_subnet_cidr',
+	//     'vid', 'not_vid',
+	//     'fabric', 'not_fabric',
+	//     'subnet', 'not_subnet',
+	//     'mode'
+}
+
+// Validate ensures that a Label is specified and that there is at least one
+// Space or NotSpace value set.
+func (a *InterfaceSpec) Validate() error {
+	if a.Label == "" {
+		return errors.NotValidf("missing Label")
+	}
+	// Empty strings are not valid.
+	// At least one value must be set.
+	values := 0
+	for _, v := range a.Space {
+		if v == "" {
+			return errors.NotValidf("empty Space constraint")
+		}
+		values++
+	}
+	for _, v := range a.NotSpace {
+		if v == "" {
+			return errors.NotValidf("empty NotSpace constraint")
+		}
+		values++
+	}
+	if values == 0 {
+		return errors.NotValidf("missing constraints")
+	}
+
+	return nil
+}
+
+// String returns the interface spec as MaaS requires it.
+func (a *InterfaceSpec) String() string {
+	var values []string
+	for _, v := range a.Space {
+		values = append(values, "space="+v)
+	}
+	for _, v := range a.NotSpace {
+		values = append(values, "not_space="+v)
+	}
+	return fmt.Sprintf("%s:%s", a.Label, strings.Join(values, ","))
+}
+
 // AllocateMachineArgs is an argument struct for passing args into Machine.Allocate.
 type AllocateMachineArgs struct {
 	Hostname     string
@@ -288,37 +390,61 @@ type AllocateMachineArgs struct {
 	NotTags   []string
 	Zone      string
 	NotInZone []string
-	// Storage is an optional value listing one or more storage constraints.
-	// Format is an optional label, followed by an optional colon, then size
-	// (which is mandatory) followed by an optional comma seperated list of tags
-	// in parentheses.
-	//
-	//   Examples:
-	//
-	//   200(ssd,removable),400(ssd),300
-	//   - 200GB disk with ssd and removable tag
-	//   - 400GB disk with ssd tag
-	//   - 300GB disk
-	//
-	//  root:80(ssd),data:400
-	//  - 80+GB disk with ssd tag, name the constraint "root"
-	//  - 400+GB disk, name the consrtaint "data"
-	Storage string
-	// Interfaces is an optional value listing one or more interface constraints.
-	// Format of the string is semi-colon delimited labelled constraints. Each
-	// labelled constraint has the format:
-	//   <label>:<key>=<value>[,<key2>=<value2>[,...]]'
-	// The label is used in the ConstraintMatch structure returned from
-	// AllocateMachine. Valid keys are:
-	//     'space', 'not_space', 'fabric_class', 'not_fabric_class',
-	//     'subnet_cidr', 'not_subnet_cidr', 'vid', 'not_vid',
-	//     'fabric', 'not_fabric', 'subnet', 'not_subnet', 'mode'
-	Interfaces string
+	// Storage represents the required disks on the Machine. If any are specified
+	// the first value is used for the root disk.
+	Storage []StorageSpec
+	// Interfaces represents a number of required interfaces on the machine.
+	// Each InterfaceSpec relates to an individual network interface.
+	Interfaces []InterfaceSpec
 	AgentName  string
 	Comment    string
 	DryRun     bool
 }
 
+// Validate makes sure that any labels specifed in Storage or Interfaces
+// are unique, and that the required specifications are valid.
+func (a *AllocateMachineArgs) Validate() error {
+	storageLabels := set.NewStrings()
+	for _, spec := range a.Storage {
+		if err := spec.Validate(); err != nil {
+			return errors.Annotate(err, "Storage")
+		}
+		if spec.Label != "" {
+			if storageLabels.Contains(spec.Label) {
+				return errors.NotValidf("reusing storage label %q", spec.Label)
+			}
+			storageLabels.Add(spec.Label)
+		}
+	}
+	interfaceLabels := set.NewStrings()
+	for _, spec := range a.Interfaces {
+		if err := spec.Validate(); err != nil {
+			return errors.Annotate(err, "Interfaces")
+		}
+		if interfaceLabels.Contains(spec.Label) {
+			return errors.NotValidf("reusing interface label %q", spec.Label)
+		}
+		interfaceLabels.Add(spec.Label)
+	}
+	return nil
+}
+
+func (a *AllocateMachineArgs) storage() string {
+	var values []string
+	for _, spec := range a.Storage {
+		values = append(values, spec.String())
+	}
+	return strings.Join(values, ",")
+}
+
+func (a *AllocateMachineArgs) interfaces() string {
+	var values []string
+	for _, spec := range a.Interfaces {
+		values = append(values, spec.String())
+	}
+	return strings.Join(values, ";")
+}
+
 // ConstraintMatches provides a way for the caller of AllocateMachine to determine
 //.how the allocated machine matched the storage and interfaces constraints specified.
 // The labels that were used in the constraints are the keys in the maps.
@@ -343,8 +469,8 @@ func (c *controller) AllocateMachine(args AllocateMachineArgs) (Machine, Constra
 	params.MaybeAddInt("mem", args.MinMemory)
 	params.MaybeAddMany("tags", args.Tags)
 	params.MaybeAddMany("not_tags", args.NotTags)
-	params.MaybeAdd("storage", args.Storage)
-	params.MaybeAdd("interfaces", args.Interfaces)
+	params.MaybeAdd("storage", args.storage())
+	params.MaybeAdd("interfaces", args.interfaces())
 	params.MaybeAdd("zone", args.Zone)
 	params.MaybeAddMany("not_in_zone", args.NotInZone)
 	params.MaybeAdd("agent_name", args.AgentName)
diff --git a/controller_test.go b/controller_test.go
index ce16dc0..7d3a546 100644
--- a/controller_test.go
+++ b/controller_test.go
@@ -264,6 +264,156 @@ func (s *controllerSuite) TestMachinesArgs(c *gc.C) {
 	c.Assert(request.URL.Query(), gc.HasLen, 6)
 }
 
+func (s *controllerSuite) TestStorageSpec(c *gc.C) {
+	for i, test := range []struct {
+		spec StorageSpec
+		err  string
+		repr string
+	}{{
+		spec: StorageSpec{},
+		err:  "Size value 0 not valid",
+	}, {
+		spec: StorageSpec{Size: -10},
+		err:  "Size value -10 not valid",
+	}, {
+		spec: StorageSpec{Size: 200},
+		repr: "200",
+	}, {
+		spec: StorageSpec{Label: "foo", Size: 200},
+		repr: "foo:200",
+	}, {
+		spec: StorageSpec{Size: 200, Tags: []string{"foo", ""}},
+		err:  "empty tag not valid",
+	}, {
+		spec: StorageSpec{Size: 200, Tags: []string{"foo"}},
+		repr: "200(foo)",
+	}, {
+		spec: StorageSpec{Label: "omg", Size: 200, Tags: []string{"foo", "bar"}},
+		repr: "omg:200(foo,bar)",
+	}} {
+		c.Logf("test %d", i)
+		err := test.spec.Validate()
+		if test.err == "" {
+			c.Assert(err, jc.ErrorIsNil)
+			c.Assert(test.spec.String(), gc.Equals, test.repr)
+		} else {
+			c.Assert(err, jc.Satisfies, errors.IsNotValid)
+			c.Assert(err.Error(), gc.Equals, test.err)
+		}
+	}
+}
+
+func (s *controllerSuite) TestInterfaceSpec(c *gc.C) {
+	for i, test := range []struct {
+		spec InterfaceSpec
+		err  string
+		repr string
+	}{{
+		spec: InterfaceSpec{},
+		err:  "missing Label not valid",
+	}, {
+		spec: InterfaceSpec{Label: "foo"},
+		err:  "missing constraints not valid",
+	}, {
+		spec: InterfaceSpec{Label: "foo", Space: []string{""}},
+		err:  "empty Space constraint not valid",
+	}, {
+		spec: InterfaceSpec{Label: "foo", NotSpace: []string{""}},
+		err:  "empty NotSpace constraint not valid",
+	}, {
+		spec: InterfaceSpec{Label: "foo", Space: []string{"magic"}},
+		repr: "foo:space=magic",
+	}, {
+		// NOTE: not entirely sure that specifying more than one space makes sense.
+		spec: InterfaceSpec{Label: "foo", Space: []string{"magic", "home"}},
+		repr: "foo:space=magic,space=home",
+	}, {
+		spec: InterfaceSpec{Label: "foo", NotSpace: []string{"weird"}},
+		repr: "foo:not_space=weird",
+	}, {
+		spec: InterfaceSpec{Label: "foo", NotSpace: []string{"weird", "place"}},
+		repr: "foo:not_space=weird,not_space=place",
+	}, {
+		spec: InterfaceSpec{
+			Label:    "foo",
+			Space:    []string{"magic", "home"},
+			NotSpace: []string{"weird", "place"},
+		},
+		repr: "foo:space=magic,space=home,not_space=weird,not_space=place",
+	}} {
+		c.Logf("test %d", i)
+		err := test.spec.Validate()
+		if test.err == "" {
+			c.Check(err, jc.ErrorIsNil)
+			c.Check(test.spec.String(), gc.Equals, test.repr)
+		} else {
+			c.Check(err, jc.Satisfies, errors.IsNotValid)
+			c.Check(err.Error(), gc.Equals, test.err)
+		}
+	}
+}
+
+func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
+	for i, test := range []struct {
+		args       AllocateMachineArgs
+		err        string
+		storage    string
+		interfaces string
+	}{{
+		args: AllocateMachineArgs{},
+	}, {
+		args: AllocateMachineArgs{
+			Storage: []StorageSpec{{}},
+		},
+		err: "Storage: Size value 0 not valid",
+	}, {
+		args: AllocateMachineArgs{
+			Storage: []StorageSpec{{Size: 200}, {Size: 400, Tags: []string{"ssd"}}},
+		},
+		storage: "200,400(ssd)",
+	}, {
+		args: AllocateMachineArgs{
+			Storage: []StorageSpec{
+				{Label: "foo", Size: 200},
+				{Label: "foo", Size: 400, Tags: []string{"ssd"}},
+			},
+		},
+		err: `reusing storage label "foo" not valid`,
+	}, {
+		args: AllocateMachineArgs{
+			Interfaces: []InterfaceSpec{{}},
+		},
+		err: "Interfaces: missing Label not valid",
+	}, {
+		args: AllocateMachineArgs{
+			Interfaces: []InterfaceSpec{
+				{Label: "foo", Space: []string{"magic"}},
+				{Label: "bar", Space: []string{"other"}},
+			},
+		},
+		interfaces: "foo:space=magic;bar:space=other",
+	}, {
+		args: AllocateMachineArgs{
+			Interfaces: []InterfaceSpec{
+				{Label: "foo", Space: []string{"magic"}},
+				{Label: "foo", Space: []string{"other"}},
+			},
+		},
+		err: `reusing interface label "foo" not valid`,
+	}} {
+		c.Logf("test %d", i)
+		err := test.args.Validate()
+		if test.err == "" {
+			c.Check(err, jc.ErrorIsNil)
+			c.Check(test.args.storage(), gc.Equals, test.storage)
+			c.Check(test.args.interfaces(), gc.Equals, test.interfaces)
+		} else {
+			c.Check(err, jc.Satisfies, errors.IsNotValid)
+			c.Check(err.Error(), gc.Equals, test.err)
+		}
+	}
+}
+
 func (s *controllerSuite) addAllocateReponse(c *gc.C, status int, interfaceMatches map[string]int) {
 	constraints := make(map[string]interface{})
 	if interfaceMatches != nil {
@@ -290,7 +440,10 @@ func (s *controllerSuite) TestAllocateMachineInterfacesMatch(c *gc.C) {
 	controller := s.getController(c)
 	_, match, err := controller.AllocateMachine(AllocateMachineArgs{
 		// This isn't actually used, but here to show how it should be used.
-		Interfaces: "database:space=space-0",
+		Interfaces: []InterfaceSpec{{
+			Label: "database",
+			Space: []string{"space-0"},
+		}},
 	})
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(match.Interfaces, gc.HasLen, 1)
@@ -306,13 +459,15 @@ func (s *controllerSuite) TestAllocateMachineInterfacesMatchMissing(c *gc.C) {
 	})
 	controller := s.getController(c)
 	_, _, err := controller.AllocateMachine(AllocateMachineArgs{
-		// This isn't actually used, but here to show how it should be used.
-		Interfaces: "database:space=space-0",
+		Interfaces: []InterfaceSpec{{
+			Label: "database",
+			Space: []string{"space-0"},
+		}},
 	})
 	c.Assert(err, jc.Satisfies, IsDeserializationError)
 }
 
-func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
+func (s *controllerSuite) TestAllocateMachineArgsForm(c *gc.C) {
 	s.addAllocateReponse(c, http.StatusOK, nil)
 	controller := s.getController(c)
 	// Create an arg structure that sets all the values.
@@ -323,8 +478,8 @@ func (s *controllerSuite) TestAllocateMachineArgs(c *gc.C) {
 		MinMemory:    20000,
 		Tags:         []string{"good"},
 		NotTags:      []string{"bad"},
-		Storage:      "root:200(ssd)",
-		Interfaces:   "default:vid=1;db:space=magic",
+		Storage:      []StorageSpec{{Label: "root", Size: 200}},
+		Interfaces:   []InterfaceSpec{{Label: "default", Space: []string{"magic"}}},
 		Zone:         "magic",
 		NotInZone:    []string{"not-magic"},
 		AgentName:    "agent 42",

From 7b10c111cdf99a3ceb12e5a5620c2d89b6ae9480 Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 16:48:51 +1200
Subject: [PATCH 5/6] typo

---
 controller.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/controller.go b/controller.go
index 29d528e..728c417 100644
--- a/controller.go
+++ b/controller.go
@@ -284,7 +284,7 @@ func (c *controller) Machines(args MachinesArgs) ([]Machine, error) {
 type StorageSpec struct {
 	// Label is optional and an arbitrary string.
 	// Labels need to be unique across the StorageSpec elements specified
-	// in the AllcoateMachineArgs.
+	// in the AllocateMachineArgs.
 	Label string
 	// Size is required and refers to the required minimum size in GB.
 	Size int

From 343e1d88f9c699f08c4b2b8b1e185133e5894110 Mon Sep 17 00:00:00 2001
From: Tim Penhey <tim@penhey.net>
Date: Fri, 8 Apr 2016 16:50:06 +1200
Subject: [PATCH 6/6] typo

---
 controller.go | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/controller.go b/controller.go
index 728c417..83d2873 100644
--- a/controller.go
+++ b/controller.go
@@ -282,9 +282,8 @@ func (c *controller) Machines(args MachinesArgs) ([]Machine, error) {
 // StorageSpec represents one element of storage constraints necessary
 // to be satisfied to allocate a machine.
 type StorageSpec struct {
-	// Label is optional and an arbitrary string.
-	// Labels need to be unique across the StorageSpec elements specified
-	// in the AllocateMachineArgs.
+	// Label is optional and an arbitrary string. Labels need to be unique
+	// across the StorageSpec elements specified in the AllocateMachineArgs.
 	Label string
 	// Size is required and refers to the required minimum size in GB.
 	Size int
@@ -322,7 +321,7 @@ func (s *StorageSpec) String() string {
 // InterfaceSpec represents one elemenet of network related constraints.
 type InterfaceSpec struct {
 	// Label is required and an arbitrary string. Labels need to be unique
-	// across the InterfaceSpec elements specified in the AllcoateMachineArgs.
+	// across the InterfaceSpec elements specified in the AllocateMachineArgs.
 	// The label is returned in the ConstraintMatches response from
 	// AllocateMachine.
 	Label    string