Fix calculation of the next time that Usage will execute in removeRawUsageRecords (#12518)

* Fix calculation of the next time that Usage will execute in `removeRawUsageRecords`

* Address copilot reviews

Author: winterhazel

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

@@ -17,7 +17,6 @@
 package com.cloud.usage;
 
 import java.util.ArrayList;
-import java.util.Calendar;
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
@@ -35,6 +34,7 @@
 import org.apache.cloudstack.usage.Usage;
 import org.apache.cloudstack.usage.UsageService;
 import org.apache.cloudstack.usage.UsageTypes;
+import org.apache.cloudstack.utils.usage.UsageUtils;
 import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.jetbrains.annotations.NotNull;
@@ -127,14 +127,25 @@ public class UsageServiceImpl extends ManagerBase implements UsageService, Manag
     @Inject
     private NetworkOfferingDao _networkOfferingDao;
 
+    private TimeZone usageExecutionTimeZone = TimeZone.getTimeZone("GMT");
+
+    private static final long REMOVE_RAW_USAGE_RECORDS_WINDOW_IN_MS = 15 * 60 * 1000;
+
     public UsageServiceImpl() {
     }
 
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
         super.configure(name, params);
+
         String timeZoneStr = ObjectUtils.defaultIfNull(_configDao.getValue(Config.UsageAggregationTimezone.toString()), "GMT");
         _usageTimezone = TimeZone.getTimeZone(timeZoneStr);
+
+        String executionTimeZone = _configDao.getValue(Config.UsageExecutionTimezone.toString());
+        if (executionTimeZone != null) {
+            usageExecutionTimeZone = TimeZone.getTimeZone(executionTimeZone);
+        }
+
         return true;
     }
 
@@ -465,35 +476,28 @@ public TimeZone getUsageTimezone() {
     @Override
     public boolean removeRawUsageRecords(RemoveRawUsageRecordsCmd cmd) throws InvalidParameterValueException {
         Integer interval = cmd.getInterval();
-        if (interval != null && interval > 0 ) {
-            String jobExecTime = _configDao.getValue(Config.UsageStatsJobExecTime.toString());
-            if (jobExecTime != null ) {
-                String[] segments = jobExecTime.split(":");
-                if (segments.length == 2) {
-                    String timeZoneStr = _configDao.getValue(Config.UsageExecutionTimezone.toString());
-                    if (timeZoneStr == null) {
-                        timeZoneStr = "GMT";
-                    }
-                    TimeZone tz = TimeZone.getTimeZone(timeZoneStr);
-                    Calendar cal = Calendar.getInstance(tz);
-                    cal.setTime(new Date());
-                    long curTS = cal.getTimeInMillis();
-                    cal.set(Calendar.HOUR_OF_DAY, Integer.parseInt(segments[0]));
-                    cal.set(Calendar.MINUTE, Integer.parseInt(segments[1]));
-                    cal.set(Calendar.SECOND, 0);
-                    cal.set(Calendar.MILLISECOND, 0);
-                    long execTS = cal.getTimeInMillis();
-                    logger.debug("Trying to remove old raw cloud_usage records older than " + interval + " day(s), current time=" + curTS + " next job execution time=" + execTS);
-                    // Let's avoid cleanup when job runs and around a 15 min interval
-                    if (Math.abs(curTS - execTS) < 15 * 60 * 1000) {
-                        return false;
-                    }
-                }
+        if (interval == null || interval <= 0) {
+            throw new InvalidParameterValueException("Interval should be greater than 0.");
+        }
+
+        String jobExecTime = _configDao.getValue(Config.UsageStatsJobExecTime.toString());
+        Date previousJobExecTime = UsageUtils.getPreviousJobExecutionTime(usageExecutionTimeZone, jobExecTime);
+        Date nextJobExecTime = UsageUtils.getNextJobExecutionTime(usageExecutionTimeZone, jobExecTime);
+        if (ObjectUtils.allNotNull(previousJobExecTime, nextJobExecTime)) {
+            logger.debug("Next Usage job is scheduled to execute at [{}]; previous execution was at [{}].",
+                    DateUtil.displayDateInTimezone(usageExecutionTimeZone, nextJobExecTime), DateUtil.displayDateInTimezone(usageExecutionTimeZone, previousJobExecTime));
+            Date now = new Date();
+            if (nextJobExecTime.getTime() - now.getTime() < REMOVE_RAW_USAGE_RECORDS_WINDOW_IN_MS) {
+                logger.info("Not removing any cloud_usage records because the next Usage job is scheduled to execute in less than {} minute(s).", REMOVE_RAW_USAGE_RECORDS_WINDOW_IN_MS / 60000);
+                return false;
+            } else if (now.getTime() - previousJobExecTime.getTime() < REMOVE_RAW_USAGE_RECORDS_WINDOW_IN_MS) {
+                logger.info("Not removing any cloud_usage records because the last Usage job executed in less than {} minute(s) ago.", REMOVE_RAW_USAGE_RECORDS_WINDOW_IN_MS / 60000);
+                return false;
             }
-            _usageDao.expungeAllOlderThan(interval, ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
-        } else {
-            throw new InvalidParameterValueException("Invalid interval value. Interval to remove cloud_usage records should be greater than 0");
         }
+
+        logger.info("Removing cloud_usage records older than {} day(s).", interval);
+        _usageDao.expungeAllOlderThan(interval, ConfigurationManagerImpl.DELETE_QUERY_BATCH_SIZE.value());
         return true;
     }
 }

usage/src/main/java/com/cloud/usage/UsageManagerImpl.java

@@ -198,7 +198,9 @@ public class UsageManagerImpl extends ManagerBase implements UsageManager, Runna
     private Future _heartbeat = null;
     private Future _sanity = null;
     private boolean  usageSnapshotSelection = false;
+
     private static TimeZone usageAggregationTimeZone = TimeZone.getTimeZone("GMT");
+    private static TimeZone usageExecutionTimeZone = TimeZone.getTimeZone("GMT");
 
     public UsageManagerImpl() {
     }
@@ -253,6 +255,9 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
         if (aggregationTimeZone != null && !aggregationTimeZone.isEmpty()) {
             usageAggregationTimeZone = TimeZone.getTimeZone(aggregationTimeZone);
         }
+        if (execTimeZone != null) {
+            usageExecutionTimeZone = TimeZone.getTimeZone(execTimeZone);
+        }
 
         try {
             if ((execTime == null) || (aggregationRange == null)) {
@@ -261,34 +266,18 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
                 throw new ConfigurationException("Missing configuration values for usage job, usage.stats.job.exec.time = " + execTime +
                         ", usage.stats.job.aggregation.range = " + aggregationRange);
             }
-            String[] execTimeSegments = execTime.split(":");
-            if (execTimeSegments.length != 2) {
-                logger.error("Unable to parse usage.stats.job.exec.time");
-                throw new ConfigurationException("Unable to parse usage.stats.job.exec.time '" + execTime + "'");
-            }
-            int hourOfDay = Integer.parseInt(execTimeSegments[0]);
-            int minutes = Integer.parseInt(execTimeSegments[1]);
-
-            Date currentDate = new Date();
-            _jobExecTime.setTime(currentDate);
-
-            _jobExecTime.set(Calendar.HOUR_OF_DAY, hourOfDay);
-            _jobExecTime.set(Calendar.MINUTE, minutes);
-            _jobExecTime.set(Calendar.SECOND, 0);
-            _jobExecTime.set(Calendar.MILLISECOND, 0);
-
-            TimeZone jobExecTimeZone = execTimeZone != null ? TimeZone.getTimeZone(execTimeZone) : Calendar.getInstance().getTimeZone();
-            _jobExecTime.setTimeZone(jobExecTimeZone);
 
-            // if the hour to execute the job has already passed, roll the day forward to the next day
-            if (_jobExecTime.getTime().before(currentDate)) {
-                _jobExecTime.roll(Calendar.DAY_OF_YEAR, true);
+            Date nextJobExecTime = UsageUtils.getNextJobExecutionTime(usageExecutionTimeZone, execTime);
+            if (nextJobExecTime == null) {
+                throw new ConfigurationException(String.format("Unable to parse configuration 'usage.stats.job.exec.time' value [%s].", execTime));
             }
+            _jobExecTime.setTimeZone(usageExecutionTimeZone);
+            _jobExecTime.setTime(nextJobExecTime);
 
             logger.info("Usage is configured to execute in time zone [{}], at [{}], each [{}] minutes; the current time in that timezone is [{}] and the " +
                             "next job is scheduled to execute at [{}]. During its execution, Usage will aggregate stats according to the time zone [{}] defined in global setting [usage.aggregation.timezone].",
-                    jobExecTimeZone.getID(), execTime, aggregationRange, DateUtil.displayDateInTimezone(jobExecTimeZone, currentDate),
-                    DateUtil.displayDateInTimezone(jobExecTimeZone, _jobExecTime.getTime()), usageAggregationTimeZone.getID());
+                    usageExecutionTimeZone.getID(), execTime, aggregationRange, DateUtil.displayDateInTimezone(usageExecutionTimeZone, new Date()),
+                    DateUtil.displayDateInTimezone(usageExecutionTimeZone, _jobExecTime.getTime()), usageAggregationTimeZone.getID());
 
             _aggregationDuration = Integer.parseInt(aggregationRange);
             if (_aggregationDuration < UsageUtils.USAGE_AGGREGATION_RANGE_MIN) {

utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java

@@ -19,6 +19,57 @@
 
 package org.apache.cloudstack.utils.usage;
 
+import com.cloud.utils.DateUtil;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Calendar;
+import java.util.Date;
+import java.util.TimeZone;
+
 public class UsageUtils {
+    protected static Logger logger = LogManager.getLogger(UsageUtils.class);
+
     public static final int USAGE_AGGREGATION_RANGE_MIN = 1;
+
+    public static Date getNextJobExecutionTime(TimeZone usageTimeZone, String jobExecTimeConfig) {
+        return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, true);
+    }
+
+    public static Date getPreviousJobExecutionTime(TimeZone usageTimeZone, String jobExecTimeConfig) {
+        return getJobExecutionTime(usageTimeZone, jobExecTimeConfig, false);
+    }
+
+    protected static Date getJobExecutionTime(TimeZone usageTimeZone, String jobExecTimeConfig, boolean next) {
+        String[] execTimeSegments = jobExecTimeConfig.split(":");
+        if (execTimeSegments.length != 2) {
+            logger.warn("Unable to parse configuration 'usage.stats.job.exec.time'.");
+            return null;
+        }
+        int hourOfDay;
+        int minutes;
+        try {
+            hourOfDay = Integer.parseInt(execTimeSegments[0]);
+            minutes = Integer.parseInt(execTimeSegments[1]);
+        } catch (NumberFormatException e) {
+            logger.warn("Unable to parse configuration 'usage.stats.job.exec.time' due to non-numeric values in [{}].", jobExecTimeConfig, e);
+            return null;
+        }
+
+        Date currentDate = DateUtil.currentGMTTime();
+        Calendar jobExecTime = Calendar.getInstance(usageTimeZone);
+        jobExecTime.setTime(currentDate);
+        jobExecTime.set(Calendar.HOUR_OF_DAY, hourOfDay);
+        jobExecTime.set(Calendar.MINUTE, minutes);
+        jobExecTime.set(Calendar.SECOND, 0);
+        jobExecTime.set(Calendar.MILLISECOND, 0);
+
+        if (next && jobExecTime.getTime().before(currentDate)) {
+            jobExecTime.add(Calendar.DAY_OF_YEAR, 1);
+        } else if (!next && jobExecTime.getTime().after(currentDate)) {
+            jobExecTime.add(Calendar.DAY_OF_YEAR, -1);
+        }
+
+        return jobExecTime.getTime();
+    }
 }

utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java

@@ -0,0 +1,135 @@
+//
+// 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.utils.usage;
+
+import com.cloud.utils.DateUtil;
+import junit.framework.TestCase;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.util.Date;
+import java.util.TimeZone;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UsageUtilsTest extends TestCase {
+
+    TimeZone usageTimeZone = TimeZone.getTimeZone("GMT-3");
+
+    @Test
+    public void getJobExecutionTimeTestReturnsNullWhenConfigurationValueIsInvalid() {
+        Date result = UsageUtils.getNextJobExecutionTime(usageTimeZone, "test");
+        assertNull(result);
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenNextIsTrueAndExecutionTimeHasNotPassed() {
+        Date currentDate = new Date();
+        currentDate.setTime(1724296800000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "00:30", true);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1724297400000L, result.getTime());
+        }
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenNextIsTrueAndExecutionTimeHasPassed() {
+        Date currentDate = new Date();
+        currentDate.setTime(1724297460000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "00:30", true);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1724383800000L, result.getTime());
+        }
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenNextIsFalseAndExecutionTimeHasNotPassed() {
+        Date currentDate = new Date();
+        currentDate.setTime(1724296800000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "00:30", false);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1724211000000L, result.getTime());
+        }
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenNextIsFalseAndExecutionTimeHasPassed() {
+        Date currentDate = new Date();
+        currentDate.setTime(1724297460000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "00:30", false);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1724297400000L, result.getTime());
+        }
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenNextExecutionIsOnNextYear() {
+        Date currentDate = new Date();
+        currentDate.setTime(1767236340000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "00:00", true);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1767236400000L, result.getTime());
+        }
+    }
+
+    @Test
+    public void getJobExecutionTimeTestReturnsExpectedDateWhenPreviousExecutionWasOnPreviousYear() {
+        Date currentDate = new Date();
+        currentDate.setTime(1767236460000L);
+
+        try (MockedStatic<DateUtil> dateUtilMockedStatic = Mockito.mockStatic(DateUtil.class)) {
+            dateUtilMockedStatic.when(DateUtil::currentGMTTime).thenReturn(currentDate);
+
+            Date result = UsageUtils.getJobExecutionTime(usageTimeZone, "23:59", false);
+
+            Assert.assertNotNull(result);
+            Assert.assertEquals(1767236340000L, result.getTime());
+        }
+    }
+
+}