OpenStack Compute (nova)

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

lp831497
Merge into peer-review

Revision history for this message

Salvatore Orlando (salvatore-orlando) wrote on 2011-08-30:

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

Agreed.

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

That probably makes sense. I'm not an ESX expert. How frequently non-ephemeral bindings occur? Or are they ephemeral because we create them in this way?

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

Very good.

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

I think so :). Thanks for testing it with big strings.

>
>
> > 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 :)

I think the branch is ok now.
I'm planning to do some functional tests on my ESX server here in Cambourne, but I'm still installing the environment.
However, as the unit tests now pass, and pep8 is clean, I reckon it is safe to propose the code for merge in lp:nova!

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

Agreed.

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

That probably makes sense. I'm not an ESX expert. How frequently non-ephemeral bindings occur? Or are they ephemeral because we create them in this way?

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

Very good.

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

I think so :). Thanks for testing it with big strings.

> 
> 
> > 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 :)

I think the branch is ok now. 
I'm planning to do some functional tests on my ESX server here in Cambourne, but I'm still installing the environment. 
However, as the unit tests now pass, and pep8 is clean, I reckon it is safe to propose the code for merge in lp:nova!

review: Approve

« Back to merge proposal