nvazquez commented on code in PR #10560:
URL: https://github.com/apache/cloudstack/pull/10560#discussion_r2008012433
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -3470,6 +3480,42 @@ protected ServiceOfferingVO createServiceOffering(final
long userId, final boole
}
}
+ /**
+ * This method will return valid and non-empty expiryAction when
+ * "instance.lease.enabled" feature is enabled at global level
+ * leaseDuration is positive > -1 and has valid leaseExpiryAction provided
or configured
+ * @param leaseDuration
+ * @param cmdExpiryAction
+ * @return leaseExpiryAction
+ */
+ public static String validateAndGetLeaseExpiryAction(Long leaseDuration,
String cmdExpiryAction) {
+ String leaseExpiryAction = null;
+ if (!VMLeaseManagerImpl.InstanceLeaseEnabled.value()
+ || (leaseDuration == null &&
StringUtils.isEmpty(cmdExpiryAction))) { // both are null
Review Comment:
Could cmdExpiryAction be passed as an empty string (not null) or is invoked
with null values also? This method would be catching both cases. If only
checking for null, can we use `ObjectUtils.allNull(leaseDuration,
cmdExpiryAction)` instead?
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -251,7 +251,14 @@ public class CreateServiceOfferingCmd extends BaseCmd {
since="4.20")
private Boolean purgeResources;
+ @Parameter(name = ApiConstants.INSTANCE_LEASE_DURATION, type =
CommandType.LONG,
+ description = "Number of days instance is leased for.",
+ since = "4.21.0")
+ private Long leaseDuration;
Review Comment:
Minor one, I think it could be an Integer instead
##########
api/src/main/java/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java:
##########
@@ -330,4 +335,8 @@ protected void updateVMResponse(List<UserVmResponse>
response) {
vmResponse.setResourceIconResponse(iconResponse);
}
}
+
+ public Boolean getOnlyLeasedInstances() {
+ return onlyLeasedInstances;
Review Comment:
Can we use `BooleanUtils.toBoolean()` instead and return a boolean?
##########
server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManagerImpl.java:
##########
@@ -0,0 +1,291 @@
+/*
+ * 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.vm.lease;
+
+import com.cloud.alert.AlertManager;
+import com.cloud.api.ApiGsonHelper;
+import com.cloud.api.query.dao.UserVmJoinDao;
+import com.cloud.api.query.vo.UserVmJoinVO;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.StringUtils;
+import com.cloud.utils.component.ComponentContext;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.cloud.utils.db.GlobalLock;
+import org.apache.cloudstack.api.ApiCommandResourceType;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.command.user.vm.DestroyVMCmd;
+import org.apache.cloudstack.api.command.user.vm.StopVMCmd;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+import org.apache.cloudstack.framework.jobs.AsyncJobDispatcher;
+import org.apache.cloudstack.framework.jobs.AsyncJobManager;
+import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.commons.lang3.time.DateUtils;
+
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+public class VMLeaseManagerImpl extends ManagerBase implements VMLeaseManager,
Configurable {
+
+ public static ConfigKey<Boolean> InstanceLeaseEnabled = new
ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Boolean.class,
+ "instance.lease.enabled", "false", "Indicates whether to enable
the Instance Lease feature",
+ true, List.of(ConfigKey.Scope.Global));
+
+ private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5;
// 5 seconds
+
+ @Inject
+ private UserVmJoinDao userVmJoinDao;
+
+ @Inject
+ private AlertManager alertManager;
+
+ @Inject
+ private AsyncJobManager asyncJobManager;
+
+ private AsyncJobDispatcher asyncJobDispatcher;
+
+ ScheduledExecutorService vmLeaseExecutor;
+ ScheduledExecutorService vmLeaseAlertExecutor;
+
+ @Override
+ public String getConfigComponentName() {
+ return VMLeaseManager.class.getSimpleName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey[]{
+ InstanceLeaseEnabled,
+ InstanceLeaseDuration,
+ InstanceLeaseExpiryAction,
+ InstanceLeaseSchedulerInterval,
+ InstanceLeaseAlertSchedule,
+ InstanceLeaseAlertStartsAt
+ };
+ }
+
+ public void setAsyncJobDispatcher(final AsyncJobDispatcher dispatcher) {
+ 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");
+ }
+ 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();
+ return true;
+ }
+
+ class VMLeaseSchedulerTask extends ManagedContextRunnable {
+ @Override
+ protected void runInContext() {
+ Date currentTimestamp = DateUtils.round(new Date(),
Calendar.MINUTE);
+ String displayTime =
DateUtil.displayDateInTimezone(DateUtil.GMT_TIMEZONE, currentTimestamp);
+ logger.debug("VMLeaseSchedulerTask is being called at {}",
displayTime);
+ if (!InstanceLeaseEnabled.value()) {
+ logger.debug("Instance lease feature is disabled, no action is
required");
+ return;
+ }
+
+ GlobalLock scanLock =
GlobalLock.getInternLock("VMLeaseSchedulerTask");
+ try {
+ if
(scanLock.lock(ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION)) {
+ try {
+ reallyRun();
+ } finally {
+ scanLock.unlock();
+ }
+ }
+ } finally {
+ scanLock.releaseRef();
+ }
+ }
+ }
+
+ class VMLeaseAlertSchedulerTask extends ManagedContextRunnable {
+ @Override
+ protected void runInContext() {
+ // as feature is disabled, no action is required
+ if (!InstanceLeaseEnabled.value()) {
Review Comment:
I was wondering about the use case: user deploys a leased VM for lets say 10
days, 7 days later the administrator disables the feature. I would agree new
deployments may not support leased VMs, but shouldn't the existing leased
actions be executed despite the feature has been disabled?
##########
server/src/main/java/com/cloud/api/query/QueryManagerImpl.java:
##########
@@ -1329,6 +1330,11 @@ private Pair<List<Long>, Integer>
searchForUserVMIdsAndCount(ListVMsCmd cmd) {
}
}
+ boolean requestingOnlyLeasedInstances = cmd.getOnlyLeasedInstances()
!= null && cmd.getOnlyLeasedInstances();
+ if (!VMLeaseManagerImpl.InstanceLeaseEnabled.value() &&
requestingOnlyLeasedInstances) {
+ throw new InvalidParameterValueException("Enable lease feature to
use leased=true");
Review Comment:
Maybe reword the message to something like: Cannot list leased instances
because the Instance Lease feature is disabled, please enable it to list leased
instances
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -6251,9 +6267,111 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
}
}
+
+ applyLeaseOnCreateInstance(vm, leaseDuration, leaseExpiryAction,
svcOffering);
return vm;
}
+ protected void validateLeaseProperties(Long leaseDuration, String
leaseExpiryAction) {
+ if (!VMLeaseManagerImpl.InstanceLeaseEnabled.value()
+ || (leaseDuration == null &&
StringUtils.isEmpty(leaseExpiryAction))) { // if both are null
Review Comment:
You could also add ` || (leaseDuration != null && leaseDuration < 1)` and
reduce the if block below
--
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]
