Merge lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-volumes into lp://qastaging/~hudson-openstack/nova/trunk

Added prerequisite branch

It looks like there are changes in this merge proposal that are in your Keys branch? If your servicify-nova-api already has the changes from the Keys branch in it, then you need to mark the merge proposal for the servicify-nova-api as having a pre-requisite on the keys branch... otherwise, the merge proposal diffs from dependent branches contain the changes from their parent branches...

The api-keys and api-volumes patch became all intertwined with servicify and some other branches, so I'm just proposing merging them both together in this patch.

I like where this is going. I'd like to give this a test. Is there a command line utility somewhere that you are using to try this out? Can we get some instructions for how to put it through its paces?

Vish

I've primarily been using the stub-API implementation in the unit test
suite, as right now I'm just adding these APIs in support of unit testing.

However, I did add volumes to (my fork of) jk0's openstack python bindings
on github:
https://github.com/justinsb/python-novatools (my fork)
https://github.com/justinsb/python-novatools/commit/13a58c7a7f0ecb451e272e9b5b0b099bcef3c0f0
(the
patch)

I'm not really sure what the state of play is on the bindings. Are we
bringing jk0's bindings into the core project? Should I submit a pull
request to jk0 in the meantime and we can work on github until this is
brought into the main tree? Should I be extending django-nova instead?

I'm happy to add support for zones & test that the volume support actually
works outside of unit tests though - just let me know where I should be
pushing code!

On Sat, Feb 26, 2011 at 2:07 AM, Vish Ishaya <email address hidden> wrote:

> Review: Needs Information
> I like where this is going. I'd like to give this a test. Is there a
> command line utility somewhere that you are using to try this out? Can we
> get some instructions for how to put it through its paces?
>
> Vish
> --
>
> https://code.launchpad.net/~justin-fathomdb/nova/justinsb-openstack-api-volumes/+merge/50868
> You are the owner of
> lp:~justin-fathomdb/nova/justinsb-openstack-api-volumes.
>

Sorry - I meant "support for keys & test that the volume support actually
works", rather than zones.

On Sat, Feb 26, 2011 at 2:12 PM, Justin Santa Barbara
<email address hidden>wrote:

> I've primarily been using the stub-API implementation in the unit test
> suite, as right now I'm just adding these APIs in support of unit testing.
>
> However, I did add volumes to (my fork of) jk0's openstack python bindings
> on github:
> https://github.com/justinsb/python-novatools (my fork)
>
> https://github.com/justinsb/python-novatools/commit/13a58c7a7f0ecb451e272e9b5b0b099bcef3c0f0 (the
> patch)
>
> I'm not really sure what the state of play is on the bindings. Are we
> bringing jk0's bindings into the core project? Should I submit a pull
> request to jk0 in the meantime and we can work on github until this is
> brought into the main tree? Should I be extending django-nova instead?
>
> I'm happy to add support for zones & test that the volume support actually
> works outside of unit tests though - just let me know where I should be
> pushing code!
>
>
>
>
> On Sat, Feb 26, 2011 at 2:07 AM, Vish Ishaya <email address hidden>wrote:
>
>> Review: Needs Information
>> I like where this is going. I'd like to give this a test. Is there a
>> command line utility somewhere that you are using to try this out? Can we
>> get some instructions for how to put it through its paces?
>>
>> Vish
>> --
>>
>> https://code.launchpad.net/~justin-fathomdb/nova/justinsb-openstack-api-volumes/+merge/50868
>> You are the owner of
>> lp:~justin-fathomdb/nova/justinsb-openstack-api-volumes.
>>
>
>

OK, I kind of understand the serviceify branch now. In Glance, we took a completely different strategy for integration/functional-like testing by actually starting the services using the bin/glance-control server script (it's almost identical to Swift's bin/swift-init) with temporary config files, and then issuing requests against the servers using both a programmatic client class as well as things like curl.

But, I see what you're doing here and I think it's workable.

Stuff like this is alarming though:

1030 + # TODO(justinsb): Shutdown WSGI & anything else we startup
1031 + # WSGI shutdown broken :-(
1032 + # self.wsgi_server.terminate()
1033 + # self.wsgi_server = None

Also, you've got a bunch of commented-out test code in the volume stuff. Not sure if you want to uncomment that, or if you need to remove it?

Cheers!
jay

Jay: I think that starting services in-process is a nice way to do this in
tests, rather than relying on a 'deus ex machina', because that way you can
start services on unused ports (particularly useful when you want multiple
services), or start them with different configs, or get access to internal
state for testing etc.

The WSGI shutdown broken thing is why I wanted known_bugs. That's a no-go,
so I can either leave it commented out or just delete it. I think commented
out is better.

Otherwise I'm going to do a cleanup when this branch's pre-reqs are approved
for merge.

On Mon, Mar 7, 2011 at 3:33 PM, Jay Pipes <email address hidden> wrote:

> OK, I kind of understand the serviceify branch now. In Glance, we took a
> completely different strategy for integration/functional-like testing by
> actually starting the services using the bin/glance-control server script
> (it's almost identical to Swift's bin/swift-init) with temporary config
> files, and then issuing requests against the servers using both a
> programmatic client class as well as things like curl.
>
> But, I see what you're doing here and I think it's workable.
>
> Stuff like this is alarming though:
>
> 1030 + # TODO(justinsb): Shutdown WSGI & anything else we startup
> 1031 + # WSGI shutdown broken :-(
> 1032 + # self.wsgi_server.terminate()
> 1033 + # self.wsgi_server = None
>
> Also, you've got a bunch of commented-out test code in the volume stuff.
> Not sure if you want to uncomment that, or if you need to remove it?
>
> Cheers!
> jay
> --
>
> https://code.launchpad.net/~justin-fathomdb/nova/justinsb-openstack-api-volumes/+merge/50868
> You are the owner of
> lp:~justin-fathomdb/nova/justinsb-openstack-api-volumes.
>

> Jay: I think that starting services in-process is a nice way to do this in
> tests, rather than relying on a 'deus ex machina', because that way you can
> start services on unused ports (particularly useful when you want multiple
> services), or start them with different configs, or get access to internal
> state for testing etc.

This is exactly what we do in Glance. :) We don't start the bin/glance-api "in process" in the functional tests because I've found that there are a number of things that come up when you start a process daemonized that were being masked by the in-process stuff (like logging)

> The WSGI shutdown broken thing is why I wanted known_bugs. That's a no-go,
> so I can either leave it commented out or just delete it. I think commented
> out is better.

Ah, ok. Well, the note and commented out is fine I suppose. Just file a bug on it ;)

> Otherwise I'm going to do a cleanup when this branch's pre-reqs are approved
> for merge.

I only know of the one pre-req (servicify). Are you referring to others? (keys, perhaps?)

-jay

OK, I've re-read this patch about 7 times now. I hate to do this, but I'm going to say Disapprove on this one. What is unfortunate is that this patch gloms together a whole bunch of *new* stuff along with tests of that new stuff. It doesn't actually add tests for the existing OpenStack API functionality, which is what was needed for Cactus stability. I believe this patch only destabilizes Cactus by adding "test-only" functionality to the OpenStack API that isn't even in the OpenStack API spec and then testing these new codepaths while not testing anything else.

I'm sorry, Justin, but this code just doesn't pass muster on something to go into Cactus, IMHO.

If this patch (and a number of the merge props that depend on it) were broken down into more manageable chunks, and chunks that had bugs and blueprints specific to Cactus, it may have been doable, but the string of merge proposals that this depends on as well as this patch just don't work for me.

If others feel very strongly about this, feel free to comment on this merge proposal. I have to move on to the other merge proposals tied to specific blueprints for Cactus, though.

-jay

Jay and I have discussed this, and I think it boils down to a difference of interpretation of "stability":

Internal Stability - i.e. stability of the code. We minimize code changes, so that we will eventually find and fix all the bugs.

External Stability - i.e. a nova that works for users. It's OK to make big code changes as long as the goal is to produce a more reliable, less buggy product. We accept that we may sometimes introduce bugs to get to a reliable product faster.

My goal is "External Stability" - my tests try to do exactly what an external user would do. I trust that if these tests pass, nova won't blow up immediately for users. (This isn't true of the current tests.) Towards that goal, I'll refactor code, I'll introduce (flag-disabled) APIs until we have a formal API for the required functionality (e.g. keys), I'll write a simple OpenStack API client until we decide whether to merge one into lp:nova.

Of course, "Internal Stability" Jay then (rightly, based on his definition) rejects this as not being "on-goal" for Cactus.

Jay and I then argue in circles, but I believe it boils down to "What does stability mean?"

So: What does stability mean?