You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Koushik Das <ko...@citrix.com> on 2013/02/01 20:10:35 UTC
Re: Review Request: Patch1 for feature - (CLOUDSTACK-657) VMware vNetwork
Distributed Virtual Switch support in CloudStack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9189/#review16022
-----------------------------------------------------------
I see that you have splitted the patches. Can you provide some details about what each individual patch is doing?
api/src/com/cloud/network/TrafficLabel.java
<https://reviews.apache.org/r/9189/#comment34322>
Some comment would help as to why this is needed.
plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java
<https://reviews.apache.org/r/9189/#comment34324>
What are the possible values for networkLabel. Currently you are handling the format a,b,c. What if someone passes only a or a,b or a,b,c,d?
plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java
<https://reviews.apache.org/r/9189/#comment34325>
What is the need for this setter? Doesn't provide good encapsulation. If _vSwitchType is set in the ctor and then modified using the setter there is discrepancy between _networkLabel and this.
vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java
<https://reviews.apache.org/r/9189/#comment34326>
TODO?
- Koushik Das
On Jan. 31, 2013, 5:21 p.m., Sateesh Chodapuneedi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9189/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2013, 5:21 p.m.)
>
>
> Review request for cloudstack, Murali Reddy and Kelven Yang.
>
>
> Description
> -------
>
> This is 1st patch for feature 'Support for VMware dvSwitch in CloudStack'.
> Virtual switch type could be chosen at zone level or at cluster level for specific traffic type.
> UI functionality is also included.
> autoExpand of dvPortGroup is available in code but disabled as its breaking because vCenter 4.1 does not support autoExpand feature.
>
>
> This addresses bug CLOUDSTACK-657.
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/TrafficLabel.java PRE-CREATION
> plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java PRE-CREATION
> vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9189/diff/
>
>
> Testing
> -------
>
> Manual testing:-
> 1) Tested guest traffic over dvSwitch on a dedicated physical network. In this case management and public traffic uses standard vSwitch on a common physical network.
> 2) Tested both guest traffic and public traffic over dvSwitch on a physical network.
> 3) Use optional parameters added to AddClusterCmd to override Zone level network traffic label. Tested 2 clusters, one with standard vSwitch and other with dvSwitch.
> 4) Tested all 3 traffic types on single physical network with global parameter 'vmware.use.dvswitch' set to false. This is default configuration scenario.
>
>
> Added following tests,
> 1) Test fetching dvSwitch object from vCenter
> 2) Test for presence of dvPortGroup
> 3) Test presence of dvPortGroup
> 4) Test get existing dvPortGroup
> 5) fetch dvPortGroup configuration
> 6) Test compare dvPortGroup configuration
> 7) Test update dvPortGroup configuration
>
>
> Thanks,
>
> Sateesh Chodapuneedi
>
>
Re: Review Request: Patch1 for feature - (CLOUDSTACK-657) VMware vNetwork
Distributed Virtual Switch support in CloudStack
Posted by Sateesh Chodapuneedi <sa...@citrix.com>.
> On Feb. 1, 2013, 7:10 p.m., Koushik Das wrote:
> > plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java, line 15
> > <https://reviews.apache.org/r/9189/diff/1/?file=254158#file254158line15>
> >
> > What are the possible values for networkLabel. Currently you are handling the format a,b,c. What if someone passes only a or a,b or a,b,c,d?
Let me provide an expression that would best represent the format - [["Name of vSwitch/dvSwitch"][,["VLAN ID"][,["vSwitch Type"]]]]
Description - All the 3 fields are optional
Default values for the 3 fields :-
vSwitch0 for "Name of vSwitch/dvSwitch"
null (which would be untagged network) for "VLAN ID"
"VMware vNetwork Standard virtual switch" for "vSwitch Type"
Possible values for 3 fields:-
"Name of vSwitch/dvSwitch" - any valid vSwitch name or just empty
"VLAN ID" - 1 to 4095 or just empty
"vSwitch Type" - vmwaredvs (for vmware dvswitch) or vmwaresvs (for vmware standard vswitch) or just empty
As per above mentioned format, furnishing few possible values for networkLabel,
[1] ""
[2] "Name of vSwitch/dvSwitch"
[3] "Name of vSwitch/dvSwitch","VLAN ID"
[4] "Name of vSwitch/dvSwitch","VLAN ID","vSwitch Type" - Example: dvSwitch0,100,vmwaredvs
[5] "Name of vSwitch/dvSwitch",,"vSwitch Type" - Example: dvSwitch0,,vmwaredvs
> On Feb. 1, 2013, 7:10 p.m., Koushik Das wrote:
> > vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java, line 28
> > <https://reviews.apache.org/r/9189/diff/1/?file=254159#file254159line28>
> >
> > TODO?
TODO meant to quote number of dvports per dvportgroup (which is maximum number of interfaces that could participate in a virtual network) could be updated here. It's place holder (to place code to update number of dvports) in case of such a requirement/enhancement request.
> On Feb. 1, 2013, 7:10 p.m., Koushik Das wrote:
> > plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java, line 64
> > <https://reviews.apache.org/r/9189/diff/1/?file=254158#file254158line64>
> >
> > What is the need for this setter? Doesn't provide good encapsulation. If _vSwitchType is set in the ctor and then modified using the setter there is discrepancy between _networkLabel and this.
_networkLabel comes from traffic label specified in zone configuration.
Idea is to provide the administrator more choice in choosing the type of virtual switch to be used at cluster level too. That means admin can choose to use dvSwitch for specific cluster where as all other clusters in that zone would use standard vswitch. Hence trying to override what is present in _networkLabel.
- Sateesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9189/#review16022
-----------------------------------------------------------
On Feb. 5, 2013, 2:21 a.m., Sateesh Chodapuneedi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9189/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 2:21 a.m.)
>
>
> Review request for cloudstack, Murali Reddy and Kelven Yang.
>
>
> Description
> -------
>
> This is 1st patch for feature 'Support for VMware dvSwitch in CloudStack'.
> This contains 3 newly introduced classes. Added apache license header for all 3 files.
> [1]TrafficLable and [2]VmwareTrafficLabel classes are to define and encapsulate virtual switch type per traffic type along with other network label fields (VLAN ID and physical network).
> [3]DistributedVirtualSwitchMO class is wrapper class for vSphere API calls specific to a distributed virtual switch in a vCenter datacenter.
> Feature highlights:
> Virtual switch type could be chosen at zone level or at cluster level for specific traffic type. All virtual network orchestration would use the specified virtual switch. So far CloudStack could use only vSwitch of type standard vSwitch, with this feature CloudStack can use VMware dvSwitch as well. Support for VMware dvSwitch is available for guest and public traffic types.
> autoExpand of dvPortGroup is available in code but disabled as its breaking because vCenter 4.1 does not support autoExpand feature.
>
>
> This addresses bug CLOUDSTACK-657.
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/TrafficLabel.java PRE-CREATION
> api/src/com/cloud/network/TrafficLabel.java PRE-CREATION
> plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java PRE-CREATION
> vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9189/diff/
>
>
> Testing
> -------
>
> Manual testing:-
> 1) Tested guest traffic over dvSwitch on a dedicated physical network. In this case management and public traffic uses standard vSwitch on a common physical network.
> 2) Tested both guest traffic and public traffic over dvSwitch on a physical network.
> 3) Use optional parameters added to AddClusterCmd to override Zone level network traffic label. Tested 2 clusters, one with standard vSwitch and other with dvSwitch.
> 4) Tested all 3 traffic types on single physical network with global parameter 'vmware.use.dvswitch' set to false. This is default configuration scenario.
>
>
> Added following tests,
> 1) Test fetching dvSwitch object from vCenter
> 2) Test for presence of dvPortGroup
> 3) Test presence of dvPortGroup
> 4) Test get existing dvPortGroup
> 5) fetch dvPortGroup configuration
> 6) Test compare dvPortGroup configuration
> 7) Test update dvPortGroup configuration
>
>
> Thanks,
>
> Sateesh Chodapuneedi
>
>
Re: Review Request: Patch1 for feature - (CLOUDSTACK-657) VMware vNetwork
Distributed Virtual Switch support in CloudStack
Posted by Koushik Das <ko...@citrix.com>.
> On Feb. 1, 2013, 7:10 p.m., Koushik Das wrote:
> > plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java, line 64
> > <https://reviews.apache.org/r/9189/diff/1/?file=254158#file254158line64>
> >
> > What is the need for this setter? Doesn't provide good encapsulation. If _vSwitchType is set in the ctor and then modified using the setter there is discrepancy between _networkLabel and this.
>
> Sateesh Chodapuneedi wrote:
> _networkLabel comes from traffic label specified in zone configuration.
> Idea is to provide the administrator more choice in choosing the type of virtual switch to be used at cluster level too. That means admin can choose to use dvSwitch for specific cluster where as all other clusters in that zone would use standard vswitch. Hence trying to override what is present in _networkLabel.
Why only switch type and not name?
- Koushik
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9189/#review16022
-----------------------------------------------------------
On Feb. 5, 2013, 4:09 a.m., Sateesh Chodapuneedi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9189/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2013, 4:09 a.m.)
>
>
> Review request for cloudstack, Murali Reddy and Kelven Yang.
>
>
> Description
> -------
>
> This is 1st patch for feature 'Support for VMware dvSwitch in CloudStack'.
> This contains 3 newly introduced classes. Added apache license header for all 3 files.
> [1]TrafficLable and [2]VmwareTrafficLabel classes are to define and encapsulate virtual switch type per traffic type along with other network label fields (VLAN ID and physical network).
> [3]DistributedVirtualSwitchMO class is wrapper class for vSphere API calls specific to a distributed virtual switch in a vCenter datacenter.
> Feature highlights:
> Virtual switch type could be chosen at zone level or at cluster level for specific traffic type. All virtual network orchestration would use the specified virtual switch. So far CloudStack could use only vSwitch of type standard vSwitch, with this feature CloudStack can use VMware dvSwitch as well. Support for VMware dvSwitch is available for guest and public traffic types.
> autoExpand of dvPortGroup is available in code but disabled as its breaking because vCenter 4.1 does not support autoExpand feature.
>
>
> This addresses bug CLOUDSTACK-657.
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/TrafficLabel.java PRE-CREATION
> plugins/hypervisors/vmware/src/com/cloud/network/VmwareTrafficLabel.java PRE-CREATION
> vmware-base/src/com/cloud/hypervisor/vmware/mo/DistributedVirtualSwitchMO.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/9189/diff/
>
>
> Testing
> -------
>
> Manual testing:-
> 1) Tested guest traffic over dvSwitch on a dedicated physical network. In this case management and public traffic uses standard vSwitch on a common physical network.
> 2) Tested both guest traffic and public traffic over dvSwitch on a physical network.
> 3) Use optional parameters added to AddClusterCmd to override Zone level network traffic label. Tested 2 clusters, one with standard vSwitch and other with dvSwitch.
> 4) Tested all 3 traffic types on single physical network with global parameter 'vmware.use.dvswitch' set to false. This is default configuration scenario.
>
>
> Added following tests,
> 1) Test fetching dvSwitch object from vCenter
> 2) Test for presence of dvPortGroup
> 3) Test presence of dvPortGroup
> 4) Test get existing dvPortGroup
> 5) fetch dvPortGroup configuration
> 6) Test compare dvPortGroup configuration
> 7) Test update dvPortGroup configuration
>
>
> Thanks,
>
> Sateesh Chodapuneedi
>
>