Skip to content

Commit

Permalink
Fix resource count discrepancies (apache#8302)
Browse files Browse the repository at this point in the history 
* Fix resource count discrepancies

* Fixup while removing vm

* Fix discrepancies when starting VMs

* Fixup tests

* Fix failing tests

* Don't take lock when amount is negative

---------

Co-authored-by: dahn <[email protected]>
  • Loading branch information
@vishesh92 @DaanHoogland
vishesh92 and DaanHoogland authored Mar 13, 2024
1 parent 6dc3d06 commit e87c6cf
Show file tree
Hide file tree
Showing 18 changed files with 398 additions and 154 deletions.
2 changes: 2 additions & 0 deletions api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@

Resource.ResourceType getResourceType();

Long getResourceId();

String getTag();

Long getReservedAmount();
Expand Down
53 changes: 33 additions & 20 deletions engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the Apacohe Software Foundation (ASF) under one
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
Expand Down Expand Up @@ -49,6 +49,7 @@
import javax.naming.ConfigurationException;
import javax.persistence.EntityExistsException;

import com.cloud.configuration.Resource;
import com.cloud.domain.Domain;
import com.cloud.domain.dao.DomainDao;
import com.cloud.network.vpc.VpcVO;
Expand Down Expand Up @@ -87,6 +88,7 @@
import org.apache.cloudstack.framework.messagebus.MessageHandler;
import org.apache.cloudstack.jobs.JobInfo;
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.cloudstack.reservation.dao.ReservationDao;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.apache.cloudstack.storage.to.VolumeObjectTO;
Expand Down Expand Up @@ -296,6 +298,8 @@ public class VirtualMachineManagerImpl extends ManagerBase implements VirtualMac
@Inject
private VMInstanceDao _vmDao;
@Inject
private ReservationDao _reservationDao;
@Inject
private ServiceOfferingDao _offeringDao;
@Inject
private DiskOfferingDao _diskOfferingDao;
Expand Down Expand Up @@ -914,7 +918,7 @@ protected boolean checkWorkItems(final VMInstanceVO vm, final State state) throw

@DB
protected Ternary<VMInstanceVO, ReservationContext, ItWorkVO> changeToStartState(final VirtualMachineGuru vmGuru, final VMInstanceVO vm, final User caller,
final Account account) throws ConcurrentOperationException {
final Account account, Account owner, ServiceOfferingVO offering, VirtualMachineTemplate template) throws ConcurrentOperationException {
final long vmId = vm.getId();

ItWorkVO work = new ItWorkVO(UUID.randomUUID().toString(), _nodeId, State.Starting, vm.getType(), vm.getId());
Expand All @@ -934,6 +938,9 @@ public Ternary<VMInstanceVO, ReservationContext, ItWorkVO> doInTransaction(final
if (logger.isDebugEnabled()) {
logger.debug("Successfully transitioned to start state for " + vm + " reservation id = " + work.getId());
}
if (VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
_resourceLimitMgr.incrementVmResourceCount(owner.getAccountId(), vm.isDisplay(), offering, template);
}
return new Ternary<>(vm, context, work);
}

Expand Down Expand Up @@ -1126,7 +1133,10 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil

final VirtualMachineGuru vmGuru = getVmGuru(vm);

final Ternary<VMInstanceVO, ReservationContext, ItWorkVO> start = changeToStartState(vmGuru, vm, caller, account);
final Account owner = _entityMgr.findById(Account.class, vm.getAccountId());
final ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
final VirtualMachineTemplate template = _entityMgr.findByIdIncludingRemoved(VirtualMachineTemplate.class, vm.getTemplateId());
final Ternary<VMInstanceVO, ReservationContext, ItWorkVO> start = changeToStartState(vmGuru, vm, caller, account, owner, offering, template);
if (start == null) {
return;
}
Expand All @@ -1136,8 +1146,6 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil
ItWorkVO work = start.third();

VMInstanceVO startedVm = null;
final ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
final VirtualMachineTemplate template = _entityMgr.findByIdIncludingRemoved(VirtualMachineTemplate.class, vm.getTemplateId());

DataCenterDeployment plan = new DataCenterDeployment(vm.getDataCenterId(), vm.getPodIdToDeployIn(), null, null, null, null, ctx);
if (planToDeploy != null && planToDeploy.getDataCenterId() != 0) {
Expand All @@ -1150,12 +1158,6 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil

final HypervisorGuru hvGuru = _hvGuruMgr.getGuru(vm.getHypervisorType());

// check resource count if ResourceCountRunningVMsonly.value() = true
final Account owner = _entityMgr.findById(Account.class, vm.getAccountId());
if (VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
_resourceLimitMgr.incrementVmResourceCount(owner.getAccountId(), vm.isDisplay(), offering, template);
}

boolean canRetry = true;
ExcludeList avoids = null;
try {
Expand Down Expand Up @@ -2277,16 +2279,21 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
_workDao.update(work.getId(), work);
}

boolean result = stateTransitTo(vm, Event.OperationSucceeded, null);
if (result) {
vm.setPowerState(PowerState.PowerOff);
_vmDao.update(vm.getId(), vm);
if (VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
_resourceLimitMgr.decrementVmResourceCount(vm.getAccountId(), vm.isDisplay(), offering, template);
boolean result = Transaction.execute(new TransactionCallbackWithException<Boolean, NoTransitionException>() {
@Override
public Boolean doInTransaction(TransactionStatus status) throws NoTransitionException {
boolean result = stateTransitTo(vm, Event.OperationSucceeded, null);

if (result && VirtualMachine.Type.User.equals(vm.type) && ResourceCountRunningVMsonly.value()) {
ServiceOfferingVO offering = _offeringDao.findById(vm.getId(), vm.getServiceOfferingId());
VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId());
_resourceLimitMgr.decrementVmResourceCount(vm.getAccountId(), vm.isDisplay(), offering, template);
}
return result;
}
} else {
});

if (!result) {
throw new CloudRuntimeException("unable to stop " + vm);
}
} catch (final NoTransitionException e) {
Expand Down Expand Up @@ -2319,6 +2326,12 @@ public boolean stateTransitTo(final VirtualMachine vm1, final VirtualMachine.Eve
vm.setLastHostId(vm.getHostId());
}
}

if (e.equals(VirtualMachine.Event.DestroyRequested) || e.equals(VirtualMachine.Event.ExpungeOperation)) {
_reservationDao.setResourceId(Resource.ResourceType.user_vm, null);
_reservationDao.setResourceId(Resource.ResourceType.cpu, null);
_reservationDao.setResourceId(Resource.ResourceType.memory, null);
}
return _stateMachine.transitTo(vm, e, new Pair<>(vm.getHostId(), hostId), _vmDao);
}

Expand Down
10 changes: 8 additions & 2 deletions engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,7 @@ public void checkIfVmNetworkDetailsReturnedIsCorrect() {
public void testOrchestrateStartNonNullPodId() throws Exception {
VMInstanceVO vmInstance = new VMInstanceVO();
ReflectionTestUtils.setField(vmInstance, "id", 1L);
ReflectionTestUtils.setField(vmInstance, "accountId", 1L);
ReflectionTestUtils.setField(vmInstance, "uuid", "vm-uuid");
ReflectionTestUtils.setField(vmInstance, "serviceOfferingId", 2L);
ReflectionTestUtils.setField(vmInstance, "instanceName", "myVm");
Expand All @@ -1019,6 +1020,7 @@ public void testOrchestrateStartNonNullPodId() throws Exception {
User user = mock(User.class);

Account account = mock(Account.class);
Account owner = mock(Account.class);

ReservationContext ctx = mock(ReservationContext.class);

Expand All @@ -1042,12 +1044,13 @@ public void testOrchestrateStartNonNullPodId() throws Exception {
doReturn(vmGuru).when(virtualMachineManagerImpl).getVmGuru(vmInstance);

Ternary<VMInstanceVO, ReservationContext, ItWorkVO> start = new Ternary<>(vmInstance, ctx, work);
Mockito.doReturn(start).when(virtualMachineManagerImpl).changeToStartState(vmGuru, vmInstance, user, account);
Mockito.doReturn(start).when(virtualMachineManagerImpl).changeToStartState(vmGuru, vmInstance, user, account, owner, serviceOffering, template);

when(ctx.getJournal()).thenReturn(Mockito.mock(Journal.class));

when(serviceOfferingDaoMock.findById(vmInstance.getId(), vmInstance.getServiceOfferingId())).thenReturn(serviceOffering);

when(_entityMgr.findById(Account.class, vmInstance.getAccountId())).thenReturn(owner);
when(_entityMgr.findByIdIncludingRemoved(VirtualMachineTemplate.class, vmInstance.getTemplateId())).thenReturn(template);

Host destHost = mock(Host.class);
Expand Down Expand Up @@ -1099,6 +1102,7 @@ public void testOrchestrateStartNonNullPodId() throws Exception {
public void testOrchestrateStartNullPodId() throws Exception {
VMInstanceVO vmInstance = new VMInstanceVO();
ReflectionTestUtils.setField(vmInstance, "id", 1L);
ReflectionTestUtils.setField(vmInstance, "accountId", 1L);
ReflectionTestUtils.setField(vmInstance, "uuid", "vm-uuid");
ReflectionTestUtils.setField(vmInstance, "serviceOfferingId", 2L);
ReflectionTestUtils.setField(vmInstance, "instanceName", "myVm");
Expand All @@ -1112,6 +1116,7 @@ public void testOrchestrateStartNullPodId() throws Exception {
User user = mock(User.class);

Account account = mock(Account.class);
Account owner = mock(Account.class);

ReservationContext ctx = mock(ReservationContext.class);

Expand All @@ -1135,12 +1140,13 @@ public void testOrchestrateStartNullPodId() throws Exception {
doReturn(vmGuru).when(virtualMachineManagerImpl).getVmGuru(vmInstance);

Ternary<VMInstanceVO, ReservationContext, ItWorkVO> start = new Ternary<>(vmInstance, ctx, work);
Mockito.doReturn(start).when(virtualMachineManagerImpl).changeToStartState(vmGuru, vmInstance, user, account);
Mockito.doReturn(start).when(virtualMachineManagerImpl).changeToStartState(vmGuru, vmInstance, user, account, owner, serviceOffering, template);

when(ctx.getJournal()).thenReturn(Mockito.mock(Journal.class));

when(serviceOfferingDaoMock.findById(vmInstance.getId(), vmInstance.getServiceOfferingId())).thenReturn(serviceOffering);

when(_entityMgr.findById(Account.class, vmInstance.getAccountId())).thenReturn(owner);
when(_entityMgr.findByIdIncludingRemoved(VirtualMachineTemplate.class, vmInstance.getTemplateId())).thenReturn(template);

Host destHost = mock(Host.class);
Expand Down
1 change: 1 addition & 0 deletions engine/schema/src/main/java/com/cloud/storage/VolumeVO.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ public class VolumeVO implements Volume {
@Column(name = "encrypt_format")
private String encryptFormat;


// Real Constructor
public VolumeVO(Type type, String name, long dcId, long domainId,
long accountId, long diskOfferingId, Storage.ProvisioningType provisioningType, long size,
Expand Down
35 changes: 34 additions & 1 deletion engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.stream.Collectors;

import javax.inject.Inject;

import com.cloud.configuration.Resource;
import com.cloud.utils.db.Transaction;
import com.cloud.utils.db.TransactionCallback;
import org.apache.cloudstack.reservation.ReservationVO;
import org.apache.cloudstack.reservation.dao.ReservationDao;
import org.apache.commons.collections.CollectionUtils;
import org.springframework.stereotype.Component;

Expand Down Expand Up @@ -71,6 +77,8 @@ public class VolumeDaoImpl extends GenericDaoBase<VolumeVO, Long> implements Vol
protected GenericSearchBuilder<VolumeVO, SumCount> secondaryStorageSearch;
private final SearchBuilder<VolumeVO> poolAndPathSearch;
@Inject
ReservationDao reservationDao;
@Inject
ResourceTagDao _tagsDao;

protected static final String SELECT_VM_SQL = "SELECT DISTINCT instance_id from volumes v where v.host_id = ? and v.mirror_state = ?";
Expand Down Expand Up @@ -443,6 +451,7 @@ public VolumeDaoImpl() {
CountByAccount.and("account", CountByAccount.entity().getAccountId(), SearchCriteria.Op.EQ);
CountByAccount.and("state", CountByAccount.entity().getState(), SearchCriteria.Op.NIN);
CountByAccount.and("displayVolume", CountByAccount.entity().isDisplayVolume(), Op.EQ);
CountByAccount.and("idNIN", CountByAccount.entity().getId(), Op.NIN);
CountByAccount.done();

primaryStorageSearch = createSearchBuilder(SumCount.class);
Expand All @@ -454,6 +463,7 @@ public VolumeDaoImpl() {
primaryStorageSearch.and("displayVolume", primaryStorageSearch.entity().isDisplayVolume(), Op.EQ);
primaryStorageSearch.and("isRemoved", primaryStorageSearch.entity().getRemoved(), Op.NULL);
primaryStorageSearch.and("NotCountStates", primaryStorageSearch.entity().getState(), Op.NIN);
primaryStorageSearch.and("idNIN", primaryStorageSearch.entity().getId(), Op.NIN);
primaryStorageSearch.done();

primaryStorageSearch2 = createSearchBuilder(SumCount.class);
Expand All @@ -468,6 +478,7 @@ public VolumeDaoImpl() {
primaryStorageSearch2.and("displayVolume", primaryStorageSearch2.entity().isDisplayVolume(), Op.EQ);
primaryStorageSearch2.and("isRemoved", primaryStorageSearch2.entity().getRemoved(), Op.NULL);
primaryStorageSearch2.and("NotCountStates", primaryStorageSearch2.entity().getState(), Op.NIN);
primaryStorageSearch2.and("idNIN", primaryStorageSearch2.entity().getId(), Op.NIN);
primaryStorageSearch2.done();

secondaryStorageSearch = createSearchBuilder(SumCount.class);
Expand Down Expand Up @@ -506,15 +517,24 @@ public Pair<Long, Long> getCountAndTotalByPool(long poolId) {

@Override
public Long countAllocatedVolumesForAccount(long accountId) {
List<ReservationVO> reservations = reservationDao.getReservationsForAccount(accountId, Resource.ResourceType.volume, null);
List<Long> reservedResourceIds = reservations.stream().filter(reservation -> reservation.getReservedAmount() > 0).map(ReservationVO::getResourceId).collect(Collectors.toList());

SearchCriteria<Long> sc = CountByAccount.create();
sc.setParameters("account", accountId);
sc.setParameters("state", Volume.State.Destroy, Volume.State.Expunged);
sc.setParameters("state", State.Destroy, State.Expunged);
sc.setParameters("displayVolume", 1);
if (CollectionUtils.isNotEmpty(reservedResourceIds)) {
sc.setParameters("idNIN", reservedResourceIds.toArray());
}
return customSearch(sc, null).get(0);
}

@Override
public long primaryStorageUsedForAccount(long accountId, List<Long> virtualRouters) {
List<ReservationVO> reservations = reservationDao.getReservationsForAccount(accountId, Resource.ResourceType.volume, null);
List<Long> reservedResourceIds = reservations.stream().filter(reservation -> reservation.getReservedAmount() > 0).map(ReservationVO::getResourceId).collect(Collectors.toList());

SearchCriteria<SumCount> sc;
if (!virtualRouters.isEmpty()) {
sc = primaryStorageSearch2.create();
Expand All @@ -526,6 +546,9 @@ public long primaryStorageUsedForAccount(long accountId, List<Long> virtualRoute
sc.setParameters("states", State.Allocated);
sc.setParameters("NotCountStates", State.Destroy, State.Expunged);
sc.setParameters("displayVolume", 1);
if (CollectionUtils.isNotEmpty(reservedResourceIds)) {
sc.setParameters("idNIN", reservedResourceIds.toArray());
}
List<SumCount> storageSpace = customSearch(sc, null);
if (storageSpace != null) {
return storageSpace.get(0).sum;
Expand Down Expand Up @@ -863,4 +886,14 @@ public List<VolumeVO> listAllocatedVolumesForAccountDiskOfferingIdsAndNotForVms(
}
return listBy(sc);
}

@Override
public VolumeVO persist(VolumeVO entity) {
return Transaction.execute((TransactionCallback<VolumeVO>) status -> {
VolumeVO volume = super.persist(entity);
reservationDao.setResourceId(Resource.ResourceType.volume, volume.getId());
reservationDao.setResourceId(Resource.ResourceType.primary_storage, volume.getId());
return volume;
});
}
}
Loading

0 comments on commit e87c6cf

Please sign in to comment.