Re: [PR] guard against stopping stats background threads [cloudstack]

["Copilot (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org