Skip to content

Commit 61488bc

Browse files
vishesh92vishesh92
committed
Fix resource count discrepancies
1 parent a15b706 commit 61488bc

File tree

5 files changed

+74
-33
lines changed

5 files changed

+74
-33
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ protected ReservationVO()
5454
{}
5555

5656
public ReservationVO(Long accountId, Long domainId, Resource.ResourceType resourceType, Long delta) {
57-
if (delta == null || delta <= 0) {
57+
if (delta == null) {
5858
throw new CloudRuntimeException("resource reservations can not be made for no resources");
5959
}
6060
this.accountId = accountId;

‎engine/schema/src/main/resources/META-INF/db/schema-41810to41900.sql‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,3 +314,5 @@ CREATE TABLE `cloud_usage`.`bucket_statistics` (
314314
`size` bigint unsigned COMMENT 'total size of bucket objects',
315315
PRIMARY KEY(`id`)
316316
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
317+
318+
ALTER TABLE `cloud`.`resource_reservation` MODIFY `amount` bigint signed NOT NULL;

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

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,17 +63,13 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun
6363
this.amount = amount;
6464
this.reservation = null;
6565
setGlobalLock(account, resourceType);
66-
if (this.amount != null && this.amount <= 0) {
67-
if(LOG.isDebugEnabled()){
68-
LOG.debug(String.format("not reserving no amount of resources for %s in domain %d, type: %s, %s ", account.getAccountName(), account.getDomainId(), resourceType, amount));
69-
}
70-
this.amount = null;
71-
}
7266

7367
if (this.amount != null) {
7468
if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) {
7569
try {
76-
resourceLimitService.checkResourceLimit(account,resourceType,amount);
70+
if (amount > 0) {
71+
resourceLimitService.checkResourceLimit(account, resourceType, amount);
72+
}
7773
ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount);
7874
this.reservation = reservationDao.persist(reservationVO);
7975
CallContext.current().putContextParameter(getContextParameterKey(), reservation.getId());

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

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,23 @@ public void doInTransactionWithoutResult(TransactionStatus status) throws CloudR
197197
});
198198
}
199199

200+
protected void removeResourceReservationIfNeededAndDecrementResourceCount(final long accountId, final ResourceType type, final long numToDecrement) {
201+
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() {
202+
@Override
203+
public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException {
204+
205+
Object obj = CallContext.current().getContextParameter(String.format("%s-%s", ResourceReservation.class.getSimpleName(), type.getName()));
206+
if (obj instanceof Long) {
207+
reservationDao.remove((long)obj);
208+
}
209+
if (!updateResourceCountForAccount(accountId, type, false, numToDecrement)) {
210+
_alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_UPDATE_RESOURCE_COUNT, 0L, 0L, "Failed to decrement resource count of type " + type + " for account id=" + accountId,
211+
"Failed to decrement resource count of type " + type + " for account id=" + accountId + "; use updateResourceCount API to recalculate/fix the problem");
212+
}
213+
}
214+
});
215+
}
216+
200217
@Override
201218
public boolean start() {
202219
if (_resourceCountCheckInterval > 0) {
@@ -307,10 +324,7 @@ public void decrementResourceCount(long accountId, ResourceType type, Long... de
307324
}
308325
long numToDecrement = (delta.length == 0) ? 1 : delta[0].longValue();
309326

310-
if (!updateResourceCountForAccount(accountId, type, false, numToDecrement)) {
311-
_alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_UPDATE_RESOURCE_COUNT, 0L, 0L, "Failed to decrement resource count of type " + type + " for account id=" + accountId,
312-
"Failed to decrement resource count of type " + type + " for account id=" + accountId + "; use updateResourceCount API to recalculate/fix the problem");
313-
}
327+
removeResourceReservationIfNeededAndDecrementResourceCount(accountId, type, numToDecrement);
314328
}
315329

316330
@Override
@@ -939,6 +953,8 @@ public Long doInTransaction(TransactionStatus status) {
939953
long accountResourceCount = recalculateAccountResourceCount(account.getId(), type);
940954
newResourceCount += accountResourceCount; // add account's resource count to parent domain count
941955
}
956+
long reservationCount = reservationDao.getDomainReservation(domainId, type);
957+
newResourceCount = newResourceCount - reservationCount;
942958
_resourceCountDao.setResourceCount(domainId, ResourceOwnerType.Domain, type, newResourceCount);
943959

944960
if (oldResourceCount != newResourceCount) {
@@ -953,7 +969,7 @@ public Long doInTransaction(TransactionStatus status) {
953969

954970
@DB
955971
protected long recalculateAccountResourceCount(final long accountId, final ResourceType type) {
956-
final Long newCount;
972+
Long newCount;
957973
if (type == Resource.ResourceType.user_vm) {
958974
newCount = _userVmDao.countAllocatedVMsForAccount(accountId, VirtualMachineManager.ResourceCountRunningVMsonly.value());
959975
} else if (type == Resource.ResourceType.volume) {
@@ -984,31 +1000,34 @@ protected long recalculateAccountResourceCount(final long accountId, final Resou
9841000
throw new InvalidParameterValueException("Unsupported resource type " + type);
9851001
}
9861002

1003+
long reservationCount = reservationDao.getAccountReservation(accountId, type);
1004+
newCount = newCount == null ? -1 * reservationCount : newCount - reservationCount;
1005+
9871006
long oldCount = 0;
9881007
final ResourceCountVO accountRC = _resourceCountDao.findByOwnerAndType(accountId, ResourceOwnerType.Account, type);
9891008
if (accountRC != null) {
9901009
oldCount = accountRC.getCount();
9911010
}
9921011

993-
if (newCount == null || !newCount.equals(oldCount)) {
1012+
if (!newCount.equals(oldCount)) {
1013+
Long finalNewCount = newCount;
9941014
Transaction.execute(new TransactionCallbackNoReturn() {
9951015
@Override
9961016
public void doInTransactionWithoutResult(TransactionStatus status) {
9971017
lockAccountAndOwnerDomainRows(accountId, type);
998-
_resourceCountDao.setResourceCount(accountId, ResourceOwnerType.Account, type, (newCount == null) ? 0 : newCount);
1018+
_resourceCountDao.setResourceCount(accountId, ResourceOwnerType.Account, type, finalNewCount);
9991019
}
10001020
});
10011021
}
10021022

10031023
// No need to log message for primary and secondary storage because both are recalculating the
10041024
// resource count which will not lead to any discrepancy.
1005-
if (newCount != null && !newCount.equals(oldCount) &&
1006-
type != Resource.ResourceType.primary_storage && type != Resource.ResourceType.secondary_storage) {
1025+
if (!newCount.equals(oldCount) && type != ResourceType.primary_storage && type != ResourceType.secondary_storage) {
10071026
s_logger.warn("Discrepancy in the resource count " + "(original count=" + oldCount + " correct count = " + newCount + ") for type " + type +
10081027
" for account ID " + accountId + " is fixed during resource count recalculation.");
10091028
}
10101029

1011-
return (newCount == null) ? 0 : newCount;
1030+
return newCount;
10121031
}
10131032

10141033
public long countCpusForAccount(long accountId) {

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

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,17 @@
3131
import org.junit.Test;
3232
import org.junit.runner.RunWith;
3333
import org.mockito.Mock;
34-
import org.mockito.Mockito;
34+
import org.mockito.MockedStatic;
3535
import org.mockito.MockitoAnnotations;
3636
import org.mockito.junit.MockitoJUnitRunner;
3737

3838
import static org.junit.Assert.assertEquals;
39-
import static org.junit.Assert.assertNull;
4039
import static org.junit.Assert.fail;
41-
import static org.mockito.Mockito.lenient;
40+
import static org.mockito.ArgumentMatchers.any;
41+
import static org.mockito.ArgumentMatchers.anyInt;
42+
import static org.mockito.Mockito.mockStatic;
43+
import static org.mockito.Mockito.times;
44+
import static org.mockito.Mockito.verify;
4245
import static org.mockito.Mockito.when;
4346

4447
@RunWith(MockitoJUnitRunner.class)
@@ -59,25 +62,35 @@ public class CheckedReservationTest {
5962

6063
private AutoCloseable closeable;
6164

65+
private MockedStatic<GlobalLock> globalLockMocked;
66+
6267
@Before
6368
public void setup() {
6469
closeable = MockitoAnnotations.openMocks(this);
70+
when(reservation.getId()).thenReturn(1l);
71+
when(reservationDao.persist(any())).thenReturn(reservation);
72+
when(account.getId()).thenReturn(1l);
73+
when(account.getDomainId()).thenReturn(4l);
74+
when(reservationDao.persist(any())).thenReturn(reservation);
75+
globalLockMocked = mockStatic(GlobalLock.class);
76+
when(GlobalLock.getInternLock(any())).thenReturn(quotaLimitLock);
6577
}
6678

6779
@After
6880
public void tearDown() throws Exception {
6981
closeable.close();
82+
globalLockMocked.close();
7083
}
7184

7285
@Test
73-
public void getId() {
74-
when(account.getDomainId()).thenReturn(4l);
75-
// Some weird behaviour depending on whether the database is up or not.
76-
lenient().when(reservationDao.persist(Mockito.any())).thenReturn(reservation);
77-
lenient().when(reservation.getId()).thenReturn(1L);
78-
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService) ) {
79-
long id = cr.getId();
80-
assertEquals(1L, id);
86+
public void testPositiveAmount() {
87+
when(quotaLimitLock.lock(anyInt())).thenReturn(true);
88+
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm, 10l, reservationDao, resourceLimitService)) {
89+
assertEquals(1L, cr.getAccountId().longValue());
90+
assertEquals(1L, cr.getId());
91+
assertEquals(4l, cr.getDomainId().longValue());
92+
assertEquals(Resource.ResourceType.user_vm, cr.getResourceType());
93+
assertEquals(10l, cr.getReservedAmount().longValue());
8194
} catch (NullPointerException npe) {
8295
fail("NPE caught");
8396
} catch (ResourceAllocationException rae) {
@@ -91,16 +104,27 @@ public void getId() {
91104
}
92105

93106
@Test
94-
public void getNoAmount() {
95-
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,-11l, reservationDao, resourceLimitService) ) {
96-
Long amount = cr.getReservedAmount();
97-
assertNull(amount);
107+
public void testNegativeAmount() throws ResourceAllocationException {
108+
when(quotaLimitLock.lock(anyInt())).thenReturn(true);
109+
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu, -11l, reservationDao, resourceLimitService)) {
110+
assertEquals(1L, cr.getAccountId().longValue());
111+
assertEquals(1L, cr.getId());
112+
assertEquals(4l, cr.getDomainId().longValue());
113+
assertEquals(Resource.ResourceType.cpu, cr.getResourceType());
114+
assertEquals(-11l, cr.getReservedAmount().longValue());
98115
} catch (NullPointerException npe) {
99116
fail("NPE caught");
100117
} catch (ResourceAllocationException rae) {
101118
throw new CloudRuntimeException(rae);
102119
} catch (Exception e) {
103120
throw new RuntimeException(e);
104121
}
122+
verify(resourceLimitService, times(0)).checkResourceLimit(account, Resource.ResourceType.user_vm, 1l);
123+
}
124+
125+
@Test(expected = ResourceAllocationException.class)
126+
public void testLockTimeout() throws ResourceAllocationException {
127+
when(quotaLimitLock.lock(anyInt())).thenReturn(false);
128+
CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu, 1l, reservationDao, resourceLimitService);
105129
}
106130
}

0 commit comments

Comments
 (0)