VMware: wait for NSX DV port group readiness

framework/config/src/main/java/org/apache/cloudstack/framework/config/ValidatedConfigKey.java

@@ -32,7 +32,7 @@ public Consumer<T> getValidator() {
 
     public void validateValue(String value) {
         if (validator != null) {
-            validator.accept((T) value);
+            validator.accept(valueOf(value));
         }
     }
 }

framework/config/src/test/java/org/apache/cloudstack/framework/config/ConfigKeyTest.java

@@ -17,6 +17,7 @@
 package org.apache.cloudstack.framework.config;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Assert;
 import org.junit.Test;
@@ -76,4 +77,26 @@ public void testGetDescendants() {
             Assert.assertTrue(descendant == Scope.Cluster || descendant == Scope.StoragePool || descendant == Scope.ImageStore);
         }
     }
+
+    @Test
+    public void testValidatedNumericValue() {
+        AtomicReference<Long> received = new AtomicReference<>();
+        ValidatedConfigKey<Long> key = new ValidatedConfigKey<>("Advanced", Long.class, "test.validated", "0",
+                "Test configuration", true, Scope.Global, null, received::set);
+        key.validateValue("60000");
+        Assert.assertEquals(Long.valueOf(60000), received.get());
+        Assert.assertThrows(NumberFormatException.class, () -> key.validateValue("invalid"));
+        Assert.assertEquals(Long.valueOf(60000), received.get());
+    }
+
+    @Test
+    public void testValidatedStringValueIsUnchanged() {
+        AtomicReference<String> received = new AtomicReference<>();
+        ValidatedConfigKey<String> key = new ValidatedConfigKey<>("Advanced", String.class, "test.validated", null,
+                "Test configuration", true, Scope.Global, null, received::set);
+        key.validateValue("  value  ");
+        Assert.assertEquals("  value  ", received.get());
+        key.validateValue(null);
+        Assert.assertNull(received.get());
+    }
 }

plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManager.java

@@ -21,7 +21,9 @@
 import java.util.Map;
 
 import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.ValidatedConfigKey;
 
+import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.hypervisor.Hypervisor.HypervisorType;
 import com.cloud.hypervisor.vmware.mo.HostMO;
 import com.cloud.hypervisor.vmware.util.VmwareContext;
@@ -34,6 +36,16 @@ public interface VmwareManager {
     public static final ConfigKey<Long> s_vmwareNicHotplugWaitTimeout = new ConfigKey<Long>("Advanced", Long.class, "vmware.nic.hotplug.wait.timeout", "15000",
             "Wait timeout (milli seconds) for hot plugged NIC of VM to be detected by guest OS.", false, ConfigKey.Scope.Global);
 
+    ConfigKey<Long> VmwareNsxDvPortGroupWaitTimeout = new ValidatedConfigKey<>("Advanced", Long.class,
+            "vmware.nsx.dvportgroup.wait.timeout", "60000",
+            "Maximum polling time (milliseconds) per network preparation for an NSX-backed distributed virtual port group to appear in vCenter. "
+                    + "Set to 0 for a single lookup. Must not be negative. Capped by the VMware operation timeout; vCenter API request timeouts are separate.",
+            true, ConfigKey.Scope.Global, null, timeout -> {
+                if (timeout == null || timeout < 0) {
+                    throw new InvalidParameterValueException("vmware.nsx.dvportgroup.wait.timeout must be a non-negative number of milliseconds");
+                }
+            });
+
     public static final ConfigKey<Long> templateCleanupInterval = new ConfigKey<Long>("Advanced", Long.class, "vmware.full.clone.template.cleanup.period", "0",
             "period (in minutes) between the start of template cleanup jobs for VMWare full cloned Templates.", false, ConfigKey.Scope.Global);
 

plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java

@@ -301,7 +301,7 @@ public String getConfigComponentName() {
 
     @Override
     public ConfigKey<?>[] getConfigKeys() {
-        return new ConfigKey<?>[] {s_vmwareNicHotplugWaitTimeout, s_vmwareCleanOldWorderVMs, templateCleanupInterval, s_vmwareSearchExcludeFolder, s_vmwareOVAPackageTimeout, s_vmwareCleanupPortGroups, VMWARE_STATS_TIME_WINDOW, VmwareUserVmNicDeviceType};
+        return new ConfigKey<?>[] {s_vmwareNicHotplugWaitTimeout, VmwareNsxDvPortGroupWaitTimeout, s_vmwareCleanOldWorderVMs, templateCleanupInterval, s_vmwareSearchExcludeFolder, s_vmwareOVAPackageTimeout, s_vmwareCleanupPortGroups, VMWARE_STATS_TIME_WINDOW, VmwareUserVmNicDeviceType};
     }
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {

plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java

@@ -4039,7 +4039,7 @@ private String getVlanInfo(NicTO nicTo, String defaultVlan) {
         return defaultVlan;
     }
 
-    private Pair<ManagedObjectReference, String> prepareNetworkFromNicInfo(HostMO hostMo, NicTO nicTo, boolean configureVServiceInNexus, String networkName, VirtualMachine.Type vmType)
+    Pair<ManagedObjectReference, String> prepareNetworkFromNicInfo(HostMO hostMo, NicTO nicTo, boolean configureVServiceInNexus, String networkName, VirtualMachine.Type vmType)
             throws Exception {
 
         Ternary<String, String, String> switchDetails = getTargetSwitch(nicTo);
@@ -4067,14 +4067,26 @@ private Pair<ManagedObjectReference, String> prepareNetworkFromNicInfo(HostMO ho
                 // plumb this network to the isolated vlan.
                 svlanId = getPvlanInfo(nicTo);
             }
+            long networkTimeoutMs = _opsTimeout;
+            if (switchType == VirtualSwitchType.VMwareDistributedVirtualSwitch && nicTo.getBroadcastType() == BroadcastDomainType.NSX && networkName != null) {
+                long nsxTimeoutMs = getNsxDvPortGroupWaitTimeout();
+                if (nsxTimeoutMs < 0) {
+                    throw new IllegalArgumentException(VmwareManager.VmwareNsxDvPortGroupWaitTimeout.key() + " must not be negative");
+                }
+                networkTimeoutMs = Math.min(_opsTimeout, nsxTimeoutMs);
+            }
             networkInfo = HypervisorHostHelper.prepareNetwork(switchName, namePrefix, hostMo, vlanId, svlanId,
-                    nicTo.getNetworkRateMbps(), nicTo.getNetworkRateMulticastMbps(), _opsTimeout, switchType,
+                    nicTo.getNetworkRateMbps(), nicTo.getNetworkRateMulticastMbps(), networkTimeoutMs, switchType,
                     _portsPerDvPortGroup, nicTo.getGateway(), configureVServiceInNexus, nicTo.getBroadcastType(), _vsmCredentials, nicTo.getDetails(), networkName);
         }
 
         return networkInfo;
     }
 
+    protected long getNsxDvPortGroupWaitTimeout() {
+        return VmwareManager.VmwareNsxDvPortGroupWaitTimeout.value();
+    }
+
     // return Ternary <switch name, switch type, vlan tagging>
     private Ternary<String, String, String> getTargetSwitch(NicTO nicTo) throws CloudException {
         TrafficType[] supportedTrafficTypes = new TrafficType[]{TrafficType.Guest, TrafficType.Public, TrafficType.Control, TrafficType.Management, TrafficType.Storage};

plugins/hypervisors/vmware/src/test/java/com/cloud/hypervisor/vmware/resource/VmwareResourceTest.java

@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.any;
@@ -40,6 +41,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 
 import com.cloud.agent.api.Command;
 import com.cloud.agent.api.ScaleVmAnswer;
@@ -67,6 +69,12 @@
 import org.mockito.Mockito;
 import org.mockito.Spy;
 import org.mockito.junit.MockitoJUnitRunner;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
+import org.apache.cloudstack.framework.messagebus.MessageBus;
+import org.springframework.test.util.ReflectionTestUtils;
 
 import com.cloud.agent.api.Answer;
 import com.cloud.agent.api.CheckGuestOsMappingAnswer;
@@ -106,6 +114,15 @@
 import com.vmware.vim25.VirtualDeviceConfigSpec;
 import com.vmware.vim25.VirtualMachineConfigSpec;
 import com.vmware.vim25.VirtualMachineVideoCard;
+import com.cloud.configuration.ConfigurationManagerImpl;
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.hypervisor.vmware.manager.VmwareManager;
+import com.cloud.hypervisor.vmware.manager.VmwareManagerImpl;
+import com.cloud.hypervisor.vmware.mo.VirtualSwitchType;
+import com.cloud.network.Networks.BroadcastDomainType;
+import com.cloud.network.Networks.TrafficType;
+import com.cloud.utils.db.TransactionLegacy;
+import com.cloud.vm.VirtualMachine;
 
 @RunWith(MockitoJUnitRunner.class)
 public class VmwareResourceTest {
@@ -874,4 +891,95 @@ public void testRemoveVirtualTPMDevice() throws Exception {
         Mockito.verify(vmwareResource, Mockito.times(1)).removeVirtualTPMDevice(vmConfigSpec, tpm);
         Mockito.verify(deviceChanges, Mockito.times(1)).add(any(VirtualDeviceConfigSpec.class));
     }
+
+    private void assertNetworkTimeout(VirtualSwitchType switchType, BroadcastDomainType broadcast, String name,
+                                      long configured, long expected) throws Exception {
+        doReturn(configured).when(_resource).getNsxDvPortGroupWaitTimeout();
+        Mockito.clearInvocations(_resource);
+        NicTO nic = new NicTO();
+        nic.setName("NSX-VDS,untagged," + switchType);
+        nic.setType(TrafficType.Guest);
+        nic.setBroadcastType(broadcast);
+        nic.setBroadcastUri(broadcast.toUri(broadcast == BroadcastDomainType.Vlan ? "123" : "nsx"));
+        try (MockedStatic<HypervisorHostHelper> helper = mockStatic(HypervisorHostHelper.class)) {
+            _resource.prepareNetworkFromNicInfo(host, nic, false, name, VirtualMachine.Type.User);
+            if (switchType == VirtualSwitchType.StandardVirtualSwitch) {
+                helper.verify(() -> HypervisorHostHelper.prepareNetwork(any(), any(), eq(host), any(), any(), any(), eq(expected),
+                        eq(true), eq(broadcast), any(), any()));
+            } else {
+                helper.verify(() -> HypervisorHostHelper.prepareNetwork(any(), any(), eq(host), any(), any(), any(), any(), eq(expected),
+                        eq(switchType), Mockito.anyInt(), any(), eq(false), eq(broadcast), any(), any(), eq(name)));
+            }
+            helper.verifyNoMoreInteractions();
+            if (broadcast != BroadcastDomainType.NSX || switchType != VirtualSwitchType.VMwareDistributedVirtualSwitch || name == null) {
+                verify(_resource, never()).getNsxDvPortGroupWaitTimeout();
+            }
+        }
+    }
+
+    @Test
+    public void testNsxVisibilityTimeoutRouting() throws Exception {
+        assertEquals(60000L, new VmwareResource().getNsxDvPortGroupWaitTimeout());
+        assertTrue(Arrays.asList(new VmwareManagerImpl().getConfigKeys()).contains(VmwareManager.VmwareNsxDvPortGroupWaitTimeout));
+        for (long timeout : new long[] {0, 2500, Long.MAX_VALUE}) {
+            assertNetworkTimeout(VirtualSwitchType.VMwareDistributedVirtualSwitch, BroadcastDomainType.NSX, "nsx", timeout, Math.min(900000, timeout));
+        }
+    }
+
+    @Test
+    public void testOtherNetworkTimeoutsAreUnchanged() throws Exception {
+        assertNetworkTimeout(VirtualSwitchType.VMwareDistributedVirtualSwitch, BroadcastDomainType.Vlan, "network", -1, 900000);
+        assertNetworkTimeout(VirtualSwitchType.VMwareDistributedVirtualSwitch, BroadcastDomainType.Lswitch, "network", -1, 900000);
+        assertNetworkTimeout(VirtualSwitchType.VMwareDistributedVirtualSwitch, BroadcastDomainType.NSX, null, -1, 900000);
+        assertNetworkTimeout(VirtualSwitchType.NexusDistributedVirtualSwitch, BroadcastDomainType.NSX, "network", -1, 900000);
+        assertNetworkTimeout(VirtualSwitchType.StandardVirtualSwitch, BroadcastDomainType.Vlan, "network", -1, 900000);
+    }
+
+    @Test
+    public void testNsxTimeoutConfigurationIsValidatedBeforeSaving() {
+        String key = VmwareManager.VmwareNsxDvPortGroupWaitTimeout.key();
+        ConfigurationDao dao = mock(ConfigurationDao.class);
+        ConfigDepotImpl depot = mock(ConfigDepotImpl.class);
+        ConfigurationVO row = mock(ConfigurationVO.class);
+        MessageBus bus = mock(MessageBus.class);
+        AtomicReference<String> stored = new AtomicReference<>("60000");
+        when(dao.findByName(key)).thenReturn(row);
+        when(row.getScopes()).thenReturn(List.of(ConfigKey.Scope.Global));
+        doReturn(VmwareManager.VmwareNsxDvPortGroupWaitTimeout).when(depot).get(key);
+        when(dao.getValue(key)).thenAnswer(ignored -> stored.get());
+        when(dao.update(eq(key), anyString(), anyString())).thenAnswer(invocation -> {
+            stored.set(invocation.getArgument(2));
+            return true;
+        });
+        when(depot.getConfigStringValue(key, ConfigKey.Scope.Global, null)).thenAnswer(ignored -> stored.get());
+        ConfigurationManagerImpl manager = new ConfigurationManagerImpl();
+        ReflectionTestUtils.setField(manager, "_configDao", dao);
+        ReflectionTestUtils.setField(manager, "_configDepot", depot);
+        ReflectionTestUtils.setField(manager, "messageBus", bus);
+        ConfigDepotImpl previous = (ConfigDepotImpl) ReflectionTestUtils.getField(ConfigKey.class, "s_depot");
+        ConfigKey.init(depot);
+        try (MockedStatic<TransactionLegacy> transactions = mockStatic(TransactionLegacy.class)) {
+            for (String value : List.of("-1", "", "invalid", "1.5", "9223372036854775808")) {
+                assertThrows(InvalidParameterValueException.class,
+                        () -> manager.updateConfiguration(1, key, "Advanced", value, ConfigKey.Scope.Global, null));
+            }
+            transactions.verifyNoInteractions();
+            verify(dao, never()).update(anyString(), anyString(), anyString());
+            verify(depot, never()).invalidateConfigCache(key, ConfigKey.Scope.Global, null);
+            Mockito.verifyNoInteractions(bus);
+            assertEquals("60000", stored.get());
+            assertEquals(60000L, new VmwareResource().getNsxDvPortGroupWaitTimeout());
+            TransactionLegacy transaction = mock(TransactionLegacy.class);
+            transactions.when(TransactionLegacy::currentTxn).thenReturn(transaction);
+            for (String value : List.of("0", "60000", "2500", String.valueOf(Long.MAX_VALUE))) {
+                assertEquals(value, manager.updateConfiguration(1, key, "Advanced", value, ConfigKey.Scope.Global, null));
+                assertEquals(Long.parseLong(value), new VmwareResource().getNsxDvPortGroupWaitTimeout());
+            }
+            verify(transaction, times(4)).start();
+            verify(transaction, times(4)).commit();
+            verify(depot, times(4)).invalidateConfigCache(key, ConfigKey.Scope.Global, null);
+        } finally {
+            ConfigKey.init(previous);
+        }
+    }
 }

vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/HypervisorHostHelper.java

@@ -30,6 +30,8 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+import java.util.function.LongSupplier;
 
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -649,7 +651,7 @@ public static Pair<ManagedObjectReference, String> prepareNetwork(String physica
                 bWaitPortGroupReady = false;
             } else if (BroadcastDomainType.NSX == broadcastDomainType && Objects.nonNull(netName)){
                 networkName = netName;
-                bWaitPortGroupReady = false;
+                bWaitPortGroupReady = true;
             } else {
                 boolean dvSwitchSupportNewPolicies = (isFeatureSupportedInVcenterApiVersion(vcApiVersion, MINIMUM_VCENTER_API_VERSION_WITH_DVS_NEW_POLICIES_SUPPORT)
                         && isVersionEqualOrHigher(dvSwitchVersion, MINIMUM_DVS_VERSION_WITH_NEW_POLICIES_SUPPORT));
@@ -719,6 +721,9 @@ public static Pair<ManagedObjectReference, String> prepareNetwork(String physica
             morNetwork = dataCenterMo.getDvPortGroupMor(networkName);
         if (morNetwork == null) {
             String msg = "Failed to create guest network " + networkName;
+            if (vSwitchType == VirtualSwitchType.VMwareDistributedVirtualSwitch && broadcastDomainType == BroadcastDomainType.NSX && netName != null) {
+                msg = String.format("NSX DV port group %s was not visible in vCenter (polling timeout: %d ms)", networkName, timeOutMs);
+            }
             LOGGER.error(msg);
             throw new Exception(msg);
         }
@@ -1031,21 +1036,47 @@ public static boolean isSpecMatch(DVPortgroupConfigInfo currentDvPortgroupInfo,
     }
 
     public static ManagedObjectReference waitForDvPortGroupReady(DatacenterMO dataCenterMo, String dvPortGroupName, long timeOutMs) throws Exception {
-        ManagedObjectReference morDvPortGroup = null;
+        return waitForDvPortGroupReady(dataCenterMo, dvPortGroupName, timeOutMs, System::nanoTime, TimeUnit.NANOSECONDS::sleep);
+    }
 
-        // if DvPortGroup is just created, we may fail to retrieve it, we
-        // need to retry
-        long startTick = System.currentTimeMillis();
-        while (System.currentTimeMillis() - startTick <= timeOutMs) {
-            morDvPortGroup = dataCenterMo.getDvPortGroupMor(dvPortGroupName);
-            if (morDvPortGroup != null) {
-                break;
-            }
+    @FunctionalInterface
+    interface NanoSleeper {
+        void sleep(long nanos) throws InterruptedException;
+    }
 
-            LOGGER.info("Waiting for dvPortGroup " + dvPortGroupName + " to be ready");
-            Thread.sleep(1000);
+    static ManagedObjectReference waitForDvPortGroupReady(DatacenterMO dataCenterMo, String dvPortGroupName, long timeOutMs,
+                                                         LongSupplier nanoTime, NanoSleeper sleeper) throws Exception {
+        if (timeOutMs < 0) {
+            throw new IllegalArgumentException("DV port group readiness timeout must not be negative");
+        }
+        long timeoutNanos = TimeUnit.MILLISECONDS.toNanos(timeOutMs);
+        long startTick = nanoTime.getAsLong();
+        boolean firstLookup = true;
+        try {
+            // Always check after sleeping, even if the last sleep reaches or passes the deadline.
+            while (true) {
+                if (Thread.currentThread().isInterrupted()) {
+                    throw new InterruptedException("Interrupted while waiting for dvPortGroup " + dvPortGroupName);
+                }
+                ManagedObjectReference morDvPortGroup = dataCenterMo.getDvPortGroupMor(dvPortGroupName);
+                if (morDvPortGroup != null) {
+                    return morDvPortGroup;
+                }
+                if (firstLookup) {
+                    LOGGER.info("Waiting up to {} ms for dvPortGroup {} to be visible in vCenter", timeOutMs, dvPortGroupName);
+                    firstLookup = false;
+                }
+                long elapsedNanos = nanoTime.getAsLong() - startTick;
+                if (elapsedNanos >= timeoutNanos) {
+                    break;
+                }
+                sleeper.sleep(Math.min(TimeUnit.SECONDS.toNanos(1), timeoutNanos - elapsedNanos));
+            }
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw e;
         }
-        return morDvPortGroup;
+        return null;
     }
 
     public static boolean isSpecMatch(DVPortgroupConfigInfo configInfo, Integer vid, DVSTrafficShapingPolicy shapingPolicy) {