Handle pr review comments

Author: sudo87

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -628,8 +628,8 @@ public void onPublishMessage(String serderAddress, String subject, Object args)
                     params.put(Config.RouterAggregationCommandEachTimeout.toString(), _configDao.getValue(Config.RouterAggregationCommandEachTimeout.toString()));
                     params.put(Config.MigrateWait.toString(), _configDao.getValue(Config.MigrateWait.toString()));
                     _agentManager.propagateChangeToAgents(params);
-                } else if (VMLeaseManagerImpl.InstanceLeaseEnabled.key().equals(globalSettingUpdated) && !VMLeaseManagerImpl.InstanceLeaseEnabled.value()) {
-                    vmLeaseManager.cancelLeaseOnExistingInstances();
+                } else if (VMLeaseManagerImpl.InstanceLeaseEnabled.key().equals(globalSettingUpdated)) {
+                    vmLeaseManager.onLeaseFeatureToggle();
                 }
             }
         });

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

@@ -6341,7 +6341,7 @@ protected void applyLeaseOnUpdateInstance(UserVm instance, Integer leaseDuration
         // vm must have associated lease during deployment
         UserVmDetailVO vmDetail = userVmDetailsDao.findDetail(instance.getId(), VmDetailConstants.INSTANCE_LEASE_EXPIRY_DATE);
         if (vmDetail == null || StringUtils.isEmpty(vmDetail.getValue())) {
-            logger.debug("Lease wont be applied on instance with id: {}, it doesn't have " +
+            logger.debug("Lease won't be applied on instance with id: {}, it doesn't have " +
                     "leased associated during deployment", instanceUuid);
             return;
         }

server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManager.java

@@ -32,19 +32,15 @@ enum ExpiryAction {
 
     ConfigKey<Long> InstanceLeaseSchedulerInterval = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Long.class,
             "instance.lease.scheduler.interval", "3600", "VM Lease Scheduler interval in seconds",
-            true, List.of(ConfigKey.Scope.Global));
+            false, List.of(ConfigKey.Scope.Global));
 
     ConfigKey<Long> InstanceLeaseAlertSchedule = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Long.class,
             "instance.lease.alertscheduler.interval", "86400", "Lease Alert Scheduler interval in seconds",
-            true, List.of(ConfigKey.Scope.Global));
+            false, List.of(ConfigKey.Scope.Global));
 
     ConfigKey<Long> InstanceLeaseExpiryAlertDaysBefore = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Long.class,
             "instance.lease.alert.daysbefore", "7", "Indicates how many days in advance the alert will be triggered before expiry.",
             true, List.of(ConfigKey.Scope.Global));
 
-    /**
-     * This method will cancel lease on instances running under lease
-     * will be primarily used when feature gets disabled
-     */
-    void cancelLeaseOnExistingInstances();
+    void onLeaseFeatureToggle();
 }

server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManagerImpl.java

@@ -107,30 +107,27 @@ public void setAsyncJobDispatcher(final AsyncJobDispatcher dispatcher) {
 
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
-        try {
-            vmLeaseExecutor = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("VMLeasePollExecutor"));
-            vmLeaseAlertExecutor = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("VMLeaseAlertPollExecutor"));
-        } catch (final Exception e) {
-            throw new ConfigurationException("Unable to to configure VMLeaseManagerImpl");
+        if (InstanceLeaseEnabled.value()) {
+           scheduleLeaseExecutors();
         }
         return true;
     }
 
     @Override
     public boolean start() {
-        vmLeaseExecutor.scheduleAtFixedRate(new VMLeaseSchedulerTask(),5L, InstanceLeaseSchedulerInterval.value(), TimeUnit.SECONDS);
-        vmLeaseAlertExecutor.scheduleAtFixedRate(new VMLeaseAlertSchedulerTask(), 5L, InstanceLeaseAlertSchedule.value(), TimeUnit.SECONDS);
         return true;
     }
 
     @Override
     public boolean stop() {
-        vmLeaseExecutor.shutdown();
-        vmLeaseAlertExecutor.shutdown();
+        shutDownLeaseExecutors();
         return true;
     }
 
-    @Override
+    /**
+     * This method will cancel lease on instances running under lease
+     * will be primarily used when feature gets disabled
+     */
     public void cancelLeaseOnExistingInstances() {
         List<UserVmJoinVO> leaseExpiringForInstances = userVmJoinDao.listLeaseInstancesExpiringInDays(-1);
         logger.debug("Total instances found for lease cancellation: {}", leaseExpiringForInstances.size());
@@ -142,6 +139,45 @@ public void cancelLeaseOnExistingInstances() {
         }
     }
 
+    @Override
+    public void onLeaseFeatureToggle() {
+        boolean isLeaseFeatureEnabled = VMLeaseManagerImpl.InstanceLeaseEnabled.value();
+        if (isLeaseFeatureEnabled) {
+            scheduleLeaseExecutors();
+        } else {
+            cancelLeaseOnExistingInstances();
+            shutDownLeaseExecutors();
+        }
+    }
+
+    private void scheduleLeaseExecutors() {
+        if (vmLeaseExecutor == null || vmLeaseExecutor.isShutdown()) {
+            logger.debug("Scheduling lease executor");
+            vmLeaseExecutor = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("VMLeasePollExecutor"));
+            vmLeaseExecutor.scheduleAtFixedRate(new VMLeaseSchedulerTask(),5L, InstanceLeaseSchedulerInterval.value(), TimeUnit.SECONDS);
+        }
+
+        if (vmLeaseAlertExecutor == null || vmLeaseAlertExecutor.isShutdown()) {
+            logger.debug("Scheduling lease alert executor");
+            vmLeaseAlertExecutor = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("VMLeaseAlertPollExecutor"));
+            vmLeaseAlertExecutor.scheduleAtFixedRate(new VMLeaseAlertSchedulerTask(), 5L, InstanceLeaseAlertSchedule.value(), TimeUnit.SECONDS);
+        }
+    }
+
+    private void shutDownLeaseExecutors() {
+        if (vmLeaseExecutor != null) {
+                logger.debug("Shutting down lease executor");
+            vmLeaseExecutor.shutdown();
+            vmLeaseExecutor = null;
+        }
+
+        if (vmLeaseAlertExecutor != null) {
+            logger.debug("Shutting down lease alert executor");
+            vmLeaseAlertExecutor.shutdown();
+            vmLeaseAlertExecutor = null;
+        }
+    }
+
     class VMLeaseSchedulerTask extends ManagedContextRunnable {
         @Override
         protected void runInContext() {