Introducing granular command timeouts global setting

harikrishna-patnala · harikrishna-patnala · commit a6663031b841 · 2024-09-09T18:14:27.000+05:30
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
@@ -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 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
@@ -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,15 +425,75 @@ 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<String, Integer> 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<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) {
+                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)";
         if (hostId == null) {
             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;
@@ -988,7 +1049,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();
             }
@@ -1802,7 +1869,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
@@ -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
@@ -1226,6 +1226,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);
@@ -1354,6 +1356,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.
+     * <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 {
+            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:</br>
      * <ul>
diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java
diff --git a/test/integration/smoke/test_global_settings.py b/test/integration/smoke/test_global_settings.py
