Code review comment for lp:~openstack-gd/nova/libvirt-multinic-nova

Revision history for this message
Eldar Nugaev (reldan) wrote :

Hi Matt.
Thank you too much for your review.

We're synchronizing our changes with Trey Morris, he's working on add multi-nic support for xen https://code.launchpad.net/~tr3buchet/nova/xs_multi_nic/+merge/53458. So we did our changes by the same way and at this moment it is not changing functionality. Existed unittest already covered our changes. But we agree with you. Let us merge our changes to the trunk before FF and create additional tests as a bug fix.

Next blueprint https://blueprints.launchpad.net/nova/+spec/nova-multi-nic contains changes in db model.

> I'd like to see a lot more unit tests covering this functionality. The only
> addition I see is you updated one test to check for a mac address. Considering
> the substantial change this represents, going forward untested is pretty
> scary. Do you think you could throw a few tests in there?
>
> Nit-picks / Style junk:
>
> 42 + return (mac64_addr ^ netaddr.IPAddress('::0200:0:0:0') |
> maskIP).\
> 43 + format()
>
> I'd like to see format() indented at least 8 spaces for readability's sake.
>
> 146 + #TODO(ilyaalekseyev) If we will keep this function
> 147 + # we should cache network_info
>
> You should be consistent with your spacing. I'd move TODO over one space

« Back to merge proposal