server: fix resource reservation leakage (#9169)

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

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 org.apache.cloudstack.context.CallContext;
 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,
+@APICommand(name = "updateResourceCount",
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+            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 org.apache.cloudstack.api.InternalIdentity;
 
 import com.cloud.configuration.Resource;
 
+import java.util.Date;
+
 /**
  * an interface defining an {code}AutoClosable{code} reservation object
  */
@@ -39,4 +41,6 @@ ResourceReservation extends InternalIdentity {
     String getTag();
 
     Long getReservedAmount();
+
+    Date getCreated();
 }

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

@@ -25,10 +25,14 @@ import javax.persistence.GenerationType;
 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 class ReservationVO implements ResourceReservation {
         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 class ReservationVO implements ResourceReservation {
         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 org.apache.cloudstack.reservation.ReservationVO;
 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 class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
         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 class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
         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 ?
+        SearchCriteria<ReservationVO> sc;
-                listAccountAndTypeAndNoTagSearch.create() : listAccountAndTypeSearch.create();
+        if (tag == null) {
-        sc.setParameters(ACCOUNT_ID, accountId);
+            sc = listAccountAndTypeAndNoTagSearch.create();
-        sc.setParameters(RESOURCE_TYPE, resourceType);
+        } else {
-        if (tag != null) {
+            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 class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
     @Override
     public long getDomainReservation(Long domainId, Resource.ResourceType resourceType, String tag) {
         long total = 0;
-        SearchCriteria<ReservationVO> sc = tag == null ?
+        SearchCriteria<ReservationVO> sc;
-                listDomainAndTypeAndNoTagSearch.create() : listDomainAndTypeSearch.create();
+        if (tag == null) {
-        sc.setParameters(DOMAIN_ID, domainId);
+            sc = listDomainAndTypeAndNoTagSearch.create();
-        sc.setParameters(RESOURCE_TYPE, resourceType);
+        } else {
-        if (tag != null) {
+            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 class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
         }
     }
 
-    @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 ?
+        SearchCriteria<ReservationVO> sc;
-                listResourceByAccountAndTypeAndNoTagSearch.create() : listResourceByAccountAndTypeSearch.create();
+        if (tag == null) {
-        sc.setParameters(ACCOUNT_ID, accountId);
+            sc = listResourceByAccountAndTypeAndNoTagSearch.create();
-        sc.setParameters(RESOURCE_TYPE, type);
+        } else {
-        if (tag != null) {
+            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 class ReservationDaoImpl extends GenericDaoBase<ReservationVO, Long> impl
             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 class CheckedReservation  implements AutoCloseable {
         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);
+        try {
-        if (CollectionUtils.isNotEmpty(resourceLimitTags)) {
+            checkLimitAndPersistReservation(account, resourceType, resourceId, null, amount);
-            for (String tag : resourceLimitTags) {
+            if (CollectionUtils.isNotEmpty(resourceLimitTags)) {
-                checkLimitAndPersistReservation(account, resourceType, resourceId, tag, amount);
+                for (String tag : resourceLimitTags) {
+                    checkLimitAndPersistReservation(account, resourceType, resourceId, tag, amount);
+                }
             }
+        } catch (ResourceAllocationException rae) {
+            removeAllReservations();
+            throw rae;
         }
     }
 
@@ -147,14 +163,7 @@ public class CheckedReservation  implements AutoCloseable {
 
     @Override
     public void close() throws Exception {
-        if (CollectionUtils.isEmpty(reservations)) {
+        removeAllReservations();
-            return;
-        }
-        CallContext.current().removeContextParameter(getContextParameterKey());
-        for (ResourceReservation reservation : reservations) {
-            reservationDao.remove(reservation.getId());
-        }
-        reservations = null;
     }
 
     public Account getAccount() {

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

@@ -20,6 +20,7 @@ import static com.cloud.utils.NumbersUtil.toHumanReadableSize;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Date;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -219,8 +220,16 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim
         });
     }
 
+    private void cleanupResourceReservationsForMs() {
+        int reservationsRemoved = reservationDao.removeByMsId(ManagementServerNode.getManagementServerId());
+        if (reservationsRemoved > 0) {
+            logger.warn("Removed {} resource reservations for management server id {}", reservationsRemoved, ManagementServerNode.getManagementServerId());
+        }
+    }
+
     @Override
     public boolean start() {
+        cleanupResourceReservationsForMs();
         if (ResourceCountCheckInterval.value() >= 0) {
             ConfigKeyScheduledExecutionWrapper runner = new ConfigKeyScheduledExecutionWrapper(_rcExecutor, new ResourceCountCheckTask(), ResourceCountCheckInterval, TimeUnit.SECONDS);
             runner.start();
@@ -230,6 +239,10 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim
 
     @Override
     public boolean stop() {
+        if (_rcExecutor != null) {
+            _rcExecutor.shutdown();
+        }
+        cleanupResourceReservationsForMs();
         return true;
     }
 
@@ -1200,8 +1213,22 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim
         });
     }
 
+    protected void cleanupStaleResourceReservations(final long accountId, final ResourceType type, String tag) {
+        Long delay = ResourceReservationCleanupDelay.value();
+        if (delay == null || delay <= 0) {
+            return;
+        }
+        Date cleanupBefore = new Date(System.currentTimeMillis() - delay * 1000);
+        int rowsRemoved = reservationDao.removeStaleReservations(accountId, type, tag, cleanupBefore);
+        if (rowsRemoved > 0) {
+            logger.warn("Removed {} stale resource reservations for account {} of type {} and tag {}",
+                    rowsRemoved, accountId, type, tag);
+        }
+    }
+
     @DB
     protected long recalculateAccountResourceCount(final long accountId, final ResourceType type, String tag) {
+        cleanupStaleResourceReservations(accountId, type, tag);
         final Long newCount;
         if (type == Resource.ResourceType.user_vm) {
             newCount = calculateVmCountForAccount(accountId, tag);
@@ -2106,6 +2133,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim
     public ConfigKey<?>[] getConfigKeys() {
         return new ConfigKey<?>[] {
                 ResourceCountCheckInterval,
+                ResourceReservationCleanupDelay,
                 MaxAccountSecondaryStorage,
                 MaxProjectSecondaryStorage,
                 ResourceLimitHostTags,

server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java

@@ -24,7 +24,9 @@ import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.when;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import org.apache.cloudstack.context.CallContext;
 import org.apache.cloudstack.reservation.ReservationVO;
@@ -143,4 +145,41 @@ public class CheckedReservationTest {
             Assert.fail("Exception faced: " + e.getMessage());
         }
     }
+
+    @Test
+    public void testMultipleReservationsWithOneFailing() {
+        List<String> tags = List.of("abc", "xyz");
+        when(account.getAccountId()).thenReturn(1L);
+        when(account.getDomainId()).thenReturn(4L);
+        Map<Long, ReservationVO> persistedReservations = new HashMap<>();
+        Mockito.when(reservationDao.persist(Mockito.any(ReservationVO.class))).thenAnswer((Answer<ReservationVO>) invocation -> {
+            ReservationVO reservationVO = (ReservationVO) invocation.getArguments()[0];
+            Long id = (long) (persistedReservations.size() + 1);
+            ReflectionTestUtils.setField(reservationVO, "id", id);
+            persistedReservations.put(id, reservationVO);
+            return reservationVO;
+        });
+        Mockito.when(reservationDao.remove(Mockito.anyLong())).thenAnswer((Answer<Boolean>) invocation -> {
+            Long id = (Long) invocation.getArguments()[0];
+            persistedReservations.remove(id);
+            return true;
+        });
+        try {
+            Mockito.doThrow(ResourceAllocationException.class).when(resourceLimitService).checkResourceLimitWithTag(account, Resource.ResourceType.cpu, "xyz", 1L);
+            try (CheckedReservation vmReservation = new CheckedReservation(account, Resource.ResourceType.user_vm, tags, 1L, reservationDao, resourceLimitService);
+                 CheckedReservation cpuReservation = new CheckedReservation(account, Resource.ResourceType.cpu, tags, 1L, reservationDao, resourceLimitService);
+                 CheckedReservation memReservation = new CheckedReservation(account, Resource.ResourceType.memory, tags, 256L, reservationDao, resourceLimitService);
+            ) {
+                Assert.fail("Exception should have occurred but all reservations successful!");
+            } catch (Exception ex) {
+                if (!(ex instanceof ResourceAllocationException)) {
+                    Assert.fail(String.format("Expected ResourceAllocationException but %s occurred!", ex.getClass().getSimpleName()));
+                }
+                throw ex;
+            }
+        } catch (Exception rae) {
+            // Check if all persisted reservations are removed
+            Assert.assertTrue("All persisted reservations are not removed", persistedReservations.isEmpty());
+        }
+    }
 }