> First off, I love that fact that you're keeping the unit tests as unit tests
> (and not integration tests) ... makes the review so much easier to follow.

Yup.. something I think about when coding tests, although there are a lot of cases where unit tests are currently more like integration tests.

> 
> I guess we really need update the docs shortly after this lands.

Yeah.  I'd like to update it more myself, but I'd prefer to spend time on it after we get this merged.. Since we're very early in essex, I think this is okay.  We can file a bug after this merges.


> 
> Regarding the precooked stuff, I wonder if we could just assume all results
> are raw and strip out any potentially offending data regardless? Just be a
> little more forgiving if they don't exist.
> 
> 825     +from nova.rpc.common import RemoteError
> 
> import module not class

Copy/paste thing, but I agree.  I'll update it.

> 
> 1697    +        instances = self._schedule_run_instance(
> I thought that method returned a tuple? Is this correct?

I think you caught this below.  The schedulers' methods do return tuples so that the manager can get a 'response to return' and a 'host to schedule on'.  But the manager does really only return the 'response' portion.  I'll update/add comments in the schedulers/manager.



> 
> 
> 1950    +        instance = self.create_instance_db_entry(context,
> request_spec)
> 
> create_instance_db_entry is a static method. Should either be classname
> qualifier or remove the @staticmethod decorator, not self.

I have the same thing for 'encode_instance', etc.  I put them as static methods because they don't use 'self'... but it's a bit more clean in the code to be able to call them by self.*.  Is that a huge no-no?  If so, I think I lean towards removing the decorator even though they don't use any instance data.

> 
> 2032   # Return instance as a list and make sure the caller doesn't
> 2033    +            # cast to a compute node.
> 
>  ... not sure what the comment is trying to tell me.

Goes along with the tuple comment above.  I'll update the comment as mentioned above.

> 
> 2053    -        # Returning None short-circuits the routing to Compute (since
> 
> ... is this comment not appropriate anymore? I think some explanation of the
> None return is required (somewhere).

That comment attempts to explain why None is required, but I guess it's not descriptive enough. :)  It also goes along with the other comments above I'll update.

> 
> 2215    +            # Should only be None for tests?
> 2216    +            if filename is not None:
> Then this logic should be broken out into a separate function and stubbed in
> the test. Test case code shouldn't be in production code.

I'll do more investigation on this.  I ran into a test failure where 'filename' was not defined.

> 
> 2256    +        if isinstance(ret_val, tuple):
> ah ha ... there it is. Can we unify these results to always be a tuple? Or I
> think we'd need a test for each condition (unless I missed something there)?

I could update all scheduler methods to return a tuple, yes, and I thought about doing this, although it's only run_instance that needs to return a response.  For consistency, I'll update them all.  The alternative is that we remove the cast from driver's _schedule() and let every scheduler do its own cast like they are now doing for run_instance().

> 
> All in all ... great changes Chris! Nice to see that zone-boot and inheritance
> mess go away!

Thanks :)