vishesh92 commented on code in PR #10560:
URL: https://github.com/apache/cloudstack/pull/10560#discussion_r2107530841
##########
server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java:
##########
@@ -176,6 +177,11 @@ public ServiceOfferingResponse
newServiceOfferingResponse(ServiceOfferingJoinVO
}
}
+ if (VMLeaseManager.InstanceLeaseEnabled.value() &&
offering.getLeaseDuration() != null && offering.getLeaseDuration() > 0L) {
Review Comment:
Also, what will happen to the existing VMs with serivce offerings having
lease if the user re-enables the feature?
##########
api/src/main/java/org/apache/cloudstack/api/ApiConstants.java:
##########
@@ -269,7 +269,10 @@ public class ApiConstants {
public static final String INTERNAL_DNS2 = "internaldns2";
public static final String INTERNET_PROTOCOL = "internetprotocol";
public static final String INTERVAL_TYPE = "intervaltype";
- public static final String LOCATION_TYPE = "locationtype";
+ public static final String INSTANCE_LEASE_DURATION = "leaseduration";
+ public static final String INSTANCE_LEASE_ENABLED = "instanceleaseenabled";
Review Comment:
Should we rename it to `leaseenabled` or prepend `instance` to OTHERS
maintain consitency in naming?
##########
server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManager.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.utils.component.Manager;
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface VMLeaseManager extends Manager {
+
+ int MAX_LEASE_DURATION_DAYS = 365_00; // 100 years
+
+ enum ExpiryAction {
+ STOP,
+ DESTROY,
+ @VisibleForTesting
+ UNKNOWN
Review Comment:
The end user can launch a VM with expiry action `UNKNOWN`. This is also
being shown to the user in the error response.
```
Error: (HTTP 431, error code 4350) Invalid value provided for
leaseexpiryaction, valid values are: [STOP, DESTROY, UNKNOWN, ]
```
##########
server/src/main/java/com/cloud/api/query/dao/ServiceOfferingJoinDaoImpl.java:
##########
@@ -176,6 +177,11 @@ public ServiceOfferingResponse
newServiceOfferingResponse(ServiceOfferingJoinVO
}
}
+ if (VMLeaseManager.InstanceLeaseEnabled.value() &&
offering.getLeaseDuration() != null && offering.getLeaseDuration() > 0L) {
Review Comment:
IMO, we can remove the `InstanceLeaseEnabled` check here. If someone
disables the check later on the user won't be able to see the values for
existing service offerings.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -6263,9 +6288,121 @@ public UserVm createVirtualMachine(DeployVMCmd cmd)
throws InsufficientCapacityE
}
}
}
+ if (isLeaseFeatureEnabled) {
+ applyLeaseOnCreateInstance(vm, cmd.getLeaseDuration(),
cmd.getLeaseExpiryAction(), svcOffering);
+ }
return vm;
}
+ protected void validateLeaseProperties(Integer leaseDuration, String
leaseExpiryAction) {
+ if (ObjectUtils.allNull(leaseDuration, leaseExpiryAction) // both are
null
+ || (leaseDuration != null && leaseDuration == -1)) { //
special condition to disable lease for instance
+ return;
+ }
+
+ // any one of them have value
+ // validate leaseduration
+ if (leaseDuration == null || leaseDuration < 1 || leaseDuration >
VMLeaseManager.MAX_LEASE_DURATION_DAYS) {
+ throw new InvalidParameterValueException("Invalid leaseduration:
must be a natural number (>=1) or -1, max supported value is 36500");
+ }
+
+ if (StringUtils.isEmpty(leaseExpiryAction)) {
+ throw new InvalidParameterValueException("Provide values for both:
leaseduration and leaseexpiryaction");
+ }
+
+ try {
+ VMLeaseManager.ExpiryAction.valueOf(leaseExpiryAction);
Review Comment:
You can use `EnumUtils.getEnumIgnoreCase` to get the enum from a string.
And `EnumUtils.isValidEnumIgnoreCase` to check if it's a valid enum.
The benefit of using `EnumUtils.` is that it ignores case while checking an
enum and provides a better experience to the end user.
##########
ui/public/locales/en.json:
##########
@@ -2671,6 +2672,15 @@
"label.bucket.policy": "Bucket Policy",
"label.usersecretkey": "Secret Key",
"label.create.bucket": "Create Bucket",
+"label.lease.enable": "Enable Lease",
+"label.lease.enable.tooltip": "The instance Lease feature allows to set a
lease duration (in days) for instances, after which they automatically expire.
Upon expiry, the instance can either be stopped (powered off) or destroyed,
based on the configured policy",
Review Comment:
```suggestion
"label.lease.enable.tooltip": "The instance lease feature allows to set a
lease duration (in days) for instances, after which they automatically expire.
Upon expiry, the instance can either be stopped (powered off) or destroyed,
based on the configured policy",
```
##########
server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManagerImpl.java:
##########
@@ -0,0 +1,392 @@
+/*
+ * 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.api.ApiGsonHelper;
+import com.cloud.api.query.dao.UserVmJoinDao;
+import com.cloud.api.query.vo.UserVmJoinVO;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.Pair;
+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 com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+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.AsyncJob;
+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.framework.messagebus.MessageBus;
+import org.apache.cloudstack.framework.messagebus.MessageSubscriber;
+import org.apache.cloudstack.jobs.JobInfo;
+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.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+public class VMLeaseManagerImpl extends ManagerBase implements VMLeaseManager,
Configurable {
+ private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5;
// 5 seconds
+
+ @Inject
+ private UserVmDetailsDao userVmDetailsDao;
+
+ @Inject
+ private UserVmJoinDao userVmJoinDao;
+
+ @Inject
+ private AsyncJobManager asyncJobManager;
+ @Inject
+ private MessageBus messageBus;
+
+ private AsyncJobDispatcher asyncJobDispatcher;
+
+ ScheduledExecutorService vmLeaseExecutor;
+ ScheduledExecutorService vmLeaseExpiryEventExecutor;
+ Gson gson = ApiGsonHelper.getBuilder().create();
+ VMLeaseManagerSubscriber leaseManagerSubscriber;
+
+ public static final String JOB_INITIATOR = "jobInitiator";
+
+ @Override
+ public String getConfigComponentName() {
+ return VMLeaseManager.class.getSimpleName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey[]{
+ InstanceLeaseEnabled,
+ InstanceLeaseSchedulerInterval,
+ InstanceLeaseExpiryEventSchedulerInterval,
+ InstanceLeaseExpiryEventDaysBefore
+ };
+ }
+
+ public void setAsyncJobDispatcher(final AsyncJobDispatcher dispatcher) {
+ asyncJobDispatcher = dispatcher;
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws
ConfigurationException {
+ if (InstanceLeaseEnabled.value()) {
+ scheduleLeaseExecutors();
+ }
+ return true;
+ }
+
+ @Override
+ public boolean stop() {
+ shutDownLeaseExecutors();
+ return true;
+ }
+
+ /**
+ * This method will cancel lease on instances running under lease
+ * will be primarily used when feature gets disabled
+ */
+ public void cancelLeaseOnExistingInstances() {
Review Comment:
If someone toggles the setting by mistake and reverts it, would the older
expiry of VMs work?
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/offering/CreateServiceOfferingCmd.java:
##########
@@ -487,6 +495,14 @@ public boolean getEncryptRoot() {
return false;
}
+ public String getLeaseExpiryAction() {
+ return leaseExpiryAction;
Review Comment:
Not really an issue, but in most of the Cmd classes we return the enum
itself here.
For example:
https://github.com/apache/cloudstack/blob/99863c2fa5a57d8ddf1b4b40c647b159d96dc74c/api/src/main/java/org/apache/cloudstack/api/command/user/offering/ListDiskOfferingsCmd.java#L113-L122
##########
server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManager.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.utils.component.Manager;
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.cloudstack.framework.config.ConfigKey;
+
+import java.util.List;
+
+public interface VMLeaseManager extends Manager {
+
+ int MAX_LEASE_DURATION_DAYS = 365_00; // 100 years
+
+ enum ExpiryAction {
+ STOP,
+ DESTROY,
+ @VisibleForTesting
+ UNKNOWN
+ }
+
+ enum LeaseActionExecution {
+ PENDING,
+ DISABLED,
+ DONE,
+ CANCELLED
+ }
+
+ ConfigKey<Boolean> InstanceLeaseEnabled = new
ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Boolean.class,
+ "instance.lease.enabled", "false", "Indicates whether to enable
the Instance lease," +
+ " will be applicable only on instances created after lease is
enabled. Disabling the feature cancels lease on existing instances with lease."
+
Review Comment:
```suggestion
" will be applicable only on instances created after lease is
enabled. Disabling the feature cancels lease on existing instances with lease.
" +
```
##########
server/src/main/java/org/apache/cloudstack/vm/lease/VMLeaseManagerImpl.java:
##########
@@ -0,0 +1,392 @@
+/*
+ * 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.api.ApiGsonHelper;
+import com.cloud.api.query.dao.UserVmJoinDao;
+import com.cloud.api.query.vo.UserVmJoinVO;
+import com.cloud.event.ActionEventUtils;
+import com.cloud.event.EventTypes;
+import com.cloud.user.Account;
+import com.cloud.user.User;
+import com.cloud.utils.DateUtil;
+import com.cloud.utils.Pair;
+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 com.cloud.vm.VmDetailConstants;
+import com.cloud.vm.dao.UserVmDetailsDao;
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+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.AsyncJob;
+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.framework.messagebus.MessageBus;
+import org.apache.cloudstack.framework.messagebus.MessageSubscriber;
+import org.apache.cloudstack.jobs.JobInfo;
+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.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+public class VMLeaseManagerImpl extends ManagerBase implements VMLeaseManager,
Configurable {
+ private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 5;
// 5 seconds
+
+ @Inject
+ private UserVmDetailsDao userVmDetailsDao;
+
+ @Inject
+ private UserVmJoinDao userVmJoinDao;
+
+ @Inject
+ private AsyncJobManager asyncJobManager;
+ @Inject
+ private MessageBus messageBus;
+
+ private AsyncJobDispatcher asyncJobDispatcher;
+
+ ScheduledExecutorService vmLeaseExecutor;
+ ScheduledExecutorService vmLeaseExpiryEventExecutor;
+ Gson gson = ApiGsonHelper.getBuilder().create();
+ VMLeaseManagerSubscriber leaseManagerSubscriber;
+
+ public static final String JOB_INITIATOR = "jobInitiator";
+
+ @Override
+ public String getConfigComponentName() {
+ return VMLeaseManager.class.getSimpleName();
+ }
+
+ @Override
+ public ConfigKey<?>[] getConfigKeys() {
+ return new ConfigKey[]{
+ InstanceLeaseEnabled,
+ InstanceLeaseSchedulerInterval,
+ InstanceLeaseExpiryEventSchedulerInterval,
+ InstanceLeaseExpiryEventDaysBefore
+ };
+ }
+
+ public void setAsyncJobDispatcher(final AsyncJobDispatcher dispatcher) {
+ asyncJobDispatcher = dispatcher;
+ }
+
+ @Override
+ public boolean configure(String name, Map<String, Object> params) throws
ConfigurationException {
+ if (InstanceLeaseEnabled.value()) {
+ scheduleLeaseExecutors();
+ }
+ return true;
+ }
+
+ @Override
+ public boolean stop() {
+ shutDownLeaseExecutors();
+ return true;
+ }
+
+ /**
+ * 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());
+ for (UserVmJoinVO instance : leaseExpiringForInstances) {
+ userVmDetailsDao.addDetail(instance.getId(),
VmDetailConstants.INSTANCE_LEASE_EXECUTION,
+ LeaseActionExecution.CANCELLED.name(), false);
+ String leaseCancellationMsg = String.format("Lease is cancelled
for the instance: %s (id: %s) ", instance.getName(), instance.getUuid());
+ ActionEventUtils.onActionEvent(instance.getUserId(),
instance.getAccountId(), instance.getDomainId(),
+ EventTypes.VM_LEASE_CANCELLED, leaseCancellationMsg,
instance.getId(), ApiCommandResourceType.VirtualMachine.toString());
+ }
+ }
+
+ @Override
+ public void onLeaseFeatureToggle() {
+ boolean isLeaseFeatureEnabled =
VMLeaseManager.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 (vmLeaseExpiryEventExecutor == null ||
vmLeaseExpiryEventExecutor.isShutdown()) {
+ logger.debug("Scheduling lease expiry event executor");
+ vmLeaseExpiryEventExecutor =
Executors.newSingleThreadScheduledExecutor(new
NamedThreadFactory("VmLeaseExpiryEventExecutor"));
+ vmLeaseExpiryEventExecutor.scheduleAtFixedRate(new
VMLeaseExpiryEventSchedulerTask(), 5L,
InstanceLeaseExpiryEventSchedulerInterval.value(), TimeUnit.SECONDS);
+ }
+ addLeaseExpiryListener();
+ }
+
+ private void shutDownLeaseExecutors() {
+ if (vmLeaseExecutor != null) {
+ logger.debug("Shutting down lease executor");
Review Comment:
```suggestion
logger.debug("Shutting down lease executor");
```
--
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]
