environs: add the facility to query all running instances

environs: add the facility to query all running instances

The PA needs to be able to find out (with some degree of accuracy)
all the instance id's known to the Environ.

https://code.launchpad.net/~dave-cheney/juju-core/go-environs-get-all-instances/+merge/109565

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-06-11 05:56:30]

Please take a look.

[fwereade, 2012-06-11 08:08:36]

Implementation looks good, but I see no tests.

[rog, 2012-06-11 08:09:23]

I'm not entirely sure about this.
Also, it needs a test (in environs/jujutest).

https://codereview.appspot.com/6307071/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6307071/diff/2001/environs/interface.go#newcod...
environs/interface.go:132: // known Instances will be returned.
I'm not very happy with this. I don't like the discontinuity at zero.

How about:
// [...]
// If all is true, then all other instances found
// will be returned after the requested
// instances.
Instances(ids []string, all bool) ([]Instance, error)

Or even a new entry point, AllInstances?

[dave_cheney.net, 2012-06-11 08:15:45]

I'd prefer another method, AllInstances, to a boolean flag. 

I'll leave the decision up to you.

[rog, 2012-06-11 08:30:03]

On 11 June 2012 09:15,  <dave@cheney.net> wrote:
> I'd prefer another method, AllInstances, to a boolean flag.

i'm happy for you to go with that then.

[niemeyer, 2012-06-11 18:27:32]

I'm curious about why it needs such an AllInstances method. There are likely
good reasons, but note that our current mechanisms will retry in case an id
provided to Instances() isn't found. It gets harder to do that kind of resilient
design with a "whatever you know" interface. The implementation of the PA will
need to keep that in mind, and now blow up when the result of the AllInstances
call changes arbitrarily.

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go#newcode372
environs/ec2/ec2.go:372: // fetchInstances queries ec2 for a the best available
list of
s/a//

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go#newcode374
environs/ec2/ec2.go:374: // consistency model of the ec2 dashboard.
dashboard?

[dave_cheney.net, 2012-06-11 23:03:52]

In python the PA asks the provider for a list of running instances, then tries
to compare that with the ones in the state, looking for lost instances.

To replicate this functionality I need some way for the Environ to tell me all
the machines it thinks are running (and hence charging for).

[dave_cheney.net, 2012-06-13 04:57:23]

> I'm not entirely sure about this.
> Also, it needs a test (in environs/jujutest).

So that is where they are.

[rog, 2012-06-13 08:26:53]

On 2012/06/13 04:57:23, dfc wrote:
> So that is where they are.

not yet, it seems... :-)

[dave_cheney.net, 2012-06-13 13:04:46]

Please take a look.

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go#newcode372
environs/ec2/ec2.go:372: // fetchInstances queries ec2 for a the best available
list of
On 2012/06/11 18:27:32, niemeyer wrote:
> s/a//

Done.

https://codereview.appspot.com/6307071/diff/2001/environs/ec2/ec2.go#newcode374
environs/ec2/ec2.go:374: // consistency model of the ec2 dashboard.
On 2012/06/11 18:27:32, niemeyer wrote:
> dashboard?

Done.

https://codereview.appspot.com/6307071/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6307071/diff/2001/environs/interface.go#newcod...
environs/interface.go:132: // known Instances will be returned.
On 2012/06/11 08:09:23, rog wrote:
> I'm not very happy with this. I don't like the discontinuity at zero.
> 
> How about:
> // [...]
> // If all is true, then all other instances found
> // will be returned after the requested
> // instances.
> Instances(ids []string, all bool) ([]Instance, error)
> 
> Or even a new entry point, AllInstances?

AllInstances it is.

[niemeyer, 2012-06-13 19:42:42]

On 2012/06/11 23:03:52, dfc wrote:
> In python the PA asks the provider for a list of running instances, then tries
> to compare that with the ones in the state, looking for lost instances.
> 
> To replicate this functionality I need some way for the Environ to tell me all
> the machines it thinks are running (and hence charging for).

Okay, makes sense. In that case, it doesn't really matter if AllInstances
actually misses some. It will eventually find the lost ones that are not
registered in the state.

What we can't do is go in the other direction and consider that if Allinstances
didn't return something it means it's not running.

[niemeyer, 2012-06-13 19:47:33]

LGTM. Leaving for rog to approve.

[niemeyer, 2012-06-13 19:47:58]

Sorry, this should be part of the comment above:

https://codereview.appspot.com/6307071/diff/9002/environs/ec2/ec2.go
File environs/ec2/ec2.go (left):

https://codereview.appspot.com/6307071/diff/9002/environs/ec2/ec2.go#oldcode373
environs/ec2/ec2.go:373: if len(ids) == 0 {
This should be brought back.

https://codereview.appspot.com/6307071/diff/9002/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6307071/diff/9002/environs/interface.go#newcod...
environs/interface.go:133: // AllInstances returns all Instances currently known
to the Environ.
s/Instances/instances/
s/Environ/environment/

[dave_cheney.net, 2012-06-13 22:55:27]

Please take a look.

https://codereview.appspot.com/6307071/diff/9002/environs/ec2/ec2.go
File environs/ec2/ec2.go (left):

https://codereview.appspot.com/6307071/diff/9002/environs/ec2/ec2.go#oldcode373
environs/ec2/ec2.go:373: if len(ids) == 0 {
On 2012/06/13 19:47:58, niemeyer wrote:
> This should be brought back.

Done.

https://codereview.appspot.com/6307071/diff/9002/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6307071/diff/9002/environs/interface.go#newcod...
environs/interface.go:133: // AllInstances returns all Instances currently known
to the Environ.
On 2012/06/13 19:47:58, niemeyer wrote:
> s/Instances/instances/
> s/Environ/environment/

Done.

[niemeyer, 2012-06-15 01:14:02]

I guess rog didn't see the note. This looks good and straightforward, though.
Please feel free to go ahead. Rog's comments, if any, may be addressed in a
follow up.

https://codereview.appspot.com/6307071/diff/9002/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6307071/diff/9002/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:39: c.Assert(insts, HasLen, 2)
c.Assert(insts[0].Id(), Not(Equals), insts[1].Id())

[dave_cheney.net, 2012-06-15 01:31:16]

*** Submitted:

environs: add the facility to query all running instances

The PA needs to be able to find out (with some degree of accuracy)
all the instance id's known to the Environ.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6307071

https://codereview.appspot.com/6307071/diff/9002/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6307071/diff/9002/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:39: c.Assert(insts, HasLen, 2)
On 2012/06/15 01:14:02, niemeyer wrote:
> c.Assert(insts[0].Id(), Not(Equals), insts[1].Id()) 

Done.

[rog, 2012-06-15 05:55:33]

LGTM. sorry for the delay.

https://codereview.appspot.com/6307071/diff/13002/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6307071/diff/13002/environs/ec2/ec2.go#newcode330
environs/ec2/ec2.go:330: func (e *environ) gatherInstances(ids []string, insts
[]environs.Instance) error {
this method is associated with Instances. i think either AllInstances should
move before it, or it should move after Instances.

https://codereview.appspot.com/6307071/diff/13002/environs/ec2/ec2.go#newcode383
environs/ec2/ec2.go:383: insts = append(insts, &instance{e, &r.Instances[i]})
i think it's worth following gatherInstances here and making sure that every
instance doesn't keep all the other instances around:
inst := r.Instances[i]
insts = append(insts, &instance{e, &inst})