Introducing granular command timeouts global setting (#9659)

* Introducing granular command timeouts global setting

* fix marvin tests

* Fixed log messages

* some more log message fix

* Fix empty value setting

* Converted the global setting to non-dynamic

* set wait on command only when granular wait is defined. This is to keep the backward compatibility

* Improve error logging

Author: harikrishna-patnala

engine/components-api/src/main/java/com/cloud/agent/AgentManager.java

@@ -50,6 +50,10 @@ public interface AgentManager {
     ConfigKey<Integer> ReadyCommandWait = new ConfigKey<Integer>("Advanced", Integer.class, "ready.command.wait",
             "60", "Time in seconds to wait for Ready command to return", true);
 
+    ConfigKey<String> 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,
     }

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<Pair<Integer, Listener>> _cmdMonitors = new ArrayList<Pair<Integer, Listener>>(17);
     protected List<Pair<Integer, StartupCommandProcessor>> _creationMonitors = new ArrayList<Pair<Integer, StartupCommandProcessor>>(17);
     protected List<Long> _loadingAgents = new ArrayList<Long>();
+    protected Map<String, Integer> _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<String, Object> params) th
 
         _monitorExecutor = new ScheduledThreadPoolExecutor(1, new NamedThreadFactory("AgentMonitor"));
 
+        initializeCommandTimeouts();
+
         return true;
     }
 
@@ -424,15 +428,77 @@ 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 {
+            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<String, Integer> getCommandTimeoutsMap(String commandWaits) {
+        String[] commandPairs = commandWaits.split(",");
+        Map<String, Integer> 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)";
         if (hostId == null) {
             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 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,6 +1063,11 @@ 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) {
             if (cmd.getWait() > wait) {
                 wait = cmd.getWait();
@@ -1821,7 +1892,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 {

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);
+    }
 }

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,78 @@ 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.
+     * <ul>
+     *   <li>The value must be a comma-separated list of key-value pairs, where each value must be a positive integer.</li>
+     *   <li>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</li>
+     *   <li>Throws an {@link InvalidParameterValueException} if validation fails.</li>
+     * </ul>
+     *
+     * @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<Boolean, String> validationResult = validateCommaSeparatedKeyValueConfigWithPositiveIntegerValues(value);
+                if (!validationResult.first()) {
+                    String errMsg = validationResult.second();
+                    logger.error(validationResult.second());
+                    throw new InvalidParameterValueException(errMsg);
+                }
+            }
+        }
+    }
+
+    protected Pair<Boolean, String> 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:</br>
      * <ul>