rohityadavcloud commented on code in PR #8151:
URL: https://github.com/apache/cloudstack/pull/8151#discussion_r1374167540
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -56,6 +58,17 @@ public class TrafficTypeResponse extends BaseResponse {
@Param(description = "The network name label of the physical device
dedicated to this traffic on a HyperV host")
private String hypervNetworkLabel;
+ // why not VLAN_ID
+ @SerializedName(ApiConstants.VLAN)
+ @Param(description = "The VLAN id to be used for Management traffic by
VMware host")
+ private String vlan;
+
+
+
Review Comment:
nit - unnecessary new lines
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -4997,6 +4997,7 @@ public PhysicalNetworkTrafficType
addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 &&
isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation
methods, why are multiple isolation methods allowed per pnet? They seem to be
filterable by provider/preifx (PhysicalNetwork.java). Documentation says only
two are allowed but code and tests contradict this.
Review Comment:
Yes, that would be okay.
##########
server/src/main/java/com/cloud/api/ApiResponseHelper.java:
##########
@@ -3082,6 +3082,7 @@ public TrafficTypeResponse
createTrafficTypeResponse(PhysicalNetworkTrafficType
PhysicalNetwork pnet =
ApiDBUtils.findPhysicalNetworkById(result.getPhysicalNetworkId());
if (pnet != null) {
response.setPhysicalNetworkId(pnet.getUuid());
+ response.setIsolationMethods(pnet.getIsolationMethods());
Review Comment:
Worth testing if the serialised API response (json/xml), converts the string
list as expected.
##########
server/src/main/java/com/cloud/network/NetworkServiceImpl.java:
##########
@@ -5148,6 +5148,7 @@ public PhysicalNetworkTrafficType
addTrafficTypeToPhysicalNetwork(Long physicalN
if (TrafficType.Public.equals(trafficType)) {
List<String> isolationMethods = network.getIsolationMethods();
if ((isolationMethods.size() == 1 &&
isolationMethods.get(0).toLowerCase().equals("vxlan"))
+ // TODO: what happens if there are multiple isolation
methods, why are multiple isolation methods allowed per pnet? They seem to be
filterable by provider/preifx (PhysicalNetwork.java). Documentation says only
two are allowed but code and tests contradict this.
Review Comment:
In theory a physical network represents a network device on the phy. host;
Something say a eth0 or a bond0 (if it's a team/bond) or a bridge0 (if it's
bridge on the hypervisor host), but then a phy network could carry tagged VLAN
traffic such as eth0.10, or bridge0-10 carrying VLAN 10 etc. For the same phy.
network, a shared network with a different VLAN can be created.
I don't have the historical context to answer why it's the way it is,
generally we don't expect more than a single isolation method of a phy network
- but some network plugins can decide to do things differently for example
support both VLAN, GRE etc. so it's possible the isolate methods is a list. For
the sake of the issue you're trying to address, the scope is limited to
returning data that's already in the database.
##########
api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java:
##########
@@ -128,4 +141,19 @@ public String getOvm3Label() {
public void setOvm3Label(String ovm3Label) {
this.ovm3NetworkLabel = ovm3Label;
}
+
+ public List<String> getIsolationMethods() {
+ return isolationMethods;
+ }
+
+ public void setIsolationMethods(List<String> isolationMethods) {
+ this.isolationMethods = isolationMethods;
+ }
Review Comment:
nit - missing new line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
