From 5d41ca7149a004b411472e9844b3dfea175c2c17 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Mon, 9 Sep 2024 18:14:27 +0530 Subject: [PATCH 1/8] Introducing granular command timeouts global setting --- .../java/com/cloud/agent/AgentManager.java | 4 + .../cloud/agent/manager/AgentManagerImpl.java | 75 +++++- .../agent/manager/AgentManagerImplTest.java | 20 ++ .../ConfigurationManagerImpl.java | 72 ++++++ .../ConfigurationManagerTest.java | 218 ++++++++++++------ .../integration/smoke/test_global_settings.py | 47 ++++ 6 files changed, 362 insertions(+), 74 deletions(-) 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 2182dfc542d3..c810fdfbefd4 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 for specific commands. " + + "For example: DhcpEntryCommand=600, SavePasswordCommand=300, VmDataCommand=300", true); + 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 9333410e0aa2..b2a1e4a4e904 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 @@ -360,6 +360,7 @@ public Answer sendTo(final Long dcId, final HypervisorType type, final Command c public Answer send(final Long hostId, final Command cmd) throws AgentUnavailableException, OperationTimedoutException { final Commands cmds = new Commands(Command.OnError.Stop); cmds.addCommand(cmd); + logger.debug(String.format("Wait time set on the command %s is %d seconds", cmd, cmd.getWait())); send(hostId, cmds, cmd.getWait()); final Answer[] answers = cmds.getAnswers(); if (answers != null && !(answers[0] instanceof UnsupportedAnswer)) { @@ -424,6 +425,65 @@ private void setEmptyAnswers(final Commands commands, final Command[] cmds) { } } + protected int getTimeout(final Commands commands, int timeout) { + int result; + if (timeout > 0) { + result = timeout; + } else { + logger.debug(String.format("Considering the Wait global setting %d, since wait time set on command is 0", Wait.value())); + result = Wait.value(); + } + + int granularTimeout = getTimeoutFromGranularWaitTime(commands); + return (granularTimeout > 0) ? granularTimeout : result; + } + + protected int getTimeoutFromGranularWaitTime(final Commands commands) { + logger.debug("Looking for the commands.timeout global setting for any command-specific timeout value"); + String commandWaits = GranularWaitTimeForCommands.value().trim(); + + int maxWait = 0; + if (StringUtils.isNotEmpty(commandWaits)) { + try { + Map commandTimeouts = getCommandTimeoutsMap(commandWaits); + + for (final Command cmd : commands) { + String simpleCommandName = cmd.getClass().getSimpleName(); + Integer commandTimeout = commandTimeouts.get(simpleCommandName); + + if (commandTimeout != null) { + logger.debug(String.format("Timeout %d found for command %s in commands.timeout global setting", commandTimeout, cmd.toString())); + if (commandTimeout > maxWait) { + maxWait = commandTimeout; + } + } + } + } catch (Exception e) { + logger.error(String.format("Error while processing the commands.timeout global setting for the granular timeouts for the command, " + + "falling back to the command timeout: %s", e.getMessage())); + } + } + + return maxWait; + } + + 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) { + String commandName = parts[0].trim(); + int commandTimeout = Integer.parseInt(parts[1].trim()); + commandTimeouts.put(commandName, commandTimeout); + } else { + logger.warn(String.format("Invalid format in commands.timeout global setting: %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,9 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th throw new AgentUnavailableException(-1); } - if (timeout <= 0) { - timeout = Wait.value(); + int wait = getTimeout(commands, timeout); + for (Command cmd : commands) { + cmd.setWait(wait); } if (CheckTxnBeforeSending.value()) { @@ -454,7 +515,7 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th 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,7 +1058,13 @@ public Answer easySend(final Long hostId, final Command cmd) { @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) { + logger.debug(String.format("Wait time set on the command %s is %d", cmd, cmd.getWait())); if (cmd.getWait() > wait) { wait = cmd.getWait(); } @@ -1821,7 +1888,7 @@ public String getConfigComponentName() { @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 452cfd90056f..52b7ed775335 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 void testNotifyMonitorsOfConnectionWhenStoragePoolConnectionHostFailure() } 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 dee1aa81758d..cb835875408c 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 @@ protected String validateConfigurationValue(String name, String value, String sc 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,76 @@ protected String validateValueRange(String name, String value, Class type, Co 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 { + String[] commands = value.split(","); + for (String command : commands) { + command = command.trim(); + if (!command.contains("=")) { + String errorMessage = "Validation failed: Command '" + command + "' does not contain '='."; + return new Pair<>(false, errorMessage); + } + + String[] parts = command.split("="); + if (parts.length != 2) { + String errorMessage = "Validation failed: Command '" + command + "' is not properly formatted."; + return new Pair<>(false, errorMessage); + } + + String commandName = parts[0].trim(); + String valueString = parts[1].trim(); + + if (commandName.isEmpty()) { + String errorMessage = "Validation failed: Command name is missing in '" + command + "'."; + return new Pair<>(false, errorMessage); + } + + try { + int num = Integer.parseInt(valueString); + if (num <= 0) { + String errorMessage = "Validation failed: The value for command '" + commandName + "' is not greater than 0. Invalid value: " + num; + return new Pair<>(false, errorMessage); + } + } catch (NumberFormatException e) { + String errorMessage = "Validation failed: The value for command '" + commandName + "' is not a valid integer. Invalid value: " + valueString; + return new Pair<>(false, errorMessage); + } + } + + return new Pair<>(true, ""); + } catch (Exception e) { + String errorMessage = "Validation failed: An error occurred while parsing the command string. Error: " + 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 ceffe0193770..69251173ea3c 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.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.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 @@ void runDedicatePublicIpRangeInvalidRange() throws Exception { 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 @@ void runDedicatePublicIpRangeDedicatedRange() throws Exception { 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 @@ void runDedicatePublicIpRangeInvalidZone() throws Exception { 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 @@ void runDedicatePublicIpRangeIPAddressAllocated() throws Exception { 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 @@ void runReleasePublicIpRangePostiveTest1() throws Exception { 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 @@ void runReleasePublicIpRangePostiveTest2() throws Exception { 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 @@ void runReleasePublicIpRangeInvalidIpRange() throws Exception { 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 @@ void runReleaseNonDedicatedPublicIpRange() throws Exception { 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 void validateInvalidSourceNatTypeForSourceNatServiceCapablitiesTest() { 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 void validateInvalidBooleanValueForSourceNatServiceCapablitiesTest() { 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 void validateInvalidCapabilityForSourceNatServiceCapablitiesTest() { 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 void validateInvalidStaticNatServiceCapablitiesTest() { 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 void isRedundantRouter() { 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 void isSharedSourceNat() { 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 void isNotSharedSourceNat() { 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 void sourceNatCapabilitiesContainValidValues() { 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 void validateTFStaticNatServiceCapablitiesTest() { 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 void hasSameSubnetTest() { //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 void hasSameSubnetTest() { 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 void testGetVlanNumberFromUriInvalidSintax() { @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 void validateMaximumIopsAndBytesLengthTestDefaultLengthConfigs() { @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 void testCreateDataCenterGuestIpv6Prefix() { 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 void testListDataCenterGuestIpv6Prefixes() { 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 void testDeleteDataCenterGuestIpv6Prefix() { 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 void testCreateEdgeZone() { 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 void testCreateCoreZone() { 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 void testCreateBasicZone() { 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 void testEdgeZoneCreatePod() { 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."); + } + } + + @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 2018384ab3a2..24acc93e08e0 100644 --- a/test/integration/smoke/test_global_settings.py +++ b/test/integration/smoke/test_global_settings.py @@ -76,6 +76,12 @@ def tearDown(self): 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,44 @@ def test_03_config_groups(self): 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.cfgName = updateConfigurationResponse.name + listConfigurationsResponse = self.apiClient.listConfigurations(listConfigurationsCmd) + + self.assertEqual(listConfigurationsResponse.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_response = e.error + self.assertEqual(error_response['errorcode'], 431, "Expected error code 431 for invalid 21") + self.assertEqual(error_response['cserrorcode'], 4350, "Expected CS error code 4350 for value parsing failure") + self.assertIn("Validation failed", error_response['errortext'], "Expected error message related to format validation") From f4701796b66f15799e7cfa3b02e530ee3bf39886 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 10 Sep 2024 11:22:54 +0530 Subject: [PATCH 2/8] fix marvin tests --- test/integration/smoke/test_global_settings.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/test/integration/smoke/test_global_settings.py b/test/integration/smoke/test_global_settings.py index 24acc93e08e0..53f55736d4f6 100644 --- a/test/integration/smoke/test_global_settings.py +++ b/test/integration/smoke/test_global_settings.py @@ -202,11 +202,14 @@ def test_UpdateCommandsTimeoutConfigParamWithValidValue(self): self.debug("updated the parameter %s with value %s" % (updateConfigurationResponse.name, updateConfigurationResponse.value)) listConfigurationsCmd = listConfigurations.listConfigurationsCmd() - listConfigurationsCmd.cfgName = updateConfigurationResponse.name + listConfigurationsCmd.name = updateConfigurationResponse.name listConfigurationsResponse = self.apiClient.listConfigurations(listConfigurationsCmd) - self.assertEqual(listConfigurationsResponse.value, updateConfigurationResponse.value, "Check if the update API returned \ - is the same as the one we got in the list API") + 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") @@ -224,7 +227,6 @@ def test_UpdateCommandsTimeoutConfigParamWithInvalidValue(self): 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_response = e.error - self.assertEqual(error_response['errorcode'], 431, "Expected error code 431 for invalid 21") - self.assertEqual(error_response['cserrorcode'], 4350, "Expected CS error code 4350 for value parsing failure") - self.assertIn("Validation failed", error_response['errortext'], "Expected error message related to format validation") + 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") From 961755c519da251e969f72e2d1fc757543047e7c Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 11 Sep 2024 11:03:31 +0530 Subject: [PATCH 3/8] Fixed log messages --- .../src/main/java/com/cloud/agent/AgentManager.java | 2 +- .../com/cloud/agent/manager/AgentManagerImpl.java | 5 +---- .../configuration/ConfigurationManagerImpl.java | 12 ++++++------ .../configuration/ConfigurationManagerTest.java | 2 +- 4 files changed, 9 insertions(+), 12 deletions(-) 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 c810fdfbefd4..f7a000f78dd3 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 @@ -51,7 +51,7 @@ public interface AgentManager { "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 for specific commands. " + + "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", true); public enum TapAgentsAction { 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 b2a1e4a4e904..8ae2107836aa 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 @@ -430,7 +430,6 @@ protected int getTimeout(final Commands commands, int timeout) { if (timeout > 0) { result = timeout; } else { - logger.debug(String.format("Considering the Wait global setting %d, since wait time set on command is 0", Wait.value())); result = Wait.value(); } @@ -439,7 +438,6 @@ protected int getTimeout(final Commands commands, int timeout) { } protected int getTimeoutFromGranularWaitTime(final Commands commands) { - logger.debug("Looking for the commands.timeout global setting for any command-specific timeout value"); String commandWaits = GranularWaitTimeForCommands.value().trim(); int maxWait = 0; @@ -452,7 +450,6 @@ protected int getTimeoutFromGranularWaitTime(final Commands commands) { Integer commandTimeout = commandTimeouts.get(simpleCommandName); if (commandTimeout != null) { - logger.debug(String.format("Timeout %d found for command %s in commands.timeout global setting", commandTimeout, cmd.toString())); if (commandTimeout > maxWait) { maxWait = commandTimeout; } @@ -492,6 +489,7 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th } int wait = getTimeout(commands, timeout); + logger.debug(String.format("Wait time setting on %s is %d seconds", commands, wait)); for (Command cmd : commands) { cmd.setWait(wait); } @@ -1064,7 +1062,6 @@ public Answer[] send(final Long hostId, final Commands cmds) throws AgentUnavail } for (final Command cmd : cmds) { - logger.debug(String.format("Wait time set on the command %s is %d", cmd, cmd.getWait())); if (cmd.getWait() > wait) { wait = cmd.getWait(); } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index cb835875408c..f360cbdccc1c 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1408,13 +1408,13 @@ protected Pair validateCommaSeparatedKeyValueConfigWithPositive for (String command : commands) { command = command.trim(); if (!command.contains("=")) { - String errorMessage = "Validation failed: Command '" + command + "' does not contain '='."; + 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 = "Validation failed: Command '" + command + "' is not properly formatted."; + String errorMessage = String.format("Validation failed: Command '%s' is not properly formatted.", command); return new Pair<>(false, errorMessage); } @@ -1422,25 +1422,25 @@ protected Pair validateCommaSeparatedKeyValueConfigWithPositive String valueString = parts[1].trim(); if (commandName.isEmpty()) { - String errorMessage = "Validation failed: Command name is missing in '" + command + "'."; + String errorMessage = String.format("Validation failed: Command name is missing in '%s'.", commandName); return new Pair<>(false, errorMessage); } try { int num = Integer.parseInt(valueString); if (num <= 0) { - String errorMessage = "Validation failed: The value for command '" + commandName + "' is not greater than 0. Invalid value: " + num; + 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 = "Validation failed: The value for command '" + commandName + "' is not a valid integer. Invalid value: " + valueString; + 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 = "Validation failed: An error occurred while parsing the command string. Error: " + e.getMessage(); + String errorMessage = String.format("Validation failed: An error occurred while parsing the command string. Error: %s", e.getMessage()); return new Pair<>(false, errorMessage); } } diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java index 69251173ea3c..badf730a0617 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java @@ -1442,7 +1442,7 @@ public void testValidateSpecificConfigurationValues_ValidFormatWithPositiveInteg 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."); + Assert.fail("Exception should not be thrown for a valid command string with positive integers, but there is an error " + e); } } From 703ae5a39746e27605a8970f409ee2c1d8f07ea8 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 11 Sep 2024 11:53:05 +0530 Subject: [PATCH 4/8] some more log message fix --- .../src/main/java/com/cloud/agent/manager/AgentManagerImpl.java | 1 - .../java/com/cloud/configuration/ConfigurationManagerImpl.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) 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 8ae2107836aa..2eef9a109274 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 @@ -360,7 +360,6 @@ public Answer sendTo(final Long dcId, final HypervisorType type, final Command c public Answer send(final Long hostId, final Command cmd) throws AgentUnavailableException, OperationTimedoutException { final Commands cmds = new Commands(Command.OnError.Stop); cmds.addCommand(cmd); - logger.debug(String.format("Wait time set on the command %s is %d seconds", cmd, cmd.getWait())); send(hostId, cmds, cmd.getWait()); final Answer[] answers = cmds.getAnswers(); if (answers != null && !(answers[0] instanceof UnsupportedAnswer)) { diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index f360cbdccc1c..6495d0cb614d 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1422,7 +1422,7 @@ protected Pair validateCommaSeparatedKeyValueConfigWithPositive String valueString = parts[1].trim(); if (commandName.isEmpty()) { - String errorMessage = String.format("Validation failed: Command name is missing in '%s'.", commandName); + String errorMessage = String.format("Validation failed: Command name is missing in '%s'.", command); return new Pair<>(false, errorMessage); } From f8ee3637b5544811c05c2f6572be8945e4d5ce29 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 13 Sep 2024 10:46:44 +0530 Subject: [PATCH 5/8] Fix empty value setting --- .../ConfigurationManagerImpl.java | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 6495d0cb614d..d7e2160ef35b 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -1404,37 +1404,39 @@ protected void validateSpecificConfigurationValues(String name, String value, Cl protected Pair validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(String value) { try { - 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); - } + 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[] 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(); + 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); - } + 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); + 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); } - } 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); } } From 87547140ba473c03ac0efb5d283eed2128db277f Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 18 Sep 2024 16:53:30 +0530 Subject: [PATCH 6/8] Converted the global setting to non-dynamic --- .../java/com/cloud/agent/AgentManager.java | 2 +- .../cloud/agent/manager/AgentManagerImpl.java | 41 +++++++++++-------- 2 files changed, 24 insertions(+), 19 deletions(-) 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 f7a000f78dd3..81525ca13f1b 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 @@ -52,7 +52,7 @@ public interface AgentManager { 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", true); + "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 2eef9a109274..9916192d841a 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.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 boolean configure(final String name, final Map params) th _monitorExecutor = new ScheduledThreadPoolExecutor(1, new NamedThreadFactory("AgentMonitor")); + initializeCommandTimeouts(); + return true; } @@ -437,32 +441,33 @@ protected int getTimeout(final Commands commands, int timeout) { } protected int getTimeoutFromGranularWaitTime(final Commands commands) { - String commandWaits = GranularWaitTimeForCommands.value().trim(); - int maxWait = 0; - if (StringUtils.isNotEmpty(commandWaits)) { - try { - Map commandTimeouts = getCommandTimeoutsMap(commandWaits); - - for (final Command cmd : commands) { - String simpleCommandName = cmd.getClass().getSimpleName(); - Integer commandTimeout = commandTimeouts.get(simpleCommandName); - - if (commandTimeout != null) { - if (commandTimeout > maxWait) { - maxWait = commandTimeout; - } - } + 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; } - } catch (Exception e) { - logger.error(String.format("Error while processing the commands.timeout global setting for the granular timeouts for the command, " + - "falling back to the command timeout: %s", e.getMessage())); } } return maxWait; } + private void initializeCommandTimeouts() { + String commandWaits = GranularWaitTimeForCommands.value().trim(); + if (StringUtils.isNotEmpty(commandWaits)) { + try { + _commandTimeouts = getCommandTimeoutsMap(commandWaits); + logger.info(String.format("Timeouts for management server internal commands successfully initialized from global setting commands.timeout: %s", _commandTimeouts)); + } catch (Exception e) { + logger.error("Error initializing command timeouts map: " + e.getMessage()); + _commandTimeouts = new HashMap<>(); + } + } + } + private Map getCommandTimeoutsMap(String commandWaits) { String[] commandPairs = commandWaits.split(","); Map commandTimeouts = new HashMap<>(); From ee992632e229195c1cfa8a69dd64842cfa95e409 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Fri, 20 Sep 2024 17:23:18 +0530 Subject: [PATCH 7/8] set wait on command only when granular wait is defined. This is to keep the backward compatibility --- .../main/java/com/cloud/agent/manager/AgentManagerImpl.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 9916192d841a..17721ed02473 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 @@ -495,7 +495,11 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th int wait = getTimeout(commands, timeout); logger.debug(String.format("Wait time setting on %s is %d seconds", commands, wait)); for (Command cmd : commands) { - cmd.setWait(wait); + String simpleCommandName = cmd.getClass().getSimpleName(); + Integer commandTimeout = _commandTimeouts.get(simpleCommandName); + if (commandTimeout != null) { + cmd.setWait(wait); + } } if (CheckTxnBeforeSending.value()) { From 124da2b245751bcc4e190f6a59138f8b9cfa5fb0 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Tue, 7 Jan 2025 11:04:49 +0530 Subject: [PATCH 8/8] Improve error logging --- .../cloud/agent/manager/AgentManagerImpl.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) 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 17721ed02473..63e975195341 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 @@ -458,13 +458,8 @@ protected int getTimeoutFromGranularWaitTime(final Commands commands) { private void initializeCommandTimeouts() { String commandWaits = GranularWaitTimeForCommands.value().trim(); if (StringUtils.isNotEmpty(commandWaits)) { - try { - _commandTimeouts = getCommandTimeoutsMap(commandWaits); - logger.info(String.format("Timeouts for management server internal commands successfully initialized from global setting commands.timeout: %s", _commandTimeouts)); - } catch (Exception e) { - logger.error("Error initializing command timeouts map: " + e.getMessage()); - _commandTimeouts = new HashMap<>(); - } + _commandTimeouts = getCommandTimeoutsMap(commandWaits); + logger.info(String.format("Timeouts for management server internal commands successfully initialized from global setting commands.timeout: %s", _commandTimeouts)); } } @@ -475,11 +470,15 @@ private Map getCommandTimeoutsMap(String commandWaits) { for (String commandPair : commandPairs) { String[] parts = commandPair.trim().split("="); if (parts.length == 2) { - String commandName = parts[0].trim(); - int commandTimeout = Integer.parseInt(parts[1].trim()); - commandTimeouts.put(commandName, commandTimeout); + 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.warn(String.format("Invalid format in commands.timeout global setting: %s", commandPair)); + logger.error(String.format("Error initialising the timeouts for management server internal commands. Invalid format in commands.timeout: %s", commandPair)); } } return commandTimeouts;