Merge lp:~wallyworld/juju-core/fix-instance-type-matching into lp:~go-bot/juju-core/trunk

[Ian Booth]

Fix instance type selection

Instance type selection will, by default, choose
an instance with enough memory for mong (1024M).
That is unless the user has specified a memory
constraint. If no matching instances are found,
an error is returned. Previously, instead of
an error an arbitary instance with 1024M would
be chosen.

It's perhaps easiest to see the behaviour by
looking at the tests in instancetype_test. With
the old code, several of the tests fail, especially
the memory ones where other constraints are used also.

Fix instance type selection

Instance type selection will, by default, choose
an instance with enough memory for mong (1024M).
That is unless the user has specified a memory
constraint. If no matching instances are found,
an error is returned. Previously, instead of
an error an arbitary instance with 1024M would
be chosen.

It's perhaps easiest to see the behaviour by
looking at the tests in instancetype_test. With
the old code, several of the tests fail, especially
the memory ones where other constraints are used also.

BTW - I tested on Canonistack and EC2. On Canonistack, the default instance
selection was m1.tiny (512MB) but this branch fixes that so that now
cpu1-ram1-disk10-ephemeral20 (1024MB) is used. On EC2, attempting to deploy with
cpu-cores=9000 results in an error.

https://codereview.appspot.com/58950043/

[Ian Booth (wallyworld)]

Reviewers: mp+204159_code.launchpad.net,

Message:
Please take a look.

Description:
Fix instance type selection

Instance type selection will, by default, choose
an instance with enough memory for mong (1024M).
That is unless the user has specified a memory
constraint. If no matching instances are found,
an error is returned. Previously, instead of
an error an arbitary instance with 1024M would
be chosen.

It's perhaps easiest to see the behaviour by
looking at the tests in instancetype_test. With
the old code, several of the tests fail, especially
the memory ones where other constraints are used also.

https://code.launchpad.net/~wallyworld/juju-core/fix-instance-type-matching/+merge/204159

(do not edit description out of merge proposal)

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

Affected files (+86, -40 lines):
   A [revision details]
   M environs/instances/instancetype.go
   M environs/instances/instancetype_test.go

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/58950043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/58950043/

[Dimiter Naydenov (dimitern)]

LGTM with some suggestions.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode98
environs/instances/instancetype.go:98: // (previously an instance type
with largest available memory was returned even if it didn't
I don't think we need to explain how it was previously.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode105
environs/instances/instancetype.go:105: minMemCons := ic.Constraints
Why not just:
cons.Mem = uint64(minMemoryHeuristic)
?

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go#newcode155
environs/instances/instancetype_test.go:155: about: "small mem
specified, use that even though less than needed for mongodb",
I think if the user overrode our minimum memory heuristic, we should at
least print out a warning, saying bootstrap might fail due to
insufficient memory for mongo.

https://codereview.appspot.com/58950043/

[Ian Booth (wallyworld)]

Please take a look.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode98
environs/instances/instancetype.go:98: // (previously an instance type
with largest available memory was returned even if it didn't
On 2014/02/10 08:27:13, dimitern wrote:
> I don't think we need to explain how it was previously.

Done.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode98
environs/instances/instancetype.go:98: // (previously an instance type
with largest available memory was returned even if it didn't
On 2014/02/10 08:27:13, dimitern wrote:
> I don't think we need to explain how it was previously.

Done.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode105
environs/instances/instancetype.go:105: minMemCons := ic.Constraints
On 2014/02/10 08:27:13, dimitern wrote:
> Why not just:
> cons.Mem = uint64(minMemoryHeuristic)
> ?

Yeah, i refactored some stuff and left it in a bad state. Fixed. It does
need to be a pointer but there's no need for the intermediate object.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go#newcode155
environs/instances/instancetype_test.go:155: about: "small mem
specified, use that even though less than needed for mongodb",
On 2014/02/10 08:27:13, dimitern wrote:
> I think if the user overrode our minimum memory heuristic, we should
at least
> print out a warning, saying bootstrap might fail due to insufficient
memory for
> mongo.

Not sure about that. The user could also specify poorly speced systems
for charms and we don't emit anything there. This behaviour, allowing
the user to specify small memory, is not new - the tests are just being
improved. We have never in the past emitted a warning AFAIK.

https://codereview.appspot.com/58950043/

[Dimiter Naydenov (dimitern)]

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go#newcode155
environs/instances/instancetype_test.go:155: about: "small mem
specified, use that even though less than needed for mongodb",
On 2014/02/11 00:39:04, wallyworld wrote:
> On 2014/02/10 08:27:13, dimitern wrote:
> > I think if the user overrode our minimum memory heuristic, we should
at least
> > print out a warning, saying bootstrap might fail due to insufficient
memory
> for
> > mongo.

> Not sure about that. The user could also specify poorly speced systems
for
> charms and we don't emit anything there. This behaviour, allowing the
user to
> specify small memory, is not new - the tests are just being improved.
We have
> never in the past emitted a warning AFAIK.

It's true we never said anything, but it's the UX improvement an
important goal? An overly conservative user might pick 256M so it's
cheap, but then will get frustrated why he can't bootstrap (perhaps
without a nice error in the log).

https://codereview.appspot.com/58950043/

[William Reade (fwereade)]

LGTM, but an explicit tie-breaker would be really nice.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go#newcode155
environs/instances/instancetype_test.go:155: about: "small mem
specified, use that even though less than needed for mongodb",
On 2014/02/11 09:59:57, dimitern wrote:
> On 2014/02/11 00:39:04, wallyworld wrote:
> > On 2014/02/10 08:27:13, dimitern wrote:
> > > I think if the user overrode our minimum memory heuristic, we
should at
> least
> > > print out a warning, saying bootstrap might fail due to
insufficient memory
> > for
> > > mongo.
> >
> > Not sure about that. The user could also specify poorly speced
systems for
> > charms and we don't emit anything there. This behaviour, allowing
the user to
> > specify small memory, is not new - the tests are just being
improved. We have
> > never in the past emitted a warning AFAIK.

> It's true we never said anything, but it's the UX improvement an
important goal?
> An overly conservative user might pick 256M so it's cheap, but then
will get
> frustrated why he can't bootstrap (perhaps without a nice error in the
log).

We *might* want to do this on a state server (although really we should
just take what we *know* we need in that case) but this isn't the right
layer IMO. They asked explicitly, and they got it ;).

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instancetype.go
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instancetype.go#newcode99
environs/instances/instancetype.go:99: // - if no matches and no mem
constraint specified, try again and return any matching instance
"any": would be nice to tie-break equal-greatest-memory machines by cost

https://codereview.appspot.com/58950043/

[Ian Booth (wallyworld)]

I added a cost based tie breaker

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instancetype.go
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instancetype.go#newcode99
environs/instances/instancetype.go:99: // - if no matches and no mem
constraint specified, try again and return any matching instance
On 2014/02/13 15:19:21, fwereade wrote:
> "any": would be nice to tie-break equal-greatest-memory machines by
cost

Done.

https://codereview.appspot.com/58950043/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/fix-instance-type-matching into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.017s
ok launchpad.net/juju-core/agent 1.194s
ok launchpad.net/juju-core/agent/tools 0.213s
ok launchpad.net/juju-core/bzr 6.780s
ok launchpad.net/juju-core/cert 3.008s
ok launchpad.net/juju-core/charm 0.573s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.037s
ok launchpad.net/juju-core/cloudinit/sshinit 1.142s
ok launchpad.net/juju-core/cmd 0.222s
ok launchpad.net/juju-core/cmd/charm-admin 0.872s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 231.353s
ok launchpad.net/juju-core/cmd/jujud 53.685s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 10.742s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.026s
ok launchpad.net/juju-core/container 0.052s
ok launchpad.net/juju-core/container/factory 0.052s
ok launchpad.net/juju-core/container/kvm 0.286s
ok launchpad.net/juju-core/container/kvm/mock 0.039s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.322s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.270s
ok launchpad.net/juju-core/environs 3.016s
ok launchpad.net/juju-core/environs/bootstrap 4.429s
ok launchpad.net/juju-core/environs/cloudinit 0.673s
ok launchpad.net/juju-core/environs/config 2.480s
ok launchpad.net/juju-core/environs/configstore 0.039s
ok launchpad.net/juju-core/environs/filestorage 0.031s
ok launchpad.net/juju-core/environs/httpstorage 0.969s
ok launchpad.net/juju-core/environs/imagemetadata 0.640s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.060s
ok launchpad.net/juju-core/environs/jujutest 0.226s
ok launchpad.net/juju-core/environs/manual 8.993s
ok launchpad.net/juju-core/environs/simplestreams 0.354s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.253s
ok launchpad.net/juju-core/environs/storage 1.158s
ok launchpad.net/juju-core/environs/sync 33.514s
ok launchpad.net/juju-core/environs/testing 0.199s
ok launchpad.net/juju-core/environs/tools 6.880s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.028s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 23.377s
ok launchpad.net/juju-core/juju/osenv 0.019s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.023s
ok launchpad.net/juj...

=== modified file 'environs/instances/instancetype.go'
--- environs/instances/instancetype.go	2013-09-27 18:08:05 +0000
+++ environs/instances/instancetype.go	2014-02-14 01:03:32 +0000
@@ -74,52 +74,44 @@
 // minMemoryHeuristic is the assumed minimum amount of memory (in MB) we prefer in order to run a server (1GB)
 const minMemoryHeuristic = 1024
 
+// matchingTypesForConstraint returns instance types from allTypes which match cons.
+func matchingTypesForConstraint(allTypes []InstanceType, cons constraints.Value) []InstanceType {
+	var matchingTypes []InstanceType
+	for _, itype := range allTypes {
+		itype, ok := itype.match(cons)
+		if !ok {
+			continue
+		}
+		matchingTypes = append(matchingTypes, itype)
+	}
+	return matchingTypes
+}
+
 // getMatchingInstanceTypes returns all instance types matching ic.Constraints and available
 // in ic.Region, sorted by increasing region-specific cost (if known).
 func getMatchingInstanceTypes(ic *InstanceConstraint, allInstanceTypes []InstanceType) ([]InstanceType, error) {
-	cons := ic.Constraints
 	region := ic.Region
 	var itypes []InstanceType
 
-	// Iterate over allInstanceTypes, finding matching ones.
-	for _, itype := range allInstanceTypes {
-		itype, ok := itype.match(cons)
-		if !ok {
-			continue
-		}
-		itypes = append(itypes, itype)
+	// Rules used to select instance types:
+	// - non memory constraints like cpu-cores etc are always honoured
+	// - if no mem constraint specified, try opinionated default with enough mem to run a server.
+	// - if no matches and no mem constraint specified, try again and return any matching instance
+	//   with the largest memory
+	cons := ic.Constraints
+	if ic.Constraints.Mem == nil {
+		minMem := uint64(minMemoryHeuristic)
+		cons.Mem = &minMem
 	}
+	itypes = matchingTypesForConstraint(allInstanceTypes, cons)
 
-	if len(itypes) == 0 {
-		// No matching instance types were found, so the fallback is to:
-		// 1. Sort by memory and find the smallest matching both the required architecture
-		//    and our own heuristic: minimum amount of memory required to run a realistic server, or
-		// 2. Sort by memory in reverse order and return the largest one, which will hopefully work,
-		//    albeit not the best match
-		archCons := constraints.Value{Arch: ic.Constraints.Arch}
-		for _, itype := range allInstanceTypes {
-			itype, ok := itype.match(archCons)
-			if !ok {
-				continue
-			}
-			itypes = append(itypes, itype)
-		}
-		sort.Sort(byMemory(itypes))
-		var fallbackType *InstanceType
-		// 1. check for smallest instance type that can realistically run a server
-		for _, itype := range itypes {
-			if itype.Mem >= minMemoryHeuristic {
-				itcopy := itype
-				fallbackType = &itcopy
-				break
-			}
-		}
-		if fallbackType == nil && len(itypes) > 0 {
-			// 2. just get the one with the largest memory
-			fallbackType = &itypes[len(itypes)-1]
-		}
-		if fallbackType != nil {
-			itypes = []InstanceType{*fallbackType}
+	// No matches using opinionated default, so if no mem constraint specified,
+	// look for matching instance with largest memory.
+	if len(itypes) == 0 && ic.Constraints.Mem == nil {
+		itypes = matchingTypesForConstraint(allInstanceTypes, ic.Constraints)
+		if len(itypes) > 0 {
+			sort.Sort(byMemory(itypes))
+			itypes = []InstanceType{itypes[len(itypes)-1]}
 		}
 	}
 	// If we have matching instance types, we can return those, sorted by cost.
@@ -129,7 +121,7 @@
 	}
 
 	// No luck, so report the error.
-	return nil, fmt.Errorf("no instance types in %s matching constraints %q", region, cons)
+	return nil, fmt.Errorf("no instance types in %s matching constraints %q", region, ic.Constraints)
 }
 
 // tagsMatch returns if the tags in wanted all exist in have.
@@ -185,5 +177,11 @@
 func (s byMemory) Len() int      { return len(s) }
 func (s byMemory) Swap(i, j int) { s[i], s[j] = s[j], s[i] }
 func (s byMemory) Less(i, j int) bool {
-	return s[i].Mem < s[j].Mem
+	inst0, inst1 := &s[i], &s[j]
+	if inst0.Mem != inst1.Mem {
+		return s[i].Mem < s[j].Mem
+	}
+	// Memory is equal, so use cost as a tie breaker.
+	// Result is in descending order of cost so instance with lowest cost is used.
+	return inst0.Cost > inst1.Cost
 }

=== modified file 'environs/instances/instancetype_test.go'
--- environs/instances/instancetype_test.go	2013-09-20 02:53:59 +0000
+++ environs/instances/instancetype_test.go	2014-02-14 01:03:32 +0000
@@ -140,24 +140,59 @@
 		arches:         []string{"arm"},
 	},
 	{
-		about: "fallback instance type, enough memory for mongodb",
-		cons:  "mem=8G",
-		itypesToUse: []InstanceType{
+		about: "enough memory for mongodb if mem not specified",
+		cons:  "cpu-cores=4",
+		itypesToUse: []InstanceType{
+			{Id: "5", Name: "it-5", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 2},
+			{Id: "4", Name: "it-4", Arches: []string{"amd64"}, Mem: 2048, CpuCores: 4},
+			{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 4},
+			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256, CpuCores: 4},
+			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4},
+		},
+		expectedItypes: []string{"it-3", "it-4"},
+	},
+	{
+		about: "small mem specified, use that even though less than needed for mongodb",
+		cons:  "mem=300M",
+		itypesToUse: []InstanceType{
+			{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 2048},
+			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
+			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
+		},
+		expectedItypes: []string{"it-1", "it-3"},
+	},
+	{
+		about: "mem specified and match found",
+		cons:  "mem=4G arch=amd64",
+		itypesToUse: []InstanceType{
+			{Id: "4", Name: "it-4", Arches: []string{"arm"}, Mem: 8096},
 			{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 4096},
 			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 2048},
 			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
 		},
+		expectedItypes: []string{"it-3"},
+	},
+	{
+		about: "largest mem available matching other constraints if mem not specified",
+		cons:  "cpu-cores=4",
+		itypesToUse: []InstanceType{
+			{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 2},
+			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256, CpuCores: 4},
+			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4},
+		},
+		expectedItypes: []string{"it-1"},
+	},
+	{
+		about: "largest mem available matching other constraints if mem not specified, cost is tie breaker",
+		cons:  "cpu-cores=4",
+		itypesToUse: []InstanceType{
+			{Id: "4", Name: "it-4", Arches: []string{"amd64"}, Mem: 1024, CpuCores: 2},
+			{Id: "3", Name: "it-3", Arches: []string{"amd64"}, Mem: 256, CpuCores: 4},
+			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4, Cost: 50},
+			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512, CpuCores: 4, Cost: 100},
+		},
 		expectedItypes: []string{"it-2"},
 	},
-	{
-		about: "fallback instance type, not enough memory for mongodb",
-		cons:  "mem=4G",
-		itypesToUse: []InstanceType{
-			{Id: "2", Name: "it-2", Arches: []string{"amd64"}, Mem: 256},
-			{Id: "1", Name: "it-1", Arches: []string{"amd64"}, Mem: 512},
-		},
-		expectedItypes: []string{"it-1"},
-	},
 }
 
 func constraint(region, cons string) *InstanceConstraint {
@@ -195,6 +230,12 @@
 
 	_, err = getMatchingInstanceTypes(constraint("test", "arch=i386 mem=8G"), instanceTypes)
 	c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "arch=i386 mem=8192M"`)
+
+	_, err = getMatchingInstanceTypes(constraint("test", "cpu-cores=9000"), instanceTypes)
+	c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "cpu-cores=9000"`)
+
+	_, err = getMatchingInstanceTypes(constraint("test", "mem=90000M"), instanceTypes)
+	c.Check(err, gc.ErrorMatches, `no instance types in test matching constraints "mem=90000M"`)
 }
 
 var instanceTypeMatchTests = []struct {

=== modified file 'provider/ec2/image_test.go'
--- provider/ec2/image_test.go	2014-01-29 00:14:51 +0000
+++ provider/ec2/image_test.go	2014-02-14 01:03:32 +0000
@@ -94,7 +94,7 @@
 		series: "precise",
 		arches: both,
 		cons:   "cpu-power=",
-		itype:  "t1.micro",
+		itype:  "m1.small",
 		image:  "ami-00000033",
 	}, {
 		series: "precise",

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2014-01-29 06:45:16 +0000
+++ provider/openstack/local_test.go	2014-02-14 01:03:32 +0000
@@ -530,7 +530,7 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(stateData.StateInstances, gc.HasLen, 1)
 
-	expectedHardware := instance.MustParseHardware("arch=amd64 cpu-cores=1 mem=512M")
+	expectedHardware := instance.MustParseHardware("arch=amd64 cpu-cores=1 mem=2G")
 	insts, err := env.AllInstances()
 	c.Assert(err, gc.IsNil)
 	c.Assert(insts, gc.HasLen, 1)
@@ -609,7 +609,7 @@
 	env := s.Open(c)
 
 	// An error occurs if no suitable image is found.
-	_, err := openstack.FindInstanceSpec(env, "saucy", "amd64", "mem=8G")
+	_, err := openstack.FindInstanceSpec(env, "saucy", "amd64", "mem=1G")
 	c.Assert(err, gc.ErrorMatches, `no "saucy" images in some-region with arches \[amd64\]`)
 }