Code review comment for lp:~sateesh-chodapuneedi/nova/lp831497

Thanks Salvatore for reviewing the branch.
I have addressed your comments and updated the branch.
Please find my comments inline.

> 17 + # vlan_num, bridge, net_attrs=None):
>
> Commented code lines usually cause a "Needs fixing" review, better remove them
> :)

Removed this comment.

> VMWareVlanBridgeDriver.unplug
>
> I understand leaving it unimplemented, as it is the same approach adopted for
> libvirt and xenapi. We might think in the future to add something which
> removes the port group if the VIF being unplugged is the last one on that port
> group. I don't see a case for doing it now, though.

I think it's better to address this as separate bug, leaving this patch to address basic multi-nic only.

> 47 + vif_spec_list = []
> 48 + for vif_info in vif_infos:
> 49 + vif_spec = create_network_spec(client_factory,
> 50 + vif_info[0],
> 51 + vif_info[1],
> 52 + vif_info[2])
>
> This is another minor thing. Can we use a dictionary instead of a sequence for
> vif_info? That would improve readability, and make the code less error-prone.

True, addressed now.

> Also, It would be great if we can address Arvind Somya's comment in
> vm_util.create_network_spec, line 109

Yes, Arvind's fix to add support to distribute virtual port group does support only if the binding type is ' ephemeral'. We can try to address this for other types of binding that are, static and dynamic. But I think it would be better to have separate launchpad bug to address this effort.

>
> In vm_util.create_network_spec we use fixed temporary key for the device
> (-47). Isn't there a chance to have issues if two network specs are created
> for the same vm with the same key?
No, there is no chance of key clash, as it's temporary in the specification. And we are not colliding with key of existing device. Because all existing devices will have +ve keys.
Here is some text from API documentation about the device keys.

"When adding new devices, it may be necessary for a client to assign keys temporarily in order to associate controllers with devices in configuring a virtual machine. However, the server does not allow a client to reassign a device key, and the server may assign a different value from the one passed during configuration. Clients should ensure that existing device keys are not reused as temporary key values for the new device to be added (for example, by using negative integers as temporary keys)."

> 190 + interface_str = "%s;%s;%s;%s;%s;%s" % \
> 191 + (info['mac'],
> 192 + ip_v4 and ip_v4['ip'] or '',
> 193 + ip_v4 and ip_v4['netmask'] or '',
> 194 + info['gateway'],
> 195 + info['broadcast'],
> 196 + dns)
>
> The machine_id_str now is the concatenation of
> MAC,ip,netmask,gateway,broadcast, and DNS for each interface. Is there a limit
> on the number of characters for this string? We will roughly have 92
> characters for each interface. I understand this is important since the guest
> tools require the machine id to be structured like this.

The machind_id_str is being set to machine.id configuration parameter of the VM, which is written to vmx file. I have searched for any maximum limit for this configuration parameters but could not find it yet. I guess the limit would be quite high because the value is being stored in vmx file, which will stay on disk. Also I have tested the string in with over 3000 characters and found it working, which would account for more than 30 NICs of information if we take 100 characters per each NIC record. May be we can consider is safe for now.

> However, several tests in VMWareAPITestCase (10) fail. This might be due to
> something being wrong on my dev machine. Do they pass on your dev machine?

Yes, I have observed this and fixed it. This code that caused the failure is fake_get_network() that returns a list with fake network information. It's fault that did not comeup before because of lesser code coverage. Now I have modified this to return dictionary object which is apt as per the code that refers to network discovered from the associated bridge name.

Also did pep8 checks.

> Cheers,
> Salvatore

Kindly go through the branch and let me know your view. Thanks again :)