Copilot commented on code in PR #14270:
URL: https://github.com/apache/cloudstack/pull/14270#discussion_r4142461918
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/BaseDeployVMCmd.java:
##########
@@ -875,4 +875,8 @@ public String getEventDescription() {
public ApiCommandResourceType getApiResourceType() {
return ApiCommandResourceType.VirtualMachine;
}
+
+ public boolean isCancellable() {
+ return true;
+ }
Review Comment:
This method is never consulted by cancellation: `AsyncJobManagerImpl` reads
`APICommand.cancellable()` from the concrete command annotation. Both
`DeployVMCmd` and `CreateVMFromBackupCmd` retain the annotation default
`false`, so active deploy jobs are always rejected despite this change claiming
they are cancellable. Set `cancellable = true` on the applicable concrete
`@APICommand` annotations (or change the cancellation contract consistently).
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentAttache.java:
##########
@@ -231,6 +234,24 @@ protected synchronized void cancel(final long seq) {
}
}
+ /** Default false: an attache that cannot ask its resource must not claim
it. */
+ protected boolean isExecutionCancellable(final long seq) {
+ return false;
+ }
+
+ /** Returns true only when the resource's work was stopped (or nothing was
left to stop). */
+ protected boolean cancelRunning(final long seq) {
+ return false;
+ }
+
+ /** 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);
+ cancel(seq);
+ return stopped;
+ }
Review Comment:
If the resource refuses cancellation after the earlier probe (for example,
the task finishes or becomes non-cancellable in between), this still marks the
sequence cancelled and disconnects its listener.
`AgentManagerImpl.cancelJobExecution` then reports refusal, but the waiting
worker receives `OperationCancelledException` instead of being allowed to
finish, so the supposedly refused cancellation can fail the job. Only
mark/cancel the attachment after `cancelRunning` succeeds.
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java:
##########
@@ -2064,6 +2109,99 @@ protected List<Long> findAgentsBehindOnPing() {
}
}
+ /** Stops in-flight work of jobs cancelled on this management server and
acknowledges them. */
+ protected class CancelledJobsCheckTask extends ManagedContextRunnable {
+ @Override
+ protected void runInContext() {
+ try {
+ final long now = System.currentTimeMillis();
+ _cancelledJobs.values().removeIf(firstSeen -> now - firstSeen
> CANCELLED_JOB_MEMORY_MS);
+
+ for (final AsyncJobVO job :
asyncJobManager.listCancelledJobsExecutingOn(_nodeId)) {
+ _cancelledJobs.putIfAbsent(job.getId(), now);
+ if
(_jobToHostIdAndReqSequenceMap.containsKey(job.getId())) {
+ 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());
+ }
+ }
+ asyncJobManager.finalizeCancelledJob(job.getId());
Review Comment:
When cancellation reaches the management server executing a remote job and
an in-flight command cannot be stopped, this branch logs that the command will
continue but immediately finalizes/removes the job as CANCELLED. The backend
can therefore keep mutating resources after the job is terminal, and the row
will no longer be polled for another stop attempt. Keep the job assigned and
retry until its in-flight work is stopped or naturally exits; only then
finalize it.
##########
server/src/main/java/com/cloud/usage/UsageServiceImpl.java:
##########
@@ -500,4 +504,71 @@ public boolean
removeRawUsageRecords(RemoveRawUsageRecordsCmd cmd) throws Invali
_usageDao.expungeAllOlderThan(interval,
ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
return true;
}
+
+ @Override
+ public ListResponse<UsageJobResponse> getUsageJobs(ListUsageJobsCmd cmd) {
+ Filter usageJobFilter = new Filter(UsageJobVO.class, "id", true,
cmd.getStartIndex(), cmd.getPageSizeVal());
+ SearchCriteria<UsageJobVO> sc = _usageJobDao.createSearchCriteria();
+
+ if (StringUtils.isNotBlank(cmd.getUsageServer())) {
+ sc.addAnd("host", SearchCriteria.Op.EQ,
cmd.getUsageServer().trim());
+ }
+
+ Object startDate = cmd.getStartDate();
+ if (startDate != null) {
+ sc.addAnd("startDate", SearchCriteria.Op.GTEQ, startDate);
+ }
+
+ Object endDate = cmd.getEndDate();
+ if (endDate != null) {
+ sc.addAnd("startDate", SearchCriteria.Op.LTEQ, endDate);
+ }
+
+ if (cmd.getDuration() != null) {
+ // usage job timestamps are GMT
+ Date lastDate = new Date(DateUtil.currentGMTTime().getTime() -
TimeUnit.HOURS.toMillis(cmd.getDuration()));
+
+ SearchCriteria<UsageJobVO> scc =
_usageJobDao.createSearchCriteria();
+ scc.addOr("startDate", SearchCriteria.Op.GTEQ, lastDate);
+ scc.addOr("endDate", SearchCriteria.Op.GTEQ, lastDate);
+ sc.addAnd("startDate", SearchCriteria.Op.SC, scc);
+ }
+
+ Pair<List<UsageJobVO>, Integer> usageJobs = null;
+ TransactionLegacy txn =
TransactionLegacy.open(TransactionLegacy.USAGE_DB);
+ try {
+ usageJobs = _usageJobDao.searchAndCount(sc, usageJobFilter);
+ } finally {
+ txn.close();
+
+ // switch back to VMOPS_DB
+ TransactionLegacy swap =
TransactionLegacy.open(TransactionLegacy.CLOUD_DB);
+ swap.close();
+ }
+
+ ListResponse<UsageJobResponse> response = new ListResponse<>();
+ if (usageJobs != null) {
+ List<UsageJobResponse> responses = new ArrayList<>();
+ for (UsageJobVO job : usageJobs.first()) {
+ UsageJobResponse jobResponse = createUsageJobResponse(job);
+ responses.add(jobResponse);
+ }
+ response.setResponses(responses, usageJobs.second());
+ }
+
+ return response;
+ }
+
+ private UsageJobResponse createUsageJobResponse(UsageJobVO job) {
+ UsageJobResponse jobResponse = new UsageJobResponse();
+ jobResponse.setUsageServer(job.getHost());
+ jobResponse.setJobType(job.getJobType());
+ jobResponse.setScheduled(job.getScheduled());
+ jobResponse.setStartDate(job.getStartDate());
+ jobResponse.setEndDate(job.getEndDate());
+ jobResponse.setSuccess(job.getSuccess());
+ jobResponse.setHeartbeat(job.getHeartbeat());
Review Comment:
The new response declares `executiontime` and the UI displays it, but this
mapper never copies `UsageJobVO.getExecTime()`. Consequently every
`listUsageJobs` response omits the execution time and the new table column
remains empty.
##########
engine/orchestration/src/main/java/com/cloud/agent/manager/DirectAgentAttache.java:
##########
@@ -139,14 +237,17 @@ protected void finalize() throws Throwable {
private synchronized void queueTask(Task task) {
tasks.add(task);
+ _taskRequests.put(task._req.getSequence(), task._req);
}
private synchronized void scheduleFromQueue() {
logger.trace("Agent attache [id: {}, uuid: {}, name: {}], task queue
size={}, outstanding tasks={}",
_id, _uuid, _name, tasks.size(), _outstandingTaskCount.get());
while (!tasks.isEmpty() && _outstandingTaskCount.get() <
_agentMgr.getDirectAgentThreadCap()) {
_outstandingTaskCount.incrementAndGet();
- _agentMgr.getDirectAgentPool().execute(tasks.remove());
+ Task task = tasks.remove();
+ Future<?> future = _agentMgr.getDirectAgentPool().submit(task);
+ _taskFutures.put(task._req.getSequence(), future);
Review Comment:
`submit` may execute and finish `task` before the returned future is
inserted. In that ordering, the task's `finally` removes a missing entry and
this subsequent `put` permanently retains a completed future, leaking one map
entry per sufficiently fast request and making the sequence appear to have a
running task. Remove a future that is already done after insertion; if it
finishes later, the existing `finally` handles it.
##########
ui/src/config/section/activity.js:
##########
@@ -0,0 +1,145 @@
+// 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.
+
+import store from '@/store'
+
+export default {
+ name: 'activity',
+ title: 'label.activity',
+ icon: 'AuditOutlined',
+ permission: ['listEvents'],
+ children: [
Review Comment:
This parent permission hides the entire Activity section unless `listEvents`
is granted, even when a role has `listAsyncJobs` or `listAlerts`. Route
permissions are enforced before child filtering
(`ui/src/store/modules/permission.js:44-55`), so custom roles with only Jobs or
Alerts access cannot reach their permitted child. Leave the heterogeneous
parent unrestricted and let each child's permission control visibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
