nova double: regexp filtering by name

nova double: regexp filtering by name

Filtering by server name in ListServers/Detail() now
supports regular expression, as Openstack does.
Implemented in the double and added tests.
Needed in juju to filter instances by environment
(now part of the instance name when started by juju).

https://code.launchpad.net/~dimitern/goose/nova-instance-filtering/+merge/146647

(do not edit description out of merge proposal)

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

Please take a look.

[jameinel, 2013-02-05 16:46:14]

This seems fine, though it really feels like we are missing live tests here.
Especially to test that how we think the test double should work actually
matches how a few regex's are evaluated by the server.

Is there a way to trigger regex searches without having to spin up lots of
machines (just to match against names). If not, we can probably spin up say 3
and compare.

Also, it would be good to have some tests for what happens when we issue bad
regexes and trap the error conditions, so we can simulate those in the test
doubles, too.

Using MustCompile() should change, but otherwise LGTM.

https://codereview.appspot.com/7309045/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7309045/diff/1/testservices/novaservice/servic...
testservices/novaservice/service.go:252: rex := regexp.MustCompile(val)
Must compile triggers a panic() if it fails to compile. Which is why you use it
for initializing values. But it would be really bad if you accidentally sent
incorrect regexp and it crashed the test server. So we'll want to use the error
checking version here.

[dimitern, 2013-02-07 20:13:27]

Please take a look.

https://codereview.appspot.com/7309045/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7309045/diff/1/testservices/novaservice/servic...
testservices/novaservice/service.go:252: rex := regexp.MustCompile(val)
On 2013/02/05 16:46:14, jameinel wrote:
> Must compile triggers a panic() if it fails to compile. Which is why you use
it
> for initializing values. But it would be really bad if you accidentally sent
> incorrect regexp and it crashed the test server. So we'll want to use the
error
> checking version here.

This is now gone and changed.

[wallyworld, 2013-02-08 03:49:16]

LGTM.
I wonder if we want to do a live test with multiple filters. It seems to me we
should have such a test. It will be nice to ensure that Canonistack and HP
CLound and ... all behave as we are expecting.

[jameinel, 2013-02-08 04:50:17]

I think LGTM, some tweaks requested.

Can we get away with removing Add() entirely, rather than just documenting that
Add really means Set? (I'd rather not have code that *looks* like it is adding,
when it is actually overriding.)

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go#newcode90
nova/nova.go:90: 
Why have an Add function if you are changing everything to use Set() ? If it is
true that only one value is supported, is there a way to get only
ACTIVE+BUILDING nodes (so we don't see SHUTDOWN nodes, etc)?

Can we just not implement Add() since it would be confusing to call Add() and
not have the values stack as expected?

Or is this just a shortcut and the real Openstack supports Add() and we are just
doing Set() until we decide we need more functionality? If so, we probably want
to add a comment about "For now..."

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:261: }
You could probably just do
 copy(servers, n.servers)
http://golang.org/pkg/builtin/#copy

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:265: var matched []nova.ServerDetail
I realize you use matched later, but I'm pretty sure you don't ever actually
want the variable to leak to the next clause. As such, it seems better to just
use a local variable here.
Possibly even pull the whole loop of matching against status into a separate
private function for clarity.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:279: if nameRex :=
filter.Get(nova.FilterServer); nameRex != "" {
This block seems useful as another private func.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:282: fmt.Printf("cannot compile regexp
filter %q: %v\n", filter, err)
Should this be some sort of logging?

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service_http_test.go (left):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_http_test.go:593: filter.Add(nova.FilterServer,
"srv2")
It seems we could support Add by doing a regex "|" operation. ("srv1|srv2"
should match both).

However, if you've tested against Canonistack and "Add" doesn't actually build
them up, then certainly just go with Set().

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service_http_test.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_http_test.go:595:
c.Assert(expected.Servers[0].Name, DeepEquals, servers[0].Name)
I'm pretty sure Name and Id should be "Equals" not "DeepEquals". You did confirm
that passing 2 "name=" values into the request results in you only getting 1
value from Canonistack?

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service_test.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:305: }
These all seem like separate tests, rather than more assertions on the same
test.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:317: }
If we decide we don't need Add, then we probably don't need this test.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:338: func (s *NovaSuite)
TestAllServersWithRegexFilters(c *C) {
This looks like a good test of it.

[dimitern, 2013-02-08 12:33:53]

On 2013/02/08 03:49:16, wallyworld wrote:
> LGTM.
> I wonder if we want to do a live test with multiple filters. It seems to me we
> should have such a test. It will be nice to ensure that Canonistack and HP
> CLound and ... all behave as we are expecting.

As discussed with Roger, I implemented in juju filtering only by name and then
iterating over the servers to filter out ones not ACTIVE or BUILD. Since we're
not able reliably to launch multiple machines and have them in different
statuses, and filtering by status only allows a single status, I don't really
see the point in filtering on both name and status. Thoughts?

[dimitern, 2013-02-08 13:00:40]

Please take a look.

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go#newcode90
nova/nova.go:90: 
On 2013/02/08 04:50:17, jameinel wrote:
> Why have an Add function if you are changing everything to use Set() ? If it
is
> true that only one value is supported, is there a way to get only
> ACTIVE+BUILDING nodes (so we don't see SHUTDOWN nodes, etc)?
> 
> Can we just not implement Add() since it would be confusing to call Add() and
> not have the values stack as expected?
> 
> Or is this just a shortcut and the real Openstack supports Add() and we are
just
> doing Set() until we decide we need more functionality? If so, we probably
want
> to add a comment about "For now..."

No, OpenStack does not support multiple value filters, so effectively Add() and
Set() are equivalent for our purposes.  I could though have Add() panic, so we
don't accidentally call it instead of Set(), but I figured it's better like this
- it's documented and it behaves consistently.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:261: }
On 2013/02/08 04:50:17, jameinel wrote:
> You could probably just do
>  copy(servers, n.servers)
> http://golang.org/pkg/builtin/#copy

No, I tried that - copy() works on slices, and strings, not maps.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:265: var matched []nova.ServerDetail
On 2013/02/08 04:50:17, jameinel wrote:
> I realize you use matched later, but I'm pretty sure you don't ever actually
> want the variable to leak to the next clause. As such, it seems better to just
> use a local variable here.
> Possibly even pull the whole loop of matching against status into a separate
> private function for clarity.

Ok, I'll make matched a local variable, but I don't think splitting these 2
cases in separate functions will provide benefits.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:279: if nameRex :=
filter.Get(nova.FilterServer); nameRex != "" {
On 2013/02/08 04:50:17, jameinel wrote:
> This block seems useful as another private func.

Why? it's short and readable? I went over several iterations to minimize code
duplication here.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service.go:282: fmt.Printf("cannot compile regexp
filter %q: %v\n", filter, err)
On 2013/02/08 04:50:17, jameinel wrote:
> Should this be some sort of logging?

We're not using logging for the doubles, maybe we should. Anyway, since this is
only the double, I think a Printf serves the same purpose.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service_http_test.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_http_test.go:595:
c.Assert(expected.Servers[0].Name, DeepEquals, servers[0].Name)
On 2013/02/08 04:50:17, jameinel wrote:
> I'm pretty sure Name and Id should be "Equals" not "DeepEquals". You did
confirm
> that passing 2 "name=" values into the request results in you only getting 1
> value from Canonistack?

Yes, I can confirm that - nova uses only the last filter specified. But you're
right - these asserts should use Equals instead. Done.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
File testservices/novaservice/service_test.go (right):

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:305: }
On 2013/02/08 04:50:17, jameinel wrote:
> These all seem like separate tests, rather than more assertions on the same
> test.
I don't think so - they're all related and the setup code will be duplicated.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:317: }
On 2013/02/08 04:50:17, jameinel wrote:
> If we decide we don't need Add, then we probably don't need this test.
Agreed, but I needed a test to confirm the behavior.

https://codereview.appspot.com/7309045/diff/1004/testservices/novaservice/ser...
testservices/novaservice/service_test.go:338: func (s *NovaSuite)
TestAllServersWithRegexFilters(c *C) {
On 2013/02/08 04:50:17, jameinel wrote:
> This looks like a good test of it.
Yes, it is.

[dimitern, 2013-02-08 13:03:52]

*** Submitted through the bot.

I think it's better to have it landed and possibly change it later, even more
with the upcoming release today.

[dimitern, 2013-02-08 13:10:01]

On 2013/02/08 13:03:52, dimitern wrote:
> *** Submitted through the bot.
> 
> I think it's better to have it landed and possibly change it later, even more
> with the upcoming release today.

Oops, merge conflicts - resubmitting after fixing.

[dimitern, 2013-02-08 13:10:28]

Please take a look.

[rog, 2013-02-08 13:46:34]

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7309045/diff/1004/nova/nova.go#newcode90
nova/nova.go:90: 
On 2013/02/08 13:00:40, dimitern wrote:
> On 2013/02/08 04:50:17, jameinel wrote:
> > Why have an Add function if you are changing everything to use Set() ? If it
> is
> > true that only one value is supported, is there a way to get only
> > ACTIVE+BUILDING nodes (so we don't see SHUTDOWN nodes, etc)?
> > 
> > Can we just not implement Add() since it would be confusing to call Add()
and
> > not have the values stack as expected?
> > 
> > Or is this just a shortcut and the real Openstack supports Add() and we are
> just
> > doing Set() until we decide we need more functionality? If so, we probably
> want
> > to add a comment about "For now..."
> 
> No, OpenStack does not support multiple value filters, so effectively Add()
and
> Set() are equivalent for our purposes.  I could though have Add() panic, so we
> don't accidentally call it instead of Set(), but I figured it's better like
this
> - it's documented and it behaves consistently.

if Add doesn't work, why have the method at all?

[dimitern, 2013-02-08 14:15:21]

On 2013/02/08 13:46:34, rog wrote:
> https://codereview.appspot.com/7309045/diff/1004/nova/nova.go
> File nova/nova.go (right):
> 
> https://codereview.appspot.com/7309045/diff/1004/nova/nova.go#newcode90
> nova/nova.go:90: 
> On 2013/02/08 13:00:40, dimitern wrote:
> > On 2013/02/08 04:50:17, jameinel wrote:
> > > Why have an Add function if you are changing everything to use Set() ? If
it
> > is
> > > true that only one value is supported, is there a way to get only
> > > ACTIVE+BUILDING nodes (so we don't see SHUTDOWN nodes, etc)?
> > > 
> > > Can we just not implement Add() since it would be confusing to call Add()
> and
> > > not have the values stack as expected?
> > > 
> > > Or is this just a shortcut and the real Openstack supports Add() and we
are
> > just
> > > doing Set() until we decide we need more functionality? If so, we probably
> > want
> > > to add a comment about "For now..."
> > 
> > No, OpenStack does not support multiple value filters, so effectively Add()
> and
> > Set() are equivalent for our purposes.  I could though have Add() panic, so
we
> > don't accidentally call it instead of Set(), but I figured it's better like
> this
> > - it's documented and it behaves consistently.
> 
> if Add doesn't work, why have the method at all?

As discussed, I proposed a follow-up:
https://codereview.appspot.com/7301064