Fix instance type selection

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://code.launchpad.net/~wallyworld/juju-core/fix-instance-type-matching/+merge/204159

(do not edit description out of merge proposal)

[wallyworld, 2014-01-31 07:10:09]

Please take a look.

[wallyworld, 2014-02-01 00:39:32]

Please take a look.

[wallyworld, 2014-02-01 00:42:48]

Please take a look.

[dimitern, 2014-02-10 08:27:13]

LGTM with some suggestions.

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

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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/instanc...
environs/instances/instancetype.go:105: minMemCons := ic.Constraints
Why not just:
cons.Mem = uint64(minMemoryHeuristic)
?

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

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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.

[wallyworld, 2014-02-11 00:39:03]

Please take a look.

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

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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/instanc...
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/instanc...
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/instanc...
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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.

[dimitern, 2014-02-11 09:59:57]

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

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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).

[fwereade, 2014-02-13 15:19:21]

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

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

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instanc...
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/instanc...
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instanc...
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

[wallyworld, 2014-02-13 23:55:22]

I added a cost based tie breaker

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

https://codereview.appspot.com/58950043/diff/60001/environs/instances/instanc...
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.