prevent nic removal on out of bounds router stop (apache#8371)

DaanHoogland · vishesh92 · weizhouapache · web-flow · commit a0e592e945e4 · 2024-02-16T14:33:22.000+01:00
Co-authored-by: Vishesh &lt;vishesh92@gmail.com&gt;
Co-authored-by: Wei Zhou &lt;weizhou@apache.org&gt;
diff --git a/api/src/main/java/com/cloud/vm/NicProfile.java b/api/src/main/java/com/cloud/vm/NicProfile.java
@@ -442,6 +442,6 @@ public void deallocate() {
 
     @Override
     public String toString() {
-        return String.format("NicProfile %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "vmId", "reservationId", "iPv4Address", "broadcastUri"));
+        return String.format("NicProfile %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "vmId", "deviceId", "broadcastUri", "reservationId", "iPv4Address"));
     }
 }
diff --git a/core/src/main/java/com/cloud/serializer/GsonHelper.java b/core/src/main/java/com/cloud/serializer/GsonHelper.java
@@ -51,6 +51,8 @@ public class GsonHelper {
         GsonBuilder loggerBuilder = new GsonBuilder();
         loggerBuilder.disableHtmlEscaping();
         loggerBuilder.setExclusionStrategies(new LoggingExclusionStrategy(s_logger));
+        loggerBuilder.serializeSpecialFloatingPointValues();
+        // maybe add loggerBuilder.serializeNulls(); as well?
         s_gogger = setDefaultGsonConfig(loggerBuilder);
         s_logger.info("Default Builder inited.");
     }
diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
@@ -2310,12 +2310,12 @@ public void releaseNic(final VirtualMachineProfile vmProfile, final Nic nic) thr
 
     @DB
     protected void releaseNic(final VirtualMachineProfile vmProfile, final long nicId) throws ConcurrentOperationException, ResourceUnavailableException {
-        final Pair<Network, NicProfile> networkToRelease = Transaction.execute(new TransactionCallback<Pair<Network, NicProfile>>() {
+        final Pair<Network, NicProfile> networkToRelease = Transaction.execute(new TransactionCallback<>() {
             @Override
             public Pair<Network, NicProfile> doInTransaction(final TransactionStatus status) {
                 final NicVO nic = _nicDao.lockRow(nicId, true);
                 if (nic == null) {
-                    throw new ConcurrentOperationException("Unable to acquire lock on nic " + nic);
+                    throw new ConcurrentOperationException(String.format("Unable to acquire lock on nic id=%d", nicId));
                 }
 
                 final Nic.State originalState = nic.getState();
@@ -2329,6 +2329,9 @@ public Pair<Network, NicProfile> doInTransaction(final TransactionStatus status)
                         final NicProfile profile = new NicProfile(nic, network, nic.getBroadcastUri(), nic.getIsolationUri(), null, _networkModel
                                 .isSecurityGroupSupportedInNetwork(network), _networkModel.getNetworkTag(vmProfile.getHypervisorType(), network));
                         if (guru.release(profile, vmProfile, nic.getReservationId())) {
+                            if (s_logger.isDebugEnabled()) {
+                                s_logger.debug(String.format("The nic %s on %s was released according to %s by guru %s, now updating record.", nic, profile, vmProfile, guru));
+                            }
                             applyProfileToNicForRelease(nic, profile);
                             nic.setState(Nic.State.Allocated);
                             if (originalState == Nic.State.Reserved) {
@@ -2338,7 +2341,7 @@ public Pair<Network, NicProfile> doInTransaction(final TransactionStatus status)
                             }
                         }
                         // Perform release on network elements
-                        return new Pair<Network, NicProfile>(network, profile);
+                        return new Pair<>(network, profile);
                     } else {
                         nic.setState(Nic.State.Allocated);
                         updateNic(nic, network.getId(), -1);
@@ -2435,7 +2438,7 @@ protected void removeNic(final VirtualMachineProfile vm, final NicVO nic) {
             for (final NetworkElement element : networkElements) {
                 if (providersToImplement.contains(element.getProvider())) {
                     if (s_logger.isDebugEnabled()) {
-                        s_logger.debug("Asking " + element.getName() + " to release " + nic);
+                        s_logger.debug(String.format("Asking %s to release %s, according to the reservation strategy %s", element.getName(), nic, nic.getReservationStrategy()));
                     }
                     try {
                         element.release(network, profile, vm, null);
diff --git a/engine/schema/src/main/java/com/cloud/vm/NicVO.java b/engine/schema/src/main/java/com/cloud/vm/NicVO.java
@@ -30,6 +30,7 @@
 import javax.persistence.Table;
 import javax.persistence.Transient;
 
+import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 
@@ -329,17 +330,7 @@ public void setCreated(Date created) {
 
     @Override
     public String toString() {
-        return new StringBuilder("Nic[").append(id)
-            .append("-")
-            .append(instanceId)
-            .append("-")
-            .append(deviceId)
-            .append("-")
-            .append(reservationId)
-            .append("-")
-            .append(iPv4Address)
-            .append("]")
-            .toString();
+        return String.format("Nic %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "instanceId", "deviceId", "broadcastUri", "reservationId", "iPv4Address"));
     }
 
     @Override
diff --git a/server/src/main/java/com/cloud/network/guru/ControlNetworkGuru.java b/server/src/main/java/com/cloud/network/guru/ControlNetworkGuru.java
@@ -21,6 +21,7 @@
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
 
+import com.cloud.network.router.VirtualNetworkApplianceManager;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
 import org.apache.log4j.Logger;
 
@@ -166,18 +167,24 @@ public boolean release(NicProfile nic, VirtualMachineProfile vm, String reservat
         assert nic.getTrafficType() == TrafficType.Control;
         HypervisorType hType = vm.getHypervisorType();
         if ( ( (hType == HypervisorType.VMware) || (hType == HypervisorType.Hyperv) )&& isRouterVm(vm)) {
+            if (!VirtualNetworkApplianceManager.RemoveControlIpOnStop.valueIn(vm.getVirtualMachine().getDataCenterId())) {
+                if (s_logger.isDebugEnabled()) {
+                    s_logger.debug(String.format("not releasing %s from %s with reservationId %s, as systemvm.release.control.ip.on.stop is set to false for the data center.", nic, vm, reservationId));
+                }
+                return true;
+            }
             long dcId = vm.getVirtualMachine().getDataCenterId();
             DataCenterVO dcVo = _dcDao.findById(dcId);
             if (dcVo.getNetworkType() != NetworkType.Basic) {
                 super.release(nic, vm, reservationId);
                 if (s_logger.isDebugEnabled()) {
-                    s_logger.debug("Released nic: " + nic);
+                    s_logger.debug(String.format("Released nic: %s for vm %s", nic, vm));
                 }
                 return true;
             } else {
                 nic.deallocate();
                 if (s_logger.isDebugEnabled()) {
-                    s_logger.debug("Released nic: " + nic);
+                    s_logger.debug(String.format("Released nic: %s for vm %s", nic, vm));
                 }
                 return true;
             }
@@ -187,7 +194,7 @@ public boolean release(NicProfile nic, VirtualMachineProfile vm, String reservat
 
         nic.deallocate();
         if (s_logger.isDebugEnabled()) {
-            s_logger.debug("Released nic: " + nic);
+            s_logger.debug(String.format("Released nic: %s for vm %s", nic, vm));
         }
 
         return true;
diff --git a/server/src/main/java/com/cloud/network/guru/PodBasedNetworkGuru.java b/server/src/main/java/com/cloud/network/guru/PodBasedNetworkGuru.java
@@ -159,7 +159,7 @@ public boolean release(NicProfile nic, VirtualMachineProfile vm, String reservat
         nic.deallocate();
 
         if (s_logger.isDebugEnabled()) {
-            s_logger.debug("Released nic: " + nic);
+            s_logger.debug(String.format("Released nic: %s for vm %s", nic, vm));
         }
 
         return true;
diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManager.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManager.java
diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java

Original file line number	Diff line number	Diff line change
`@@ -442,6 +442,6 @@ public void deallocate() {`
`442`	`442`
`443`	`443`	`@Override`
`444`	`444`	`public String toString() {`
`445`		`- return String.format("NicProfile %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "vmId", "reservationId", "iPv4Address", "broadcastUri"));`
	`445`	`+ return String.format("NicProfile %s", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(this, "id", "vmId", "deviceId", "broadcastUri", "reservationId", "iPv4Address"));`
`446`	`446`	`}`
`447`	`447`	`}`
Original file line number	Diff line number	Diff line change
`@@ -51,6 +51,8 @@ public class GsonHelper {`
`51`	`51`	`GsonBuilder loggerBuilder = new GsonBuilder();`
`52`	`52`	`loggerBuilder.disableHtmlEscaping();`
`53`	`53`	`loggerBuilder.setExclusionStrategies(new LoggingExclusionStrategy(s_logger));`
	`54`	`+ loggerBuilder.serializeSpecialFloatingPointValues();`
	`55`	`+ // maybe add loggerBuilder.serializeNulls(); as well?`
`54`	`56`	`s_gogger = setDefaultGsonConfig(loggerBuilder);`
`55`	`57`	`s_logger.info("Default Builder inited.");`
`56`	`58`	`}`
Original file line number	Diff line number	Diff line change
`@@ -159,7 +159,7 @@ public boolean release(NicProfile nic, VirtualMachineProfile vm, String reservat`
`159`	`159`	`nic.deallocate();`
`160`	`160`
`161`	`161`	`if (s_logger.isDebugEnabled()) {`
`162`		`- s_logger.debug("Released nic: " + nic);`
	`162`	`+ s_logger.debug(String.format("Released nic: %s for vm %s", nic, vm));`
`163`	`163`	`}`
`164`	`164`
`165`	`165`	`return true;`