rhtyd commented on a change in pull request #4561:
URL: https://github.com/apache/cloudstack/pull/4561#discussion_r565907779
##########
File path:
api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkCmd.java
##########
@@ -16,13 +16,13 @@
// under the License.
package org.apache.cloudstack.api.command.user.network;
+import org.apache.cloudstack.api.BaseCmd;
import org.apache.log4j.Logger;
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.ApiErrorCode;
-import org.apache.cloudstack.api.BaseCmd;
Review comment:
Import reshuffle does not look sortable. You can fix your intellij
settings or get preset settings (mine are here
https://github.com/rhtyd/dotfiles/blob/master/intellij/settings.jar)
##########
File path: core/src/main/java/com/cloud/agent/api/MigrateCommand.java
##########
@@ -40,6 +40,7 @@
private boolean executeInSequence = false;
private List<MigrateDiskInfo> migrateDiskInfoList = new ArrayList<>();
private Map<String, DpdkTO> dpdkInterfaceMapping = new HashMap<>();
+ Map<String, Boolean> vlanToPersistenceMap;
Review comment:
Just a note - if field is not initialised, the consumer of getter must
do a NPE check.
##########
File path:
plugins/api/discovery/src/main/java/org/apache/cloudstack/discovery/ApiDiscoveryServiceImpl.java
##########
@@ -67,7 +68,7 @@ public boolean start() {
if (s_apiNameDiscoveryResponseMap == null) {
long startTime = System.nanoTime();
s_apiNameDiscoveryResponseMap = new HashMap<String,
ApiDiscoveryResponse>();
- Set<Class<?>> cmdClasses = new HashSet<Class<?>>();
+ Set<Class<?>> cmdClasses = new LinkedHashSet<Class<?>>();
Review comment:
minor minor nit - The other PR/fix has come here.
##########
File path:
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -2864,6 +2962,33 @@ public boolean shutdownNetworkElementsAndResources(final
ReservationContext cont
return success;
}
+ private void cleanupPersistentnNetworkResources(NetworkVO network) {
+ long networkOfferingId = network.getNetworkOfferingId();
+ NetworkOfferingVO offering =
_networkOfferingDao.findById(networkOfferingId);
+ if (offering != null) {
+ if (networkMeetsPersistenceCriteria(network, offering, true) &&
+
_networksDao.getOtherPersistentNetworksCount(network.getId(),
network.getBroadcastUri().toString(), offering.isPersistent()) == 0) {
+ List<HostVO> hosts =
resourceManager.listAllUpAndEnabledHostsInOneZoneByType(Host.Type.Routing,
network.getDataCenterId());
+ for (HostVO host : hosts) {
+ try {
+ NicTO to = createNicTOFromNetworkAndOffering(network,
offering, host);
+ CleanupPersistentNetworkResourceCommand cmd = new
CleanupPersistentNetworkResourceCommand(to);
+ CleanupPersistentNetworkResourceAnswer answer =
(CleanupPersistentNetworkResourceAnswer) _agentMgr.send(host.getId(), cmd);
+ if (answer == null) {
+ s_logger.warn("Unable to get an answer to the
CleanupPersistentNetworkResourceCommand from agent:" + host.getId());
+ }
+
+ if (!answer.getResult()) {
+ s_logger.warn("Unable to setup agent " +
host.getId() + " due to " + answer.getDetails());
+ }
+ } catch (Exception e) {
+ s_logger.warn("Failed to cleanup network resources");
Review comment:
Can we log on which host it was failed, or one of the warning/log above?
##########
File path:
core/src/main/java/com/cloud/agent/api/CleanupPersistentNetworkResourceCommand.java
##########
@@ -0,0 +1,43 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.agent.api;
+
+import com.cloud.agent.api.to.NicTO;
+
+public class CleanupPersistentNetworkResourceCommand extends Command{
Review comment:
minor nit - space after `Command {`
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java
##########
@@ -376,6 +376,7 @@ private void deleteVnetBr(String brName) {
command.add("-v", vNetId);
command.add("-p", pName);
command.add("-b", brName);
+ command.add("-d", String.valueOf(deleteBr));
Review comment:
should `deleteBr` be used to add the -d command?
##########
File path: core/src/main/java/com/cloud/agent/api/StopCommand.java
##########
@@ -36,6 +36,7 @@
String controlIp = null;
boolean forceStop = false;
private Map<String, DpdkTO> dpdkInterfaceMapping;
+ Map<String, Boolean> vlanToPersistenceMap;
Review comment:
same as before, NPE check for consumer
##########
File path:
core/src/main/java/com/cloud/agent/api/SetupPersistentNetworkCommand.java
##########
@@ -0,0 +1,41 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.agent.api;
+
+import com.cloud.agent.api.to.NicTO;
+
+public class SetupPersistentNetworkCommand extends Command{
Review comment:
minor nit - same `Command {`
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1827,6 +1848,63 @@ private void orchestrateStop(final String vmUuid, final
boolean cleanUpEvenIfUna
advanceStop(vm, cleanUpEvenIfUnableToStop);
}
+ private void getPersistenceMap(Map<String, Boolean> vlanToPersistenceMap,
NetworkVO networkVO) {
+ NetworkOfferingVO offeringVO =
networkOfferingDao.findById(networkVO.getNetworkOfferingId());
+ if (offeringVO != null) {
+ Pair<String, Boolean> data = getVMNetworkDetails(networkVO,
offeringVO.isPersistent());
+ Boolean shouldDeleteNwResource =
(MapUtils.isNotEmpty(vlanToPersistenceMap) && data != null) ?
vlanToPersistenceMap.get(data.first()) : null;
+ if (data != null && (shouldDeleteNwResource == null ||
shouldDeleteNwResource)) {
+ vlanToPersistenceMap.put(data.first(), data.second());
+ }
+ }
+ }
+
+ 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());
+ getPersistenceMap(vlanToPersistenceMap, networkVO);
+ }
+ if (userVmJoinVOS.isEmpty()) {
+ VMInstanceVO vmInstanceVO = _vmDao.findById(vmId);
+ if (vmInstanceVO != null && vmInstanceVO.getType() ==
VirtualMachine.Type.DomainRouter) {
+ DomainRouterJoinVO routerVO =
domainRouterJoinDao.findById(vmId);
+ NetworkVO networkVO =
_networkDao.findById(routerVO.getNetworkId());
+ getPersistenceMap(vlanToPersistenceMap, networkVO);
+ }
+ }
+ return vlanToPersistenceMap;
+ }
+
+ /**
+ *
+ * @param networkVO - the network object used to determine the vlanId from
the broadcast URI
+ * @param isPersistent - indicates if the corresponding network's network
offering is Persistent
+ *
+ * @return <VlanId, ShouldKVMBridgeBeDeleted> - basically returns the vlan
ID which is used to determine the
+ * bridge name for KVM hypervisor and based on the network and isolation
type and persistent setting of the offering
+ * we decide whether the bridge is to be deleted (KVM) if the last VM in
that host is destroyed / migrated
+ */
+ private Pair<String, Boolean> getVMNetworkDetails(NetworkVO networkVO,
boolean isPersistent) {
+ URI broadcastUri = networkVO.getBroadcastUri();
+ if (broadcastUri != null) {
+ String scheme = broadcastUri.getScheme();
+ String vlanId =
Networks.BroadcastDomainType.getValue(broadcastUri);
+ boolean shouldDelete = !((networkVO.getGuestType() ==
Network.GuestType.L2 || networkVO.getGuestType() == Network.GuestType.Isolated)
&&
Review comment:
What about vxlan, @Pearl1594 are persistent networks support for VXLAN
isolation?
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1640,7 +1657,11 @@ private void unmanageVMNics(VirtualMachineProfile
profile, VMInstanceVO vm) {
protected boolean sendStop(final VirtualMachineGuru guru, final
VirtualMachineProfile profile, final boolean force, final boolean
checkBeforeCleanup) {
final VirtualMachine vm = profile.getVirtualMachine();
+ Map<String, Boolean> vlanToPersistenceMap =
getVlanToPersistenceMapForVM(vm.getId());
StopCommand stpCmd = new StopCommand(vm,
getExecuteInSequence(vm.getHypervisorType()), checkBeforeCleanup);
+ if (MapUtils.isNotEmpty(vlanToPersistenceMap)) {
+ stpCmd.setVlanToPersistenceMap(vlanToPersistenceMap);
Review comment:
minor nit - could be simply made to set the
`getVlanToPersistenceMapForVM` output
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/BridgeVifDriver.java
##########
@@ -436,4 +437,20 @@ public boolean isExistingBridge(String bridgeName) {
return false;
}
}
+
+ @Override
+ public void deleteBr(NicTO nic) {
+ String vlanId =
Networks.BroadcastDomainType.getValue(nic.getBroadcastUri());
+ String trafficLabel = nic.getName();
+ String pifName = _pifs.get(trafficLabel);
+ if (pifName == null) {
+ // if not found in bridge map, maybe traffic label refers to pif
already?
+ File pif = new File("/sys/class/net/" + trafficLabel);
+ if (pif.isDirectory()) {
+ pifName = trafficLabel;
+ }
+ }
+ String brName = generateVnetBrName(pifName, vlanId);
Review comment:
should null check be done here again?
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCleanupPersistentNetworkResourceCommandWrapper.java
##########
@@ -0,0 +1,44 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package com.cloud.hypervisor.kvm.resource.wrapper;
+
+import org.apache.log4j.Logger;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.CleanupPersistentNetworkResourceAnswer;
+import com.cloud.agent.api.CleanupPersistentNetworkResourceCommand;
+import com.cloud.agent.api.to.NicTO;
+import com.cloud.hypervisor.kvm.resource.BridgeVifDriver;
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.hypervisor.kvm.resource.VifDriver;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.ResourceWrapper;
+
+@ResourceWrapper(handles = CleanupPersistentNetworkResourceCommand.class)
+public class LibvirtCleanupPersistentNetworkResourceCommandWrapper extends
CommandWrapper<CleanupPersistentNetworkResourceCommand, Answer,
LibvirtComputingResource> {
+ private static final Logger s_logger =
Logger.getLogger(LibvirtCleanupPersistentNetworkResourceCommandWrapper.class);
+ @Override
+ public Answer execute(CleanupPersistentNetworkResourceCommand command,
LibvirtComputingResource serverResource) {
+ NicTO nic = command.getNicTO();
+ VifDriver driver = serverResource.getVifDriver(nic.getType());
+ if (driver instanceof BridgeVifDriver) {
Review comment:
Is persistent network/L2 not supported for OVS bridge?
----------------------------------------------------------------
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]
