diff --git a/engine/components-api/src/main/java/com/cloud/agent/AgentManager.java b/engine/components-api/src/main/java/com/cloud/agent/AgentManager.java index 2182dfc542d..81525ca13f1 100644 --- a/engine/components-api/src/main/java/com/cloud/agent/AgentManager.java +++ b/engine/components-api/src/main/java/com/cloud/agent/AgentManager.java @@ -50,6 +50,10 @@ public interface AgentManager { ConfigKey ReadyCommandWait = new ConfigKey("Advanced", Integer.class, "ready.command.wait", "60", "Time in seconds to wait for Ready command to return", true); + ConfigKey GranularWaitTimeForCommands = new ConfigKey<>("Advanced", String.class, "commands.timeout", "", + "This timeout overrides the wait global config. This holds a comma separated key value pairs containing timeout (in seconds) for specific commands. " + + "For example: DhcpEntryCommand=600, SavePasswordCommand=300, VmDataCommand=300", false); + public enum TapAgentsAction { Add, Del, Contains, } diff --git a/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java index 9333410e0aa..63e97519534 100644 --- a/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java @@ -53,6 +53,7 @@ import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext; import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.outofbandmanagement.dao.OutOfBandManagementDao; import org.apache.cloudstack.utils.identity.ManagementServerNode; +import org.apache.commons.collections.MapUtils; import org.apache.commons.lang3.BooleanUtils; import com.cloud.agent.AgentManager; @@ -139,6 +140,7 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl protected List> _cmdMonitors = new ArrayList>(17); protected List> _creationMonitors = new ArrayList>(17); protected List _loadingAgents = new ArrayList(); + protected Map _commandTimeouts = new HashMap<>(); private int _monitorId = 0; private final Lock _agentStatusLock = new ReentrantLock(); @@ -241,6 +243,8 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl _monitorExecutor = new ScheduledThreadPoolExecutor(1, new NamedThreadFactory("AgentMonitor")); + initializeCommandTimeouts(); + return true; } @@ -424,6 +428,62 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl } } + protected int getTimeout(final Commands commands, int timeout) { + int result; + if (timeout > 0) { + result = timeout; + } else { + result = Wait.value(); + } + + int granularTimeout = getTimeoutFromGranularWaitTime(commands); + return (granularTimeout > 0) ? granularTimeout : result; + } + + protected int getTimeoutFromGranularWaitTime(final Commands commands) { + int maxWait = 0; + if (MapUtils.isNotEmpty(_commandTimeouts)) { + for (final Command cmd : commands) { + String simpleCommandName = cmd.getClass().getSimpleName(); + Integer commandTimeout = _commandTimeouts.get(simpleCommandName); + if (commandTimeout != null && commandTimeout > maxWait) { + maxWait = commandTimeout; + } + } + } + + return maxWait; + } + + private void initializeCommandTimeouts() { + String commandWaits = GranularWaitTimeForCommands.value().trim(); + if (StringUtils.isNotEmpty(commandWaits)) { + _commandTimeouts = getCommandTimeoutsMap(commandWaits); + logger.info(String.format("Timeouts for management server internal commands successfully initialized from global setting commands.timeout: %s", _commandTimeouts)); + } + } + + private Map getCommandTimeoutsMap(String commandWaits) { + String[] commandPairs = commandWaits.split(","); + Map commandTimeouts = new HashMap<>(); + + for (String commandPair : commandPairs) { + String[] parts = commandPair.trim().split("="); + if (parts.length == 2) { + try { + String commandName = parts[0].trim(); + int commandTimeout = Integer.parseInt(parts[1].trim()); + commandTimeouts.put(commandName, commandTimeout); + } catch (NumberFormatException e) { + logger.error(String.format("Initialising the timeouts using commands.timeout: %s for management server internal commands failed with error %s", commandPair, e.getMessage())); + } + } else { + logger.error(String.format("Error initialising the timeouts for management server internal commands. Invalid format in commands.timeout: %s", commandPair)); + } + } + return commandTimeouts; + } + @Override public Answer[] send(final Long hostId, final Commands commands, int timeout) throws AgentUnavailableException, OperationTimedoutException { assert hostId != null : "Who's not checking the agent id before sending? ... (finger wagging)"; @@ -431,8 +491,14 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl throw new AgentUnavailableException(-1); } - if (timeout <= 0) { - timeout = Wait.value(); + int wait = getTimeout(commands, timeout); + logger.debug(String.format("Wait time setting on %s is %d seconds", commands, wait)); + for (Command cmd : commands) { + String simpleCommandName = cmd.getClass().getSimpleName(); + Integer commandTimeout = _commandTimeouts.get(simpleCommandName); + if (commandTimeout != null) { + cmd.setWait(wait); + } } if (CheckTxnBeforeSending.value()) { @@ -454,7 +520,7 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl final Request req = new Request(hostId, agent.getName(), _nodeId, cmds, commands.stopOnError(), true); req.setSequence(agent.getNextSequence()); - final Answer[] answers = agent.send(req, timeout); + final Answer[] answers = agent.send(req, wait); notifyAnswersToMonitors(hostId, req.getSequence(), answers); commands.setAnswers(answers); return answers; @@ -997,6 +1063,11 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl @Override public Answer[] send(final Long hostId, final Commands cmds) throws AgentUnavailableException, OperationTimedoutException { int wait = 0; + if (cmds.size() > 1) { + logger.debug(String.format("Checking the wait time in seconds to be used for the following commands : %s. If there are multiple commands sent at once," + + "then max wait time of those will be used", cmds)); + } + for (final Command cmd : cmds) { if (cmd.getWait() > wait) { wait = cmd.getWait(); @@ -1821,7 +1892,7 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[] { CheckTxnBeforeSending, Workers, Port, Wait, AlertWait, DirectAgentLoadSize, - DirectAgentPoolSize, DirectAgentThreadCap, EnableKVMAutoEnableDisable, ReadyCommandWait }; + DirectAgentPoolSize, DirectAgentThreadCap, EnableKVMAutoEnableDisable, ReadyCommandWait, GranularWaitTimeForCommands }; } protected class SetHostParamsListener implements Listener { diff --git a/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java b/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java index 452cfd90056..52b7ed77533 100644 --- a/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java @@ -83,4 +83,24 @@ public class AgentManagerImplTest { } Mockito.verify(mgr, Mockito.times(1)).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.eq(Status.Event.AgentDisconnected), Mockito.eq(true), Mockito.eq(true)); } + + @Test + public void testGetTimeoutWithPositiveTimeout() { + Commands commands = Mockito.mock(Commands.class); + int timeout = 30; + int result = mgr.getTimeout(commands, timeout); + + Assert.assertEquals(30, result); + } + + @Test + public void testGetTimeoutWithGranularTimeout() { + Commands commands = Mockito.mock(Commands.class); + Mockito.doReturn(50).when(mgr).getTimeoutFromGranularWaitTime(commands); + + int timeout = 0; + int result = mgr.getTimeout(commands, timeout); + + Assert.assertEquals(50, result); + } } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index dee1aa81758..d7e2160ef35 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1245,6 +1245,8 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati type = configuration.getType(); } + validateSpecificConfigurationValues(name, value, type); + boolean isTypeValid = validateValueType(value, type); if (!isTypeValid) { return String.format("Value [%s] is not a valid [%s].", value, type); @@ -1373,6 +1375,78 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return validateIfStringValueIsInRange(name, value, range); } + /** + * Validates configuration values for the given name, value, and type. + *
    + *
  • The value must be a comma-separated list of key-value pairs, where each value must be a positive integer.
    • + *
  • Each key-value pair must be in the format "command=value", with the value being a positive integer greater than 0, + * otherwise fails with an error message
    • + *
  • Throws an {@link InvalidParameterValueException} if validation fails.
    • + *
+ * + * @param name the configuration name + * @param value the configuration value as a comma-separated string of key-value pairs + * @param type the configuration type, expected to be String + * @throws InvalidParameterValueException if validation fails with a specific error message + */ + protected void validateSpecificConfigurationValues(String name, String value, Class type) { + if (type.equals(String.class)) { + if (name.equals(AgentManager.GranularWaitTimeForCommands.toString())) { + Pair validationResult = validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(value); + if (!validationResult.first()) { + String errMsg = validationResult.second(); + logger.error(validationResult.second()); + throw new InvalidParameterValueException(errMsg); + } + } + } + } + + protected Pair validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(String value) { + try { + if (StringUtils.isNotEmpty(value)) { + String[] commands = value.split(","); + for (String command : commands) { + command = command.trim(); + if (!command.contains("=")) { + String errorMessage = String.format("Validation failed: Command '%s' does not contain '='.", command); + return new Pair<>(false, errorMessage); + } + + String[] parts = command.split("="); + if (parts.length != 2) { + String errorMessage = String.format("Validation failed: Command '%s' is not properly formatted.", command); + return new Pair<>(false, errorMessage); + } + + String commandName = parts[0].trim(); + String valueString = parts[1].trim(); + + if (commandName.isEmpty()) { + String errorMessage = String.format("Validation failed: Command name is missing in '%s'.", command); + return new Pair<>(false, errorMessage); + } + + try { + int num = Integer.parseInt(valueString); + if (num <= 0) { + String errorMessage = String.format("Validation failed: The value for command '%s' is not greater than 0. Invalid value: %d", commandName, num); + return new Pair<>(false, errorMessage); + } + } catch (NumberFormatException e) { + String errorMessage = String.format("Validation failed: The value for command '%s' is not a valid integer. Invalid value: %s", commandName, valueString); + return new Pair<>(false, errorMessage); + } + } + } + + return new Pair<>(true, ""); + } catch (Exception e) { + String errorMessage = String.format("Validation failed: An error occurred while parsing the command string. Error: %s", e.getMessage()); + return new Pair<>(false, errorMessage); + } + } + /** * Returns a boolean indicating whether a Config's range should be validated. It should not be validated when:
*
    diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java index ceffe019377..badf730a061 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java @@ -17,6 +17,7 @@ package com.cloud.configuration; +import com.cloud.agent.AgentManager; import com.cloud.api.query.dao.NetworkOfferingJoinDao; import com.cloud.api.query.vo.NetworkOfferingJoinVO; import com.cloud.configuration.Resource.ResourceType; @@ -71,6 +72,7 @@ import com.cloud.user.ResourceLimitService; import com.cloud.user.User; import com.cloud.user.UserVO; import com.cloud.user.dao.AccountDao; +import com.cloud.utils.Pair; import com.cloud.utils.db.Filter; import com.cloud.utils.db.SearchCriteria; import com.cloud.utils.db.TransactionLegacy; @@ -124,7 +126,10 @@ import java.util.Set; import java.util.UUID; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; @@ -361,7 +366,7 @@ public class ConfigurationManagerTest { try { configurationMgr.dedicatePublicIpRange(dedicatePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Unable to find vlan by id")); + assertTrue(e.getMessage().contains("Unable to find vlan by id")); } finally { txn.close("runDedicatePublicIpRangeInvalidRange"); } @@ -390,7 +395,7 @@ public class ConfigurationManagerTest { try { configurationMgr.dedicatePublicIpRange(dedicatePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Public IP range has already been dedicated")); + assertTrue(e.getMessage().contains("Public IP range has already been dedicated")); } finally { txn.close("runDedicatePublicIpRangePublicIpRangeDedicated"); } @@ -416,7 +421,7 @@ public class ConfigurationManagerTest { try { configurationMgr.dedicatePublicIpRange(dedicatePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Public IP range can be dedicated to an account only in the zone of type Advanced")); + assertTrue(e.getMessage().contains("Public IP range can be dedicated to an account only in the zone of type Advanced")); } finally { txn.close("runDedicatePublicIpRangeInvalidZone"); } @@ -443,7 +448,7 @@ public class ConfigurationManagerTest { try { configurationMgr.dedicatePublicIpRange(dedicatePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Public IP address in range is allocated to another account")); + assertTrue(e.getMessage().contains("Public IP address in range is allocated to another account")); } finally { txn.close("runDedicatePublicIpRangeIPAddressAllocated"); } @@ -465,7 +470,7 @@ public class ConfigurationManagerTest { when(configurationMgr._accountVlanMapDao.remove(anyLong())).thenReturn(true); try { Boolean result = configurationMgr.releasePublicIpRange(releasePublicIpRangesCmd); - Assert.assertTrue(result); + assertTrue(result); } catch (Exception e) { logger.info("exception in testing runReleasePublicIpRangePostiveTest1 message: " + e.toString()); } finally { @@ -499,7 +504,7 @@ public class ConfigurationManagerTest { when(configurationMgr._accountVlanMapDao.remove(anyLong())).thenReturn(true); try { Boolean result = configurationMgr.releasePublicIpRange(releasePublicIpRangesCmd); - Assert.assertTrue(result); + assertTrue(result); } catch (Exception e) { logger.info("exception in testing runReleasePublicIpRangePostiveTest2 message: " + e.toString()); } finally { @@ -514,7 +519,7 @@ public class ConfigurationManagerTest { try { configurationMgr.releasePublicIpRange(releasePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("Please specify a valid IP range id")); + assertTrue(e.getMessage().contains("Please specify a valid IP range id")); } finally { txn.close("runReleasePublicIpRangeInvalidIpRange"); } @@ -530,7 +535,7 @@ public class ConfigurationManagerTest { try { configurationMgr.releasePublicIpRange(releasePublicIpRangesCmd); } catch (Exception e) { - Assert.assertTrue(e.getMessage().contains("as it not dedicated to any domain and any account")); + assertTrue(e.getMessage().contains("as it not dedicated to any domain and any account")); } finally { txn.close("runReleaseNonDedicatedPublicIpRange"); } @@ -570,10 +575,10 @@ public class ConfigurationManagerTest { try { configurationMgr.validateSourceNatServiceCapablities(sourceNatServiceCapabilityMap); } catch (InvalidParameterValueException e) { - Assert.assertTrue(e.getMessage(), e.getMessage().contains("Either peraccount or perzone source NAT type can be specified for SupportedSourceNatTypes")); + assertTrue(e.getMessage(), e.getMessage().contains("Either peraccount or perzone source NAT type can be specified for SupportedSourceNatTypes")); caught = true; } - Assert.assertTrue("should not be accepted", caught); + assertTrue("should not be accepted", caught); } @Test @@ -585,10 +590,10 @@ public class ConfigurationManagerTest { try { configurationMgr.validateSourceNatServiceCapablities(sourceNatServiceCapabilityMap); } catch (InvalidParameterValueException e) { - Assert.assertTrue(e.getMessage(), e.getMessage().contains("Unknown specified value for RedundantRouter")); + assertTrue(e.getMessage(), e.getMessage().contains("Unknown specified value for RedundantRouter")); caught = true; } - Assert.assertTrue("should not be accepted", caught); + assertTrue("should not be accepted", caught); } @Test @@ -600,10 +605,10 @@ public class ConfigurationManagerTest { try { configurationMgr.validateSourceNatServiceCapablities(sourceNatServiceCapabilityMap); } catch (InvalidParameterValueException e) { - Assert.assertTrue(e.getMessage(), e.getMessage().contains("Only SupportedSourceNatTypes, Network.Capability[name=RedundantRouter] capabilities can be specified for source nat service")); + assertTrue(e.getMessage(), e.getMessage().contains("Only SupportedSourceNatTypes, Network.Capability[name=RedundantRouter] capabilities can be specified for source nat service")); caught = true; } - Assert.assertTrue("should not be accepted", caught); + assertTrue("should not be accepted", caught); } @Test @@ -622,10 +627,10 @@ public class ConfigurationManagerTest { try { configurationMgr.validateStaticNatServiceCapablities(staticNatServiceCapabilityMap); } catch (InvalidParameterValueException e) { - Assert.assertTrue(e.getMessage(), e.getMessage().contains("(frue and talse)")); + assertTrue(e.getMessage(), e.getMessage().contains("(frue and talse)")); caught = true; } - Assert.assertTrue("should not be accepted", caught); + assertTrue("should not be accepted", caught); } @Test @@ -634,7 +639,7 @@ public class ConfigurationManagerTest { Map sourceNatServiceCapabilityMap = new HashMap<>(); sourceNatServiceCapabilityMap.put(Capability.SupportedSourceNatTypes, "peraccount"); sourceNatServiceCapabilityMap.put(Capability.RedundantRouter, "true"); - Assert.assertTrue(configurationMgr.isRedundantRouter(providers, Network.Service.SourceNat, sourceNatServiceCapabilityMap)); + assertTrue(configurationMgr.isRedundantRouter(providers, Network.Service.SourceNat, sourceNatServiceCapabilityMap)); } @Test @@ -642,7 +647,7 @@ public class ConfigurationManagerTest { Map> serviceCapabilityMap = new HashMap<>(); Map sourceNatServiceCapabilityMap = new HashMap<>(); sourceNatServiceCapabilityMap.put(Capability.SupportedSourceNatTypes, "perzone"); - Assert.assertTrue(configurationMgr.isSharedSourceNat(serviceCapabilityMap, sourceNatServiceCapabilityMap)); + assertTrue(configurationMgr.isSharedSourceNat(serviceCapabilityMap, sourceNatServiceCapabilityMap)); } @Test @@ -650,7 +655,7 @@ public class ConfigurationManagerTest { Map> serviceCapabilityMap = new HashMap<>(); Map sourceNatServiceCapabilityMap = new HashMap<>(); sourceNatServiceCapabilityMap.put(Capability.SupportedSourceNatTypes, "peraccount"); - Assert.assertFalse(configurationMgr.isSharedSourceNat(serviceCapabilityMap, sourceNatServiceCapabilityMap)); + assertFalse(configurationMgr.isSharedSourceNat(serviceCapabilityMap, sourceNatServiceCapabilityMap)); } @Test @@ -659,7 +664,7 @@ public class ConfigurationManagerTest { sourceNatServiceCapabilityMap.put(Capability.SupportedSourceNatTypes, "peraccount"); sourceNatServiceCapabilityMap.put(Capability.RedundantRouter, "True"); - Assert.assertTrue(configurationMgr.sourceNatCapabilitiesContainValidValues(sourceNatServiceCapabilityMap)); + assertTrue(configurationMgr.sourceNatCapabilitiesContainValidValues(sourceNatServiceCapabilityMap)); } @Test @@ -690,13 +695,13 @@ public class ConfigurationManagerTest { try { configurationMgr.validateStaticNatServiceCapablities(staticNatServiceCapabilityMap); } catch (InvalidParameterValueException e) { - Assert.assertTrue( + assertTrue( e.getMessage(), e.getMessage().contains( "Capability " + Capability.AssociatePublicIP.getName() + " can only be set when capability " + Capability.ElasticIp.getName() + " is true")); caught = true; } - Assert.assertTrue("should not be accepted", caught); + assertTrue("should not be accepted", caught); } @Test @@ -961,27 +966,27 @@ public class ConfigurationManagerTest { //Ipv4 Test boolean result; result = configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, false, null, null, null, null, null); - Assert.assertFalse(result); + assertFalse(result); try { configurationMgr.hasSameSubnet(true, "10.0.0.1", "255.255.255.0", "10.0.0.2", "255.255.255.0", "10.0.0.2", "10.0.0.10", false, null, null, null, null, null); Assert.fail(); } catch (InvalidParameterValueException e) { - Assert.assertEquals(e.getMessage(), "The gateway of the subnet should be unique. The subnet already has a gateway 10.0.0.1"); + assertEquals(e.getMessage(), "The gateway of the subnet should be unique. The subnet already has a gateway 10.0.0.1"); } try { configurationMgr.hasSameSubnet(true, "10.0.0.1", "255.255.0.0", "10.0.0.2", "255.255.255.0", "10.0.0.2", "10.0.0.10", false, null, null, null, null, null); Assert.fail(); } catch (InvalidParameterValueException e){ - Assert.assertEquals(e.getMessage(), "The subnet you are trying to add is a subset of the existing subnet having gateway 10.0.0.1 and netmask 255.255.0.0"); + assertEquals(e.getMessage(), "The subnet you are trying to add is a subset of the existing subnet having gateway 10.0.0.1 and netmask 255.255.0.0"); } try { configurationMgr.hasSameSubnet(true, "10.0.0.1", "255.255.255.0", "10.0.0.2", "255.255.0.0", "10.0.0.2", "10.0.0.10", false, null, null, null, null, null); Assert.fail(); } catch (InvalidParameterValueException e) { - Assert.assertEquals(e.getMessage(), "The subnet you are trying to add is a superset of the existing subnet having gateway 10.0.0.1 and netmask 255.255.255.0"); + assertEquals(e.getMessage(), "The subnet you are trying to add is a superset of the existing subnet having gateway 10.0.0.1 and netmask 255.255.255.0"); } result = configurationMgr.hasSameSubnet(true, "10.0.0.1", "255.255.255.0", "10.0.0.1", "255.255.255.0", "10.0.0.2", "10.0.0.10", false, null, null, null, null, null); - Assert.assertTrue(result); + assertTrue(result); //Ipv6 Test Network ipV6Network = mock(Network.class); @@ -992,35 +997,35 @@ public class ConfigurationManagerTest { doThrow(new InvalidParameterValueException("ip6Gateway and ip6Cidr should be defined when startIPv6/endIPv6 are passed in")).when(configurationMgr._networkModel).checkIp6Parameters(Mockito.anyString(), Mockito.anyString(), Mockito.isNull(String.class), Mockito.isNull(String.class)); configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, "2001:db8:0:f101::1", "2001:db8:0:f101::0/64", "2001:db8:0:f101::2", "2001:db8:0:f101::a", ipV6Network); - Assert.assertTrue(result); + assertTrue(result); try { configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, "2001:db8:0:f101::2", "2001:db8:0:f101::0/64", "2001:db8:0:f101::2", "2001:db8:0:f101::a", ipV6Network); Assert.fail(); } catch (InvalidParameterValueException e){ - Assert.assertEquals(e.getMessage(), "The input gateway 2001:db8:0:f101::2 is not same as network gateway 2001:db8:0:f101::1"); + assertEquals(e.getMessage(), "The input gateway 2001:db8:0:f101::2 is not same as network gateway 2001:db8:0:f101::1"); } try { configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, "2001:db8:0:f101::1", "2001:db8:0:f101::0/63", "2001:db8:0:f101::2", "2001:db8:0:f101::a", ipV6Network); Assert.fail(); } catch (InvalidParameterValueException e){ - Assert.assertEquals(e.getMessage(), "The input cidr 2001:db8:0:f101::0/63 is not same as network cidr 2001:db8:0:f101::0/64"); + assertEquals(e.getMessage(), "The input cidr 2001:db8:0:f101::0/63 is not same as network cidr 2001:db8:0:f101::0/64"); } try { configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, "2001:db8:0:f101::1", "2001:db8:0:f101::0/64", "2001:db9:0:f101::2", "2001:db9:0:f101::a", ipV6Network); Assert.fail(); } catch (InvalidParameterValueException e) { - Assert.assertEquals(e.getMessage(), "Exception from Mock: startIPv6 is not in ip6cidr indicated network!"); + assertEquals(e.getMessage(), "Exception from Mock: startIPv6 is not in ip6cidr indicated network!"); } try { configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, "2001:db8:0:f101::1", "2001:db8:0:f101::0/64", "2001:db8:0:f101::a", "2001:db9:0:f101::2", ipV6Network); Assert.fail(); } catch(InvalidParameterValueException e) { - Assert.assertEquals(e.getMessage(), "Exception from Mock: endIPv6 is not in ip6cidr indicated network!"); + assertEquals(e.getMessage(), "Exception from Mock: endIPv6 is not in ip6cidr indicated network!"); } result = configurationMgr.hasSameSubnet(false, null, null, null, null, null, null, true, null, null, "2001:db8:0:f101::2", "2001:db8:0:f101::a", ipV6Network); - Assert.assertTrue(result); + assertTrue(result); } @Test(expected = CloudRuntimeException.class) @@ -1035,12 +1040,12 @@ public class ConfigurationManagerTest { @Test public void testGetVlanNumberFromUriVlan() { - Assert.assertEquals("7", configurationMgr.getVlanNumberFromUri("vlan://7")); + assertEquals("7", configurationMgr.getVlanNumberFromUri("vlan://7")); } @Test public void testGetVlanNumberFromUriUntagged() { - Assert.assertEquals("untagged", configurationMgr.getVlanNumberFromUri("vlan://untagged")); + assertEquals("untagged", configurationMgr.getVlanNumberFromUri("vlan://untagged")); } @Test @@ -1080,48 +1085,48 @@ public class ConfigurationManagerTest { @Test public void shouldUpdateDiskOfferingTests(){ - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString(), Mockito.anyString(), Mockito.any(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), Mockito.any(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); - Assert.assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString(), Mockito.anyString(), nullable(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), Mockito.anyString(), Mockito.anyInt(), Mockito.anyBoolean(), Mockito.anyString(), Mockito.anyString(), Mockito.any(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(Mockito.anyString(), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), Mockito.any(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), Mockito.anyString(), nullable(Integer.class), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), Mockito.anyInt(), nullable(Boolean.class), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), Mockito.anyBoolean(), nullable(String.class), nullable(String.class), nullable(DiskOffering.State.class))); + assertTrue(configurationMgr.shouldUpdateDiskOffering(nullable(String.class), nullable(String.class), nullable(int.class), nullable(Boolean.class), Mockito.anyString(), Mockito.anyString(), nullable(DiskOffering.State.class))); } @Test public void shouldUpdateDiskOfferingTestFalse(){ - Assert.assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null, null, null)); + assertFalse(configurationMgr.shouldUpdateDiskOffering(null, null, null, null, null, null, null)); } @Test public void shouldUpdateIopsRateParametersTestFalse() { - Assert.assertFalse(configurationMgr.shouldUpdateIopsRateParameters(null, null, null, null, null, null)); + assertFalse(configurationMgr.shouldUpdateIopsRateParameters(null, null, null, null, null, null)); } @Test public void shouldUpdateIopsRateParametersTests(){ - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong())); - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong())); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong())); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateIopsRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong())); } @Test public void shouldUpdateBytesRateParametersTestFalse() { - Assert.assertFalse(configurationMgr.shouldUpdateBytesRateParameters(null, null, null, null, null, null)); + assertFalse(configurationMgr.shouldUpdateBytesRateParameters(null, null, null, null, null, null)); } @Test public void shouldUpdateBytesRateParametersTests(){ - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong())); - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class))); - Assert.assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong())); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong())); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong(), nullable(Long.class))); + assertTrue(configurationMgr.shouldUpdateBytesRateParameters(nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), nullable(Long.class), Mockito.anyLong())); } @Test @@ -1235,10 +1240,10 @@ public class ConfigurationManagerTest { return prefixVO; }); configurationMgr.createDataCenterGuestIpv6Prefix(cmd); - Assert.assertEquals(1, persistedPrefix.size()); + assertEquals(1, persistedPrefix.size()); DataCenterGuestIpv6PrefixVO prefixVO = persistedPrefix.get(0); - Assert.assertEquals(zoneId, prefixVO.getDataCenterId()); - Assert.assertEquals(prefix, prefixVO.getPrefix()); + assertEquals(zoneId, prefixVO.getDataCenterId()); + assertEquals(prefix, prefixVO.getPrefix()); } @Test @@ -1255,17 +1260,17 @@ public class ConfigurationManagerTest { Mockito.mock(DataCenterGuestIpv6PrefixVO.class), Mockito.mock(DataCenterGuestIpv6PrefixVO.class))); List prefixes = configurationMgr.listDataCenterGuestIpv6Prefixes(cmd); - Assert.assertEquals(1, prefixes.size()); + assertEquals(1, prefixes.size()); ListGuestNetworkIpv6PrefixesCmd cmd1 = Mockito.mock(ListGuestNetworkIpv6PrefixesCmd.class); Mockito.when(cmd1.getId()).thenReturn(null); Mockito.when(cmd1.getZoneId()).thenReturn(1L); prefixes = configurationMgr.listDataCenterGuestIpv6Prefixes(cmd1); - Assert.assertEquals(2, prefixes.size()); + assertEquals(2, prefixes.size()); ListGuestNetworkIpv6PrefixesCmd cmd2 = Mockito.mock(ListGuestNetworkIpv6PrefixesCmd.class); Mockito.when(cmd2.getId()).thenReturn(null); Mockito.when(cmd2.getZoneId()).thenReturn(null); prefixes = configurationMgr.listDataCenterGuestIpv6Prefixes(cmd2); - Assert.assertEquals(3, prefixes.size()); + assertEquals(3, prefixes.size()); } @Test(expected = InvalidParameterValueException.class) @@ -1304,8 +1309,8 @@ public class ConfigurationManagerTest { return true; }); configurationMgr.deleteDataCenterGuestIpv6Prefix(cmd); - Assert.assertEquals(1, removedPrefix.size()); - Assert.assertEquals(prefixId, removedPrefix.get(0)); + assertEquals(1, removedPrefix.size()); + assertEquals(prefixId, removedPrefix.get(0)); } @Test(expected = InvalidParameterValueException.class) @@ -1355,8 +1360,8 @@ public class ConfigurationManagerTest { mockPersistDatacenterForCreateZone(); DataCenter zone = configurationMgr.createZone(cmd); Assert.assertNotNull(zone); - Assert.assertEquals(NetworkType.Advanced, zone.getNetworkType()); - Assert.assertEquals(DataCenter.Type.Edge, zone.getType()); + assertEquals(NetworkType.Advanced, zone.getNetworkType()); + assertEquals(DataCenter.Type.Edge, zone.getType()); } @Test @@ -1368,8 +1373,8 @@ public class ConfigurationManagerTest { mockPersistDatacenterForCreateZone(); DataCenter zone = configurationMgr.createZone(cmd); Assert.assertNotNull(zone); - Assert.assertEquals(NetworkType.Advanced, zone.getNetworkType()); - Assert.assertEquals(DataCenter.Type.Core, zone.getType()); + assertEquals(NetworkType.Advanced, zone.getNetworkType()); + assertEquals(DataCenter.Type.Core, zone.getType()); } @Test @@ -1381,7 +1386,7 @@ public class ConfigurationManagerTest { mockPersistDatacenterForCreateZone(); DataCenter zone = configurationMgr.createZone(cmd); Assert.assertNotNull(zone); - Assert.assertEquals(NetworkType.Basic, zone.getNetworkType()); + assertEquals(NetworkType.Basic, zone.getNetworkType()); } @Test(expected = InvalidParameterValueException.class) @@ -1428,4 +1433,77 @@ public class ConfigurationManagerTest { Mockito.doNothing().when(messageBus).publish(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); configurationMgr.createPod(zoneId, "TestPod", null, null, null, null, null); } + + @Test + public void testValidateSpecificConfigurationValues_ValidFormatWithPositiveIntegers() { + String name = AgentManager.GranularWaitTimeForCommands.toString(); + String validValue = "CopyCommand=120, DeleteCommand= 60"; + + try { + configurationMgr.validateSpecificConfigurationValues(name, validValue, String.class); + } catch (InvalidParameterValueException e) { + Assert.fail("Exception should not be thrown for a valid command string with positive integers, but there is an error " + e); + } + } + + @Test + public void testValidateSpecificConfigurationValues_InvalidFormat() { + String name = AgentManager.GranularWaitTimeForCommands.toString(); + String invalidValue = "{\"CopyCommand\": 120}"; + + try { + configurationMgr.validateSpecificConfigurationValues(name, invalidValue, String.class); + Assert.fail("Exception should be thrown for an invalid command string."); + } catch (InvalidParameterValueException e) { + assertTrue(e.getMessage().contains("does not contain '='.")); + } + } + + @Test + public void testValidCommandString() { + final String input = "DhcpEntryCommand=600, SavePasswordCommand=300, VmDataCommand=300"; + final Pair result = configurationMgr.validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(input); + assertTrue("Expected validation to pass", result.first()); + assertEquals("Expected no error message", "", result.second()); + } + + @Test + public void testInvalidCommandValue() { + final String input = "DhcpEntryCommand=600, SavePasswordCommand=300, VmDataCommand=invalid"; + final Pair result = configurationMgr.validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(input); + assertFalse("Expected validation to fail", result.first()); + assertEquals("Expected specific error message", + "Validation failed: The value for command 'VmDataCommand' is not a valid integer. Invalid value: invalid", + result.second()); + } + + @Test + public void testCommandWithZeroValue() { + final String input = "DhcpEntryCommand=600, SavePasswordCommand=0, VmDataCommand=300"; + final Pair result = configurationMgr.validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(input); + assertFalse("Expected validation to fail", result.first()); + assertEquals("Expected specific error message", + "Validation failed: The value for command 'SavePasswordCommand' is not greater than 0. Invalid value: 0", + result.second()); + } + + @Test + public void testCommandWithNegativeValue() { + final String input = "DhcpEntryCommand=-100, SavePasswordCommand=300, VmDataCommand=300"; + final Pair result = configurationMgr.validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(input); + assertFalse("Expected validation to fail", result.first()); + assertEquals("Expected specific error message", + "Validation failed: The value for command 'DhcpEntryCommand' is not greater than 0. Invalid value: -100", + result.second()); + } + + @Test + public void testInvalidCommandStructure() { + final String input = "DhcpEntryCommand600, SavePasswordCommand=300, VmDataCommand=300"; + final Pair result = configurationMgr.validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(input); + assertFalse("Expected validation to fail", result.first()); + assertEquals("Expected specific error message", + "Validation failed: Command 'DhcpEntryCommand600' does not contain '='.", + result.second()); + } } diff --git a/test/integration/smoke/test_global_settings.py b/test/integration/smoke/test_global_settings.py index 2018384ab3a..53f55736d4f 100644 --- a/test/integration/smoke/test_global_settings.py +++ b/test/integration/smoke/test_global_settings.py @@ -76,6 +76,12 @@ class TestUpdateConfigWithScope(cloudstackTestCase): updateConfigurationCmd.scopeid = 1 self.apiClient.updateConfiguration(updateConfigurationCmd) + + updateConfigurationCmd = updateConfiguration.updateConfigurationCmd() + updateConfigurationCmd.name = "commands.timeout" + updateConfigurationCmd.value = "" + self.apiClient.updateConfiguration(updateConfigurationCmd) + class TestListConfigurations(cloudstackTestCase): """ Test to list configurations (global settings) @@ -181,3 +187,46 @@ class TestListConfigurations(cloudstackTestCase): subgroup=subgroup) self.assertNotEqual(len(listConfigurationsResponse), 0, "Check if the list configurations API returns a non-empty response") self.debug("Total %d configurations for group %s, subgroup %s" % (len(listConfigurationsResponse), group, subgroup)) + + @attr(tags=["devcloud", "basic", "advanced"], required_hardware="false") + def test_UpdateCommandsTimeoutConfigParamWithValidValue(self): + """ + test update configuration setting for commands.timeout with valid value + @return: + """ + updateConfigurationCmd = updateConfiguration.updateConfigurationCmd() + updateConfigurationCmd.name = "commands.timeout" + updateConfigurationCmd.value = "DhcpEntryCommand= 600, SavePasswordCommand= 300, VmDataCommand= 300" + + updateConfigurationResponse = self.apiClient.updateConfiguration(updateConfigurationCmd) + self.debug("updated the parameter %s with value %s" % (updateConfigurationResponse.name, updateConfigurationResponse.value)) + + listConfigurationsCmd = listConfigurations.listConfigurationsCmd() + listConfigurationsCmd.name = updateConfigurationResponse.name + listConfigurationsResponse = self.apiClient.listConfigurations(listConfigurationsCmd) + + for item in listConfigurationsResponse: + if item.name == updateConfigurationResponse.name: + configParam = item + + self.assertEqual(configParam.value, updateConfigurationResponse.value, "Check if the update API returned is the same as the one we got in the list API") + + + @attr(tags=["devcloud", "basic", "advanced"], required_hardware="false") + def test_UpdateCommandsTimeoutConfigParamWithInvalidValue(self): + """ + Test update configuration setting for commands.timeout with invalid valid value + @return: + """ + updateConfigurationCmd = updateConfiguration.updateConfigurationCmd() + updateConfigurationCmd.name = "commands.timeout" + updateConfigurationCmd.value = "StartCommand: 1" # Intentionally providing invalid format + + try: + self.apiClient.updateConfiguration(updateConfigurationCmd) + self.fail("API call should have failed due to invalid format, but it succeeded.") + except Exception as e: + self.debug("Caught expected exception: %s" % str(e)) + error_message = str(e) + self.assertIn("errorCode: 431", error_message, "Expected error code 431 for invalid format") + self.assertIn("Validation failed", error_message, "Expected validation failure message")