>     619: This new method name implies returning a single fixed_ip, but it
> still returns a list. Would you mind changing it back? Same goes for the
> 'fixed_ip_get_by_virtual_interface.' I feel like that name should be
> pluralized since it also returns a list.
> 
> I went back and forth about this a few times.. If, for example, an instance
> may have multiple widgets, then it seems widget_get_by_instance() should
> return that list. Otherwise we'd have widget_get_all_by_instance() and
> widget_get_first_by_instance() and widget_get_last_by_instance() and..... In
> addition, there are other similar functions, should they all be changed, and
> to what? widgets_get_by_instance(), widget_get_all_by_instance(),
> widgets_get_all_by_instance() ? There doesn't seem to be a standard. Maybe we
> should set one.

I see what you mean. We also have the methods 'instance_get_fixed_addresses' which is another option.

>     2106: Can you make this inherit from NovaException and change the name to
> something more descriptive? Maybe "VirtualInterfaceCreationFailed" or
> something. I know it's long, but it describes the error better than just
> "VirtualInterface."
> 
> Is it the name of the exception that is important or the message? For example,
> I don't see why we'd want to have 50 different VirtualInterface exception
> classes when we can have one that can handle any and all VirtualInterface
> exception messages. If there is a reason, please excuse my ignorance.

It may just be my preference, but I like to use exception names to communicate the actual error, while the message can have more of a description and any extra information (through keyword arguments). I also like the inheritance hierarchy because you can try/except a more basic 'VirtualInterfaceError' and catch any of its more specific subclasses. Again, it may just be my personal preference. Maybe somebody else can chime in here.

>     3947: Aren't we supposed to put "Copyright 2011 OpenStack LLC." on all of
> the copyright notices, with any other contributing companies listed under it?
> I may just not know the correct policy here.
> 
> someone else will have to answer this. I don't know anything about it. Once I
> noticed people putting their own name there I stopped caring.

Vish, Jay, etc: Is this documented anywhere?

>     As for all of the skipped tests: I would prefer to see them fixed, but if
> it is going to be a lot of work, I am okay leaving them for now. I think we
> may want to file a bug or something so we don't forget about it.
> 
> Skipped tests. Horrible I know but there is a reason for madness. Some of the
> changes to nova impact the hypervisors (and the API's as well as you noted,
> but I think I've got that sorted so they work). I don't think that as a nova
> developer I should be required to support any and all hypervisors that are
> included in the project. Even what's more I surely can't be required to test
> all of my changes across all of the hypervisors. This partly resulted in the
> formation of the lieutenants for the different aspects of nova. I know for a
> fact that my changes have broken the hypervisors that I haven't chosen to
> support, and I'm fine with that because I can't really do anything about it
> and have no intention of using these hypervisors. That said, I will certainly
> help the lieutenants or anyone in the community using those hypervisors in any
> way I can to update their hypervisors and hypervisor tests to work with
> changes that I have put into nova trunk.
> 
> I'm happy with filing bugs, but it'll be hard to forget as it shows up every
> time you run the unittests.

Yeah, I think you're good here. As we add support for more hypervisors, it will be difficult for a single developer to update them all.

> thanks for the thorough review! I'll get on it.

You're very welcome. I look forward to getting this merged in to trunk.