OpenStack Compute (nova)

Code review comment for lp:~tr3buchet/nova/multi_nic

multi_nic
Merge into trunk

Revision history for this message

Trey Morris (tr3buchet) wrote on 2011-06-16:

Brian, I agree it's definitely too long. I should have come up with a better way to do incremental merges along the way to finishing.

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.

1646-1648: This change doesn't seem related to this merge prop. I personally prefer the code you replaced.
4279-4284: This change is also unnecessary.

I have no memory of altering either of these files..... That scares me a little, maybe a bad merge? I'll change them back.. I know for certain I never touched an old migration.

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.

2129: Could you make NoFixedIpsDefinedForHost inherit from NoFixedIpsDefined? If there are any other exceptions like this that you added I would appreciate it if you would define the correct inheritance. It can be really handy.

It also seems that NoFloatingIpsDefined, NoFloatingIpsDefinedForHost, and NoFloatingIpsDefinedForInstance aren't used anymore. Should we delete those?

I can do this yes. But I'd still like a response to the question posed in the previous paragraph. Seems cleaner to have a FixedIP class exception and it handled all of the possible messages.

Some of the floating ip exceptions are not being used and they should be.

I corrected an error where someone had been using floating ip exception classes with the fixed ips by copy and pasting what they had done for floating ips in the exceptions and didn't really put much thought into the exceptions themselves. I guess they never raised similar exceptions for the floating ips. I can correct this.

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.

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.
--

I think we may want to follow up with another merge prop to make the OSAPI display this new information correctly. Right now we have hard-coded public/private networks. We should really be using the new network labels. Again, this isn't something I expect in this MP, just something I don't want us to forget about.

Yep definitely agree. We shouldn't copy the ec2 api functionality that fixed_ips are private and floating_ips are public because there are use cases for public fixed_ips and use cases that ignore floating ips altogether. I believe the OSAPI should expose the labels for fixed ips and also floating ips as exactly that which they are.

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