Skip to content

Commit e7ec0d4

Browse files
vishesh92vishesh92
committed
Fix resource count discrepancies
1 parent 7243946 commit e7ec0d4

File tree

6 files changed

+114
-35
lines changed

6 files changed

+114
-35
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-41810to41820-cleanup.sql‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
2+
-- Licensed to the Apache Software Foundation (ASF) under one
3+
-- or more contributor license agreements. See the NOTICE file
4+
-- distributed with this work for additional information
5+
-- regarding copyright ownership. The ASF licenses this file
6+
-- to you under the Apache License, Version 2.0 (the
7+
-- "License"); you may not use this file except in compliance
8+
-- with the License. You may obtain a copy of the License at
9+
--
10+
-- http://www.apache.org/licenses/LICENSE-2.0
11+
--
12+
-- Unless required by applicable law or agreed to in writing,
13+
-- software distributed under the License is distributed on an
14+
-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
-- KIND, either express or implied. See the License for the
16+
-- specific language governing permissions and limitations
17+
-- under the License.
18+
19+
--;
20+
-- Schema upgrade cleanup from 4.18.0.0 to 4.18.1.0
21+
--;
22+
23+
ALTER TABLE `cloud`.`resource_reservation` MODIFY `amount` bigint unsigned NOT NULL;

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
-- Licensed to the Apache Software Foundation (ASF) under one
2+
-- or more contributor license agreements. See the NOTICE file
3+
-- distributed with this work for additional information
4+
-- regarding copyright ownership. The ASF licenses this file
5+
-- to you under the Apache License, Version 2.0 (the
6+
-- "License"); you may not use this file except in compliance
7+
-- with the License. You may obtain a copy of the License at
8+
--
9+
-- http://www.apache.org/licenses/LICENSE-2.0
10+
--
11+
-- Unless required by applicable law or agreed to in writing,
12+
-- software distributed under the License is distributed on an
13+
-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
-- KIND, either express or implied. See the License for the
15+
-- specific language governing permissions and limitations
16+
-- under the License.
17+
18+
--;
19+
-- Schema upgrade from 4.18.1.0 to 4.18.2.0
20+
--;
21+
22+
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 & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,24 @@ public void doInTransactionWithoutResult(TransactionStatus status) throws CloudR
190190
});
191191
}
192192

193+
protected void removeResourceReservationIfNeededAndDecrementResourceCount(final long accountId,
194+
final ResourceType type, final long numToDecrement) {
195+
Transaction.execute(new TransactionCallbackWithExceptionNoReturn<CloudRuntimeException>() {
196+
@Override
197+
public void doInTransactionWithoutResult(TransactionStatus status) throws CloudRuntimeException {
198+
199+
Object obj = CallContext.current().getContextParameter(String.format("%s-%s", ResourceReservation.class.getSimpleName(), type.getName()));
200+
if (obj instanceof Long) {
201+
reservationDao.remove((long) obj);
202+
}
203+
if (!updateResourceCountForAccount(accountId, type, false, numToDecrement)) {
204+
_alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_UPDATE_RESOURCE_COUNT, 0L, 0L, "Failed to decrement resource count of type " + type + " for account id=" + accountId,
205+
"Failed to decrement resource count of type " + type + " for account id=" + accountId + "; use updateResourceCount API to recalculate/fix the problem");
206+
}
207+
}
208+
});
209+
}
210+
193211
@Override
194212
public boolean start() {
195213
if (_resourceCountCheckInterval > 0) {
@@ -299,11 +317,7 @@ public void decrementResourceCount(long accountId, ResourceType type, Long... de
299317
return;
300318
}
301319
long numToDecrement = (delta.length == 0) ? 1 : delta[0].longValue();
302-
303-
if (!updateResourceCountForAccount(accountId, type, false, numToDecrement)) {
304-
_alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_UPDATE_RESOURCE_COUNT, 0L, 0L, "Failed to decrement resource count of type " + type + " for account id=" + accountId,
305-
"Failed to decrement resource count of type " + type + " for account id=" + accountId + "; use updateResourceCount API to recalculate/fix the problem");
306-
}
320+
removeResourceReservationIfNeededAndDecrementResourceCount(accountId, type, numToDecrement);
307321
}
308322

309323
@Override
@@ -941,6 +955,8 @@ public Long doInTransaction(TransactionStatus status) {
941955
newResourceCount += accountResourceCount; // add account's resource count to parent domain count
942956
}
943957
}
958+
long reservationCount = reservationDao.getDomainReservation(domainId, type);
959+
newResourceCount = newResourceCount - reservationCount;
944960
_resourceCountDao.setResourceCount(domainId, ResourceOwnerType.Domain, type, newResourceCount);
945961

946962
if (oldResourceCount != newResourceCount) {
@@ -955,7 +971,7 @@ public Long doInTransaction(TransactionStatus status) {
955971

956972
@DB
957973
protected long recalculateAccountResourceCount(final long accountId, final ResourceType type) {
958-
final Long newCount;
974+
Long newCount;
959975
if (type == Resource.ResourceType.user_vm) {
960976
newCount = _userVmDao.countAllocatedVMsForAccount(accountId, VirtualMachineManager.ResourceCountRunningVMsonly.value());
961977
} else if (type == Resource.ResourceType.volume) {
@@ -985,32 +1001,34 @@ protected long recalculateAccountResourceCount(final long accountId, final Resou
9851001
} else {
9861002
throw new InvalidParameterValueException("Unsupported resource type " + type);
9871003
}
1004+
long reservationCount = reservationDao.getAccountReservation(accountId, type);
1005+
newCount = newCount == null ? -1 * reservationCount : newCount - reservationCount;
9881006

9891007
long oldCount = 0;
9901008
final ResourceCountVO accountRC = _resourceCountDao.findByOwnerAndType(accountId, ResourceOwnerType.Account, type);
9911009
if (accountRC != null) {
9921010
oldCount = accountRC.getCount();
9931011
}
9941012

995-
if (newCount == null || !newCount.equals(oldCount)) {
1013+
if (!newCount.equals(oldCount)) {
1014+
Long finalNewCount = newCount;
9961015
Transaction.execute(new TransactionCallbackNoReturn() {
9971016
@Override
9981017
public void doInTransactionWithoutResult(TransactionStatus status) {
9991018
lockAccountAndOwnerDomainRows(accountId, type);
1000-
_resourceCountDao.setResourceCount(accountId, ResourceOwnerType.Account, type, (newCount == null) ? 0 : newCount);
1019+
_resourceCountDao.setResourceCount(accountId, ResourceOwnerType.Account, type, finalNewCount);
10011020
}
10021021
});
10031022
}
10041023

10051024
// No need to log message for primary and secondary storage because both are recalculating the
10061025
// resource count which will not lead to any discrepancy.
1007-
if (newCount != null && !newCount.equals(oldCount) &&
1008-
type != Resource.ResourceType.primary_storage && type != Resource.ResourceType.secondary_storage) {
1026+
if (!newCount.equals(oldCount) && type != ResourceType.primary_storage && type != ResourceType.secondary_storage) {
10091027
s_logger.warn("Discrepancy in the resource count " + "(original count=" + oldCount + " correct count = " + newCount + ") for type " + type +
10101028
" for account ID " + accountId + " is fixed during resource count recalculation.");
10111029
}
10121030

1013-
return (newCount == null) ? 0 : newCount;
1031+
return newCount;
10141032
}
10151033

10161034
public long countCpusForAccount(long accountId) {

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

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,23 @@
2828
import org.apache.cloudstack.reservation.dao.ReservationDao;
2929
import org.junit.Before;
3030
import org.junit.Test;
31+
import org.junit.runner.RunWith;
3132
import org.mockito.Mock;
33+
import org.powermock.api.mockito.PowerMockito;
3234
import org.powermock.core.classloader.annotations.PrepareForTest;
35+
import org.powermock.modules.junit4.PowerMockRunner;
3336

3437
import static org.junit.Assert.assertEquals;
35-
import static org.junit.Assert.assertNull;
3638
import static org.junit.Assert.fail;
3739
import static org.mockito.ArgumentMatchers.any;
3840
import static org.mockito.ArgumentMatchers.anyInt;
41+
import static org.mockito.Mockito.times;
42+
import static org.mockito.Mockito.verify;
3943
import static org.mockito.Mockito.when;
4044
import static org.mockito.MockitoAnnotations.initMocks;
4145

42-
@PrepareForTest(CheckedReservation.class)
46+
@PrepareForTest({CheckedReservation.class, GlobalLock.class})
47+
@RunWith(PowerMockRunner.class)
4348
public class CheckedReservationTest {
4449

4550
@Mock
@@ -59,44 +64,59 @@ public class CheckedReservationTest {
5964
public void setup() {
6065
initMocks(this);
6166
when(reservation.getId()).thenReturn(1l);
67+
when(reservationDao.persist(any())).thenReturn(reservation);
68+
when(account.getId()).thenReturn(1l);
69+
when(account.getDomainId()).thenReturn(4l);
70+
when(reservationDao.persist(any())).thenReturn(reservation);
71+
PowerMockito.mockStatic(GlobalLock.class);
72+
when(GlobalLock.getInternLock(any())).thenReturn(quotaLimitLock);
6273
}
6374

6475
@Test
65-
public void getId() {
66-
when(reservationDao.persist(any())).thenReturn(reservation);
67-
when(account.getAccountId()).thenReturn(1l);
68-
when(account.getDomainId()).thenReturn(4l);
76+
public void testPositiveAmount() throws ResourceAllocationException {
6977
when(quotaLimitLock.lock(anyInt())).thenReturn(true);
70-
boolean fail = false;
71-
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) {
72-
long id = cr.getId();
73-
assertEquals(1l, id);
78+
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,10l, reservationDao, resourceLimitService); ) {
79+
assertEquals(1L, cr.getAccountId().longValue());
80+
assertEquals(1L, cr.getId());
81+
assertEquals(4l, cr.getDomainId().longValue());
82+
assertEquals(Resource.ResourceType.user_vm, cr.getResourceType());
83+
assertEquals(10l, cr.getReservedAmount().longValue());
7484
} catch (NullPointerException npe) {
7585
fail("NPE caught");
7686
} catch (ResourceAllocationException rae) {
77-
// this does not work on all plafroms because of the static methods being used in the global lock mechanism
87+
// this does not work on all platforms because of the static methods being used in the global lock mechanism
7888
// normally one would
7989
// throw new CloudRuntimeException(rae);
8090
// but we'll ignore this for platforms that can not humour the static bits of the system.
8191
} catch (Exception e) {
8292
throw new RuntimeException(e);
8393
}
94+
verify(resourceLimitService, times(1)).checkResourceLimit(account, Resource.ResourceType.user_vm, 10l);
8495
}
8596

8697
@Test
87-
public void getNoAmount() {
88-
when(reservationDao.persist(any())).thenReturn(reservation);
89-
when(account.getAccountId()).thenReturn(1l);
90-
boolean fail = false;
98+
public void testNegativeAmount() throws ResourceAllocationException {
99+
when(quotaLimitLock.lock(anyInt())).thenReturn(true);
91100
try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,-11l, reservationDao, resourceLimitService); ) {
92101
Long amount = cr.getReservedAmount();
93-
assertNull(amount);
102+
assertEquals(1L, cr.getAccountId().longValue());
103+
assertEquals(1L, cr.getId());
104+
assertEquals(4l, cr.getDomainId().longValue());
105+
assertEquals(Resource.ResourceType.cpu, cr.getResourceType());
106+
assertEquals(-11l, cr.getReservedAmount().longValue());
94107
} catch (NullPointerException npe) {
95108
fail("NPE caught");
96109
} catch (ResourceAllocationException rae) {
97110
throw new CloudRuntimeException(rae);
98111
} catch (Exception e) {
99112
throw new RuntimeException(e);
100113
}
114+
verify(resourceLimitService, times(0)).checkResourceLimit(account, Resource.ResourceType.user_vm, 1l);
115+
}
116+
117+
@Test(expected = ResourceAllocationException.class)
118+
public void testLockTimeout() throws ResourceAllocationException {
119+
when(quotaLimitLock.lock(anyInt())).thenReturn(false);
120+
CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,1l, reservationDao, resourceLimitService);
101121
}
102122
}

0 commit comments

Comments
 (0)