review comments

Author: sureshanaparti

api/src/main/java/org/apache/cloudstack/api/command/user/job/ListAsyncJobsCmd.java

@@ -46,7 +46,7 @@ public class ListAsyncJobsCmd extends BaseListAccountResourcesCmd {
     @Parameter(name = ApiConstants.START_DATE, type = CommandType.DATE, description = "The start date from which the async jobs should be listed. Only jobs created on or after this date will be included. (use format \"yyyy-MM-dd'T'HH:mm:ss'+'SSSS\")")
     private Date startDate;
 
-    @Parameter(name = ApiConstants.END_DATE, type = CommandType.DATE, description = "The end date up to which the async jobs should be listed. Only jobs created on or before this date will be included. (use format \"yyyy-MM-dd'T'HH:mm:ss'+'SSSS\")")
+    @Parameter(name = ApiConstants.END_DATE, type = CommandType.DATE, description = "The end date up to which the async jobs should be listed. Only jobs created on or before this date will be included. (use format \"yyyy-MM-dd'T'HH:mm:ss'+'SSSS\")", since = "24.0")
     private Date endDate;
 
     @Parameter(name = ApiConstants.MANAGEMENT_SERVER_ID, type = CommandType.UUID, entityType = ManagementServerResponse.class, description = "The id of the management server", since="4.19")

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

@@ -247,9 +247,13 @@ protected boolean cancelRunning(final long seq) {
     /** Job-cancel path; distinct from cancel(seq), which is also the timeout path and never reaches the hypervisor. */
     public boolean cancelExecution(final long seq) {
         _cancelledSequences.add(seq);
-        final boolean stopped = cancelRunning(seq);
+        if (!cancelRunning(seq)) {
+            // refused: the command runs on and its sender must see the real answer
+            _cancelledSequences.remove(seq);
+            return false;
+        }
         cancel(seq);
-        return stopped;
+        return true;
     }
 
     protected synchronized int findRequest(final Request req) {

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

@@ -2123,7 +2123,8 @@ protected void runInContext() {
                         logger.info("Job-{} on {} {} was cancelled, stopping its in-flight commands",
                                 job.getId(), job.getInstanceType(), job.getInstanceId());
                         if (!cancelJobExecution(job.getId(), "Job was cancelled")) {
-                            logger.warn("Not every in-flight command of cancelled job-{} could be stopped; the rest will run to completion", job.getId());
+                            logger.warn("Not every in-flight command of cancelled job-{} could be stopped; retrying on the next check", job.getId());
+                            continue;
                         }
                     }
                     asyncJobManager.finalizeCancelledJob(job.getId());

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

@@ -248,6 +248,10 @@ private synchronized void scheduleFromQueue() {
             Task task = tasks.remove();
             Future<?> future = _agentMgr.getDirectAgentPool().submit(task);
             _taskFutures.put(task._req.getSequence(), future);
+            if (future.isDone()) {
+                // the task may have finished (and cleaned up) before the future was recorded
+                _taskFutures.remove(task._req.getSequence());
+            }
         }
     }
 

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

@@ -31,13 +31,17 @@
 import com.cloud.host.dao.HostDetailsDao;
 import com.cloud.hypervisor.Hypervisor;
 import com.cloud.utils.Pair;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mockito;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
 
 public class AgentManagerImplTest {
@@ -172,4 +176,41 @@ public void testGetHostSshPortWithKVMHostCustomPort() {
         int hostSshPort = mgr.getHostSshPort(host);
         Assert.assertEquals(3922, hostSshPort);
     }
+
+    private AsyncJobManager cancelledJobOnThisServer(final long jobId) {
+        final AsyncJobManager asyncJobManager = Mockito.mock(AsyncJobManager.class);
+        mgr.asyncJobManager = asyncJobManager;
+        final AsyncJobVO job = new AsyncJobVO();
+        job.setId(jobId);
+        Mockito.when(asyncJobManager.listCancelledJobsExecutingOn(mgr._nodeId)).thenReturn(Collections.singletonList(job));
+        return asyncJobManager;
+    }
+
+    @Test
+    public void testCancelledJobWithoutInFlightCommandsIsFinalisedAtOnce() {
+        final AsyncJobManager asyncJobManager = cancelledJobOnThisServer(42L);
+
+        mgr.new CancelledJobsCheckTask().runInContext();
+
+        Mockito.verify(asyncJobManager).finalizeCancelledJob(42L);
+        Assert.assertTrue(mgr.isJobCancelled(42L));
+    }
+
+    @Test
+    public void testCancelledJobStaysAssignedUntilItsCommandCanBeStopped() throws Exception {
+        final AsyncJobManager asyncJobManager = cancelledJobOnThisServer(42L);
+        final AgentAttache attache = Mockito.mock(AgentAttache.class);
+        Mockito.doReturn(attache).when(mgr).getAttache(1L);
+        mgr._jobToHostIdAndReqSequenceMap.put(42L, new HashSet<>(Collections.singleton(new Pair<>(1L, 11L))));
+
+        Mockito.when(attache.isExecutionCancellable(11L)).thenReturn(false);
+        mgr.new CancelledJobsCheckTask().runInContext();
+        Mockito.verify(asyncJobManager, Mockito.never()).finalizeCancelledJob(Mockito.anyLong());
+        Mockito.verify(attache, Mockito.never()).cancelExecution(Mockito.anyLong());
+
+        Mockito.when(attache.isExecutionCancellable(11L)).thenReturn(true);
+        Mockito.when(attache.cancelExecution(11L)).thenReturn(true);
+        mgr.new CancelledJobsCheckTask().runInContext();
+        Mockito.verify(asyncJobManager).finalizeCancelledJob(42L);
+    }
 }

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

@@ -28,6 +28,7 @@
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import com.cloud.agent.Listener;
 import com.cloud.agent.api.CheckHealthCommand;
 import com.cloud.agent.transport.Request;
 import com.cloud.hypervisor.Hypervisor;
@@ -86,28 +87,32 @@ public void testCancelQueuedTaskDropsItBeforeItReachesTheResource() throws Excep
     @Test
     public void testCancelRunningTaskStopsTheResourceThenTheThread() throws Exception {
         final Request request = submitRunning(101L);
+        directAgentAttache.registerListener(101L, waiter());
         Mockito.doReturn(true).when(_resource).isRequestSequenceCancellable(101L);
         Mockito.doReturn(true).when(_resource).cancelRequestSequence(101L);
 
         Assert.assertTrue(directAgentAttache.isExecutionCancellable(101L));
-        directAgentAttache.cancelExecution(101L);
+        Assert.assertTrue(directAgentAttache.cancelExecution(101L));
 
         Mockito.verify(_resource).cancelRequestSequence(101L);
         Mockito.verify(runningTask).cancel(true);
         Assert.assertTrue(request.isCancelled());
+        Assert.assertFalse(directAgentAttache._waitForList.containsKey(101L));
     }
 
     @Test
     public void testCancelLeavesNonCancellableRunningTaskAlone() throws Exception {
         final Request request = submitRunning(303L);
+        directAgentAttache.registerListener(303L, waiter());
         Mockito.doReturn(false).when(_resource).isRequestSequenceCancellable(303L);
 
         Assert.assertFalse(directAgentAttache.isExecutionCancellable(303L));
-        directAgentAttache.cancelExecution(303L);
+        Assert.assertFalse(directAgentAttache.cancelExecution(303L));
 
         Mockito.verify(_resource, Mockito.never()).cancelRequestSequence(Mockito.anyLong());
         Mockito.verify(runningTask, Mockito.never()).cancel(Mockito.anyBoolean());
         Assert.assertFalse(request.isCancelled());
+        Assert.assertTrue(directAgentAttache._waitForList.containsKey(303L));
     }
 
     @Test
@@ -121,6 +126,12 @@ private Request newRequest(final long seq) {
         return request;
     }
 
+    private Listener waiter() {
+        final Listener listener = Mockito.mock(Listener.class);
+        Mockito.when(listener.getTimeout()).thenReturn(-1);
+        return listener;
+    }
+
     private Request submitRunning(final long seq) throws Exception {
         Mockito.doReturn(2).when(_agentMgr).getDirectAgentThreadCap();
         Mockito.doReturn(directAgentPool).when(_agentMgr).getDirectAgentPool();

server/src/main/java/com/cloud/usage/UsageServiceImpl.java

@@ -566,6 +566,7 @@ private UsageJobResponse createUsageJobResponse(UsageJobVO job) {
         jobResponse.setScheduled(job.getScheduled());
         jobResponse.setStartDate(job.getStartDate());
         jobResponse.setEndDate(job.getEndDate());
+        jobResponse.setExecutionTime(job.getExecTime());
         jobResponse.setSuccess(job.getSuccess());
         jobResponse.setHeartbeat(job.getHeartbeat());
         jobResponse.setObjectName("usagejobs");

ui/src/config/section/activity.js

@@ -21,7 +21,6 @@ export default {
   name: 'activity',
   title: 'label.activity',
   icon: 'AuditOutlined',
-  permission: ['listEvents'],
   children: [
     {
       name: 'jobs',