Skip to content

Fix resource count discrepancies #8302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yadvr merged 9 commits into from
Mar 13, 2024
Merged

Fix resource count discrepancies #8302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c6f1ce
Fix resource count discrepancies
vishesh92 Dec 11, 2023
e3ef2a9
Fixup while removing vm
vishesh92 Feb 12, 2024
1adf986
Fix discrepancies when starting VMs
vishesh92 Feb 13, 2024
7b38fe2
Fixup tests
vishesh92 Feb 13, 2024
a8cfdbe
Fix failing tests
vishesh92 Feb 21, 2024
e990eac
Merge branch 'main' into fix-resource-count-discrepancies
vishesh92 Feb 26, 2024
509f0d3
Merge branch 'main' into fix-resource-count-discrepancies
DaanHoogland Feb 26, 2024
3eb213a
Don't take lock when amount is negative
vishesh92 Feb 29, 2024
7dcc999
Merge remote-tracking branch 'origin/main' into fix-resource-count-di…
vishesh92 Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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