Flexible timeout for commands MS sends to the agents

core/src/main/java/com/cloud/agent/api/CheckHealthCommand.java

@@ -21,10 +21,6 @@
 
 public class CheckHealthCommand extends Command {
 
-    public CheckHealthCommand() {
-        setWait(50);
-    }
-
     @Override
     public boolean executeInSequence() {
         return false;

core/src/main/java/com/cloud/agent/api/CheckOnHostCommand.java

@@ -31,7 +31,6 @@ protected CheckOnHostCommand() {
 
     public CheckOnHostCommand(Host host) {
         this.host = new HostTO(host);
-        setWait(20);
     }
 
     public CheckOnHostCommand(Host host, boolean reportCheckFailureIfOneStorageIsDown) {

core/src/main/java/com/cloud/agent/api/CheckVirtualMachineCommand.java

@@ -29,7 +29,6 @@ protected CheckVirtualMachineCommand() {
 
     public CheckVirtualMachineCommand(String vmName) {
         this.vmName = vmName;
-        setWait(20);
     }
 
     public String getVmName() {

core/src/main/java/org/apache/cloudstack/ca/SetupCertificateCommand.java

@@ -45,7 +45,7 @@ public SetupCertificateCommand(final Certificate certificate) {
         if (certificate == null) {
             throw new CloudRuntimeException("A null certificate was provided to setup");
         }
-        setWait(60);
+
         try {
             this.certificate = CertUtils.x509CertificateToPem(certificate.getClientCertificate());
             this.caCertificates = CertUtils.x509CertificatesToPem(certificate.getCaCertificates());

core/src/main/java/org/apache/cloudstack/ca/SetupKeyStoreCommand.java

@@ -35,7 +35,7 @@ public class SetupKeyStoreCommand extends NetworkElementCommand {
 
     public SetupKeyStoreCommand(final int validityDays) {
         super();
-        setWait(60);
+
         this.validityDays = validityDays;
         if (this.validityDays < 1) {
             this.validityDays = 1;

core/src/test/java/org/apache/cloudstack/api/agent/test/CheckHealthCommandTest.java

@@ -20,7 +20,6 @@
 package org.apache.cloudstack.api.agent.test;
 
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 
 import org.junit.Test;
 
@@ -29,12 +28,6 @@
 public class CheckHealthCommandTest {
     CheckHealthCommand chc = new CheckHealthCommand();
 
-    @Test
-    public void testGetWait() {
-        int wait = chc.getWait();
-        assertTrue(wait == 50);
-    }
-
     @Test
     public void testExecuteInSequence() {
         boolean b = chc.executeInSequence();

core/src/test/java/org/apache/cloudstack/api/agent/test/CheckOnHostCommandTest.java

@@ -532,12 +532,6 @@ public void testGetResourceState() {
         assertTrue(r == ResourceState.Enabled);
     }
 
-    @Test
-    public void testGetWait() {
-        int wait = cohc.getWait();
-        assertTrue(20 == wait);
-    }
-
     @Test
     public void testExecuteInSequence() {
         boolean b = cohc.executeInSequence();

engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java

@@ -25,6 +25,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -34,6 +35,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.stream.Collectors;
 
 import javax.inject.Inject;
 import javax.naming.ConfigurationException;
@@ -46,7 +48,9 @@
 import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
 import org.apache.cloudstack.framework.config.ConfigKey;
 import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.config.dao.CommandTimeoutDao;
 import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.CommandTimeoutVO;
 import org.apache.cloudstack.framework.jobs.AsyncJob;
 import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext;
 import org.apache.cloudstack.managed.context.ManagedContextRunnable;
@@ -157,6 +161,10 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl
     protected HostPodDao _podDao = null;
     @Inject
     protected ConfigurationDao _configDao = null;
+
+    @Inject
+    protected CommandTimeoutDao commandTimeoutDao;
+
     @Inject
     protected ClusterDao _clusterDao = null;
 
@@ -430,9 +438,7 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th
             throw new AgentUnavailableException(-1);
         }
 
-        if (timeout <= 0) {
-            timeout = Wait.value();
-        }
+        timeout = getTimeoutForCommands(commands, timeout);
 
         if (CheckTxnBeforeSending.value()) {
             if (!noDbTxn()) {
@@ -459,6 +465,35 @@ public Answer[] send(final Long hostId, final Commands commands, int timeout) th
         return answers;
     }
 
+    protected int getTimeoutForCommands(Commands commands, int timeout) {
+        Set<String> commandsClassPath = commands.getCommands().stream().map(command -> command.getClass().getName()).collect(Collectors.toSet());
+
+        if (timeout > 0) {
+            logger.trace("The timeout [{}] was already defined for commands {}; therefore, we will use it instead of searching for the max value in table " +
+                "[{}].", timeout, commandsClassPath, CommandTimeoutVO.TABLE_NAME);
+            return timeout;
+        }
+
+        logger.trace("The timeout for commands {} was not defined yet; therefore, we will search for the max value between the commands in table " +
+            "[{}].", commandsClassPath, CommandTimeoutVO.TABLE_NAME);
+        timeout = commandTimeoutDao.findMaxTimeoutBetweenCommands(commandsClassPath);
+
+        if (timeout > 0) {
+            logger.trace("We found [{}] as the max timeout between commands {} in table [{}]; using it.", timeout, commandsClassPath,
+                CommandTimeoutVO.TABLE_NAME);
+        } else {
+            timeout = getWaitValue();
+            logger.trace("We did not find a max timeout between commands %s in table [{}]; therefore, we will fallback to the value of " +
+                    "configuration [{}]: [{}].", commandsClassPath, CommandTimeoutVO.TABLE_NAME, Wait.key(), timeout);
+        }
+
+        return timeout;
+    }
+
+    protected Integer getWaitValue() {
+        return Wait.value();
+    }
+
     protected Status investigate(final AgentAttache agent) {
         final Long hostId = agent.getId();
         final HostVO host = _hostDao.findById(hostId);

engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java

@@ -26,61 +26,103 @@
 import com.cloud.host.Status;
 import com.cloud.host.dao.HostDao;
 import com.cloud.utils.Pair;
+import org.apache.cloudstack.framework.config.dao.CommandTimeoutDao;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
 
-import java.util.ArrayList;
+import java.util.List;
 
+@RunWith(MockitoJUnitRunner.class)
 public class AgentManagerImplTest {
 
-    private HostDao hostDao;
+    @Mock
+    private HostDao hostDaoMock;
+
+    @Mock
     private Listener storagePoolMonitor;
-    private AgentAttache attache;
-    private AgentManagerImpl mgr = Mockito.spy(new AgentManagerImpl());
+
+    @Mock
+    private Commands commandsMock;
+
+    @Mock
+    private CommandTimeoutDao commandTimeoutDaoMock;
+
+    @InjectMocks
+    @Spy
+    private AgentManagerImpl agentManagerImplSpy;
+
     private HostVO host;
-    private StartupCommand[] cmds;
+    private StartupCommand[] cmds = new StartupCommand[]{ new StartupRoutingCommand() };
+    private AgentAttache attache = new ConnectedAgentAttache(null, 1L, "kvm-attache", null, false);
 
     @Before
-    public void setUp() throws Exception {
+    public void setUp() {
         host = new HostVO("some-Uuid");
         host.setDataCenterId(1L);
-        cmds = new StartupCommand[]{new StartupRoutingCommand()};
-        attache = new ConnectedAgentAttache(null, 1L, "kvm-attache", null, false);
 
-        hostDao = Mockito.mock(HostDao.class);
-        storagePoolMonitor = Mockito.mock(Listener.class);
-
-        mgr._hostDao = hostDao;
-        mgr._hostMonitors = new ArrayList<>();
-        mgr._hostMonitors.add(new Pair<>(0, storagePoolMonitor));
+        agentManagerImplSpy._hostMonitors = List.of(new Pair<>(0, storagePoolMonitor));
     }
 
     @Test
     public void testNotifyMonitorsOfConnectionNormal() throws ConnectionException {
-        Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(host);
+        Mockito.when(hostDaoMock.findById(Mockito.anyLong())).thenReturn(host);
         Mockito.doNothing().when(storagePoolMonitor).processConnect(Mockito.eq(host), Mockito.eq(cmds[0]), Mockito.eq(false));
-        Mockito.doReturn(true).when(mgr).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean());
-        Mockito.doReturn(Mockito.mock(Answer.class)).when(mgr).easySend(Mockito.anyLong(), Mockito.any(ReadyCommand.class));
-        Mockito.doReturn(true).when(mgr).agentStatusTransitTo(Mockito.eq(host), Mockito.eq(Status.Event.Ready), Mockito.anyLong());
+        Mockito.doReturn(true).when(agentManagerImplSpy).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean());
+        Mockito.doReturn(Mockito.mock(Answer.class)).when(agentManagerImplSpy).easySend(Mockito.anyLong(), Mockito.any(ReadyCommand.class));
+        Mockito.doReturn(true).when(agentManagerImplSpy).agentStatusTransitTo(Mockito.eq(host), Mockito.eq(Status.Event.Ready), Mockito.anyLong());
 
-        final AgentAttache agentAttache = mgr.notifyMonitorsOfConnection(attache, cmds, false);
+        final AgentAttache agentAttache = agentManagerImplSpy.notifyMonitorsOfConnection(attache, cmds, false);
         Assert.assertTrue(agentAttache.isReady()); // Agent is in UP state
     }
 
     @Test
     public void testNotifyMonitorsOfConnectionWhenStoragePoolConnectionHostFailure() throws ConnectionException {
         ConnectionException connectionException = new ConnectionException(true, "storage pool could not be connected on host");
-        Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(host);
+        Mockito.when(hostDaoMock.findById(Mockito.anyLong())).thenReturn(host);
         Mockito.doThrow(connectionException).when(storagePoolMonitor).processConnect(Mockito.eq(host), Mockito.eq(cmds[0]), Mockito.eq(false));
-        Mockito.doReturn(true).when(mgr).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean());
+        Mockito.doReturn(true).when(agentManagerImplSpy).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean());
         try {
-            mgr.notifyMonitorsOfConnection(attache, cmds, false);
+            agentManagerImplSpy.notifyMonitorsOfConnection(attache, cmds, false);
             Assert.fail("Connection Exception was expected");
         } catch (ConnectionException e) {
             Assert.assertEquals(e.getMessage(), connectionException.getMessage());
         }
-        Mockito.verify(mgr, Mockito.times(1)).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.eq(Status.Event.AgentDisconnected), Mockito.eq(true), Mockito.eq(true));
+        Mockito.verify(agentManagerImplSpy, Mockito.times(1)).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.eq(Status.Event.AgentDisconnected), Mockito.eq(true), Mockito.eq(true));
+    }
+
+    @Test
+    public void getTimeoutForCommandsTestReturnPassedTimeout() {
+        int expected = 42;
+        int result = agentManagerImplSpy.getTimeoutForCommands(commandsMock, expected);
+
+        Assert.assertEquals(expected, result);
+    }
+
+    @Test
+    public void getTimeoutForCommandsTestReturnTimeoutFromTable() {
+        int expected = 42;
+
+        Mockito.doReturn(expected).when(commandTimeoutDaoMock).findMaxTimeoutBetweenCommands(Mockito.any());
+        int result = agentManagerImplSpy.getTimeoutForCommands(commandsMock, 0);
+
+        Assert.assertEquals(expected, result);
+    }
+
+    @Test
+    public void getTimeoutForCommandsTestFallbackToConfiguration() {
+        int expected = 42;
+
+        Mockito.doReturn(0).when(commandTimeoutDaoMock).findMaxTimeoutBetweenCommands(Mockito.any());
+        Mockito.doReturn(expected).when(agentManagerImplSpy).getWaitValue();
+        int result = agentManagerImplSpy.getTimeoutForCommands(commandsMock, 0);
+
+        Assert.assertEquals(expected, result);
     }
 }

engine/schema/src/main/resources/META-INF/db/schema-41910to42000.sql

@@ -82,3 +82,25 @@ CREATE TABLE IF NOT EXISTS `cloud_usage`.`quota_email_configuration`(
 
 -- Add `is_implicit` column to `host_tags` table
 CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.host_tags', 'is_implicit', 'int(1) UNSIGNED NOT NULL DEFAULT 0 COMMENT "If host tag is implicit or explicit" ');
+
+-- Create command_timeout table and populate it
+CREATE TABLE IF NOT EXISTS `cloud`.`command_timeout` (
+     id bigint(20) unsigned not null auto_increment primary key,
+     command_classpath text not null,
+     timeout int not null,
+     created datetime not null,
+     updated datetime not null,
+     unique key (command_classpath(50))
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+
+INSERT INTO `cloud`.`command_timeout` (command_classpath, timeout, created, updated)
+VALUES
+    ('org.apache.cloudstack.ca.SetupCertificateCommand', 60, now(), now()),
+    ('com.cloud.agent.api.CheckS2SVpnConnectionsCommand', 30, now(), now()),
+    ('com.cloud.agent.api.CheckOnHostCommand', 20, now(), now()),
+    ('com.cloud.agent.api.CheckVirtualMachineCommand', 20, now(), now()),
+    ('com.cloud.agent.api.CheckRouterCommand', 30, now(), now()),
+    ('com.cloud.agent.api.CheckHealthCommand', 50, now(), now()),
+    ('com.cloud.agent.api.routing.GetAutoScaleMetricsCommand', 30, now(), now()),
+    ('org.apache.cloudstack.ca.SetupKeyStoreCommand', 30, now(), now()),
+    ('org.apache.cloudstack.storage.command.browser.ListDataStoreObjectsCommand', 15, now(), now());

framework/config/src/main/java/org/apache/cloudstack/framework/config/dao/CommandTimeoutDao.java

@@ -0,0 +1,28 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.framework.config.dao;
+
+import com.cloud.utils.db.GenericDao;
+import org.apache.cloudstack.framework.config.impl.CommandTimeoutVO;
+
+import java.util.Set;
+
+public interface CommandTimeoutDao extends GenericDao<CommandTimeoutVO, String> {
+
+    int findMaxTimeoutBetweenCommands(Set<String> commandsClassPath);
+
+}

framework/config/src/main/java/org/apache/cloudstack/framework/config/dao/CommandTimeoutDaoImpl.java

@@ -0,0 +1,47 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.framework.config.dao;
+
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.GenericSearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+import org.apache.cloudstack.framework.config.impl.CommandTimeoutVO;
+import org.apache.commons.lang3.ObjectUtils;
+
+import java.util.Set;
+
+public class CommandTimeoutDaoImpl extends GenericDaoBase<CommandTimeoutVO, String> implements CommandTimeoutDao {
+
+    private GenericSearchBuilder<CommandTimeoutVO, Integer> maxCommandTimeoutSearchBuilder;
+
+    public CommandTimeoutDaoImpl() {
+        super();
+
+        maxCommandTimeoutSearchBuilder = createSearchBuilder(Integer.class);
+        maxCommandTimeoutSearchBuilder.select(null, SearchCriteria.Func.MAX, maxCommandTimeoutSearchBuilder.entity().getTimeout());
+        maxCommandTimeoutSearchBuilder.and("command_classpath", maxCommandTimeoutSearchBuilder.entity().getCommandClasspath(), SearchCriteria.Op.IN);
+        maxCommandTimeoutSearchBuilder.done();
+    }
+
+    @Override
+    public int findMaxTimeoutBetweenCommands(Set<String> commandsClassPath) {
+        SearchCriteria<Integer> searchCriteria = maxCommandTimeoutSearchBuilder.create();
+        searchCriteria.setParameters("command_classpath", commandsClassPath.toArray());
+        Integer max = customSearch(searchCriteria, null).get(0);
+        return ObjectUtils.defaultIfNull(max, 0);
+    }
+}