Merge lp:~dimitern/juju-core/033-openstack-pick-instance-type-based-on-constraints into lp:~juju/juju-core/trunk

[Dimiter Naydenov]

openstack: pick flavor based on constraints

WIP, for discussion
Not tested properly yet.

https://codereview.appspot.com/8753044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+158889_code.launchpad.net,

Message:
Comments on implementation as agreed online.

https://codereview.appspot.com/8753044/diff/1/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/8753044/diff/1/environs/openstack/image.go#newcode110
environs/openstack/image.go:110: if flavor.RAM >= minMemoryForMongoDB {
use minMemory IFF there's no explicit memory constraint.

Also: remove the MongoDB context from the name.

Description:
openstack: pick flavor based on constraints

WIP, for discussion
Not tested properly yet.

https://code.launchpad.net/~dimitern/juju-core/033-openstack-pick-instance-type-based-on-constraints/+merge/158889

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/openstack/export_test.go
   M environs/openstack/image.go
   M environs/openstack/live_test.go
   M environs/openstack/provider.go

[Dimiter Naydenov (dimitern)]

Another comment

https://codereview.appspot.com/8753044/diff/1/environs/openstack/image.go
File environs/openstack/image.go (right):

https://codereview.appspot.com/8753044/diff/1/environs/openstack/image.go#newcode75
environs/openstack/image.go:75: allFlavors, err :=
novaClient.ListFlavorsDetail()
extract the logic below in a separate func, taking flavors list and
default flavor name, so it can be tested separately.

https://codereview.appspot.com/8753044/

=== modified file 'environs/openstack/export_test.go'
--- environs/openstack/export_test.go	2013-04-14 22:12:49 +0000
+++ environs/openstack/export_test.go	2013-04-15 11:03:26 +0000
@@ -4,6 +4,7 @@
 	"fmt"
 	"launchpad.net/goose/nova"
 	"launchpad.net/goose/swift"
+	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/jujutest"
 	"launchpad.net/juju-core/state"
@@ -99,13 +100,14 @@
 	return instanceAddress(addresses)
 }
 
-func FindInstanceSpec(e environs.Environ, series, arch, flavor string) (imageId, flavorId string, err error) {
+func FindInstanceSpec(e environs.Environ, series, arch, flavor string, cons constraints.Value) (imageId, flavorId string, err error) {
 	env := e.(*environ)
-	spec, err := findInstanceSpec(env, &instanceConstraint{
-		series: series,
-		arch:   arch,
-		region: env.ecfg().region(),
-		flavor: flavor,
+	spec, err := findInstanceSpec(env, &instanceFilter{
+		series:        series,
+		arch:          arch,
+		region:        env.ecfg().region(),
+		defaultFlavor: flavor,
+		constraints:   cons,
 	})
 	if err == nil {
 		imageId = spec.imageId

=== modified file 'environs/openstack/image.go'
--- environs/openstack/image.go	2013-01-30 00:18:03 +0000
+++ environs/openstack/image.go	2013-04-15 11:03:26 +0000
@@ -2,50 +2,124 @@
 
 import (
 	"fmt"
+	"launchpad.net/goose/nova"
+	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
+	"sort"
 )
 
-// instanceConstraint constrains the possible instances that may be
-// chosen by the Openstack provider.
-type instanceConstraint struct {
-	series string // Ubuntu release name.
-	arch   string
-	region string
-	flavor string
+// minMemoryForMongoDB is the assumed minimum amount of memory we require in order to run MongoDB (1GB)
+const minMemoryForMongoDB = 1 << 30
+
+// instanceFilter defines a filter to match a suitable OpenStack
+// flavor to use when starting an instance.
+type instanceFilter struct {
+	series        string // Ubuntu release name.
+	arch          string
+	region        string
+	defaultFlavor string
+	constraints   constraints.Value
 }
 
-// instanceSpec specifies a particular kind of instance.
+// instanceSpec specifies a particular combination of flavor and image
+// to start an instance.
 type instanceSpec struct {
 	imageId  string
 	flavorId string
 }
 
-func findInstanceSpec(e *environ, constraint *instanceConstraint) (*instanceSpec, error) {
-	nova := e.nova()
-	flavors, err := nova.ListFlavors()
+// flavorsByCost defines a list of flavors sortable by cost.
+type flavorsByCost []nova.FlavorDetail
+
+func (f flavorsByCost) Len() int           { return len(f) }
+func (f flavorsByCost) Swap(i, j int)      { f[i], f[j] = f[j], f[i] }
+func (f flavorsByCost) Less(i, j int) bool { return f[i].RAM < f[j].RAM }
+
+// reverseByCost implements a reverse sort by cost.
+type reverseByCost struct {
+	sort.Interface
+}
+
+func (r reverseByCost) Less(i, j int) bool { return r.Interface.Less(j, i) }
+
+// flavorMatches returns true if the given flavor matches the given constraints.
+func flavorMatches(flavor *nova.FlavorDetail, cons constraints.Value) bool {
+	if cons.Mem != nil && uint64(flavor.RAM) < *cons.Mem {
+		return false
+	}
+	if cons.CpuCores != nil && uint64(flavor.VCPUs) < *cons.CpuCores {
+		return false
+	}
+	// NOTE: CpuPower constraint is not supported by OpenStack.
+	return true
+}
+
+// findInstanceSpec picks a suitable combination of flavor and image
+// based on default flavor and constraints.
+// TODO(dimitern) also pick image using clouddata once done.
+func findInstanceSpec(e *environ, filter *instanceFilter) (*instanceSpec, error) {
+	novaClient := e.nova()
+
+	// TODO(wallyworld) - we want to search for an image based on the
+	// series, arch, region like for ec2 providers and
+	// http://cloud-images.ubuntu.com but there's nothing to support
+	// that yet. So we allow the user to configure a default image Id
+	// to use.
+	defaultImageId := e.ecfg().defaultImageId()
+	if defaultImageId == "" {
+		return nil, fmt.Errorf("Unable to find image for series/arch/region %s/%s/%s and no default specified.",
+			filter.series, filter.arch, filter.region)
+	}
+
+	// 1. Get all flavors to match against the constraints.
+	allFlavors, err := novaClient.ListFlavorsDetail()
 	if err != nil {
 		return nil, err
 	}
-	var flavorId string
-	for _, flavor := range flavors {
-		if flavor.Name == constraint.flavor {
-			flavorId = flavor.Id
-			break
-		}
-	}
-	if flavorId == "" {
-		return nil, environs.NotFoundError{fmt.Errorf("No such flavor %s", constraint.flavor)}
-	}
-	// TODO(wallyworld) - we want to search for an image based on the series, arch, region like for ec2 providers
-	// and http://cloud-images.ubuntu.com but there's nothing to support that yet.
-	// So we allow the user to configure a default image Id to use.
-	imageId := e.ecfg().defaultImageId()
-	if imageId == "" {
-		return nil, fmt.Errorf("Unable to find image for series/arch/region %s/%s/%s and no default specified.",
-			constraint.series, constraint.arch, constraint.region)
-	}
+	matchingFlavors := make(map[string]nova.FlavorDetail)
+	matchingByCost := make(flavorsByCost, len(allFlavors))
+	var defaultFlavorId string
+	// 2. Filter out flavors not matching the constraints and pick the
+	// default flavor id based on the given name.
+	for _, flavor := range allFlavors {
+		if flavor.Name == filter.defaultFlavor {
+			defaultFlavorId = flavor.Id
+		}
+		if flavorMatches(&flavor, filter.constraints) {
+			matchingFlavors[flavor.Id] = flavor
+			matchingByCost = append(matchingByCost, flavor)
+		}
+	}
+	if len(matchingFlavors) == 0 {
+		return nil, fmt.Errorf("no flavors found matching specified constraints: %v", filter.constraints)
+	}
+	// 3. If default flavor is among the matching ones, pick that.
+	if _, ok := matchingFlavors[defaultFlavorId]; ok {
+		return &instanceSpec{
+			imageId:  defaultImageId,
+			flavorId: defaultFlavorId,
+		}, nil
+	} else if defaultFlavorId == "" {
+		return nil, environs.NotFoundError{fmt.Errorf("no such flavor %s", filter.defaultFlavor)}
+	}
+	// 4. Sort by cost (memory) and find the cheapest matching both
+	// user's explicit constraints and our own heuristic: minimum
+	// amount of memory required to run MongoDB.
+	sort.Sort(matchingByCost)
+	for _, flavor := range matchingByCost {
+		if flavor.RAM >= minMemoryForMongoDB {
+			return &instanceSpec{
+				imageId:  defaultImageId,
+				flavorId: flavor.Id,
+			}, nil
+		}
+	}
+	// 5. Failing that, sort in reverse order and return the most
+	// expensive one, which will hopefully work, albeit not the best
+	// match.
+	sort.Sort(reverseByCost{matchingByCost})
 	return &instanceSpec{
-		imageId:  imageId,
-		flavorId: flavorId,
+		imageId:  defaultImageId,
+		flavorId: matchingByCost[0].Id,
 	}, nil
 }

=== modified file 'environs/openstack/live_test.go'
--- environs/openstack/live_test.go	2013-04-10 00:11:24 +0000
+++ environs/openstack/live_test.go	2013-04-15 11:03:26 +0000
@@ -7,6 +7,7 @@
 	. "launchpad.net/gocheck"
 	"launchpad.net/goose/client"
 	"launchpad.net/goose/identity"
+	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/jujutest"
 	"launchpad.net/juju-core/environs/openstack"
@@ -135,18 +136,21 @@
 	t.LoggingSuite.TearDownTest(c)
 }
 
-func (t *LiveTests) TestFindImageSpec(c *C) {
+func (t *LiveTests) TestFindInstanceSpecWithoutConstraints(c *C) {
 	instanceType := openstack.DefaultInstanceType(t.Env)
-	imageId, flavorId, err := openstack.FindInstanceSpec(t.Env, "precise", "amd64", instanceType)
+	imageId, flavorId, err := openstack.FindInstanceSpec(t.Env, "precise", "amd64", instanceType, constraints.Value{})
 	c.Assert(err, IsNil)
 	c.Assert(imageId, Equals, t.testImageId)
 	c.Assert(flavorId, Not(Equals), "")
 }
 
-func (t *LiveTests) TestFindImageBadFlavor(c *C) {
-	imageId, flavorId, err := openstack.FindInstanceSpec(t.Env, "precise", "amd64", "bad.flavor")
+func (t *LiveTests) TestFindInstanceSpecWithBadFlavor(c *C) {
+	imageId, flavorId, err := openstack.FindInstanceSpec(t.Env, "precise", "amd64", "bad.flavor", constraints.Value{})
 	_, ok := err.(environs.NotFoundError)
 	c.Assert(ok, Equals, true)
 	c.Assert(imageId, Equals, "")
 	c.Assert(flavorId, Equals, "")
 }
+
+func (t *LiveTests) TestFindInstanceSpecWithConstraints(c *C) {
+}

=== modified file 'environs/openstack/provider.go'
--- environs/openstack/provider.go	2013-04-14 22:12:49 +0000
+++ environs/openstack/provider.go	2013-04-15 11:03:26 +0000
@@ -768,11 +768,12 @@
 		// TODO(fwereade): this is somewhat crazy.
 		return nil, fmt.Errorf("cannot find image for %q", scfg.tools.Series)
 	}
-	spec, err := findInstanceSpec(e, &instanceConstraint{
-		series: scfg.tools.Series,
-		arch:   scfg.tools.Arch,
-		region: e.ecfg().region(),
-		flavor: e.ecfg().defaultInstanceType(),
+	spec, err := findInstanceSpec(e, &instanceFilter{
+		series:        scfg.tools.Series,
+		arch:          scfg.tools.Arch,
+		region:        e.ecfg().region(),
+		defaultFlavor: e.ecfg().defaultInstanceType(),
+		constraints:   scfg.constraints,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("cannot find image satisfying constraints: %v", err)