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

17 + # vlan_num, bridge, net_attrs=None):

Commented code lines usually cause a "Needs fixing" review, better remove them :)

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.

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.

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

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?

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.

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?

Cheers,
Salvatore