> On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > >
Thanks for the review Sheng. Please find my comments below. I'll upload newer diffs with some of the suggested changes shortly. > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > server/src/com/cloud/network/NetworkModelImpl.java, line 933 > > <https://reviews.apache.org/r/11019/diff/1/?file=289101#file289101line933> > > > > What's this for? I hit an NPE at com.cloud.network.NetworkModelImpl.getNetworkRate(NetworkModelImpl.java:929) because the reference to VMInstanceVO would be null since the vm (router VM in this case) wouldn't exist yet. getNetworkOfferingNetworkRate() internally returns -1 to indicate unlimited network rate if it cannot obtain the networkRate from the provided networkofferingid and dcId. So I put in a check for null values and returned -1 if the call cannot be made. But I think it's better I pass null values instead if either is null, and let getNetworkOfferingNetworkRate() take care of the values passed in. Will make the required change. > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > server/src/com/cloud/vm/UserVmManagerImpl.java, line 2848 > > <https://reviews.apache.org/r/11019/diff/1/?file=289102#file289102line2848> > > > > Can VMware just ignore the command? Rather than put hypervisor specific > > code here. Yes VMWare can ignore it because the we configure the port groups in the start command path. The migration code path works differently with DVS since we don't have to plumb it on each host again after the first time when we configure the DVS (which pushes out the required config to each host it manages). The vmwareresource will still implement a stub of this method to not cause warnings when the VM is destroyed. > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > server/src/com/cloud/vm/VirtualMachineManagerImpl.java, line 56 > > <https://reviews.apache.org/r/11019/diff/1/?file=289103#file289103line56> > > > > What's the change in this file for? Not used anywhere. Eclipse generated. Will remove it if it isn't being used anywhere. > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java, > > line 74 > > <https://reviews.apache.org/r/11019/diff/1/?file=289104#file289104line74> > > > > I am confused by pvlanid existed with vlanid here. Do you mean > > "isolated pvlan"(which is secondary vlan)? Yes > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java, > > line 485 > > <https://reviews.apache.org/r/11019/diff/1/?file=289105#file289105line485> > > > > better using e.g. secondary vlan id. pvlanid is confusing. I'll change that to isolatedpvlanid, should be clearer that way. > On May 9, 2013, 6:54 p.m., Sheng Yang wrote: > > vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java, > > line 746 > > <https://reviews.apache.org/r/11019/diff/1/?file=289105#file289105line746> > > > > Reason to remove type converting? Again, eclipse generated, but why would we need that cast? I would remove it. - Venkata Siva Vijayendra ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11019/#review20387 ----------------------------------------------------------- On May 9, 2013, 7:09 p.m., Venkata Siva Vijayendra Bhamidipati wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11019/ > ----------------------------------------------------------- > > (Updated May 9, 2013, 7:09 p.m.) > > > Review request for cloudstack, Chip Childers, Sheng Yang, Sateesh > Chodapuneedi, Kelven Yang, and Animesh Chaturvedi. > > > Description > ------- > > Please find attached the diffs for pvlan support for vmware DVSwitch > deployments on cloudstack. You will find two diffs - the parent diff is > Sateesh's fix for CLOUSTACK-2316 which is needed to be cherry-picked on the > pvlan branch from the master. The other diff contains the changes for pvlan > support. > > These diffs do not contain changes for pvlan provisioning on the Cisco Nexus > 1000v distributed virtual switch. > > > This addresses bug CLOUDSTACK-1456. > > > Diffs > ----- > > > plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java > 99ad1ca > server/src/com/cloud/network/NetworkManagerImpl.java 7a09eb5 > server/src/com/cloud/network/NetworkModelImpl.java bd62886 > server/src/com/cloud/vm/UserVmManagerImpl.java 683f0da > server/src/com/cloud/vm/VirtualMachineManagerImpl.java b0d6378 > > vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java > 247be2a > vmware-base/src/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java > 7f323c5 > > Diff: https://reviews.apache.org/r/11019/diff/ > > > Testing > ------- > > The code has been tested on the Vmware DVSwitch for advanced shared networks > on vmware cluster deployments on cloudstack. Unit tests will be the same as > those provided by Sheng as part of the overall PVLAN support for XenServer > and KVM, and will exercise the vmware pvlan code path when user VMs are > created with vNICs sitting on advanced shared networks that have the optional > Private VLAN value set during their creation. VM live migration using vmware > vMotion has also been tested with these changes on vmware and it works as > expected. > > Further testing will be carried out and this review request will be updated > accordingly. > > > Thanks, > > Venkata Siva Vijayendra Bhamidipati > >
