From bd5d8f9892f55d470a8baa1f54aefc259f30072e Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Mon, 26 Jan 2026 08:37:36 -0300 Subject: [PATCH 1/2] Fix calculation of the next time that Usage will execute in `removeRawUsageRecords` --- .../com/cloud/usage/UsageServiceImpl.java | 60 +++++----- .../com/cloud/usage/UsageManagerImpl.java | 35 ++---- .../cloudstack/utils/usage/UsageUtils.java | 44 ++++++++ .../utils/usage/UsageUtilsTest.java | 105 ++++++++++++++++++ 4 files changed, 193 insertions(+), 51 deletions(-) create mode 100644 utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java diff --git a/server/src/main/java/com/cloud/usage/UsageServiceImpl.java b/server/src/main/java/com/cloud/usage/UsageServiceImpl.java index c46f1f43fd07..28fd068dfef3 100644 --- a/server/src/main/java/com/cloud/usage/UsageServiceImpl.java +++ b/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; @@ -34,6 +33,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; @@ -126,14 +126,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 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; } @@ -464,35 +475,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.removeOldUsageRecords(interval); - } 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.removeOldUsageRecords(interval); return true; } } diff --git a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java b/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java index 99de98f56e4a..3b3f6e3d3437 100644 --- a/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java +++ b/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 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 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) { diff --git a/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java b/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java index a97aed15d360..e47906868113 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java @@ -19,6 +19,50 @@ 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 = Integer.parseInt(execTimeSegments[0]); + int minutes = Integer.parseInt(execTimeSegments[1]); + + 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.roll(Calendar.DAY_OF_YEAR, true); + } else if (!next && jobExecTime.getTime().after(currentDate)) { + jobExecTime.roll(Calendar.DAY_OF_YEAR, false); + } + + return jobExecTime.getTime(); + } } diff --git a/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java b/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java new file mode 100644 index 000000000000..5ed1e8680fc6 --- /dev/null +++ b/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java @@ -0,0 +1,105 @@ +// +// 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 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 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 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 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()); + } + } + +} From e717ae46332bea282a36daace7c50f7331954ef4 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Tue, 27 Jan 2026 09:19:46 -0300 Subject: [PATCH 2/2] Address copilot reviews --- .../cloudstack/utils/usage/UsageUtils.java | 15 +++++++--- .../utils/usage/UsageUtilsTest.java | 30 +++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java b/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java index e47906868113..861788d1918b 100644 --- a/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java +++ b/utils/src/main/java/org/apache/cloudstack/utils/usage/UsageUtils.java @@ -46,8 +46,15 @@ protected static Date getJobExecutionTime(TimeZone usageTimeZone, String jobExec logger.warn("Unable to parse configuration 'usage.stats.job.exec.time'."); return null; } - int hourOfDay = Integer.parseInt(execTimeSegments[0]); - int minutes = Integer.parseInt(execTimeSegments[1]); + 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); @@ -58,9 +65,9 @@ protected static Date getJobExecutionTime(TimeZone usageTimeZone, String jobExec jobExecTime.set(Calendar.MILLISECOND, 0); if (next && jobExecTime.getTime().before(currentDate)) { - jobExecTime.roll(Calendar.DAY_OF_YEAR, true); + jobExecTime.add(Calendar.DAY_OF_YEAR, 1); } else if (!next && jobExecTime.getTime().after(currentDate)) { - jobExecTime.roll(Calendar.DAY_OF_YEAR, false); + jobExecTime.add(Calendar.DAY_OF_YEAR, -1); } return jobExecTime.getTime(); diff --git a/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java b/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java index 5ed1e8680fc6..8b9b4910e39d 100644 --- a/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java +++ b/utils/src/test/java/org/apache/cloudstack/utils/usage/UsageUtilsTest.java @@ -102,4 +102,34 @@ public void getJobExecutionTimeTestReturnsExpectedDateWhenNextIsFalseAndExecutio } } + @Test + public void getJobExecutionTimeTestReturnsExpectedDateWhenNextExecutionIsOnNextYear() { + Date currentDate = new Date(); + currentDate.setTime(1767236340000L); + + try (MockedStatic 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 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()); + } + } + }