Copilot commented on code in PR #13659:
URL: https://github.com/apache/cloudstack/pull/13659#discussion_r3625008813
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1286,13 +1286,21 @@ protected Point createInfluxDbPoint(Object
metricsObject) {
*/
class VmStatsCleaner extends ManagedContextRunnable{
protected void runInContext() {
- cleanUpVirtualMachineStats();
+ try {
+ cleanUpVirtualMachineStats();
+ } catch (CloudRuntimeException e) {
+ logger.error("Error trying to clean up VM stats", e);
+ }
Review Comment:
Catching only CloudRuntimeException here still lets other unchecked
exceptions (e.g., NullPointerException/IllegalStateException) escape, which
will stop a ScheduledExecutorService periodic task. To fully guard against the
cleaner thread being killed by an unexpected runtime failure, catch a broader
Exception (or at least RuntimeException) and log it.
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1286,13 +1286,21 @@ protected Point createInfluxDbPoint(Object
metricsObject) {
*/
class VmStatsCleaner extends ManagedContextRunnable{
protected void runInContext() {
- cleanUpVirtualMachineStats();
+ try {
+ cleanUpVirtualMachineStats();
+ } catch (CloudRuntimeException e) {
+ logger.error("Error trying to clean up VM stats", e);
+ }
}
}
class VolumeStatsCleaner extends ManagedContextRunnable{
protected void runInContext() {
- cleanUpVolumeStats();
+ try {
+ cleanUpVolumeStats();
+ } catch (CloudRuntimeException e) {
+ logger.error("Error trying to clean up Volume stats", e);
+ }
Review Comment:
Same as VmStatsCleaner: catching only CloudRuntimeException still allows
other unchecked exceptions to terminate the scheduled cleanup task permanently.
Catch a broader Exception (or RuntimeException) so the executor keeps
rescheduling even after unexpected failures.
##########
server/src/test/java/com/cloud/server/StatsCollectorTest.java:
##########
@@ -335,6 +335,49 @@ public void cleanUpVirtualMachineStatsTestIsEnabled() {
Mockito.verify(vmStatsDaoMock).removeAllByTimestampLessThan(Mockito.any(),
Mockito.anyLong());
}
+ private void setVmDiskStatsMaxRetentionTimeValue(String value) {
+ StatsCollector.vmDiskStatsMaxRetentionTime = new
ConfigKey<Integer>("Advanced", Integer.class,
"vm.disk.stats.max.retention.time", value,
+ "The maximum time (in minutes) for keeping Volume stats
records in the database. The Volume stats cleanup process will be disabled if
this is set to 0 or less than 0.", true);
Review Comment:
This test helper redefines the vm.disk.stats.max.retention.time ConfigKey
with a description that differs from the production definition in
StatsCollector (it refers to "Volume stats" instead of "VM disks stats").
Keeping the same description as production reduces confusion and makes
failures/log output easier to interpret.
--
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]
