Remove redundant Exceptions from logs for vm schedules

[vishesh92]

Description

This PR fixes #11564

Initially the scheduler relied on the database constraints to ensure no duplicates and catch the exception and just continue. But the way persist method is implemented, it will always logs the error for SQLIntegrityConstraintViolationException. In this PR, we add a check before creating a new scheduled job.

Generated Summary

This pull request adds a safeguard to prevent duplicate VM scheduled jobs by checking for existing jobs before creating new ones. It introduces a new DAO method to efficiently find jobs by schedule and timestamp, and updates the scheduler logic to use this method. Additionally, it introduces some minor import changes.

Database access improvements:

• Added a new method findByScheduleAndTimestamp(long scheduleId, Date scheduledTimestamp) to the VMScheduledJobDao interface and its implementation, along with the necessary SearchBuilder in VMScheduledJobDaoImpl, to efficiently look up scheduled jobs by schedule ID and timestamp. [1] [2] [3] [4]

Scheduler logic enhancements:

• Updated VMSchedulerImpl.scheduleNextJob to use the new DAO method to check for an existing scheduled job before creating a new one, preventing duplicate scheduling for the same VM and time.

Code maintenance:

• Added missing imports for SQLException and SQLIntegrityConstraintViolationException in VMSchedulerImpl.java.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

[Copilot]

Pull request overview

This pull request improves the VM scheduler to prevent duplicate job scheduling and reduce redundant exception logging. The changes introduce a proactive check for existing scheduled jobs before attempting to create new ones, addressing issue #11564.

Changes:

• Added a new DAO method findByScheduleAndTimestamp to check for existing scheduled jobs
• Updated scheduler logic to query for existing jobs before attempting to insert, reducing exception occurrences
• Removed the unused exception variable from the catch clause to clean up the code

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
VMScheduledJobDao.java	Added interface method for finding jobs by schedule ID and timestamp
VMScheduledJobDaoImpl.java	Implemented the new query method using SearchBuilder pattern
VMSchedulerImpl.java	Added pre-insert check for duplicate jobs and removed exception variable name

Comments suppressed due to low confidence (3)

server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java:51

• These imports for SQLException and SQLIntegrityConstraintViolationException are not used anywhere in the code. They should be removed to keep the imports clean.

import java.time.ZonedDateTime;
import java.util.ArrayList;

server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java:175

• There is a potential race condition between the check for an existing job at line 167 and the persist operation at line 175. If two threads call scheduleNextJob concurrently with the same schedule and timestamp, both could pass the null check and attempt to insert, causing one to throw EntityExistsException. While the exception is caught, this race condition could still result in unnecessary database operations and exception handling. Consider using a database-level check or a synchronized block around the check-and-insert operation.

            logger.trace("Job is already scheduled for schedule {} at {}", vmSchedule, scheduledDateTime);
            return scheduledDateTime;
        }

        scheduledJob = new VMScheduledJobVO(vmSchedule.getVmId(), vmSchedule.getId(), vmSchedule.getAction(), scheduledDateTime);
        try {
            vmScheduledJobDao.persist(scheduledJob);
            ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, vm.getAccountId(), actionEventMap.get(vmSchedule.getAction()),
                    String.format("Scheduled action (%s) [vm: %s, schedule: %s] at %s", vmSchedule.getAction(), vm, vmSchedule, scheduledDateTime),

server/src/main/java/org/apache/cloudstack/vm/schedule/VMSchedulerImpl.java:181

• The existing test testScheduleNextJobScheduleScheduleExists does not cover the new code path introduced by findByScheduleAndTimestamp. The test should be updated to mock vmScheduledJobDao.findByScheduleAndTimestamp to return null first (for the new check) and then have persist throw EntityExistsException. Additionally, a new test should be added to verify the scenario where findByScheduleAndTimestamp returns an existing job, ensuring the method returns early without attempting to persist.

            logger.trace("Job is already scheduled for schedule {} at {}", vmSchedule, scheduledDateTime);
            return scheduledDateTime;
        }

        scheduledJob = new VMScheduledJobVO(vmSchedule.getVmId(), vmSchedule.getId(), vmSchedule.getAction(), scheduledDateTime);
        try {
            vmScheduledJobDao.persist(scheduledJob);
            ActionEventUtils.onScheduledActionEvent(User.UID_SYSTEM, vm.getAccountId(), actionEventMap.get(vmSchedule.getAction()),
                    String.format("Scheduled action (%s) [vm: %s, schedule: %s] at %s", vmSchedule.getAction(), vm, vmSchedule, scheduledDateTime),
                    vm.getId(), ApiCommandResourceType.VirtualMachine.toString(), true, 0);
        } catch (EntityExistsException exception) {
            logger.debug("Job is already scheduled.");
        }
        return scheduledDateTime;
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (5c1f931) to head (f4f07f6).
⚠️ Report is 1 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...udstack/vm/schedule/dao/VMScheduledJobDaoImpl.java	0.00%	10 Missing ⚠️
...apache/cloudstack/vm/schedule/VMSchedulerImpl.java	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12428   +/-   ##
=========================================
  Coverage     16.23%   16.23%           
  Complexity    13380    13380           
=========================================
  Files          5657     5657           
  Lines        498996   499010   +14     
  Branches      60566    60567    +1     
=========================================
+ Hits          81011    81030   +19     
+ Misses       408951   408943    -8     
- Partials       9034     9037    +3     

Flag	Coverage Δ
uitests	4.03% <ø> (ø)
unittests	17.09% <13.33%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DaanHoogland]

clgtm