From cee7a713aa2d15372684640de3e8c68ff3916bc6 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Fri, 21 Jul 2023 10:55:51 +0530 Subject: [PATCH] server: clear resource reservation and increment resource count in a transaction (#7724) This PR addresses rare case of potential overlap of resource reservation and resource count. For different resource types there could be some delay between incrementing of the resource count and clearing of the earlier done reservation. This may result in failures when there are parallel deployments happening. Signed-off-by: Abhishek Kumar --- .../cloudstack/context/CallContext.java | 5 +++ .../resourcelimit/CheckedReservation.java | 20 ++++++++---- .../ResourceLimitManagerImpl.java | 31 +++++++++++++------ 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/context/CallContext.java b/api/src/main/java/org/apache/cloudstack/context/CallContext.java index 50049b30a07..ecc109977eb 100644 --- a/api/src/main/java/org/apache/cloudstack/context/CallContext.java +++ b/api/src/main/java/org/apache/cloudstack/context/CallContext.java @@ -92,6 +92,10 @@ public class CallContext { context.put(key, value); } + public void removeContextParameter(Object key) { + context.remove(key); + } + /** * @param key any not null key object * @return the value of the key from context map @@ -234,6 +238,7 @@ public class CallContext { CallContext callContext = register(parent.getCallingUserId(), parent.getCallingAccountId()); callContext.setStartEventId(parent.getStartEventId()); callContext.setEventResourceType(eventResourceType); + callContext.putContextParameters(parent.getContextParameters()); return callContext; } diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 0075ef1b4f1..d0c20a3a7af 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -18,17 +18,19 @@ // package com.cloud.resourcelimit; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.reservation.ReservationVO; +import org.apache.cloudstack.reservation.dao.ReservationDao; +import org.apache.cloudstack.user.ResourceReservation; +import org.apache.log4j.Logger; +import org.jetbrains.annotations.NotNull; + import com.cloud.configuration.Resource.ResourceType; import com.cloud.exception.ResourceAllocationException; import com.cloud.user.Account; import com.cloud.user.ResourceLimitService; import com.cloud.utils.db.GlobalLock; -import org.apache.cloudstack.user.ResourceReservation; import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.cloudstack.reservation.ReservationVO; -import org.apache.cloudstack.reservation.dao.ReservationDao; -import org.apache.log4j.Logger; -import org.jetbrains.annotations.NotNull; public class CheckedReservation implements AutoCloseable, ResourceReservation { @@ -42,6 +44,10 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { private Long amount; private ResourceReservation reservation; + private String getContextParameterKey() { + return String.format("%s-%s", ResourceReservation.class.getSimpleName(), resourceType.getName()); + } + /** * - check if adding a reservation is allowed * - create DB entry for reservation @@ -70,6 +76,7 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { resourceLimitService.checkResourceLimit(account,resourceType,amount); ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount); this.reservation = reservationDao.persist(reservationVO); + CallContext.current().putContextParameter(getContextParameterKey(), reservationVO.getId()); } catch (NullPointerException npe) { throw new CloudRuntimeException("not enough means to check limits", npe); } finally { @@ -97,7 +104,8 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { @Override public void close() throws Exception { - if (this.reservation != null){ + if (this.reservation != null) { + CallContext.current().removeContextParameter(getContextParameterKey()); reservationDao.remove(reservation.getId()); reservation = null; } diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 9efeed39ac0..288315d5b62 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -16,6 +16,8 @@ // under the License. package com.cloud.resourcelimit; +import static com.cloud.utils.NumbersUtil.toHumanReadableSize; + import java.util.ArrayList; import java.util.Arrays; import java.util.EnumMap; @@ -103,13 +105,11 @@ import com.cloud.utils.db.TransactionCallbackNoReturn; import com.cloud.utils.db.TransactionCallbackWithExceptionNoReturn; import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.VirtualMachine.State; +import com.cloud.vm.VirtualMachineManager; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; -import static com.cloud.utils.NumbersUtil.toHumanReadableSize; - @Component public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLimitService, Configurable { public static final Logger s_logger = Logger.getLogger(ResourceLimitManagerImpl.class); @@ -173,6 +173,23 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim Map domainResourceLimitMap = new EnumMap(ResourceType.class); Map projectResourceLimitMap = new EnumMap(ResourceType.class); + protected void removeResourceReservationIfNeededAndIncrementResourceCount(final long accountId, final ResourceType type, final long numToIncrement) { + Transaction.execute(new TransactionCallbackWithExceptionNoReturn() { + @Override + public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException { + + Object obj = CallContext.current().getContextParameter(String.format("%s-%s", ResourceReservation.class.getSimpleName(), type.getName())); + if (obj instanceof Long) { + reservationDao.remove((long)obj); + } + if (!updateResourceCountForAccount(accountId, type, true, numToIncrement)) { + // we should fail the operation (resource creation) when failed to update the resource count + throw new CloudRuntimeException("Failed to increment resource count of type " + type + " for account id=" + accountId); + } + } + }); + } + @Override public boolean start() { if (_resourceCountCheckInterval > 0) { @@ -270,12 +287,8 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim return; } - long numToIncrement = (delta.length == 0) ? 1 : delta[0].longValue(); - - if (!updateResourceCountForAccount(accountId, type, true, numToIncrement)) { - // we should fail the operation (resource creation) when failed to update the resource count - throw new CloudRuntimeException("Failed to increment resource count of type " + type + " for account id=" + accountId); - } + final long numToIncrement = (delta.length == 0) ? 1 : delta[0].longValue(); + removeResourceReservationIfNeededAndIncrementResourceCount(accountId, type, numToIncrement); } @Override