nvazquez commented on a change in pull request #4561:
URL: https://github.com/apache/cloudstack/pull/4561#discussion_r550170348
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1187,6 +1200,68 @@ boolean isNetworkImplemented(final NetworkVO network) {
return implemented;
}
+ private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO
networkOfferingVO) {
+ NicTO to = new NicTO();
+ to.setBroadcastType(networkVO.getBroadcastDomainType());
+ to.setType(networkVO.getTrafficType());
+ to.setBroadcastUri(networkVO.getBroadcastUri());
+ to.setIsolationuri(networkVO.getBroadcastUri());
+
to.setNetworkRateMbps(_configMgr.getNetworkOfferingNetworkRate(networkOfferingVO.getId(),
networkVO.getDataCenterId()));
+
to.setSecurityGroupEnabled(_networkModel.isSecurityGroupSupportedInNetwork(networkVO));
+ return to;
+ }
+
+ private boolean isNtwConfiguredInCluster(HostVO hostVO, Map<Long,
List<Long>> clusterToHostsMap) {
+ Long clusterId = hostVO.getClusterId();
+ List<Long> hosts = clusterToHostsMap.get(clusterId);
+ if (hosts == null) {
+ hosts = new ArrayList<>();
+ }
+ if (hostVO.getHypervisorType() == HypervisorType.KVM) {
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+ if (hosts != null && !hosts.isEmpty()) {
+ return true;
+ }
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+
+ private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO
offering, Long dcId) throws AgentUnavailableException,
OperationTimedoutException {
+ NicTO to = getNicTO(network, offering);
+ List<ClusterVO> clusterVOS = clusterDao.listClustersByDcId(dcId);
+ List<HostVO> hosts =
resourceManager.listAllUpAndEnabledHostsInOneZoneByType(Host.Type.Routing,
dcId);
+ Collections.reverse(hosts);
+ Map<Long, List<Long>> clusterToHostsMap = new HashMap<>();
+ SetupPersistentNetworkCommand cmd = new
SetupPersistentNetworkCommand(to);
+ for (HostVO host : hosts) {
+ if (isNtwConfiguredInCluster(host, clusterToHostsMap)) {
+ continue;
+ }
+ final SetupPersistentNetworkAnswer answer =
(SetupPersistentNetworkAnswer)_agentMgr.send(host.getId(), cmd);
+
+ if (answer == null) {
+ s_logger.warn("Unable to get an answer to the
SetupPersistentNetworkCommand from agent:" + host.getId());
+
clusterToHostsMap.get(host.getClusterId()).remove(host.getId());
+ continue;
+ // throw new CloudRuntimeException("Unable to get an answer to
the SetupPersistentNetworkCommand from agent: " + host.getId());
Review comment:
Please remove
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1827,6 +1844,26 @@ private void orchestrateStop(final String vmUuid, final
boolean cleanUpEvenIfUna
advanceStop(vm, cleanUpEvenIfUnableToStop);
}
+ private Map<String, Boolean> getVlanToPersistenceMapForVM(long vmId) {
+ List<UserVmJoinVO> userVmJoinVOS = userVmJoinDao.searchByIds(vmId);
+ Map<String, Boolean> vlanToPersistenceMap = new HashMap<>();
+ for (UserVmJoinVO userVmJoinVO : userVmJoinVOS) {
+ NetworkVO networkVO =
_networkDao.findById(userVmJoinVO.getNetworkId());
+ NetworkOfferingVO offeringVO =
networkOfferingDao.findById(networkVO.getNetworkOfferingId());
+ Pair<String, Boolean> data = getVMNetworkDetails(networkVO,
offeringVO.isPersistent());
+ vlanToPersistenceMap.put(data.first(), data.second());
+ }
+ return vlanToPersistenceMap;
+ }
+
+ private Pair<String, Boolean> getVMNetworkDetails(NetworkVO networkVO,
boolean isPersistent) {
+ URI broadcastUri = networkVO.getBroadcastUri();
+ String scheme = broadcastUri.getScheme();
+ String vlanId = Networks.BroadcastDomainType.getValue(broadcastUri);
+ Boolean shouldDelete = !((networkVO.getGuestType() ==
Network.GuestType.L2 || networkVO.getGuestType() == Network.GuestType.Isolated)
&& scheme.equalsIgnoreCase("vlan") && isPersistent);
Review comment:
I think this condition could be refactored / split into multiple lines
to increase readability
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1827,6 +1844,26 @@ private void orchestrateStop(final String vmUuid, final
boolean cleanUpEvenIfUna
advanceStop(vm, cleanUpEvenIfUnableToStop);
}
+ private Map<String, Boolean> getVlanToPersistenceMapForVM(long vmId) {
+ List<UserVmJoinVO> userVmJoinVOS = userVmJoinDao.searchByIds(vmId);
+ Map<String, Boolean> vlanToPersistenceMap = new HashMap<>();
+ for (UserVmJoinVO userVmJoinVO : userVmJoinVOS) {
+ NetworkVO networkVO =
_networkDao.findById(userVmJoinVO.getNetworkId());
+ NetworkOfferingVO offeringVO =
networkOfferingDao.findById(networkVO.getNetworkOfferingId());
+ Pair<String, Boolean> data = getVMNetworkDetails(networkVO,
offeringVO.isPersistent());
+ vlanToPersistenceMap.put(data.first(), data.second());
+ }
+ return vlanToPersistenceMap;
+ }
+
+ private Pair<String, Boolean> getVMNetworkDetails(NetworkVO networkVO,
boolean isPersistent) {
Review comment:
Can you maybe rename this method and/or add javadoc to explain what does
the pair returned mean?
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##########
@@ -276,18 +277,33 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
} else {
libvirtComputingResource.destroyNetworkRulesForVM(conn, vmName);
for (final InterfaceDef iface : ifaces) {
+ String vlanId = getVlanIdFromBridgeName(iface.getBrName());
// We don't know which "traffic type" is associated with
// each interface at this point, so inform all vif drivers
final List<VifDriver> allVifDrivers =
libvirtComputingResource.getAllVifDrivers();
for (final VifDriver vifDriver : allVifDrivers) {
- vifDriver.unplug(iface);
+ vifDriver.unplug(iface, deleteBridge(vlanToPersistenceMap,
vlanId));
}
}
}
return new MigrateAnswer(command, result == null, result, null);
}
+ private String getVlanIdFromBridgeName(String brName) {
+ if (brName != null) {
Review comment:
What about `StringUtils.isNotBlank()` to also prevent empty strings?
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -3512,7 +3590,7 @@ private boolean cleanupNetworkResources(final long
networkId, final Account call
}
} catch (final ResourceUnavailableException ex) {
success = false;
- s_logger.warn("Failed to cleanup Network ACLs as a part of network
id=" + networkId + " cleanup due to resourceUnavailable ", ex);
+ s_logger.warn("Failed to c*.javaleanup Network ACLs as a part of
network id=" + networkId + " cleanup due to resourceUnavailable ", ex);
Review comment:
Small typo
##########
File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
##########
@@ -1374,10 +1374,10 @@ public Network createGuestNetwork(CreateNetworkCmd cmd)
throws InsufficientCapac
// if the network offering has persistent set to true, implement the
network
if (ntwkOff.isPersistent()) {
try {
- if (network.getState() == Network.State.Setup) {
- s_logger.debug("Network id=" + network.getId() + " is
already provisioned");
- return network;
- }
+// if (network.getState() == Network.State.Setup) {
Review comment:
Please remove
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1187,6 +1200,68 @@ boolean isNetworkImplemented(final NetworkVO network) {
return implemented;
}
+ private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO
networkOfferingVO) {
+ NicTO to = new NicTO();
+ to.setBroadcastType(networkVO.getBroadcastDomainType());
+ to.setType(networkVO.getTrafficType());
+ to.setBroadcastUri(networkVO.getBroadcastUri());
+ to.setIsolationuri(networkVO.getBroadcastUri());
+
to.setNetworkRateMbps(_configMgr.getNetworkOfferingNetworkRate(networkOfferingVO.getId(),
networkVO.getDataCenterId()));
+
to.setSecurityGroupEnabled(_networkModel.isSecurityGroupSupportedInNetwork(networkVO));
+ return to;
+ }
+
+ private boolean isNtwConfiguredInCluster(HostVO hostVO, Map<Long,
List<Long>> clusterToHostsMap) {
Review comment:
I think this method needs an explanatory javadoc as well
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1187,6 +1200,68 @@ boolean isNetworkImplemented(final NetworkVO network) {
return implemented;
}
+ private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO
networkOfferingVO) {
+ NicTO to = new NicTO();
+ to.setBroadcastType(networkVO.getBroadcastDomainType());
+ to.setType(networkVO.getTrafficType());
+ to.setBroadcastUri(networkVO.getBroadcastUri());
+ to.setIsolationuri(networkVO.getBroadcastUri());
+
to.setNetworkRateMbps(_configMgr.getNetworkOfferingNetworkRate(networkOfferingVO.getId(),
networkVO.getDataCenterId()));
+
to.setSecurityGroupEnabled(_networkModel.isSecurityGroupSupportedInNetwork(networkVO));
+ return to;
+ }
+
+ private boolean isNtwConfiguredInCluster(HostVO hostVO, Map<Long,
List<Long>> clusterToHostsMap) {
+ Long clusterId = hostVO.getClusterId();
+ List<Long> hosts = clusterToHostsMap.get(clusterId);
+ if (hosts == null) {
+ hosts = new ArrayList<>();
+ }
+ if (hostVO.getHypervisorType() == HypervisorType.KVM) {
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+ if (hosts != null && !hosts.isEmpty()) {
+ return true;
+ }
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+
+ private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO
offering, Long dcId) throws AgentUnavailableException,
OperationTimedoutException {
+ NicTO to = getNicTO(network, offering);
+ List<ClusterVO> clusterVOS = clusterDao.listClustersByDcId(dcId);
+ List<HostVO> hosts =
resourceManager.listAllUpAndEnabledHostsInOneZoneByType(Host.Type.Routing,
dcId);
+ Collections.reverse(hosts);
+ Map<Long, List<Long>> clusterToHostsMap = new HashMap<>();
+ SetupPersistentNetworkCommand cmd = new
SetupPersistentNetworkCommand(to);
+ for (HostVO host : hosts) {
+ if (isNtwConfiguredInCluster(host, clusterToHostsMap)) {
+ continue;
+ }
+ final SetupPersistentNetworkAnswer answer =
(SetupPersistentNetworkAnswer)_agentMgr.send(host.getId(), cmd);
+
+ if (answer == null) {
+ s_logger.warn("Unable to get an answer to the
SetupPersistentNetworkCommand from agent:" + host.getId());
+
clusterToHostsMap.get(host.getClusterId()).remove(host.getId());
+ continue;
+ // throw new CloudRuntimeException("Unable to get an answer to
the SetupPersistentNetworkCommand from agent: " + host.getId());
+ }
+
+ if (!answer.getResult()) {
+ s_logger.warn("Unable to setup agent " + host.getId() + " due
to " + answer.getDetails() );
+// final String msg = "Incorrect Network setup on agent,
Reinitialize agent after network names are setup, details : " +
answer.getDetails();
Review comment:
Same here
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1187,6 +1200,68 @@ boolean isNetworkImplemented(final NetworkVO network) {
return implemented;
}
+ private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO
networkOfferingVO) {
+ NicTO to = new NicTO();
+ to.setBroadcastType(networkVO.getBroadcastDomainType());
+ to.setType(networkVO.getTrafficType());
+ to.setBroadcastUri(networkVO.getBroadcastUri());
+ to.setIsolationuri(networkVO.getBroadcastUri());
+
to.setNetworkRateMbps(_configMgr.getNetworkOfferingNetworkRate(networkOfferingVO.getId(),
networkVO.getDataCenterId()));
+
to.setSecurityGroupEnabled(_networkModel.isSecurityGroupSupportedInNetwork(networkVO));
+ return to;
+ }
+
+ private boolean isNtwConfiguredInCluster(HostVO hostVO, Map<Long,
List<Long>> clusterToHostsMap) {
+ Long clusterId = hostVO.getClusterId();
+ List<Long> hosts = clusterToHostsMap.get(clusterId);
+ if (hosts == null) {
+ hosts = new ArrayList<>();
+ }
+ if (hostVO.getHypervisorType() == HypervisorType.KVM) {
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+ if (hosts != null && !hosts.isEmpty()) {
+ return true;
+ }
+ hosts.add(hostVO.getId());
+ clusterToHostsMap.put(clusterId, hosts);
+ return false;
+ }
+
+ private void setupPersistentNetwork(NetworkVO network, NetworkOfferingVO
offering, Long dcId) throws AgentUnavailableException,
OperationTimedoutException {
+ NicTO to = getNicTO(network, offering);
+ List<ClusterVO> clusterVOS = clusterDao.listClustersByDcId(dcId);
+ List<HostVO> hosts =
resourceManager.listAllUpAndEnabledHostsInOneZoneByType(Host.Type.Routing,
dcId);
+ Collections.reverse(hosts);
Review comment:
Why is the list reversed at this point?
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -1187,6 +1200,68 @@ boolean isNetworkImplemented(final NetworkVO network) {
return implemented;
}
+ private NicTO getNicTO(NetworkVO networkVO, NetworkOfferingVO
networkOfferingVO) {
Review comment:
Maybe renamed to `createNicTOFromNetworkAndOffering()` or similar?
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtMigrateCommandWrapper.java
##########
@@ -276,18 +277,33 @@ Use VIR_DOMAIN_XML_SECURE (value = 1) prior to v1.0.0.
} else {
libvirtComputingResource.destroyNetworkRulesForVM(conn, vmName);
for (final InterfaceDef iface : ifaces) {
+ String vlanId = getVlanIdFromBridgeName(iface.getBrName());
// We don't know which "traffic type" is associated with
// each interface at this point, so inform all vif drivers
final List<VifDriver> allVifDrivers =
libvirtComputingResource.getAllVifDrivers();
for (final VifDriver vifDriver : allVifDrivers) {
- vifDriver.unplug(iface);
+ vifDriver.unplug(iface, deleteBridge(vlanToPersistenceMap,
vlanId));
}
}
}
return new MigrateAnswer(command, result == null, result, null);
}
+ private String getVlanIdFromBridgeName(String brName) {
+ if (brName != null) {
+ return brName.split("-")[1];
+ }
+ return null;
+ }
+
+ private boolean deleteBridge(Map<String, Boolean> vlanToPersistenceMap,
String vlanId) {
Review comment:
Maybe renamed to `shouldDeleteBridge()` or similar?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
