From 772f76bc47d885a5bd74b54d11e90885f588a486 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 3 Jul 2023 12:22:23 +0530 Subject: [PATCH 1/2] server: clear resource reservation and increment resource count in a transaction 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 | 26 +++++++++++++------ 3 files changed, 37 insertions(+), 14 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 50049b30a07d..ecc109977eb6 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 void putContextParameter(Object key, Object value) { 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 static CallContext register(CallContext parent, ApiCommandResourceType ev 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 0075ef1b4f1a..d0c20a3a7af7 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 CheckedReservation(Account account, ResourceType resourceType, Long amoun 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 @@ protected void setQuotaLimitLock(GlobalLock quotaLimitLock) { @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 9efeed39ac07..8e7eddaca3b0 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.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); @@ -270,12 +270,22 @@ public void incrementResourceCount(long accountId, ResourceType type, Long... de return; } - long numToIncrement = (delta.length == 0) ? 1 : delta[0].longValue(); + final 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); - } + 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 From 05e57f64f47761835f88b2d27fa69dd03e5824c5 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 6 Jul 2023 14:11:01 +0530 Subject: [PATCH 2/2] refactor Signed-off-by: Abhishek Kumar --- .../ResourceLimitManagerImpl.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 8e7eddaca3b0..288315d5b621 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -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) { @@ -271,21 +288,7 @@ public void incrementResourceCount(long accountId, ResourceType type, Long... de } final long numToIncrement = (delta.length == 0) ? 1 : delta[0].longValue(); - - 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); - } - } - }); + removeResourceReservationIfNeededAndIncrementResourceCount(accountId, type, numToIncrement); } @Override