openstack: filter instances by environ

openstack: filter instances by environ

Instances() and AllInstances() now filter returned instances
by status (BUILD, ACTIVE) and name ("juju-<envname>-machine-#"),
using regular expression, as supported by Openstack. Needs the
related changes in goose:
https://codereview.appspot.com/7309045

This also enables one more live test - TestStartStop().

https://code.launchpad.net/~dimitern/juju-core/openstack-instance-filtering/+merge/146648

(do not edit description out of merge proposal)

[dimitern, 2013-02-05 15:10:56]

Please take a look.

[jameinel, 2013-02-06 07:24:11]

Overall I like where this is going, but there are a couple of small issues.

1) You have an O(N^2) algorithm when dealing with processing of servers. Which
is probably fine up to 10, but will be pretty bad at 2000.

2) You didn't add any direct tests of this code. IMO we should be reasonably
sure that we didn't break the codebase by just running the regular suite
(without -live), and only use -live to validate that our view of reality is
still correct.

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

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:632: machineName := fmt.Sprintf("juju-%s-%s",
e.Name(), state.MachineEntityName(scfg.machineId))
This feels like it should be pulled out into a separate "e.machineName" or
something like that. That way it is clearly defined in one spot, and can be
reused if we want to refer to the machine later.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:692: }
You don't use need again, so I'm guessing this was originally trying to do
filtering based on need?
If we are just going to do a boolean check, then a simple bool seems fine.

var needAny = false
for _, inst := range insts {
  if inst == nil {
    needAny = true
    break
  }
}
if not needAny {
  return nil
}

Alternatively, you could use something like need to check if you could special
case a single id (so rather than doing a regex lookup and filtering all, you
could do a single exact lookup.) That would be more like:

needMany := false
exactNeed := ""
for i, inst := rang insts {
  if inst == nil {
    if exactNeed != "" {
      // We have already gotten one that we need, now there are 2, grab them all
      needMany = true
      exactNeed = ""
      break
    } else {
      exactNeed = string(ids[i])
    }
  }
}
if exactNeed == "" && !needMany {
  return nil
}

if needMany {
  // Request all and filter (probably in another func?
} else {
  // Request just the one we care about
}

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:714: }
This is O(N^2) in the number of servers you are looking up. (If you have 50
servers missing their instances, it will do 50*50 comparisons.) Note that you
don't break out of the loop early either, so you always do N^2.

This gets really bad if you consider trying to scale to 2000 servers (4M
comparisons)

If this is the best way to do it, then the obvious thing is to build a map of
string(id) => j

We might want to use the first loop over the input to build up a map based on
what was supplied by the user, then change the loop ordering here to only loop
over the servers returned, which would then pop data out of the map as the
server information gets filled in.

I'm not quite sure how this code handles passing in repeated instances (it looks
like it will do the query and just fill in multiple locations). Is it meant to
handle it, or is it fine to just say "you should not repeat instances here". You
can handle either method (a map[string][]string rather than map[string]string,
etc).
But it would be good to clarify, and make sure the results are tested.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:718: }
This seems like the kind of place where tracking *what* instances didn't get
filled in would be a really good fit for the map reduction I described above,
rather than just counting.
That way the error that gets logged isn't "I couldn't lookup all the instances",
but instead "I failed to find details for instances X, Y and Z".

You could put the missing ids into the context parameter of the exception.
(Which would be why we want instance exceptions, rather than singleton
exceptions.)

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:755: filter.Add(nova.FilterStatus,
nova.StatusActive)
The fact that this is a duplicate of what you have earlier makes me wish it was
pulled out into a common helper. (e.getMachineFilter()) or something to that
effect. Does that seem reasonable?

[dimitern, 2013-02-08 01:06:14]

Please take a look.

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

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:632: machineName := fmt.Sprintf("juju-%s-%s",
e.Name(), state.MachineEntityName(scfg.machineId))
On 2013/02/06 07:24:11, jameinel wrote:
> This feels like it should be pulled out into a separate "e.machineName" or
> something like that. That way it is clearly defined in one spot, and can be
> reused if we want to refer to the machine later.

Done.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:692: }
On 2013/02/06 07:24:11, jameinel wrote:
> You don't use need again, so I'm guessing this was originally trying to do
> filtering based on need?
> If we are just going to do a boolean check, then a simple bool seems fine.
> 
> var needAny = false
> for _, inst := range insts {
>   if inst == nil {
>     needAny = true
>     break
>   }
> }
> if not needAny {
>   return nil
> }
> 
> Alternatively, you could use something like need to check if you could special
> case a single id (so rather than doing a regex lookup and filtering all, you
> could do a single exact lookup.) That would be more like:
> 
> needMany := false
> exactNeed := ""
> for i, inst := rang insts {
>   if inst == nil {
>     if exactNeed != "" {
>       // We have already gotten one that we need, now there are 2, grab them
all
>       needMany = true
>       exactNeed = ""
>       break
>     } else {
>       exactNeed = string(ids[i])
>     }
>   }
> }
> if exactNeed == "" && !needMany {
>   return nil
> }
> 
> if needMany {
>   // Request all and filter (probably in another func?
> } else {
>   // Request just the one we care about
> }

This comes almost directly from ec2, and looking at it now I agree it could be
simplified. I'm going with your second suggestion.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:714: }
On 2013/02/06 07:24:11, jameinel wrote:
> This is O(N^2) in the number of servers you are looking up. (If you have 50
> servers missing their instances, it will do 50*50 comparisons.) Note that you
> don't break out of the loop early either, so you always do N^2.
> 
> This gets really bad if you consider trying to scale to 2000 servers (4M
> comparisons)
> 
> If this is the best way to do it, then the obvious thing is to build a map of
> string(id) => j
> 
> We might want to use the first loop over the input to build up a map based on
> what was supplied by the user, then change the loop ordering here to only loop
> over the servers returned, which would then pop data out of the map as the
> server information gets filled in.
> 
> I'm not quite sure how this code handles passing in repeated instances (it
looks
> like it will do the query and just fill in multiple locations). Is it meant to
> handle it, or is it fine to just say "you should not repeat instances here".
You
> can handle either method (a map[string][]string rather than map[string]string,
> etc).
> But it would be good to clarify, and make sure the results are tested.

I changed the code to use a map lookup and added more tests for duplicates, etc.
It should be O(n) now or thereabouts. It also handles duplicates gracefully.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:718: }
On 2013/02/06 07:24:11, jameinel wrote:
> This seems like the kind of place where tracking *what* instances didn't get
> filled in would be a really good fit for the map reduction I described above,
> rather than just counting.
> That way the error that gets logged isn't "I couldn't lookup all the
instances",
> but instead "I failed to find details for instances X, Y and Z".
> 
> You could put the missing ids into the context parameter of the exception.
> (Which would be why we want instance exceptions, rather than singleton
> exceptions.)

Reporting which instances were not found is not required by juju, otherwise the
ec2 provider will report that. Anyway with the changes above I think now should
perform better.

https://codereview.appspot.com/7299047/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:755: filter.Add(nova.FilterStatus,
nova.StatusActive)
On 2013/02/06 07:24:11, jameinel wrote:
> The fact that this is a duplicate of what you have earlier makes me wish it
was
> pulled out into a common helper. (e.getMachineFilter()) or something to that
> effect. Does that seem reasonable?

Done.

[fwereade, 2013-02-08 13:29:42]

A few thoughts:

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:220: expect error
`err error`, perhaps?

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:224: expect: nil,
Zero values don't need to be specified, which will save a bit of space...

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:226: {
...as will putting the `{` on the same line as the preceding `},`. YMMV, but I
find it makes the tables more readable.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:283: c.Assert(inst0, NotNil)
I'd be inclined to drop this assert -- I presume StartInstance is tested
elsewhere, and if the result is nil here we'll find out soon enough ;).

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:687: for i, inst := range insts {
I have a suspicion that all this could be simplified dramatically if it took
([]state.InstanceId, map[state.InstanceId]environs.Instance); collected the
instances into the map; and just returned an []state.InstanceId (indicating the
ids which *might* be worth checking again -- ie not the empty ones, or those
referring to instances with unsuitable statuses). It could then be used as
follows:

missing := ids
found := map[state.InstanceId]environs.Instance{}
for a := shortAttempt.Start(); a.Next(); {
    if missing = e.gatherInstances(missing, found); len(missing) == 0 {
        break
    }
}

...and it would be a simple matter for Instances to loop over ids, putting
appropriate instances from found into insts, and figuring out what error (if
any) to return.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:711: return environs.ErrPartialInstances
I don't think we should keep retrying this until the attempt finishes -- it's
not going to improve with time.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:723: server, err := e.nova().GetServer(needOne)
This appears not to filter for machines in the environment.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:766: if err == nil || err !=
environs.ErrPartialInstances {
You can drop the err == nil check.

[dimitern, 2013-02-08 16:12:29]

Please take a look.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:220: expect error
On 2013/02/08 13:29:42, fwereade wrote:
> `err error`, perhaps?

Done.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:224: expect: nil,
On 2013/02/08 13:29:42, fwereade wrote:
> Zero values don't need to be specified, which will save a bit of space...

Good point! Done.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:226: {
On 2013/02/08 13:29:42, fwereade wrote:
> ...as will putting the `{` on the same line as the preceding `},`. YMMV, but I
> find it makes the tables more readable.

Done.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/local_tes...
environs/openstack/local_test.go:283: c.Assert(inst0, NotNil)
On 2013/02/08 13:29:42, fwereade wrote:
> I'd be inclined to drop this assert -- I presume StartInstance is tested
> elsewhere, and if the result is nil here we'll find out soon enough ;).

Done.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:687: for i, inst := range insts {
On 2013/02/08 13:29:42, fwereade wrote:
> I have a suspicion that all this could be simplified dramatically if it took
> ([]state.InstanceId, map[state.InstanceId]environs.Instance); collected the
> instances into the map; and just returned an []state.InstanceId (indicating
the
> ids which *might* be worth checking again -- ie not the empty ones, or those
> referring to instances with unsuitable statuses). It could then be used as
> follows:
> 
> missing := ids
> found := map[state.InstanceId]environs.Instance{}
> for a := shortAttempt.Start(); a.Next(); {
>     if missing = e.gatherInstances(missing, found); len(missing) == 0 {
>         break
>     }
> }
> 
> ...and it would be a simple matter for Instances to loop over ids, putting
> appropriate instances from found into insts, and figuring out what error (if
> any) to return.

Refactored to be simpler, as discussed.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:711: return environs.ErrPartialInstances
On 2013/02/08 13:29:42, fwereade wrote:
> I don't think we should keep retrying this until the attempt finishes -- it's
> not going to improve with time.
It makes the logic more consistent, but I agree it'll take some futile
iterations. I can make it bail out early here.
Anyway with the simplified version now, this hopefully is no longer an issue.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:723: server, err := e.nova().GetServer(needOne)
On 2013/02/08 13:29:42, fwereade wrote:
> This appears not to filter for machines in the environment.

It doesn't. I assume if you're asking for a specific instance you know it's part
of the env - isn't it?
Now this code is gone anyway.

https://codereview.appspot.com/7299047/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:766: if err == nil || err !=
environs.ErrPartialInstances {
On 2013/02/08 13:29:42, fwereade wrote:
> You can drop the err == nil check.

This code is now gone and simplified.

[dimitern, 2013-02-08 18:39:21]

Please take a look.

[dimitern, 2013-02-08 18:41:07]

*** Submitted:

openstack: filter instances by environ

Instances() and AllInstances() now filter returned instances
by status (BUILD, ACTIVE) and name ("juju-<envname>-machine-#"),
using regular expression, as supported by Openstack. Needs the
related changes in goose:
https://codereview.appspot.com/7309045

This also enables one more live test - TestStartStop().

R=jameinel, fwereade
CC=
https://codereview.appspot.com/7299047