server: fix resource reservation leakage (#9169)

Co-authored-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

Author: vishesh92

api/src/main/java/com/cloud/user/ResourceLimitService.java

@@ -38,7 +38,10 @@ public interface ResourceLimitService {
     static final ConfigKey<Long> MaxProjectSecondaryStorage = new ConfigKey<>("Project Defaults", Long.class, "max.project.secondary.storage", "400",
             "The default maximum secondary storage space (in GiB) that can be used for a project", false);
     static final ConfigKey<Long> ResourceCountCheckInterval = new ConfigKey<>("Advanced", Long.class, "resourcecount.check.interval", "300",
-            "Time (in seconds) to wait before running resource recalculation and fixing task. Default is 300 seconds, Setting this to 0 disables execution of the task", true);
+            "Time (in seconds) to wait before running resource recalculation and fixing tasks like stale resource reservation cleanup" +
+                    ". Default is 300 seconds, Setting this to 0 disables execution of the task", true);
+    static final ConfigKey<Long> ResourceReservationCleanupDelay = new ConfigKey<>("Advanced", Long.class, "resource.reservation.cleanup.delay", "3600",
+            "Time (in seconds) after which a resource reservation gets deleted. Default is 3600 seconds, Setting this to 0 disables execution of the task", true);
     static final ConfigKey<String> ResourceLimitHostTags = new ConfigKey<>("Advanced", String.class, "resource.limit.host.tags", "",
             "A comma-separated list of tags for host resource limits", true);
     static final ConfigKey<String> ResourceLimitStorageTags = new ConfigKey<>("Advanced", String.class, "resource.limit.storage.tags", "",

api/src/main/java/org/apache/cloudstack/api/command/user/resource/UpdateResourceCountCmd.java

@@ -34,8 +34,11 @@
 import com.cloud.configuration.ResourceCount;
 import com.cloud.user.Account;
 
-@APICommand(name = "updateResourceCount", description = "Recalculate and update resource count for an account or domain.", responseObject = ResourceCountResponse.class,
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+@APICommand(name = "updateResourceCount",
+            description = "Recalculate and update resource count for an account or domain. " +
+                    "This also executes some cleanup tasks before calculating resource counts.",
+            responseObject = ResourceCountResponse.class,
+            requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
 public class UpdateResourceCountCmd extends BaseCmd {
 
 

api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java

@@ -22,6 +22,8 @@
 
 import com.cloud.configuration.Resource;
 
+import java.util.Date;
+
 /**
  * an interface defining an {code}AutoClosable{code} reservation object
  */
@@ -39,4 +41,6 @@
     String getTag();
 
     Long getReservedAmount();
+
+    Date getCreated();
 }

engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java

@@ -25,10 +25,14 @@
 import javax.persistence.Id;
 import javax.persistence.Table;
 
+import com.cloud.utils.db.GenericDao;
 import org.apache.cloudstack.user.ResourceReservation;
 
 import com.cloud.configuration.Resource;
 import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.utils.identity.ManagementServerNode;
+
+import java.util.Date;
 
 @Entity
 @Table(name = "resource_reservation")
@@ -57,6 +61,12 @@ public class ReservationVO implements ResourceReservation {
     @Column(name = "amount")
     long amount;
 
+    @Column(name = "mgmt_server_id")
+    Long managementServerId;
+
+    @Column(name = GenericDao.CREATED_COLUMN)
+    private Date created;
+
     protected ReservationVO() {
     }
 
@@ -69,6 +79,7 @@ public ReservationVO(Long accountId, Long domainId, Resource.ResourceType resour
         this.resourceType = resourceType;
         this.tag = tag;
         this.amount = delta;
+        this.managementServerId = ManagementServerNode.getManagementServerId();
     }
 
     public ReservationVO(Long accountId, Long domainId, Resource.ResourceType resourceType, Long delta) {
@@ -114,4 +125,16 @@ public void setResourceId(long resourceId) {
         this.resourceId = resourceId;
     }
 
+    @Override
+    public Date getCreated() {
+        return created;
+    }
+
+    public void setCreated(Date created) {
+        this.created = created;
+    }
+
+    public Long getManagementServerId() {
+        return managementServerId;
+    }
 }

engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java

@@ -23,13 +23,17 @@
 import com.cloud.configuration.Resource;
 import com.cloud.utils.db.GenericDao;
 
+import java.util.Date;
 import java.util.List;
 
 public interface ReservationDao extends GenericDao<ReservationVO, Long> {
     long getAccountReservation(Long account, Resource.ResourceType resourceType, String tag);
     long getDomainReservation(Long domain, Resource.ResourceType resourceType, String tag);
     void setResourceId(Resource.ResourceType type, Long resourceId);
-    List<Long> getResourceIds(long accountId, Resource.ResourceType type);
     List<ReservationVO> getReservationsForAccount(long accountId, Resource.ResourceType type, String tag);
     void removeByIds(List<Long> reservationIds);
+
+    int removeByMsId(long managementServerId);
+
+    int removeStaleReservations(Long accountId, Resource.ResourceType resourceType, String tag, Date createdBefore);
 }

engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java

@@ -18,8 +18,8 @@
 //
 package org.apache.cloudstack.reservation.dao;
 
+import java.util.Date;
 import java.util.List;
-import java.util.stream.Collectors;
 
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.reservation.ReservationVO;
@@ -42,6 +42,8 @@ public class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
     private static final String ACCOUNT_ID = "accountId";
     private static final String DOMAIN_ID = "domainId";
     private static final String IDS = "ids";
+    private static final String MS_ID = "managementServerId";
+    private static final String CREATED = "created";
     private final SearchBuilder<ReservationVO> listResourceByAccountAndTypeSearch;
     private final SearchBuilder<ReservationVO> listAccountAndTypeSearch;
     private final SearchBuilder<ReservationVO> listAccountAndTypeAndNoTagSearch;
@@ -50,6 +52,7 @@ public class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
     private final SearchBuilder<ReservationVO> listDomainAndTypeAndNoTagSearch;
     private final SearchBuilder<ReservationVO> listResourceByAccountAndTypeAndNoTagSearch;
     private final SearchBuilder<ReservationVO> listIdsSearch;
+    private final SearchBuilder<ReservationVO> listMsIdSearch;
 
     public ReservationDaoImpl() {
 
@@ -71,12 +74,14 @@ public ReservationDaoImpl() {
         listAccountAndTypeSearch.and(ACCOUNT_ID, listAccountAndTypeSearch.entity().getAccountId(), SearchCriteria.Op.EQ);
         listAccountAndTypeSearch.and(RESOURCE_TYPE, listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ);
         listAccountAndTypeSearch.and(RESOURCE_TAG, listAccountAndTypeSearch.entity().getTag(), SearchCriteria.Op.EQ);
+        listAccountAndTypeSearch.and(CREATED, listAccountAndTypeSearch.entity().getCreated(), SearchCriteria.Op.LT);
         listAccountAndTypeSearch.done();
 
         listAccountAndTypeAndNoTagSearch = createSearchBuilder();
         listAccountAndTypeAndNoTagSearch.and(ACCOUNT_ID, listAccountAndTypeAndNoTagSearch.entity().getAccountId(), SearchCriteria.Op.EQ);
         listAccountAndTypeAndNoTagSearch.and(RESOURCE_TYPE, listAccountAndTypeAndNoTagSearch.entity().getResourceType(), SearchCriteria.Op.EQ);
         listAccountAndTypeAndNoTagSearch.and(RESOURCE_TAG, listAccountAndTypeAndNoTagSearch.entity().getTag(), SearchCriteria.Op.NULL);
+        listAccountAndTypeAndNoTagSearch.and(CREATED, listAccountAndTypeAndNoTagSearch.entity().getCreated(), SearchCriteria.Op.LT);
         listAccountAndTypeAndNoTagSearch.done();
 
         listDomainAndTypeSearch = createSearchBuilder();
@@ -94,18 +99,24 @@ public ReservationDaoImpl() {
         listIdsSearch = createSearchBuilder();
         listIdsSearch.and(IDS, listIdsSearch.entity().getId(), SearchCriteria.Op.IN);
         listIdsSearch.done();
+
+        listMsIdSearch = createSearchBuilder();
+        listMsIdSearch.and(MS_ID, listMsIdSearch.entity().getManagementServerId(), SearchCriteria.Op.EQ);
+        listMsIdSearch.done();
     }
 
     @Override
     public long getAccountReservation(Long accountId, Resource.ResourceType resourceType, String tag) {
         long total = 0;
-        SearchCriteria<ReservationVO> sc = tag == null ?
-                listAccountAndTypeAndNoTagSearch.create() : listAccountAndTypeSearch.create();
-        sc.setParameters(ACCOUNT_ID, accountId);
-        sc.setParameters(RESOURCE_TYPE, resourceType);
-        if (tag != null) {
+        SearchCriteria<ReservationVO> sc;
+        if (tag == null) {
+            sc = listAccountAndTypeAndNoTagSearch.create();
+        } else {
+            sc = listAccountAndTypeSearch.create();
             sc.setParameters(RESOURCE_TAG, tag);
         }
+        sc.setParameters(ACCOUNT_ID, accountId);
+        sc.setParameters(RESOURCE_TYPE, resourceType);
         List<ReservationVO> reservations = listBy(sc);
         for (ReservationVO reservation : reservations) {
             total += reservation.getReservedAmount();
@@ -116,13 +127,15 @@ public long getAccountReservation(Long accountId, Resource.ResourceType resource
     @Override
     public long getDomainReservation(Long domainId, Resource.ResourceType resourceType, String tag) {
         long total = 0;
-        SearchCriteria<ReservationVO> sc = tag == null ?
-                listDomainAndTypeAndNoTagSearch.create() : listDomainAndTypeSearch.create();
-        sc.setParameters(DOMAIN_ID, domainId);
-        sc.setParameters(RESOURCE_TYPE, resourceType);
-        if (tag != null) {
+        SearchCriteria<ReservationVO> sc;
+        if (tag == null) {
+            sc = listDomainAndTypeAndNoTagSearch.create();
+        } else {
+            sc = listDomainAndTypeSearch.create();
             sc.setParameters(RESOURCE_TAG, tag);
         }
+        sc.setParameters(DOMAIN_ID, domainId);
+        sc.setParameters(RESOURCE_TYPE, resourceType);
         List<ReservationVO> reservations = listBy(sc);
         for (ReservationVO reservation : reservations) {
             total += reservation.getReservedAmount();
@@ -149,23 +162,17 @@ public void setResourceId(Resource.ResourceType type, Long resourceId) {
         }
     }
 
-    @Override
-    public List<Long> getResourceIds(long accountId, Resource.ResourceType type) {
-        SearchCriteria<ReservationVO> sc = listResourceByAccountAndTypeSearch.create();
-        sc.setParameters(ACCOUNT_ID, accountId);
-        sc.setParameters(RESOURCE_TYPE, type);
-        return listBy(sc).stream().map(ReservationVO::getResourceId).collect(Collectors.toList());
-    }
-
     @Override
     public List<ReservationVO> getReservationsForAccount(long accountId, Resource.ResourceType type, String tag) {
-        SearchCriteria<ReservationVO> sc = tag == null ?
-                listResourceByAccountAndTypeAndNoTagSearch.create() : listResourceByAccountAndTypeSearch.create();
-        sc.setParameters(ACCOUNT_ID, accountId);
-        sc.setParameters(RESOURCE_TYPE, type);
-        if (tag != null) {
+        SearchCriteria<ReservationVO> sc;
+        if (tag == null) {
+            sc = listResourceByAccountAndTypeAndNoTagSearch.create();
+        } else {
+            sc = listResourceByAccountAndTypeSearch.create();
             sc.setParameters(RESOURCE_TAG, tag);
         }
+        sc.setParameters(ACCOUNT_ID, accountId);
+        sc.setParameters(RESOURCE_TYPE, type);
         return listBy(sc);
     }
 
@@ -177,4 +184,28 @@ public void removeByIds(List<Long> reservationIds) {
             remove(sc);
         }
     }
+
+    @Override
+    public int removeByMsId(long managementServerId) {
+        SearchCriteria<ReservationVO> sc = listMsIdSearch.create();
+        sc.setParameters(MS_ID, managementServerId);
+        return remove(sc);
+    }
+
+    @Override
+    public int removeStaleReservations(Long accountId, Resource.ResourceType resourceType, String tag,
+            Date createdBefore) {
+        SearchCriteria<ReservationVO> sc;
+        if (tag == null) {
+            sc = listAccountAndTypeAndNoTagSearch.create();
+        } else {
+            sc = listAccountAndTypeSearch.create();
+            sc.setParameters(RESOURCE_TAG, tag);
+        }
+        sc.setParameters(ACCOUNT_ID, accountId);
+        sc.setParameters(RESOURCE_TYPE, resourceType);
+        sc.setParameters(CREATED, createdBefore);
+        return remove(sc);
+    }
+
 }

engine/schema/src/main/resources/META-INF/db/schema-41910to42000.sql

@@ -29,13 +29,15 @@ DROP INDEX `i_resource_count__type_domaintId`,
 ADD UNIQUE INDEX `i_resource_count__type_tag_accountId` (`type`,`tag`,`account_id`),
 ADD UNIQUE INDEX `i_resource_count__type_tag_domaintId` (`type`,`tag`,`domain_id`);
 
-
-ALTER TABLE `cloud`.`resource_reservation`
-    ADD COLUMN `resource_id` bigint unsigned NULL;
-
 ALTER TABLE `cloud`.`resource_reservation`
     MODIFY COLUMN `amount` bigint NOT NULL;
 
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.resource_reservation', 'resource_id', 'bigint unsigned NULL COMMENT "id of the resource" ');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.resource_reservation', 'mgmt_server_id', 'bigint unsigned NULL COMMENT "management server id" ');
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.resource_reservation', 'created', 'datetime DEFAULT NULL COMMENT "date when the reservation was created" ');
+
+UPDATE `cloud`.`resource_reservation` SET `created` = now() WHERE created IS NULL;
+
 
 -- Update Default System offering for Router to 512MiB
 UPDATE `cloud`.`service_offering` SET ram_size = 512 WHERE unique_name IN ("Cloud.Com-SoftwareRouter", "Cloud.Com-SoftwareRouter-Local",

server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java

@@ -62,12 +62,28 @@ public static String getResourceReservationContextParameterKey(final ResourceTyp
         return String.format("%s-%s", ResourceReservation.class.getSimpleName(), type.getName());
     }
 
+    private void removeAllReservations() {
+        if (CollectionUtils.isEmpty(reservations)) {
+            return;
+        }
+        CallContext.current().removeContextParameter(getContextParameterKey());
+        for (ResourceReservation reservation : reservations) {
+            reservationDao.remove(reservation.getId());
+        }
+        this.reservations = null;
+    }
+
     protected void checkLimitAndPersistReservations(Account account, ResourceType resourceType, Long resourceId, List<String> resourceLimitTags, Long amount) throws ResourceAllocationException {
-        checkLimitAndPersistReservation(account, resourceType, resourceId, null, amount);
-        if (CollectionUtils.isNotEmpty(resourceLimitTags)) {
-            for (String tag : resourceLimitTags) {
-                checkLimitAndPersistReservation(account, resourceType, resourceId, tag, amount);
+        try {
+            checkLimitAndPersistReservation(account, resourceType, resourceId, null, amount);
+            if (CollectionUtils.isNotEmpty(resourceLimitTags)) {
+                for (String tag : resourceLimitTags) {
+                    checkLimitAndPersistReservation(account, resourceType, resourceId, tag, amount);
+                }
             }
+        } catch (ResourceAllocationException rae) {
+            removeAllReservations();
+            throw rae;
         }
     }
 
@@ -147,14 +163,7 @@ protected void setQuotaLimitLock(GlobalLock quotaLimitLock) {
 
     @Override
     public void close() throws Exception {
-        if (CollectionUtils.isEmpty(reservations)) {
-            return;
-        }
-        CallContext.current().removeContextParameter(getContextParameterKey());
-        for (ResourceReservation reservation : reservations) {
-            reservationDao.remove(reservation.getId());
-        }
-        reservations = null;
+        removeAllReservations();
     }
 
     public Account getAccount() {