From a9580520f9fa944029feda2a3b0268b250f708e2 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 24 Aug 2022 13:44:57 +0200 Subject: [PATCH 01/19] reservation table --- .../com/cloud/user/ResourceLimitService.java | 25 ++++++++- .../cloudstack/acl/APILimitChecker.java | 30 ----------- .../cloudstack/reservation/ReservationVO.java | 54 +++++++++++++++++++ .../reservation/dao/ReservationDao.java | 4 ++ .../reservation/dao/ReservationDaoImpl.java | 10 ++++ .../META-INF/db/schema-41710to41800.sql | 11 ++++ .../resourcelimit/CheckedReservation.java | 49 +++++++++++++++++ .../ResourceLimitManagerImpl.java | 12 ++++- .../java/com/cloud/vm/UserVmManagerImpl.java | 2 +- .../vpc/MockResourceLimitManagerImpl.java | 6 +++ 10 files changed, 168 insertions(+), 35 deletions(-) delete mode 100644 api/src/main/java/org/apache/cloudstack/acl/APILimitChecker.java create mode 100644 engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java create mode 100644 engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java create mode 100644 engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java create mode 100644 server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java diff --git a/api/src/main/java/com/cloud/user/ResourceLimitService.java b/api/src/main/java/com/cloud/user/ResourceLimitService.java index 886cd23c6390..f8155be20ba5 100644 --- a/api/src/main/java/com/cloud/user/ResourceLimitService.java +++ b/api/src/main/java/com/cloud/user/ResourceLimitService.java @@ -91,7 +91,7 @@ public interface ResourceLimitService { /** * This call should be used when we have already queried resource limit for an account. This is to handle * some corner cases where queried limit may be null. - * @param accountType + * @param accountId * @param limit * @param type * @return @@ -102,7 +102,7 @@ public interface ResourceLimitService { * Finds the resource limit for a specified domain and type. If the domain has an infinite limit, will check * up the domain hierarchy * - * @param account + * @param domain * @param type * @return resource limit */ @@ -197,4 +197,25 @@ public interface ResourceLimitService { * @param delta */ void decrementResourceCount(long accountId, ResourceType type, Boolean displayResource, Long... delta); + + /** + * Adds a reservation that will be counted in subsequent calls to {count}getResourceCount{code} until {code}this[code} + * is closed. It will create a reservation record that will be counted when resource limits are checked. + * @param account The account for which the reservation is. + * @param displayResource whether this resource is shown to users at all (if not it is not counted to limits) + * @param type resource type + * @param delta amount to reserve (will not be <+ 0) + * @return a {code}AutoClosable{Code} object representing the resource the user needs + */ + ResourceReservation getReservation(Account account, Boolean displayResource, ResourceType type, Long delta); + + /** + * an interface defining an {code}AutoClosable{code} reservation object + */ + interface ResourceReservation { + + Long getAccountId(); + ResourceType getResourceType(); + Long getReservedAmount(); + } } diff --git a/api/src/main/java/org/apache/cloudstack/acl/APILimitChecker.java b/api/src/main/java/org/apache/cloudstack/acl/APILimitChecker.java deleted file mode 100644 index 110742c059dc..000000000000 --- a/api/src/main/java/org/apache/cloudstack/acl/APILimitChecker.java +++ /dev/null @@ -1,30 +0,0 @@ -// 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 -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. -package org.apache.cloudstack.acl; - -import org.apache.cloudstack.api.ServerApiException; - -import com.cloud.user.Account; -import com.cloud.utils.component.Adapter; - -/** - * APILimitChecker checks if we should block an API request based on pre-set account based api limit. - */ -public interface APILimitChecker extends Adapter { - // Interface for checking if the account is over its api limit - void checkLimit(Account account) throws ServerApiException; -} diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java new file mode 100644 index 000000000000..5949851873cd --- /dev/null +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -0,0 +1,54 @@ +package org.apache.cloudstack.reservation; + +import com.cloud.configuration.Resource; +import com.cloud.user.ResourceLimitService; +import com.cloud.utils.exception.CloudRuntimeException; + +import javax.persistence.Column; +import javax.persistence.Entity; +import javax.persistence.GeneratedValue; +import javax.persistence.GenerationType; +import javax.persistence.Id; +import javax.persistence.Table; + +@Entity +@Table(name = "resource_reservation") +public class ReservationVO implements ResourceLimitService.ResourceReservation { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + @Column(name = "id") + Long id; + + @Column(name = "account_id") + long accountId; + + @Column(name = "resource_type", nullable = false) + Resource.ResourceType type; + + @Column(name = "account_id") + long amount; + + protected ReservationVO(Long accountId, Resource.ResourceType type, Long delta) { + if (delta == null || delta <= 0) { + throw new CloudRuntimeException("resource reservations can not be made for no resources"); + } + this.accountId = accountId; + this.type = type; + this.amount = delta; + } + + @Override + public Long getAccountId() { + return accountId; + } + + @Override + public Resource.ResourceType getResourceType() { + return type; + } + + @Override + public Long getReservedAmount() { + return null; + } +} diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java new file mode 100644 index 000000000000..782daa4997d8 --- /dev/null +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java @@ -0,0 +1,4 @@ +package org.apache.cloudstack.reservation.dao; + +public interface ReservationDao { +} diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java new file mode 100644 index 000000000000..137bc54cbb89 --- /dev/null +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -0,0 +1,10 @@ +package org.apache.cloudstack.reservation.dao; + +import com.cloud.utils.db.GenericDaoBase; + +public class ReservationDaoImpl extends GenericDaoBase implements ReservationDao { + + public ReservationDaoImpl() { + + } +} diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql b/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql index 4ec812cc1c0a..a1e6ff2d2d2d 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql @@ -27,3 +27,14 @@ WHERE so.default_use = 1 AND so.vm_type IN ('domainrouter', 'secondarystoragevm' -- Add cidr_list column to load_balancing_rules ALTER TABLE `cloud`.`load_balancing_rules` ADD cidr_list VARCHAR(4096); + +-- savely add resources in parallel +-- PR#5984 Create table to persist VM stats. +DROP TABLE IF EXISTS `cloud`.`resource_reservation`; +CREATE TABLE `cloud`.`resource_reservation` ( + `id` bigint unsigned NOT NULL auto_increment COMMENT 'id', + `account_id` bigint unsigned NOT NULL, + `resource_type` varchar(255) NOT NULL, + `amount` bigint unsigned NOT NULL, + PRIMARY KEY (`id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java new file mode 100644 index 000000000000..33b18fe0e57f --- /dev/null +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -0,0 +1,49 @@ +package com.cloud.resourcelimit; + +import com.cloud.configuration.Resource; +import com.cloud.user.Account; +import com.cloud.user.ResourceLimitService; +import com.cloud.utils.exception.CloudRuntimeException; + +public class CheckedReservation implements AutoCloseable, ResourceLimitService.ResourceReservation { + private final Account account; + private final Resource.ResourceType type; + private final Long amount; + + public CheckedReservation(Account account, Resource.ResourceType type, Long amount) { + if (amount == null || amount <= 0) { + throw new CloudRuntimeException("resource reservations can not be made for no resources"); + } + // synchronised: + // - check if adding a reservation is allowed + // - create DB entry for reservation + this.account = account; + this.type = type; + this.amount = amount; + } + + @Override + public void close() throws Exception { + // delete the reservation vo + + } + + public Account getAccount() { + return account; + } + + @Override + public Long getAccountId() { + return account.getId(); + } + + @Override + public Resource.ResourceType getResourceType() { + return null; + } + + @Override + public Long getReservedAmount() { + return null; + } +} diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 3afc47f418b9..16fb3daf2339 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -1060,7 +1060,7 @@ private boolean isDisplayFlagOn(Boolean displayResource) { // 1. If its null assume displayResource = 1 // 2. If its not null then send true if displayResource = 1 - return (displayResource == null) || (displayResource != null && displayResource); + return ! Boolean.FALSE.equals(displayResource); } @Override @@ -1103,6 +1103,14 @@ public void changeResourceCount(long accountId, ResourceType type, Boolean displ } } + @Override + public ResourceLimitService.ResourceReservation getReservation(final Account account, final Boolean displayResource, final Resource.ResourceType type, final Long delta) { + if (! Boolean.FALSE.equals(displayResource)) { + return new CheckedReservation(account, type, delta); + } + throw new CloudRuntimeException("no reservation needed for resources that display as false"); + } + @Override public String getConfigComponentName() { return ResourceLimitManagerImpl.class.getName(); @@ -1124,7 +1132,7 @@ protected void runInContext() { List domains = _domainDao.findImmediateChildrenForParent(Domain.ROOT_DOMAIN); List accounts = _accountDao.findActiveAccountsForDomain(Domain.ROOT_DOMAIN); - for (ResourceType type : ResourceCount.ResourceType.values()) { + for (ResourceType type : ResourceType.values()) { if (type.supportsOwner(ResourceOwnerType.Domain)) { recalculateDomainResourceCount(Domain.ROOT_DOMAIN, type); for (Domain domain : domains) { diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 9d56c339eed3..2ba76dc33f39 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -365,7 +365,7 @@ import com.cloud.vm.snapshot.VMSnapshotVO; import com.cloud.vm.snapshot.dao.VMSnapshotDao; -public class UserVmManagerImpl extends ManagerBase implements UserVmManager, VirtualMachineGuru, UserVmService, Configurable { +public class UserVmManagerImpl extends ManagerBase implements UserVmManager, VirtualMachineGuru, Configurable { private static final Logger s_logger = Logger.getLogger(UserVmManagerImpl.class); /** diff --git a/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java index 8ad08eef653e..d581c8f7569e 100644 --- a/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java +++ b/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java @@ -21,6 +21,7 @@ import javax.naming.ConfigurationException; +import com.cloud.utils.exception.CloudRuntimeException; import org.springframework.stereotype.Component; import com.cloud.configuration.Resource.ResourceType; @@ -171,6 +172,11 @@ public void decrementResourceCount(long accountId, ResourceType type, Boolean di //To change body of implemented methods use File | Settings | File Templates. } + @Override + public ResourceReservation getReservation(Account account, Boolean displayResource, ResourceType type, Long delta) { + throw new CloudRuntimeException("no reservation implemented for mock resource management."); + } + /* (non-Javadoc) * @see com.cloud.utils.component.Manager#configure(java.lang.String, java.util.Map) */ From 6234f192c1376ce531b6ba700bc80c9a99593f05 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 29 Aug 2022 11:53:39 +0200 Subject: [PATCH 02/19] try-with-resource --- .../com/cloud/user/ResourceLimitService.java | 12 +- .../cloudstack/user/ResourceReservation.java | 17 + .../cloudstack/reservation/ReservationVO.java | 12 +- .../reservation/dao/ReservationDao.java | 7 +- .../reservation/dao/ReservationDaoImpl.java | 26 +- ...spring-engine-schema-core-daos-context.xml | 1 + .../resourcelimit/CheckedReservation.java | 51 +- .../ResourceLimitManagerImpl.java | 52 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 499 +++++++++--------- .../vpc/MockResourceLimitManagerImpl.java | 1 + 10 files changed, 389 insertions(+), 289 deletions(-) create mode 100644 api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java diff --git a/api/src/main/java/com/cloud/user/ResourceLimitService.java b/api/src/main/java/com/cloud/user/ResourceLimitService.java index f8155be20ba5..41b2c8135cf0 100644 --- a/api/src/main/java/com/cloud/user/ResourceLimitService.java +++ b/api/src/main/java/com/cloud/user/ResourceLimitService.java @@ -24,6 +24,7 @@ import com.cloud.domain.Domain; import com.cloud.exception.ResourceAllocationException; import org.apache.cloudstack.framework.config.ConfigKey; +import org.apache.cloudstack.user.ResourceReservation; public interface ResourceLimitService { @@ -207,15 +208,6 @@ public interface ResourceLimitService { * @param delta amount to reserve (will not be <+ 0) * @return a {code}AutoClosable{Code} object representing the resource the user needs */ - ResourceReservation getReservation(Account account, Boolean displayResource, ResourceType type, Long delta); + ResourceReservation getReservation(Account account, Boolean displayResource, ResourceType type, Long delta) throws ResourceAllocationException; - /** - * an interface defining an {code}AutoClosable{code} reservation object - */ - interface ResourceReservation { - - Long getAccountId(); - ResourceType getResourceType(); - Long getReservedAmount(); - } } diff --git a/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java new file mode 100644 index 000000000000..4c57362a8716 --- /dev/null +++ b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java @@ -0,0 +1,17 @@ +package org.apache.cloudstack.user; + +import com.cloud.configuration.Resource; +import org.apache.cloudstack.api.InternalIdentity; + +/** + * an interface defining an {code}AutoClosable{code} reservation object + */ +public interface +ResourceReservation extends InternalIdentity { + + Long getAccountId(); + + Resource.ResourceType getResourceType(); + + Long getReservedAmount(); +} diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index 5949851873cd..98f31bbc4dbc 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -1,7 +1,7 @@ package org.apache.cloudstack.reservation; import com.cloud.configuration.Resource; -import com.cloud.user.ResourceLimitService; +import org.apache.cloudstack.user.ResourceReservation; import com.cloud.utils.exception.CloudRuntimeException; import javax.persistence.Column; @@ -13,7 +13,8 @@ @Entity @Table(name = "resource_reservation") -public class ReservationVO implements ResourceLimitService.ResourceReservation { +public class ReservationVO implements ResourceReservation { + @Id @GeneratedValue(strategy = GenerationType.IDENTITY) @Column(name = "id") @@ -28,7 +29,7 @@ public class ReservationVO implements ResourceLimitService.ResourceReservation { @Column(name = "account_id") long amount; - protected ReservationVO(Long accountId, Resource.ResourceType type, Long delta) { + public ReservationVO(Long accountId, Resource.ResourceType type, Long delta) { if (delta == null || delta <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } @@ -37,6 +38,11 @@ protected ReservationVO(Long accountId, Resource.ResourceType type, Long delta) this.amount = delta; } + @Override + public long getId() { + return this.id; + } + @Override public Long getAccountId() { return accountId; diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java index 782daa4997d8..6b2ab919ec60 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java @@ -1,4 +1,9 @@ package org.apache.cloudstack.reservation.dao; -public interface ReservationDao { +import com.cloud.configuration.Resource; +import org.apache.cloudstack.reservation.ReservationVO; +import com.cloud.utils.db.GenericDao; + +public interface ReservationDao extends GenericDao { + long getReservation(Long account, Resource.ResourceType type); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java index 137bc54cbb89..14296f1e94bf 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -1,10 +1,34 @@ package org.apache.cloudstack.reservation.dao; +import com.cloud.configuration.Resource; import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import org.apache.cloudstack.reservation.ReservationVO; -public class ReservationDaoImpl extends GenericDaoBase implements ReservationDao { +import java.util.List; +public class ReservationDaoImpl extends GenericDaoBase implements ReservationDao { + + private final SearchBuilder listByRegionIDAccountAndIdSearch; public ReservationDaoImpl() { + listByRegionIDAccountAndIdSearch = createSearchBuilder(); + listByRegionIDAccountAndIdSearch.and("accountId", listByRegionIDAccountAndIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + listByRegionIDAccountAndIdSearch.and("type", listByRegionIDAccountAndIdSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listByRegionIDAccountAndIdSearch.done(); + + } + @Override + public long getReservation(Long accountId, Resource.ResourceType type) { + long total = 0; + SearchCriteria sc = listByRegionIDAccountAndIdSearch.create(); + sc.setParameters("accountId", accountId); + sc.setParameters("type", type); + List reservations = listBy(sc); + for (ReservationVO reservation : reservations) { + total += reservation.getReservedAmount(); + } + return total; } } diff --git a/engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml b/engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml index f13ae6e68031..fcd3be6c92eb 100644 --- a/engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml +++ b/engine/schema/src/main/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml @@ -175,6 +175,7 @@ + diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 33b18fe0e57f..f043aead615b 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -1,31 +1,65 @@ package com.cloud.resourcelimit; import com.cloud.configuration.Resource; +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; -public class CheckedReservation implements AutoCloseable, ResourceLimitService.ResourceReservation { +import javax.inject.Inject; + +public class CheckedReservation implements AutoCloseable, ResourceReservation { + + private static final int TRY_TO_GET_LOCK_TIME = 60; + @Inject + ReservationDao reservationDao; + @Inject + ResourceLimitService resourceLimitService; private final Account account; private final Resource.ResourceType type; private final Long amount; + private ResourceReservation reservation; - public CheckedReservation(Account account, Resource.ResourceType type, Long amount) { + /** + * - check if adding a reservation is allowed + * - create DB entry for reservation + * - hold the id of this record as a ticket for implementation + * + * @param amount positive number of the resource type to reserve + * @throws ResourceAllocationException + */ + public CheckedReservation(Account account, Resource.ResourceType type, Long amount) throws ResourceAllocationException { if (amount == null || amount <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } - // synchronised: - // - check if adding a reservation is allowed - // - create DB entry for reservation this.account = account; this.type = type; this.amount = amount; + + // synchronised?: + String lockName = String.format("CheckedReservation-%s/%d", account.getUuid(), type); + GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); + if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { + try { + resourceLimitService.checkResourceLimit(account,type,amount); + ReservationVO reservationVO = new ReservationVO(account.getAccountId(), type, amount); + this.reservation = reservationDao.persist(reservationVO); + } finally { + quotaLimitLock.unlock(); + } + } else { + throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), type); + } } @Override public void close() throws Exception { // delete the reservation vo - + reservationDao.remove(reservation.getId()); } public Account getAccount() { @@ -46,4 +80,9 @@ public Resource.ResourceType getResourceType() { public Long getReservedAmount() { return null; } + + @Override + public long getId() { + return this.reservation.getId(); + } } diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 16fb3daf2339..f3cc27de8ebd 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -36,10 +36,12 @@ import org.apache.cloudstack.framework.config.Configurable; import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import org.apache.cloudstack.reservation.dao.ReservationDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao; import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; +import org.apache.cloudstack.user.ResourceReservation; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -71,7 +73,6 @@ import com.cloud.projects.ProjectAccount.Role; import com.cloud.projects.dao.ProjectAccountDao; import com.cloud.projects.dao.ProjectDao; -import com.cloud.service.dao.ServiceOfferingDao; import com.cloud.storage.DataStoreRole; import com.cloud.storage.SnapshotVO; import com.cloud.storage.VMTemplateStorageResourceAssoc.Status; @@ -113,52 +114,52 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLimitService, Configurable { public static final Logger s_logger = Logger.getLogger(ResourceLimitManagerImpl.class); - @Inject - private DomainDao _domainDao; @Inject private AccountManager _accountMgr; @Inject private AlertManager _alertMgr; @Inject - private ResourceCountDao _resourceCountDao; - @Inject - private ResourceLimitDao _resourceLimitDao; - @Inject - private UserVmDao _userVmDao; - @Inject private AccountDao _accountDao; @Inject - protected SnapshotDao _snapshotDao; + private ConfigurationDao _configDao; @Inject - protected VMTemplateDao _vmTemplateDao; + private DomainDao _domainDao; @Inject - private VolumeDao _volumeDao; + private EntityManager _entityMgr; @Inject private IPAddressDao _ipAddressDao; @Inject - private VMInstanceDao _vmDao; - @Inject - private ConfigurationDao _configDao; - @Inject - private EntityManager _entityMgr; + private NetworkDao _networkDao; @Inject private ProjectDao _projectDao; @Inject private ProjectAccountDao _projectAccountDao; @Inject - private NetworkDao _networkDao; - @Inject - private VpcDao _vpcDao; + private ResourceCountDao _resourceCountDao; @Inject - private ServiceOfferingDao _serviceOfferingDao; + private ResourceLimitDao _resourceLimitDao; @Inject - private TemplateDataStoreDao _vmTemplateStoreDao; + private ReservationDao reservationDao; @Inject - private VlanDao _vlanDao; + protected SnapshotDao _snapshotDao; @Inject private SnapshotDataStoreDao _snapshotDataStoreDao; @Inject + private TemplateDataStoreDao _vmTemplateStoreDao; + @Inject + private UserVmDao _userVmDao; + @Inject private UserVmJoinDao _userVmJoinDao; + @Inject + private VMInstanceDao _vmDao; + @Inject + protected VMTemplateDao _vmTemplateDao; + @Inject + private VolumeDao _volumeDao; + @Inject + private VpcDao _vpcDao; + @Inject + private VlanDao _vlanDao; protected GenericSearchBuilder templateSizeSearch; protected GenericSearchBuilder snapshotSizeSearch; @@ -451,7 +452,8 @@ private void checkAccountResourceLimit(final Account account, final Project proj // Check account limits long accountResourceLimit = findCorrectResourceLimitForAccount(account, type); long currentResourceCount = _resourceCountDao.getResourceCount(account.getId(), ResourceOwnerType.Account, type); - long requestedResourceCount = currentResourceCount + numResources; + long currentResourceReservation = reservationDao.getReservation(account.getId(), type); + long requestedResourceCount = currentResourceCount + currentResourceReservation + numResources; String convertedAccountResourceLimit = String.valueOf(accountResourceLimit); String convertedCurrentResourceCount = String.valueOf(currentResourceCount); @@ -1104,7 +1106,7 @@ public void changeResourceCount(long accountId, ResourceType type, Boolean displ } @Override - public ResourceLimitService.ResourceReservation getReservation(final Account account, final Boolean displayResource, final Resource.ResourceType type, final Long delta) { + public ResourceReservation getReservation(final Account account, final Boolean displayResource, final Resource.ResourceType type, final Long delta) throws ResourceAllocationException { if (! Boolean.FALSE.equals(displayResource)) { return new CheckedReservation(account, type, delta); } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 2ba76dc33f39..cb2b9a37dab0 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -51,6 +51,7 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.ParserConfigurationException; +import com.cloud.resourcelimit.CheckedReservation; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; @@ -3857,309 +3858,321 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe DiskOfferingVO diskOffering = _diskOfferingDao.findById(diskOfferingId); volumesSize += verifyAndGetDiskSize(diskOffering, diskSize); } - if (! VirtualMachineManager.ResourceCountRunningVMsonly.value()) { - resourceLimitCheck(owner, isDisplayVm, new Long(offering.getCpu()), new Long(offering.getRamSize())); + UserVmVO vm = getUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); + + _securityGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList); + + if (affinityGroupIdList != null && !affinityGroupIdList.isEmpty()) { + _affinityGroupVMMapDao.updateMap(vm.getId(), affinityGroupIdList); } - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.volume, (isIso || diskOfferingId == null ? 1 : 2)); - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.primary_storage, volumesSize); + CallContext.current().putContextParameter(VirtualMachine.class, vm.getUuid()); + return vm; + } - // verify security group ids - if (securityGroupIdList != null) { - for (Long securityGroupId : securityGroupIdList) { - SecurityGroup sg = _securityGroupDao.findById(securityGroupId); - if (sg == null) { - throw new InvalidParameterValueException("Unable to find security group by id " + securityGroupId); - } else { - // verify permissions - _accountMgr.checkAccess(caller, null, true, owner, sg); - } + private UserVmVO getUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { + try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l)) { + if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { + resourceLimitCheck(owner, isDisplayVm, new Long(offering.getCpu()), new Long(offering.getRamSize())); } - } - if (datadiskTemplateToDiskOfferringMap != null && !datadiskTemplateToDiskOfferringMap.isEmpty()) { - for (Entry datadiskTemplateToDiskOffering : datadiskTemplateToDiskOfferringMap.entrySet()) { - VMTemplateVO dataDiskTemplate = _templateDao.findById(datadiskTemplateToDiskOffering.getKey()); - DiskOffering dataDiskOffering = datadiskTemplateToDiskOffering.getValue(); + _resourceLimitMgr.checkResourceLimit(owner, ResourceType.volume, (isIso || diskOfferingId == null ? 1 : 2)); + _resourceLimitMgr.checkResourceLimit(owner, ResourceType.primary_storage, volumesSize); - if (dataDiskTemplate == null - || (!dataDiskTemplate.getTemplateType().equals(TemplateType.DATADISK)) && (dataDiskTemplate.getState().equals(VirtualMachineTemplate.State.Active))) { - throw new InvalidParameterValueException("Invalid template id specified for Datadisk template" + datadiskTemplateToDiskOffering.getKey()); - } - long dataDiskTemplateId = datadiskTemplateToDiskOffering.getKey(); - if (!dataDiskTemplate.getParentTemplateId().equals(template.getId())) { - throw new InvalidParameterValueException("Invalid Datadisk template. Specified Datadisk template" + dataDiskTemplateId - + " doesn't belong to template " + template.getId()); - } - if (dataDiskOffering == null) { - throw new InvalidParameterValueException("Invalid disk offering id " + datadiskTemplateToDiskOffering.getValue().getId() + - " specified for datadisk template " + dataDiskTemplateId); - } - if (dataDiskOffering.isCustomized()) { - throw new InvalidParameterValueException("Invalid disk offering id " + dataDiskOffering.getId() + " specified for datadisk template " + - dataDiskTemplateId + ". Custom Disk offerings are not supported for Datadisk templates"); + // verify security group ids + if (securityGroupIdList != null) { + for (Long securityGroupId : securityGroupIdList) { + SecurityGroup sg = _securityGroupDao.findById(securityGroupId); + if (sg == null) { + throw new InvalidParameterValueException("Unable to find security group by id " + securityGroupId); + } else { + // verify permissions + _accountMgr.checkAccess(caller, null, true, owner, sg); + } } - if (dataDiskOffering.getDiskSize() < dataDiskTemplate.getSize()) { - throw new InvalidParameterValueException("Invalid disk offering id " + dataDiskOffering.getId() + " specified for datadisk template " + - dataDiskTemplateId + ". Disk offering size should be greater than or equal to the template size"); + } + + if (datadiskTemplateToDiskOfferringMap != null && !datadiskTemplateToDiskOfferringMap.isEmpty()) { + for (Entry datadiskTemplateToDiskOffering : datadiskTemplateToDiskOfferringMap.entrySet()) { + VMTemplateVO dataDiskTemplate = _templateDao.findById(datadiskTemplateToDiskOffering.getKey()); + DiskOffering dataDiskOffering = datadiskTemplateToDiskOffering.getValue(); + + if (dataDiskTemplate == null + || (!dataDiskTemplate.getTemplateType().equals(TemplateType.DATADISK)) && (dataDiskTemplate.getState().equals(VirtualMachineTemplate.State.Active))) { + throw new InvalidParameterValueException("Invalid template id specified for Datadisk template" + datadiskTemplateToDiskOffering.getKey()); + } + long dataDiskTemplateId = datadiskTemplateToDiskOffering.getKey(); + if (!dataDiskTemplate.getParentTemplateId().equals(template.getId())) { + throw new InvalidParameterValueException("Invalid Datadisk template. Specified Datadisk template" + dataDiskTemplateId + + " doesn't belong to template " + template.getId()); + } + if (dataDiskOffering == null) { + throw new InvalidParameterValueException("Invalid disk offering id " + datadiskTemplateToDiskOffering.getValue().getId() + + " specified for datadisk template " + dataDiskTemplateId); + } + if (dataDiskOffering.isCustomized()) { + throw new InvalidParameterValueException("Invalid disk offering id " + dataDiskOffering.getId() + " specified for datadisk template " + + dataDiskTemplateId + ". Custom Disk offerings are not supported for Datadisk templates"); + } + if (dataDiskOffering.getDiskSize() < dataDiskTemplate.getSize()) { + throw new InvalidParameterValueException("Invalid disk offering id " + dataDiskOffering.getId() + " specified for datadisk template " + + dataDiskTemplateId + ". Disk offering size should be greater than or equal to the template size"); + } + _templateDao.loadDetails(dataDiskTemplate); + _resourceLimitMgr.checkResourceLimit(owner, ResourceType.volume, 1); + _resourceLimitMgr.checkResourceLimit(owner, ResourceType.primary_storage, dataDiskOffering.getDiskSize()); } - _templateDao.loadDetails(dataDiskTemplate); - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.volume, 1); - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.primary_storage, dataDiskOffering.getDiskSize()); } - } - // check that the affinity groups exist - if (affinityGroupIdList != null) { - for (Long affinityGroupId : affinityGroupIdList) { - AffinityGroupVO ag = _affinityGroupDao.findById(affinityGroupId); - if (ag == null) { - throw new InvalidParameterValueException("Unable to find affinity group " + ag); - } else if (!_affinityGroupService.isAffinityGroupProcessorAvailable(ag.getType())) { - throw new InvalidParameterValueException("Affinity group type is not supported for group: " + ag + " ,type: " + ag.getType() - + " , Please try again after removing the affinity group"); - } else { - // verify permissions - if (ag.getAclType() == ACLType.Domain) { - _accountMgr.checkAccess(caller, null, false, owner, ag); - // Root admin has access to both VM and AG by default, - // but - // make sure the owner of these entities is same - if (caller.getId() == Account.ACCOUNT_ID_SYSTEM || _accountMgr.isRootAdmin(caller.getId())) { - if (!_affinityGroupService.isAffinityGroupAvailableInDomain(ag.getId(), owner.getDomainId())) { - throw new PermissionDeniedException("Affinity Group " + ag + " does not belong to the VM's domain"); - } - } + // check that the affinity groups exist + if (affinityGroupIdList != null) { + for (Long affinityGroupId : affinityGroupIdList) { + AffinityGroupVO ag = _affinityGroupDao.findById(affinityGroupId); + if (ag == null) { + throw new InvalidParameterValueException("Unable to find affinity group " + ag); + } else if (!_affinityGroupService.isAffinityGroupProcessorAvailable(ag.getType())) { + throw new InvalidParameterValueException("Affinity group type is not supported for group: " + ag + " ,type: " + ag.getType() + + " , Please try again after removing the affinity group"); } else { - _accountMgr.checkAccess(caller, null, true, owner, ag); - // Root admin has access to both VM and AG by default, - // but - // make sure the owner of these entities is same - if (caller.getId() == Account.ACCOUNT_ID_SYSTEM || _accountMgr.isRootAdmin(caller.getId())) { - if (ag.getAccountId() != owner.getAccountId()) { - throw new PermissionDeniedException("Affinity Group " + ag + " does not belong to the VM's account"); + // verify permissions + if (ag.getAclType() == ACLType.Domain) { + _accountMgr.checkAccess(caller, null, false, owner, ag); + // Root admin has access to both VM and AG by default, + // but + // make sure the owner of these entities is same + if (caller.getId() == Account.ACCOUNT_ID_SYSTEM || _accountMgr.isRootAdmin(caller.getId())) { + if (!_affinityGroupService.isAffinityGroupAvailableInDomain(ag.getId(), owner.getDomainId())) { + throw new PermissionDeniedException("Affinity Group " + ag + " does not belong to the VM's domain"); + } + } + } else { + _accountMgr.checkAccess(caller, null, true, owner, ag); + // Root admin has access to both VM and AG by default, + // but + // make sure the owner of these entities is same + if (caller.getId() == Account.ACCOUNT_ID_SYSTEM || _accountMgr.isRootAdmin(caller.getId())) { + if (ag.getAccountId() != owner.getAccountId()) { + throw new PermissionDeniedException("Affinity Group " + ag + " does not belong to the VM's account"); + } } } } } } - } - if (hypervisorType != HypervisorType.BareMetal) { - // check if we have available pools for vm deployment - long availablePools = _storagePoolDao.countPoolsByStatus(StoragePoolStatus.Up); - if (availablePools < 1) { - throw new StorageUnavailableException("There are no available pools in the UP state for vm deployment", -1); + if (hypervisorType != HypervisorType.BareMetal) { + // check if we have available pools for vm deployment + long availablePools = _storagePoolDao.countPoolsByStatus(StoragePoolStatus.Up); + if (availablePools < 1) { + throw new StorageUnavailableException("There are no available pools in the UP state for vm deployment", -1); + } } - } - if (template.getTemplateType().equals(TemplateType.SYSTEM) && !CKS_NODE.equals(vmType)) { - throw new InvalidParameterValueException("Unable to use system template " + template.getId() + " to deploy a user vm"); - } - List listZoneTemplate = _templateZoneDao.listByZoneTemplate(zone.getId(), template.getId()); - if (listZoneTemplate == null || listZoneTemplate.isEmpty()) { - throw new InvalidParameterValueException("The template " + template.getId() + " is not available for use"); - } + if (template.getTemplateType().equals(TemplateType.SYSTEM) && !CKS_NODE.equals(vmType)) { + throw new InvalidParameterValueException("Unable to use system template " + template.getId() + " to deploy a user vm"); + } + List listZoneTemplate = _templateZoneDao.listByZoneTemplate(zone.getId(), template.getId()); + if (listZoneTemplate == null || listZoneTemplate.isEmpty()) { + throw new InvalidParameterValueException("The template " + template.getId() + " is not available for use"); + } - if (isIso && !template.isBootable()) { - throw new InvalidParameterValueException("Installing from ISO requires an ISO that is bootable: " + template.getId()); - } + if (isIso && !template.isBootable()) { + throw new InvalidParameterValueException("Installing from ISO requires an ISO that is bootable: " + template.getId()); + } - // Check templates permissions - _accountMgr.checkAccess(owner, AccessType.UseEntry, false, template); + // Check templates permissions + _accountMgr.checkAccess(owner, AccessType.UseEntry, false, template); - // check if the user data is correct - userData = validateUserData(userData, httpmethod); + // check if the user data is correct + userData = validateUserData(userData, httpmethod); - // Find an SSH public key corresponding to the key pair name, if one is - // given - String sshPublicKeys = ""; - String keypairnames = ""; - if (!sshKeyPairs.isEmpty()) { - List pairs = _sshKeyPairDao.findByNames(owner.getAccountId(), owner.getDomainId(), sshKeyPairs); - if (pairs == null || pairs.size() != sshKeyPairs.size()) { - throw new InvalidParameterValueException("Not all specified keyparis exist"); + // Find an SSH public key corresponding to the key pair name, if one is + // given + String sshPublicKeys = ""; + String keypairnames = ""; + if (!sshKeyPairs.isEmpty()) { + List pairs = _sshKeyPairDao.findByNames(owner.getAccountId(), owner.getDomainId(), sshKeyPairs); + if (pairs == null || pairs.size() != sshKeyPairs.size()) { + throw new InvalidParameterValueException("Not all specified keyparis exist"); + } + + sshPublicKeys = pairs.stream().map(p -> p.getPublicKey()).collect(Collectors.joining("\n")); + keypairnames = String.join(",", sshKeyPairs); } - sshPublicKeys = pairs.stream().map(p -> p.getPublicKey()).collect(Collectors.joining("\n")); - keypairnames = String.join(",", sshKeyPairs); - } + LinkedHashMap> networkNicMap = new LinkedHashMap<>(); - LinkedHashMap> networkNicMap = new LinkedHashMap<>(); + short defaultNetworkNumber = 0; + boolean securityGroupEnabled = false; + int networkIndex = 0; + for (NetworkVO network : networkList) { + if ((network.getDataCenterId() != zone.getId())) { + if (!network.isStrechedL2Network()) { + throw new InvalidParameterValueException("Network id=" + network.getId() + + " doesn't belong to zone " + zone.getId()); + } - short defaultNetworkNumber = 0; - boolean securityGroupEnabled = false; - int networkIndex = 0; - for (NetworkVO network : networkList) { - if ((network.getDataCenterId() != zone.getId())) { - if (!network.isStrechedL2Network()) { - throw new InvalidParameterValueException("Network id=" + network.getId() + - " doesn't belong to zone " + zone.getId()); + NetworkOffering ntwkOffering = _networkOfferingDao.findById(network.getNetworkOfferingId()); + Long physicalNetworkId = _networkModel.findPhysicalNetworkId(zone.getId(), ntwkOffering.getTags(), ntwkOffering.getTrafficType()); + + String provider = _ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(), Service.Connectivity); + if (!_networkModel.isProviderEnabledInPhysicalNetwork(physicalNetworkId, provider)) { + throw new InvalidParameterValueException("Network in which is VM getting deployed could not be" + + " streched to the zone, as we could not find a valid physical network"); + } } - NetworkOffering ntwkOffering = _networkOfferingDao.findById(network.getNetworkOfferingId()); - Long physicalNetworkId = _networkModel.findPhysicalNetworkId(zone.getId(), ntwkOffering.getTags(), ntwkOffering.getTrafficType()); + _accountMgr.checkAccess(owner, AccessType.UseEntry, false, network); - String provider = _ntwkSrvcDao.getProviderForServiceInNetwork(network.getId(), Service.Connectivity); - if (!_networkModel.isProviderEnabledInPhysicalNetwork(physicalNetworkId, provider)) { - throw new InvalidParameterValueException("Network in which is VM getting deployed could not be" + - " streched to the zone, as we could not find a valid physical network"); + IpAddresses requestedIpPair = null; + if (requestedIps != null && !requestedIps.isEmpty()) { + requestedIpPair = requestedIps.get(network.getId()); } - } - - _accountMgr.checkAccess(owner, AccessType.UseEntry, false, network); - IpAddresses requestedIpPair = null; - if (requestedIps != null && !requestedIps.isEmpty()) { - requestedIpPair = requestedIps.get(network.getId()); - } + if (requestedIpPair == null) { + requestedIpPair = new IpAddresses(null, null); + } else { + _networkModel.checkRequestedIpAddresses(network.getId(), requestedIpPair); + } + + NicProfile profile = new NicProfile(requestedIpPair.getIp4Address(), requestedIpPair.getIp6Address(), requestedIpPair.getMacAddress()); + profile.setOrderIndex(networkIndex); + if (defaultNetworkNumber == 0) { + defaultNetworkNumber++; + // if user requested specific ip for default network, add it + if (defaultIps.getIp4Address() != null || defaultIps.getIp6Address() != null) { + _networkModel.checkRequestedIpAddresses(network.getId(), defaultIps); + profile = new NicProfile(defaultIps.getIp4Address(), defaultIps.getIp6Address()); + } else if (defaultIps.getMacAddress() != null) { + profile = new NicProfile(null, null, defaultIps.getMacAddress()); + } - if (requestedIpPair == null) { - requestedIpPair = new IpAddresses(null, null); - } else { - _networkModel.checkRequestedIpAddresses(network.getId(), requestedIpPair); - } + profile.setDefaultNic(true); + if (!_networkModel.areServicesSupportedInNetwork(network.getId(), new Service[]{Service.UserData})) { + if ((userData != null) && (!userData.isEmpty())) { + throw new InvalidParameterValueException("Unable to deploy VM as UserData is provided while deploying the VM, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + } - NicProfile profile = new NicProfile(requestedIpPair.getIp4Address(), requestedIpPair.getIp6Address(), requestedIpPair.getMacAddress()); - profile.setOrderIndex(networkIndex); - if (defaultNetworkNumber == 0) { - defaultNetworkNumber++; - // if user requested specific ip for default network, add it - if (defaultIps.getIp4Address() != null || defaultIps.getIp6Address() != null) { - _networkModel.checkRequestedIpAddresses(network.getId(), defaultIps); - profile = new NicProfile(defaultIps.getIp4Address(), defaultIps.getIp6Address()); - } else if (defaultIps.getMacAddress() != null) { - profile = new NicProfile(null, null, defaultIps.getMacAddress()); - } - - profile.setDefaultNic(true); - if (!_networkModel.areServicesSupportedInNetwork(network.getId(), new Service[]{Service.UserData})) { - if ((userData != null) && (!userData.isEmpty())) { - throw new InvalidParameterValueException("Unable to deploy VM as UserData is provided while deploying the VM, but there is no support for " + Network.Service.UserData.getName() + " service in the default network " + network.getId()); - } + if ((sshPublicKeys != null) && (!sshPublicKeys.isEmpty())) { + throw new InvalidParameterValueException("Unable to deploy VM as SSH keypair is provided while deploying the VM, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + } - if ((sshPublicKeys != null) && (!sshPublicKeys.isEmpty())) { - throw new InvalidParameterValueException("Unable to deploy VM as SSH keypair is provided while deploying the VM, but there is no support for " + Network.Service.UserData.getName() + " service in the default network " + network.getId()); + if (template.isEnablePassword()) { + throw new InvalidParameterValueException("Unable to deploy VM as template " + template.getId() + " is password enabled, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + } } + } - if (template.isEnablePassword()) { - throw new InvalidParameterValueException("Unable to deploy VM as template " + template.getId() + " is password enabled, but there is no support for " + Network.Service.UserData.getName() + " service in the default network " + network.getId()); - } + if (_networkModel.isSecurityGroupSupportedInNetwork(network)) { + securityGroupEnabled = true; + } + List profiles = networkNicMap.get(network.getUuid()); + if (CollectionUtils.isEmpty(profiles)) { + profiles = new ArrayList<>(); } + profiles.add(profile); + networkNicMap.put(network.getUuid(), profiles); + networkIndex++; } - if (_networkModel.isSecurityGroupSupportedInNetwork(network)) { - securityGroupEnabled = true; + if (securityGroupIdList != null && !securityGroupIdList.isEmpty() && !securityGroupEnabled) { + throw new InvalidParameterValueException("Unable to deploy vm with security groups as SecurityGroup service is not enabled for the vm's network"); } - List profiles = networkNicMap.get(network.getUuid()); - if (CollectionUtils.isEmpty(profiles)) { - profiles = new ArrayList<>(); - } - profiles.add(profile); - networkNicMap.put(network.getUuid(), profiles); - networkIndex++; - } - - if (securityGroupIdList != null && !securityGroupIdList.isEmpty() && !securityGroupEnabled) { - throw new InvalidParameterValueException("Unable to deploy vm with security groups as SecurityGroup service is not enabled for the vm's network"); - } - // Verify network information - network default network has to be set; - // and vm can't have more than one default network - // This is a part of business logic because default network is required - // by Agent Manager in order to configure default - // gateway for the vm - if (defaultNetworkNumber == 0) { - throw new InvalidParameterValueException("At least 1 default network has to be specified for the vm"); - } else if (defaultNetworkNumber > 1) { - throw new InvalidParameterValueException("Only 1 default network per vm is supported"); - } - - long id = _vmDao.getNextInSequence(Long.class, "id"); + // Verify network information - network default network has to be set; + // and vm can't have more than one default network + // This is a part of business logic because default network is required + // by Agent Manager in order to configure default + // gateway for the vm + if (defaultNetworkNumber == 0) { + throw new InvalidParameterValueException("At least 1 default network has to be specified for the vm"); + } else if (defaultNetworkNumber > 1) { + throw new InvalidParameterValueException("Only 1 default network per vm is supported"); + } - if (hostName != null) { - // Check is hostName is RFC compliant - checkNameForRFCCompliance(hostName); - } + long id = _vmDao.getNextInSequence(Long.class, "id"); - String instanceName = null; - String instanceSuffix = _instance; - String uuidName = _uuidMgr.generateUuid(UserVm.class, customId); - if (_instanceNameFlag && HypervisorType.VMware.equals(hypervisorType)) { - if (StringUtils.isNotEmpty(hostName)) { - instanceSuffix = hostName; + if (hostName != null) { + // Check is hostName is RFC compliant + checkNameForRFCCompliance(hostName); } - if (hostName == null) { - if (displayName != null) { - hostName = displayName; - } else { + + String instanceName = null; + String instanceSuffix = _instance; + String uuidName = _uuidMgr.generateUuid(UserVm.class, customId); + if (_instanceNameFlag && HypervisorType.VMware.equals(hypervisorType)) { + if (StringUtils.isNotEmpty(hostName)) { + instanceSuffix = hostName; + } + if (hostName == null) { + if (displayName != null) { + hostName = displayName; + } else { + hostName = generateHostName(uuidName); + } + } + // If global config vm.instancename.flag is set to true, then CS will set guest VM's name as it appears on the hypervisor, to its hostname. + // In case of VMware since VM name must be unique within a DC, check if VM with the same hostname already exists in the zone. + VMInstanceVO vmByHostName = _vmInstanceDao.findVMByHostNameInZone(hostName, zone.getId()); + if (vmByHostName != null && vmByHostName.getState() != State.Expunging) { + throw new InvalidParameterValueException("There already exists a VM by the name: " + hostName + "."); + } + } else { + if (hostName == null) { + //Generate name using uuid and instance.name global config hostName = generateHostName(uuidName); } } - // If global config vm.instancename.flag is set to true, then CS will set guest VM's name as it appears on the hypervisor, to its hostname. - // In case of VMware since VM name must be unique within a DC, check if VM with the same hostname already exists in the zone. - VMInstanceVO vmByHostName = _vmInstanceDao.findVMByHostNameInZone(hostName, zone.getId()); - if (vmByHostName != null && vmByHostName.getState() != VirtualMachine.State.Expunging) { - throw new InvalidParameterValueException("There already exists a VM by the name: " + hostName + "."); + + if (hostName != null) { + // Check is hostName is RFC compliant + checkNameForRFCCompliance(hostName); } - } else { - if (hostName == null) { - //Generate name using uuid and instance.name global config - hostName = generateHostName(uuidName); + instanceName = VirtualMachineName.getVmName(id, owner.getId(), instanceSuffix); + if (_instanceNameFlag && HypervisorType.VMware.equals(hypervisorType) && !instanceSuffix.equals(_instance)) { + customParameters.put(VmDetailConstants.NAME_ON_HYPERVISOR, instanceName); } - } - - if (hostName != null) { - // Check is hostName is RFC compliant - checkNameForRFCCompliance(hostName); - } - instanceName = VirtualMachineName.getVmName(id, owner.getId(), instanceSuffix); - if (_instanceNameFlag && HypervisorType.VMware.equals(hypervisorType) && !instanceSuffix.equals(_instance)) { - customParameters.put(VmDetailConstants.NAME_ON_HYPERVISOR, instanceName); - } - // Check if VM with instanceName already exists. - VMInstanceVO vmObj = _vmInstanceDao.findVMByInstanceName(instanceName); - if (vmObj != null && vmObj.getState() != VirtualMachine.State.Expunging) { - throw new InvalidParameterValueException("There already exists a VM by the display name supplied"); - } + // Check if VM with instanceName already exists. + VMInstanceVO vmObj = _vmInstanceDao.findVMByInstanceName(instanceName); + if (vmObj != null && vmObj.getState() != State.Expunging) { + throw new InvalidParameterValueException("There already exists a VM by the display name supplied"); + } - checkIfHostNameUniqueInNtwkDomain(hostName, networkList); + checkIfHostNameUniqueInNtwkDomain(hostName, networkList); - long userId = CallContext.current().getCallingUserId(); - if (CallContext.current().getCallingAccount().getId() != owner.getId()) { - List userVOs = _userDao.listByAccount(owner.getAccountId()); - if (!userVOs.isEmpty()) { - userId = userVOs.get(0).getId(); + long userId = CallContext.current().getCallingUserId(); + if (CallContext.current().getCallingAccount().getId() != owner.getId()) { + List userVOs = _userDao.listByAccount(owner.getAccountId()); + if (!userVOs.isEmpty()) { + userId = userVOs.get(0).getId(); + } } - } - dynamicScalingEnabled = dynamicScalingEnabled && checkIfDynamicScalingCanBeEnabled(null, offering, template, zone.getId()); + dynamicScalingEnabled = dynamicScalingEnabled && checkIfDynamicScalingCanBeEnabled(null, offering, template, zone.getId()); - UserVmVO vm = commitUserVm(zone, template, hostName, displayName, owner, diskOfferingId, diskSize, userData, caller, isDisplayVm, keyboard, accountId, userId, offering, - isIso, sshPublicKeys, networkNicMap, id, instanceName, uuidName, hypervisorType, customParameters, dhcpOptionMap, - datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, rootDiskOfferingId, keypairnames); + UserVmVO vm = commitUserVm(zone, template, hostName, displayName, owner, diskOfferingId, diskSize, userData, caller, isDisplayVm, keyboard, accountId, userId, offering, + isIso, sshPublicKeys, networkNicMap, id, instanceName, uuidName, hypervisorType, customParameters, dhcpOptionMap, + datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, rootDiskOfferingId, keypairnames); - // Assign instance to the group - try { - if (group != null) { - boolean addToGroup = addInstanceToGroup(Long.valueOf(id), group); - if (!addToGroup) { - throw new CloudRuntimeException("Unable to assign Vm to the group " + group); + // Assign instance to the group + try { + if (group != null) { + boolean addToGroup = addInstanceToGroup(Long.valueOf(id), group); + if (!addToGroup) { + throw new CloudRuntimeException("Unable to assign Vm to the group " + group); + } } + } catch (Exception ex) { + throw new CloudRuntimeException("Unable to assign Vm to the group " + group); } - } catch (Exception ex) { - throw new CloudRuntimeException("Unable to assign Vm to the group " + group); - } - - _securityGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList); - - if (affinityGroupIdList != null && !affinityGroupIdList.isEmpty()) { - _affinityGroupVMMapDao.updateMap(vm.getId(), affinityGroupIdList); + return vm; + } catch (ResourceAllocationException | CloudRuntimeException e) { + throw e; + } catch (Exception e) { + s_logger.error("error during resource reservation and allocation", e); + throw new CloudRuntimeException(e); } - - CallContext.current().putContextParameter(VirtualMachine.class, vm.getUuid()); - return vm; } private long verifyAndGetDiskSize(DiskOfferingVO diskOffering, Long diskSize) { diff --git a/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java index d581c8f7569e..1d1da5331d92 100644 --- a/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java +++ b/server/src/test/java/com/cloud/vpc/MockResourceLimitManagerImpl.java @@ -22,6 +22,7 @@ import javax.naming.ConfigurationException; import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.user.ResourceReservation; import org.springframework.stereotype.Component; import com.cloud.configuration.Resource.ResourceType; From 3076155a1553dfe12b90b059b442d6247c322c4a Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 29 Aug 2022 10:15:40 +0200 Subject: [PATCH 03/19] domain added to reservation --- .../cloudstack/user/ResourceReservation.java | 2 ++ .../cloudstack/reservation/ReservationVO.java | 13 +++++-- .../reservation/dao/ReservationDao.java | 3 +- .../reservation/dao/ReservationDaoImpl.java | 36 ++++++++++++++----- .../META-INF/db/schema-41710to41800.sql | 1 + .../resourcelimit/CheckedReservation.java | 9 +++-- .../ResourceLimitManagerImpl.java | 5 +-- .../java/com/cloud/vm/UserVmManagerImpl.java | 4 +-- 8 files changed, 56 insertions(+), 17 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java index 4c57362a8716..ece59a46a369 100644 --- a/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java +++ b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java @@ -11,6 +11,8 @@ Long getAccountId(); + Long getDomainId(); + Resource.ResourceType getResourceType(); Long getReservedAmount(); diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index 98f31bbc4dbc..dbbb1720073b 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -23,17 +23,21 @@ public class ReservationVO implements ResourceReservation { @Column(name = "account_id") long accountId; + @Column(name = "domain_id") + long domainId; + @Column(name = "resource_type", nullable = false) Resource.ResourceType type; - @Column(name = "account_id") + @Column(name = "amount") long amount; - public ReservationVO(Long accountId, Resource.ResourceType type, Long delta) { + public ReservationVO(Long accountId, Long domainId, Resource.ResourceType type, Long delta) { if (delta == null || delta <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } this.accountId = accountId; + this.domainId = domainId; this.type = type; this.amount = delta; } @@ -48,6 +52,11 @@ public Long getAccountId() { return accountId; } + @Override + public Long getDomainId() { + return domainId; + } + @Override public Resource.ResourceType getResourceType() { return type; diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java index 6b2ab919ec60..ea00ba77c4f9 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java @@ -5,5 +5,6 @@ import com.cloud.utils.db.GenericDao; public interface ReservationDao extends GenericDao { - long getReservation(Long account, Resource.ResourceType type); + long getAccountReservation(Long account, Resource.ResourceType type); + long getDomainReservation(Long domain, Resource.ResourceType type); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java index 14296f1e94bf..b728a1d87dd6 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -10,21 +10,41 @@ public class ReservationDaoImpl extends GenericDaoBase implements ReservationDao { - private final SearchBuilder listByRegionIDAccountAndIdSearch; + private final SearchBuilder listAccountAndTypeSearch; + + private final SearchBuilder listDomainAndTypeSearch; + public ReservationDaoImpl() { - listByRegionIDAccountAndIdSearch = createSearchBuilder(); - listByRegionIDAccountAndIdSearch.and("accountId", listByRegionIDAccountAndIdSearch.entity().getAccountId(), SearchCriteria.Op.EQ); - listByRegionIDAccountAndIdSearch.and("type", listByRegionIDAccountAndIdSearch.entity().getResourceType(), SearchCriteria.Op.EQ); - listByRegionIDAccountAndIdSearch.done(); + listAccountAndTypeSearch = createSearchBuilder(); + listAccountAndTypeSearch.and("accountId", listAccountAndTypeSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + listAccountAndTypeSearch.and("resource_type", listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listAccountAndTypeSearch.done(); + listDomainAndTypeSearch = createSearchBuilder(); + listDomainAndTypeSearch.and("domainId", listDomainAndTypeSearch.entity().getDomainId(), SearchCriteria.Op.EQ); + listDomainAndTypeSearch.and("resource_type", listDomainAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listDomainAndTypeSearch.done(); } @Override - public long getReservation(Long accountId, Resource.ResourceType type) { + public long getAccountReservation(Long accountId, Resource.ResourceType type) { long total = 0; - SearchCriteria sc = listByRegionIDAccountAndIdSearch.create(); + SearchCriteria sc = listAccountAndTypeSearch.create(); sc.setParameters("accountId", accountId); - sc.setParameters("type", type); + sc.setParameters("resource_type", type); + List reservations = listBy(sc); + for (ReservationVO reservation : reservations) { + total += reservation.getReservedAmount(); + } + return total; + } + + @Override + public long getDomainReservation(Long domainId, Resource.ResourceType type) { + long total = 0; + SearchCriteria sc = listAccountAndTypeSearch.create(); + sc.setParameters("domainId", domainId); + sc.setParameters("resource_type", type); List reservations = listBy(sc); for (ReservationVO reservation : reservations) { total += reservation.getReservedAmount(); diff --git a/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql b/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql index a1e6ff2d2d2d..3fe6e8a789b6 100644 --- a/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql +++ b/engine/schema/src/main/resources/META-INF/db/schema-41710to41800.sql @@ -34,6 +34,7 @@ DROP TABLE IF EXISTS `cloud`.`resource_reservation`; CREATE TABLE `cloud`.`resource_reservation` ( `id` bigint unsigned NOT NULL auto_increment COMMENT 'id', `account_id` bigint unsigned NOT NULL, + `domain_id` bigint unsigned NOT NULL, `resource_type` varchar(255) NOT NULL, `amount` bigint unsigned NOT NULL, PRIMARY KEY (`id`) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index f043aead615b..48a41cd351cf 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -41,12 +41,12 @@ public CheckedReservation(Account account, Resource.ResourceType type, Long amou this.amount = amount; // synchronised?: - String lockName = String.format("CheckedReservation-%s/%d", account.getUuid(), type); + String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), type); GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { try { resourceLimitService.checkResourceLimit(account,type,amount); - ReservationVO reservationVO = new ReservationVO(account.getAccountId(), type, amount); + ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), type, amount); this.reservation = reservationDao.persist(reservationVO); } finally { quotaLimitLock.unlock(); @@ -71,6 +71,11 @@ public Long getAccountId() { return account.getId(); } + @Override + public Long getDomainId() { + return account.getDomainId(); + } + @Override public Resource.ResourceType getResourceType() { return null; diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index f3cc27de8ebd..65c224571049 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -429,7 +429,8 @@ private void checkDomainResourceLimit(final Account account, final Project proje if (domainId != Domain.ROOT_DOMAIN) { long domainResourceLimit = findCorrectResourceLimitForDomain(domain, type); long currentDomainResourceCount = _resourceCountDao.getResourceCount(domainId, ResourceOwnerType.Domain, type); - long requestedDomainResourceCount = currentDomainResourceCount + numResources; + long currentResourceReservation = reservationDao.getDomainReservation(domainId, type); + long requestedDomainResourceCount = currentDomainResourceCount + currentResourceReservation + numResources; String messageSuffix = " domain resource limits of Type '" + type + "'" + " for Domain Id = " + domainId + " is exceeded: Domain Resource Limit = " + toHumanReadableSize(domainResourceLimit) + ", Current Domain Resource Amount = " + toHumanReadableSize(currentDomainResourceCount) + ", Requested Resource Amount = " + toHumanReadableSize(numResources) + "."; @@ -452,7 +453,7 @@ private void checkAccountResourceLimit(final Account account, final Project proj // Check account limits long accountResourceLimit = findCorrectResourceLimitForAccount(account, type); long currentResourceCount = _resourceCountDao.getResourceCount(account.getId(), ResourceOwnerType.Account, type); - long currentResourceReservation = reservationDao.getReservation(account.getId(), type); + long currentResourceReservation = reservationDao.getAccountReservation(account.getId(), type); long requestedResourceCount = currentResourceCount + currentResourceReservation + numResources; String convertedAccountResourceLimit = String.valueOf(accountResourceLimit); diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index cb2b9a37dab0..d1e4244e2e94 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3858,7 +3858,7 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe DiskOfferingVO diskOffering = _diskOfferingDao.findById(diskOfferingId); volumesSize += verifyAndGetDiskSize(diskOffering, diskSize); } - UserVmVO vm = getUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); + UserVm vm = getUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); _securityGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList); @@ -3870,7 +3870,7 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe return vm; } - private UserVmVO getUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { + private UserVm getUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l)) { if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { resourceLimitCheck(owner, isDisplayVm, new Long(offering.getCpu()), new Long(offering.getRamSize())); From 87788ba744968973514bbbcace6ba10bacbe08b3 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 29 Aug 2022 15:06:33 +0200 Subject: [PATCH 04/19] default constructor --- .../java/org/apache/cloudstack/reservation/ReservationVO.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index dbbb1720073b..a40030b90af7 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -32,6 +32,9 @@ public class ReservationVO implements ResourceReservation { @Column(name = "amount") long amount; + protected ReservationVO() + {} + public ReservationVO(Long accountId, Long domainId, Resource.ResourceType type, Long delta) { if (delta == null || delta <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); From c0ddb01de6741b12df31bc436a16505edcb6698d Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 30 Aug 2022 10:26:41 +0200 Subject: [PATCH 05/19] proper field naming --- .../cloudstack/reservation/ReservationVO.java | 8 ++++---- .../cloudstack/reservation/dao/ReservationDao.java | 4 ++-- .../reservation/dao/ReservationDaoImpl.java | 12 ++++++------ .../cloud/resourcelimit/CheckedReservation.java | 14 +++++++------- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index a40030b90af7..03db1eab1b73 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -27,7 +27,7 @@ public class ReservationVO implements ResourceReservation { long domainId; @Column(name = "resource_type", nullable = false) - Resource.ResourceType type; + Resource.ResourceType resourceType; @Column(name = "amount") long amount; @@ -35,13 +35,13 @@ public class ReservationVO implements ResourceReservation { protected ReservationVO() {} - public ReservationVO(Long accountId, Long domainId, Resource.ResourceType type, Long delta) { + public ReservationVO(Long accountId, Long domainId, Resource.ResourceType resourceType, Long delta) { if (delta == null || delta <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } this.accountId = accountId; this.domainId = domainId; - this.type = type; + this.resourceType = resourceType; this.amount = delta; } @@ -62,7 +62,7 @@ public Long getDomainId() { @Override public Resource.ResourceType getResourceType() { - return type; + return resourceType; } @Override diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java index ea00ba77c4f9..a70c26fe20dc 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java @@ -5,6 +5,6 @@ import com.cloud.utils.db.GenericDao; public interface ReservationDao extends GenericDao { - long getAccountReservation(Long account, Resource.ResourceType type); - long getDomainReservation(Long domain, Resource.ResourceType type); + long getAccountReservation(Long account, Resource.ResourceType resourceType); + long getDomainReservation(Long domain, Resource.ResourceType resourceType); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java index b728a1d87dd6..ce6a4069d9a3 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -17,21 +17,21 @@ public class ReservationDaoImpl extends GenericDaoBase impl public ReservationDaoImpl() { listAccountAndTypeSearch = createSearchBuilder(); listAccountAndTypeSearch.and("accountId", listAccountAndTypeSearch.entity().getAccountId(), SearchCriteria.Op.EQ); - listAccountAndTypeSearch.and("resource_type", listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listAccountAndTypeSearch.and("resourceType", listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); listAccountAndTypeSearch.done(); listDomainAndTypeSearch = createSearchBuilder(); listDomainAndTypeSearch.and("domainId", listDomainAndTypeSearch.entity().getDomainId(), SearchCriteria.Op.EQ); - listDomainAndTypeSearch.and("resource_type", listDomainAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listDomainAndTypeSearch.and("resourceType", listDomainAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); listDomainAndTypeSearch.done(); } @Override - public long getAccountReservation(Long accountId, Resource.ResourceType type) { + public long getAccountReservation(Long accountId, Resource.ResourceType resourceType) { long total = 0; SearchCriteria sc = listAccountAndTypeSearch.create(); sc.setParameters("accountId", accountId); - sc.setParameters("resource_type", type); + sc.setParameters("resourceType", resourceType); List reservations = listBy(sc); for (ReservationVO reservation : reservations) { total += reservation.getReservedAmount(); @@ -40,11 +40,11 @@ public long getAccountReservation(Long accountId, Resource.ResourceType type) { } @Override - public long getDomainReservation(Long domainId, Resource.ResourceType type) { + public long getDomainReservation(Long domainId, Resource.ResourceType resourceType) { long total = 0; SearchCriteria sc = listAccountAndTypeSearch.create(); sc.setParameters("domainId", domainId); - sc.setParameters("resource_type", type); + sc.setParameters("resourceType", resourceType); List reservations = listBy(sc); for (ReservationVO reservation : reservations) { total += reservation.getReservedAmount(); diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 48a41cd351cf..7164d41fa0a0 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -20,7 +20,7 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { @Inject ResourceLimitService resourceLimitService; private final Account account; - private final Resource.ResourceType type; + private final Resource.ResourceType resourceType; private final Long amount; private ResourceReservation reservation; @@ -32,27 +32,27 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { * @param amount positive number of the resource type to reserve * @throws ResourceAllocationException */ - public CheckedReservation(Account account, Resource.ResourceType type, Long amount) throws ResourceAllocationException { + public CheckedReservation(Account account, Resource.ResourceType resourceType, Long amount) throws ResourceAllocationException { if (amount == null || amount <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } this.account = account; - this.type = type; + this.resourceType = resourceType; this.amount = amount; // synchronised?: - String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), type); + String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType); GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { try { - resourceLimitService.checkResourceLimit(account,type,amount); - ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), type, amount); + resourceLimitService.checkResourceLimit(account,resourceType,amount); + ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount); this.reservation = reservationDao.persist(reservationVO); } finally { quotaLimitLock.unlock(); } } else { - throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), type); + throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), resourceType); } } From e44c1f9fe54c110488b4cdf776d5b379efcbe33e Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 30 Aug 2022 12:43:25 +0200 Subject: [PATCH 06/19] amount getter and injection fallback --- .../org/apache/cloudstack/reservation/ReservationVO.java | 2 +- .../java/com/cloud/resourcelimit/CheckedReservation.java | 8 ++++++-- .../com/cloud/resourcelimit/ResourceLimitManagerImpl.java | 4 +++- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 8 +++++++- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index 03db1eab1b73..3f3b1af3ba31 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -67,6 +67,6 @@ public Resource.ResourceType getResourceType() { @Override public Long getReservedAmount() { - return null; + return amount; } } diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 7164d41fa0a0..3f29ae9a7427 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -32,22 +32,26 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { * @param amount positive number of the resource type to reserve * @throws ResourceAllocationException */ - public CheckedReservation(Account account, Resource.ResourceType resourceType, Long amount) throws ResourceAllocationException { + public CheckedReservation(Account account, Resource.ResourceType resourceType, Long amount, ReservationDao reservationDao, ResourceLimitService resourceLimitService) throws ResourceAllocationException { if (amount == null || amount <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } + this.resourceLimitService = resourceLimitService; + this.reservationDao = reservationDao; this.account = account; this.resourceType = resourceType; this.amount = amount; // synchronised?: - String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType); + String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { try { resourceLimitService.checkResourceLimit(account,resourceType,amount); ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount); this.reservation = reservationDao.persist(reservationVO); + } catch (NullPointerException npe) { + throw new CloudRuntimeException("not enough means to check limits", npe); } finally { quotaLimitLock.unlock(); } diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index 65c224571049..27c44f4df2f4 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -139,6 +139,8 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim @Inject private ResourceLimitDao _resourceLimitDao; @Inject + private ResourceLimitService resourceLimitService; + @Inject private ReservationDao reservationDao; @Inject protected SnapshotDao _snapshotDao; @@ -1109,7 +1111,7 @@ public void changeResourceCount(long accountId, ResourceType type, Boolean displ @Override public ResourceReservation getReservation(final Account account, final Boolean displayResource, final Resource.ResourceType type, final Long delta) throws ResourceAllocationException { if (! Boolean.FALSE.equals(displayResource)) { - return new CheckedReservation(account, type, delta); + return new CheckedReservation(account, type, delta, reservationDao, resourceLimitService); } throw new CloudRuntimeException("no reservation needed for resources that display as false"); } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index d1e4244e2e94..60cfe06ce447 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -113,6 +113,7 @@ import org.apache.cloudstack.framework.config.dao.ConfigurationDao; import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.query.QueryService; +import org.apache.cloudstack.reservation.dao.ReservationDao; import org.apache.cloudstack.snapshot.SnapshotHelper; import org.apache.cloudstack.storage.command.DeleteCommand; import org.apache.cloudstack.storage.command.DettachCommand; @@ -561,6 +562,11 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir @Autowired @Qualifier("networkHelper") protected NetworkHelper nwHelper; + @Inject + ReservationDao reservationDao; + @Inject + ResourceLimitService resourceLimitService; + @Inject private StatsCollector statsCollector; @@ -3871,7 +3877,7 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe } private UserVm getUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { - try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l)) { + try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l, reservationDao, resourceLimitService)) { if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { resourceLimitCheck(owner, isDisplayVm, new Long(offering.getCpu()), new Long(offering.getRamSize())); } From 938c5204a644d8c449a88031d2d2ea8f3b798a72 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 30 Aug 2022 14:58:40 +0200 Subject: [PATCH 07/19] conditional try with resource --- .../resourcelimit/CheckedReservation.java | 6 ----- .../java/com/cloud/vm/UserVmManagerImpl.java | 26 +++++++++++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 3f29ae9a7427..6f939a6dec23 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -10,15 +10,10 @@ import org.apache.cloudstack.reservation.ReservationVO; import org.apache.cloudstack.reservation.dao.ReservationDao; -import javax.inject.Inject; - public class CheckedReservation implements AutoCloseable, ResourceReservation { private static final int TRY_TO_GET_LOCK_TIME = 60; - @Inject ReservationDao reservationDao; - @Inject - ResourceLimitService resourceLimitService; private final Account account; private final Resource.ResourceType resourceType; private final Long amount; @@ -36,7 +31,6 @@ public CheckedReservation(Account account, Resource.ResourceType resourceType, L if (amount == null || amount <= 0) { throw new CloudRuntimeException("resource reservations can not be made for no resources"); } - this.resourceLimitService = resourceLimitService; this.reservationDao = reservationDao; this.account = account; this.resourceType = resourceType; diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 60cfe06ce447..d01df3f75248 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3864,7 +3864,7 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe DiskOfferingVO diskOffering = _diskOfferingDao.findById(diskOfferingId); volumesSize += verifyAndGetDiskSize(diskOffering, diskSize); } - UserVm vm = getUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); + UserVm vm = getCheckedUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); _securityGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList); @@ -3875,15 +3875,25 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe CallContext.current().putContextParameter(VirtualMachine.class, vm.getUuid()); return vm; } - - private UserVm getUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { - try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l, reservationDao, resourceLimitService)) { - if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { - resourceLimitCheck(owner, isDisplayVm, new Long(offering.getCpu()), new Long(offering.getRamSize())); + private UserVm getCheckedUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { + if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { + try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l, reservationDao, resourceLimitService)) { + return getUncheckedUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); + } catch (ResourceAllocationException | CloudRuntimeException e) { + throw e; + } catch (Exception e) { + s_logger.error("error during resource reservation and allocation", e); + throw new CloudRuntimeException(e); } - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.volume, (isIso || diskOfferingId == null ? 1 : 2)); - _resourceLimitMgr.checkResourceLimit(owner, ResourceType.primary_storage, volumesSize); + } else { + return getUncheckedUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); + } + } + + private UserVm getUncheckedUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { + try (CheckedReservation volumeReservation = new CheckedReservation(owner, ResourceType.volume, (isIso || diskOfferingId == null ? 1l : 2), reservationDao, resourceLimitService); + CheckedReservation primaryStorageReservation = new CheckedReservation(owner, ResourceType.primary_storage, volumesSize, reservationDao, resourceLimitService)) { // verify security group ids if (securityGroupIdList != null) { From 69585fc22df7700bbb1655fe1cf94b272e22fa68 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Tue, 30 Aug 2022 15:11:47 +0200 Subject: [PATCH 08/19] integration test for parallel vm creations --- .../smoke/test_deploy_vms_in_parallel.py | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 test/integration/smoke/test_deploy_vms_in_parallel.py diff --git a/test/integration/smoke/test_deploy_vms_in_parallel.py b/test/integration/smoke/test_deploy_vms_in_parallel.py new file mode 100644 index 000000000000..03049a29a914 --- /dev/null +++ b/test/integration/smoke/test_deploy_vms_in_parallel.py @@ -0,0 +1,172 @@ +# 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 +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" P1 for Deploy VM from ISO +""" +# Import Local Modules +from nose.plugins.attrib import attr +from marvin.cloudstackTestCase import cloudstackTestCase +from marvin.lib.base import (Account, + DiskOffering, + Domain, + Resources, + ServiceOffering, + VirtualMachine) +from marvin.lib.common import (get_zone, + get_domain, + get_test_template) +from marvin.cloudstackAPI import (deployVirtualMachine, + queryAsyncJobResult) + + +class TestDeployVMsInParallel(cloudstackTestCase): + + @classmethod + def setUpClass(cls): + + cls.testClient = super(TestDeployVMsInParallel, cls).getClsTestClient() + cls.api_client = cls.testClient.getApiClient() + + cls.testdata = cls.testClient.getParsedTestDataConfig() + # Get Zone, Domain and templates + cls.domain = get_domain(cls.api_client) + cls.zone = get_zone(cls.api_client, cls.testClient.getZoneForTests()) + cls.hypervisor = cls.testClient.getHypervisorInfo() + + cls._cleanup = [] + + cls.template = get_test_template( + cls.api_client, + cls.zone.id, + cls.hypervisor + ) + + # Create service, disk offerings etc + cls.service_offering = ServiceOffering.create( + cls.api_client, + cls.testdata["service_offering"] + ) + cls._cleanup.append(cls.service_offering) + + cls.disk_offering = DiskOffering.create( + cls.api_client, + cls.testdata["disk_offering"] + ) + cls._cleanup.append(cls.disk_offering) + + return + + @classmethod + def tearDownClass(cls): + super(TestDeployVMsInParallel, cls).tearDownClass() + + def setUp(self): + + self.apiclient = self.testClient.getApiClient() + + self.dbclient = self.testClient.getDbConnection() + self.cleanup = [] + self.hypervisor = self.testClient.getHypervisorInfo() + self.testdata["virtual_machine"]["zoneid"] = self.zone.id + self.testdata["virtual_machine"]["template"] = self.template.id + self.testdata["iso"]["zoneid"] = self.zone.id + self.domain = Domain.create( + self.apiclient, + self.testdata["domain"] + ) + self.cleanup.append(self.domain) + + self.account = Account.create( + self.apiclient, + self.testdata["account"], + domainid=self.domain.id + ) + self.cleanup.append(self.account) + + self.userApiClient = self.testClient.getUserApiClient(UserName=self.account.name, DomainName=self.domain.name) + virtual_machine = VirtualMachine.create( + self.userApiClient, + self.testdata["virtual_machine"], + accountid=self.account.name, + domainid=self.account.domainid, + templateid=self.template.id, + serviceofferingid=self.service_offering.id, + diskofferingid=self.disk_offering.id, + hypervisor=self.hypervisor + ) + self.cleanup.append(virtual_machine) + self.networkids = virtual_machine.nic[0].networkid + virtual_machine.delete(self.apiclient) + self.cleanup.remove(virtual_machine) + return + + def update_resource_limit(self, max=1): + Resources.updateLimit( + self.apiclient, + domainid=self.domain.id, + resourcetype=0, + max=max + ) + + def tearDown(self): + super(TestDeployVMsInParallel, self).tearDown() + + @attr( + tags=[ + "advanced", + "basic", + "sg"], + required_hardware="false") + def test_deploy_more_vms_than_limit_allows(self): + """ + Test Deploy Virtual Machines in parallel + + Validate the following: + 1. set limit to 2 + 2. deploy more than 2 VMs + """ + self.test_limits(vm_limit=2) + + def test_limits(self, vm_limit=1): + self.info(f"==== limit: {vm_limit} ====") + + self.update_resource_limit(max=vm_limit) + + cmd = deployVirtualMachine.deployVirtualMachineCmd() + cmd.serviceofferingid=self.service_offering.id + cmd.diskofferingid=self.disk_offering.id + cmd.templateid=self.template.id + cmd.accountid=self.account.id + cmd.domainid=self.account.domainid + cmd.zoneid=self.zone.id + cmd.networkids = self.networkids + cmd.isAsync = "false" + + responses = [] + failed = 0 + for i in range(vm_limit+3): + try: + self.info(f"==== deploying instance #{i}") + response = self.userApiClient.deployVirtualMachine(cmd, method="GET") + responses.append(response) + except Exception as e: + failed += 1 + + self.info(f"==== failed deploys: {failed} ====") + + self.assertEqual(failed, 3) + self.assertEqual(len(responses), vm_limit) # we don´t care if the deploy succeed or failed for some other reason From 7da90512dd9a53a046b9951f30b8a933be94b132 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 31 Aug 2022 09:58:10 +0200 Subject: [PATCH 09/19] licences --- .../cloudstack/user/ResourceReservation.java | 18 ++++++++++++++++++ .../cloudstack/reservation/ReservationVO.java | 18 ++++++++++++++++++ .../reservation/dao/ReservationDao.java | 18 ++++++++++++++++++ .../reservation/dao/ReservationDaoImpl.java | 18 ++++++++++++++++++ .../resourcelimit/CheckedReservation.java | 18 ++++++++++++++++++ 5 files changed, 90 insertions(+) diff --git a/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java index ece59a46a369..170193570cf1 100644 --- a/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java +++ b/api/src/main/java/org/apache/cloudstack/user/ResourceReservation.java @@ -1,3 +1,21 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package org.apache.cloudstack.user; import com.cloud.configuration.Resource; diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java index 3f3b1af3ba31..e5636f0bfc99 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/ReservationVO.java @@ -1,3 +1,21 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package org.apache.cloudstack.reservation; import com.cloud.configuration.Resource; diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java index a70c26fe20dc..eead91c7b8e9 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDao.java @@ -1,3 +1,21 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package org.apache.cloudstack.reservation.dao; import com.cloud.configuration.Resource; diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java index ce6a4069d9a3..b92664d065f4 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -1,3 +1,21 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package org.apache.cloudstack.reservation.dao; import com.cloud.configuration.Resource; diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 6f939a6dec23..8bdae5c9d5cb 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -1,3 +1,21 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package com.cloud.resourcelimit; import com.cloud.configuration.Resource; From 576d5141601f4cac4e1f91d7f7aa495010f7d2d3 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 31 Aug 2022 11:14:15 +0200 Subject: [PATCH 10/19] add cpu and mem checks --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index d01df3f75248..b2040bce04e9 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3877,7 +3877,10 @@ private UserVm createVirtualMachine(DataCenter zone, ServiceOffering serviceOffe } private UserVm getCheckedUserVmResource(DataCenter zone, String hostName, String displayName, Account owner, Long diskOfferingId, Long diskSize, List networkList, List securityGroupIdList, String group, HTTPMethod httpmethod, String userData, List sshKeyPairs, Account caller, Map requestedIps, IpAddresses defaultIps, Boolean isDisplayVm, String keyboard, List affinityGroupIdList, Map customParameters, String customId, Map> dhcpOptionMap, Map datadiskTemplateToDiskOfferringMap, Map userVmOVFPropertiesMap, boolean dynamicScalingEnabled, String vmType, VMTemplateVO template, HypervisorType hypervisorType, long accountId, ServiceOfferingVO offering, boolean isIso, Long rootDiskOfferingId, long volumesSize) throws ResourceAllocationException, StorageUnavailableException, InsufficientCapacityException { if (!VirtualMachineManager.ResourceCountRunningVMsonly.value()) { - try (CheckedReservation reservation = new CheckedReservation(owner, ResourceType.user_vm, 1l, reservationDao, resourceLimitService)) { + try (CheckedReservation vmReservation = new CheckedReservation(owner, ResourceType.user_vm, 1l, reservationDao, resourceLimitService); + CheckedReservation cpuReservation = new CheckedReservation(owner, ResourceType.cpu, Long.valueOf(offering.getCpu()), reservationDao, resourceLimitService); + CheckedReservation memReservation = new CheckedReservation(owner, ResourceType.memory, Long.valueOf(offering.getRamSize()), reservationDao, resourceLimitService); + ) { return getUncheckedUserVmResource(zone, hostName, displayName, owner, diskOfferingId, diskSize, networkList, securityGroupIdList, group, httpmethod, userData, sshKeyPairs, caller, requestedIps, defaultIps, isDisplayVm, keyboard, affinityGroupIdList, customParameters, customId, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, template, hypervisorType, accountId, offering, isIso, rootDiskOfferingId, volumesSize); } catch (ResourceAllocationException | CloudRuntimeException e) { throw e; From 3a5a28da1e44f2b1fcb12c9f99c74e9d4533d6a0 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 31 Aug 2022 13:26:55 +0200 Subject: [PATCH 11/19] bug hunting and sonar issues --- .../reservation/dao/ReservationDaoImpl.java | 19 +++--- .../resourcelimit/CheckedReservation.java | 64 +++++++++++-------- 2 files changed, 50 insertions(+), 33 deletions(-) diff --git a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java index b92664d065f4..2b8b74224f39 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/reservation/dao/ReservationDaoImpl.java @@ -28,19 +28,22 @@ public class ReservationDaoImpl extends GenericDaoBase implements ReservationDao { + private static final String RESOURCE_TYPE = "resourceType"; + private static final String ACCOUNT_ID = "accountId"; + private static final String DOMAIN_ID = "domainId"; private final SearchBuilder listAccountAndTypeSearch; private final SearchBuilder listDomainAndTypeSearch; public ReservationDaoImpl() { listAccountAndTypeSearch = createSearchBuilder(); - listAccountAndTypeSearch.and("accountId", listAccountAndTypeSearch.entity().getAccountId(), SearchCriteria.Op.EQ); - listAccountAndTypeSearch.and("resourceType", listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listAccountAndTypeSearch.and(ACCOUNT_ID, listAccountAndTypeSearch.entity().getAccountId(), SearchCriteria.Op.EQ); + listAccountAndTypeSearch.and(RESOURCE_TYPE, listAccountAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); listAccountAndTypeSearch.done(); listDomainAndTypeSearch = createSearchBuilder(); - listDomainAndTypeSearch.and("domainId", listDomainAndTypeSearch.entity().getDomainId(), SearchCriteria.Op.EQ); - listDomainAndTypeSearch.and("resourceType", listDomainAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); + listDomainAndTypeSearch.and(DOMAIN_ID, listDomainAndTypeSearch.entity().getDomainId(), SearchCriteria.Op.EQ); + listDomainAndTypeSearch.and(RESOURCE_TYPE, listDomainAndTypeSearch.entity().getResourceType(), SearchCriteria.Op.EQ); listDomainAndTypeSearch.done(); } @@ -48,8 +51,8 @@ public ReservationDaoImpl() { public long getAccountReservation(Long accountId, Resource.ResourceType resourceType) { long total = 0; SearchCriteria sc = listAccountAndTypeSearch.create(); - sc.setParameters("accountId", accountId); - sc.setParameters("resourceType", resourceType); + sc.setParameters(ACCOUNT_ID, accountId); + sc.setParameters(RESOURCE_TYPE, resourceType); List reservations = listBy(sc); for (ReservationVO reservation : reservations) { total += reservation.getReservedAmount(); @@ -61,8 +64,8 @@ public long getAccountReservation(Long accountId, Resource.ResourceType resource public long getDomainReservation(Long domainId, Resource.ResourceType resourceType) { long total = 0; SearchCriteria sc = listAccountAndTypeSearch.create(); - sc.setParameters("domainId", domainId); - sc.setParameters("resourceType", resourceType); + sc.setParameters(DOMAIN_ID, domainId); + sc.setParameters(RESOURCE_TYPE, resourceType); List reservations = listBy(sc); for (ReservationVO reservation : reservations) { total += reservation.getReservedAmount(); diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 8bdae5c9d5cb..5b5ae3b71e38 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -18,7 +18,7 @@ // package com.cloud.resourcelimit; -import com.cloud.configuration.Resource; +import com.cloud.configuration.Resource.ResourceType; import com.cloud.exception.ResourceAllocationException; import com.cloud.user.Account; import com.cloud.user.ResourceLimitService; @@ -27,14 +27,17 @@ import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.reservation.ReservationVO; import org.apache.cloudstack.reservation.dao.ReservationDao; +import org.apache.log4j.Logger; + public class CheckedReservation implements AutoCloseable, ResourceReservation { + private static Logger LOG = Logger.getLogger(CheckedReservation.class); private static final int TRY_TO_GET_LOCK_TIME = 60; ReservationDao reservationDao; private final Account account; - private final Resource.ResourceType resourceType; - private final Long amount; + private final ResourceType resourceType; + private Long amount; private ResourceReservation reservation; /** @@ -45,37 +48,48 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { * @param amount positive number of the resource type to reserve * @throws ResourceAllocationException */ - public CheckedReservation(Account account, Resource.ResourceType resourceType, Long amount, ReservationDao reservationDao, ResourceLimitService resourceLimitService) throws ResourceAllocationException { - if (amount == null || amount <= 0) { - throw new CloudRuntimeException("resource reservations can not be made for no resources"); - } + public CheckedReservation(Account account, ResourceType resourceType, Long amount, ReservationDao reservationDao, ResourceLimitService resourceLimitService) throws ResourceAllocationException { this.reservationDao = reservationDao; this.account = account; this.resourceType = resourceType; this.amount = amount; + this.reservation = null; + if (this.amount != null || this.amount <= 0) { + if(LOG.isDebugEnabled()){ + LOG.debug(String.format("not reserving no amount of resources for %s in domain %d, type: %s, %s ", account.getAccountName(), account.getDomainId(), resourceType, amount)); + } + this.amount = null; + } - // synchronised?: - String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); - GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); - if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { - try { - resourceLimitService.checkResourceLimit(account,resourceType,amount); - ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount); - this.reservation = reservationDao.persist(reservationVO); - } catch (NullPointerException npe) { - throw new CloudRuntimeException("not enough means to check limits", npe); - } finally { - quotaLimitLock.unlock(); + if (this.amount != null) { + String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); + GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); + if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { + try { + resourceLimitService.checkResourceLimit(account,resourceType,amount); + ReservationVO reservationVO = new ReservationVO(account.getAccountId(), account.getDomainId(), resourceType, amount); + this.reservation = reservationDao.persist(reservationVO); + } catch (NullPointerException npe) { + throw new CloudRuntimeException("not enough means to check limits", npe); + } finally { + quotaLimitLock.unlock(); + } + } else { + throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), resourceType); } } else { - throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), resourceType); + if(LOG.isDebugEnabled()){ + LOG.debug(String.format("not reserving no amount of resources for %s in domain %d, type: %s ", account.getAccountName(), account.getDomainId(), resourceType)); + } } } @Override public void close() throws Exception { - // delete the reservation vo - reservationDao.remove(reservation.getId()); + if (this.reservation != null){ + reservationDao.remove(reservation.getId()); + reservation = null; + } } public Account getAccount() { @@ -93,13 +107,13 @@ public Long getDomainId() { } @Override - public Resource.ResourceType getResourceType() { - return null; + public ResourceType getResourceType() { + return resourceType; } @Override public Long getReservedAmount() { - return null; + return amount; } @Override From 04b14cf98fd77e431f1ab4d75993a34871364322 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 31 Aug 2022 14:24:06 +0200 Subject: [PATCH 12/19] logic error --- .../main/java/com/cloud/resourcelimit/CheckedReservation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 5b5ae3b71e38..a1295da7d328 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -54,7 +54,7 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun this.resourceType = resourceType; this.amount = amount; this.reservation = null; - if (this.amount != null || this.amount <= 0) { + if (this.amount != null && this.amount <= 0) { if(LOG.isDebugEnabled()){ LOG.debug(String.format("not reserving no amount of resources for %s in domain %d, type: %s, %s ", account.getAccountName(), account.getDomainId(), resourceType, amount)); } From 589fd31d9742b7e95c09653c915f58c96304e807 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Wed, 31 Aug 2022 16:04:37 +0200 Subject: [PATCH 13/19] checked and tested --- .../resourcelimit/CheckedReservation.java | 2 +- .../resourcelimit/CheckedReservationTest.java | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index a1295da7d328..c5aecb80d179 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -31,7 +31,7 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { - private static Logger LOG = Logger.getLogger(CheckedReservation.class); + private static final Logger LOG = Logger.getLogger(CheckedReservation.class); private static final int TRY_TO_GET_LOCK_TIME = 60; ReservationDao reservationDao; diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java new file mode 100644 index 000000000000..80c0fa7deaec --- /dev/null +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -0,0 +1,52 @@ +package com.cloud.resourcelimit; + +import com.cloud.configuration.Resource; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.user.Account; +import com.cloud.user.ResourceLimitService; +import org.apache.cloudstack.reservation.ReservationVO; +import org.apache.cloudstack.reservation.dao.ReservationDao; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; + +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; + +public class CheckedReservationTest { + + @Mock + Account account; + @Mock + ReservationDao reservationDao; + @Mock + ResourceLimitService resourceLimitService; + + @Mock + ReservationVO reservation = new ReservationVO(1l, 1l, Resource.ResourceType.user_vm, 1l); + + @Before + public void setup() { + initMocks(this); + when(reservation.getId()).thenReturn(1l); + } + @Test + public void getId() { + when(reservationDao.persist(any())).thenReturn(reservation); + when(account.getAccountId()).thenReturn(1l); + boolean fail = false; + long id = 0l; + try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { + id = cr.getId(); + } catch (NullPointerException npe) { + fail = true; + } catch (ResourceAllocationException rae) { + // too bad + } catch (Exception e) { + throw new RuntimeException(e); + } + assertTrue(id == 1l); + } +} \ No newline at end of file From d4b2ba154dc835d50262781c412b9db4c20f65bc Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 1 Sep 2022 09:56:23 +0200 Subject: [PATCH 14/19] license --- .../resourcelimit/CheckedReservationTest.java | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index 80c0fa7deaec..34947a4e934b 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -1,9 +1,28 @@ +// +// 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 +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// package com.cloud.resourcelimit; import com.cloud.configuration.Resource; import com.cloud.exception.ResourceAllocationException; import com.cloud.user.Account; import com.cloud.user.ResourceLimitService; +import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.reservation.ReservationVO; import org.apache.cloudstack.reservation.dao.ReservationDao; import org.junit.Before; @@ -11,6 +30,7 @@ import org.mockito.Mock; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; @@ -32,6 +52,7 @@ public void setup() { initMocks(this); when(reservation.getId()).thenReturn(1l); } + @Test public void getId() { when(reservationDao.persist(any())).thenReturn(reservation); @@ -41,12 +62,30 @@ public void getId() { try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { id = cr.getId(); } catch (NullPointerException npe) { - fail = true; + fail("NPE caught"); } catch (ResourceAllocationException rae) { - // too bad + throw new CloudRuntimeException(rae); } catch (Exception e) { throw new RuntimeException(e); } assertTrue(id == 1l); } + + @Test + public void getNoAmount() { + when(reservationDao.persist(any())).thenReturn(reservation); + when(account.getAccountId()).thenReturn(1l); + boolean fail = false; + Long id = 0l; + try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,-11l, reservationDao, resourceLimitService); ) { + id = cr.getReservedAmount(); + } catch (NullPointerException npe) { + fail("NPE caught"); + } catch (ResourceAllocationException rae) { + throw new CloudRuntimeException(rae); + } catch (Exception e) { + throw new RuntimeException(e); + } + assertTrue(id == null); + } } \ No newline at end of file From 0f5e2658bea86704db02ac57145aabf01f56e3b0 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 1 Sep 2022 10:35:23 +0200 Subject: [PATCH 15/19] unit test fails in travis --- .../main/java/com/cloud/resourcelimit/CheckedReservation.java | 2 +- .../java/com/cloud/resourcelimit/CheckedReservationTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index c5aecb80d179..2282e1bf6b68 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -33,7 +33,7 @@ public class CheckedReservation implements AutoCloseable, ResourceReservation { private static final Logger LOG = Logger.getLogger(CheckedReservation.class); - private static final int TRY_TO_GET_LOCK_TIME = 60; + private static final int TRY_TO_GET_LOCK_TIME = 120; ReservationDao reservationDao; private final Account account; private final ResourceType resourceType; diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index 34947a4e934b..14e248b8f546 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -57,6 +57,7 @@ public void setup() { public void getId() { when(reservationDao.persist(any())).thenReturn(reservation); when(account.getAccountId()).thenReturn(1l); + when(account.getDomainId()).thenReturn(4l); boolean fail = false; long id = 0l; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { From f80e7b76f53ed24893a9714ca112de11ed436375 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 1 Sep 2022 15:35:06 +0200 Subject: [PATCH 16/19] small refactor --- .../resourcelimit/CheckedReservation.java | 18 +++++++++++++++--- .../resourcelimit/CheckedReservationTest.java | 13 ++++++++++++- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 2282e1bf6b68..83ffb9c5dee9 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -28,12 +28,14 @@ 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 { private static final Logger LOG = Logger.getLogger(CheckedReservation.class); private static final int TRY_TO_GET_LOCK_TIME = 120; + private GlobalLock quotaLimitLock; ReservationDao reservationDao; private final Account account; private final ResourceType resourceType; @@ -54,6 +56,7 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun this.resourceType = resourceType; this.amount = amount; this.reservation = null; + setGlobalLock(account, resourceType); if (this.amount != null && this.amount <= 0) { if(LOG.isDebugEnabled()){ LOG.debug(String.format("not reserving no amount of resources for %s in domain %d, type: %s, %s ", account.getAccountName(), account.getDomainId(), resourceType, amount)); @@ -62,8 +65,6 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun } if (this.amount != null) { - String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); - GlobalLock quotaLimitLock = GlobalLock.getInternLock(lockName); if(quotaLimitLock.lock(TRY_TO_GET_LOCK_TIME)) { try { resourceLimitService.checkResourceLimit(account,resourceType,amount); @@ -75,7 +76,7 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun quotaLimitLock.unlock(); } } else { - throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", lockName), resourceType); + throw new ResourceAllocationException(String.format("unable to acquire resource reservation \"%s\"", quotaLimitLock.getName()), resourceType); } } else { if(LOG.isDebugEnabled()){ @@ -84,6 +85,17 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun } } + @NotNull + private void setGlobalLock(Account account, ResourceType resourceType) { + final GlobalLock quotaLimitLock; + String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); + setQuotaLimitLock(GlobalLock.getInternLock(lockName)); + } + + protected void setQuotaLimitLock(GlobalLock quotaLimitLock) { + this.quotaLimitLock = quotaLimitLock; + } + @Override public void close() throws Exception { if (this.reservation != null){ diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index 14e248b8f546..48fa19a28341 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -22,19 +22,23 @@ import com.cloud.exception.ResourceAllocationException; import com.cloud.user.Account; import com.cloud.user.ResourceLimitService; +import com.cloud.utils.db.GlobalLock; import com.cloud.utils.exception.CloudRuntimeException; import org.apache.cloudstack.reservation.ReservationVO; import org.apache.cloudstack.reservation.dao.ReservationDao; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; +import org.powermock.core.classloader.annotations.PrepareForTest; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; +@PrepareForTest(CheckedReservation.class) public class CheckedReservationTest { @Mock @@ -47,6 +51,9 @@ public class CheckedReservationTest { @Mock ReservationVO reservation = new ReservationVO(1l, 1l, Resource.ResourceType.user_vm, 1l); + @Mock + GlobalLock quotaLimitLock; + @Before public void setup() { initMocks(this); @@ -58,6 +65,7 @@ public void getId() { when(reservationDao.persist(any())).thenReturn(reservation); when(account.getAccountId()).thenReturn(1l); when(account.getDomainId()).thenReturn(4l); + when(quotaLimitLock.lock(anyInt())).thenReturn(true); boolean fail = false; long id = 0l; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { @@ -65,7 +73,10 @@ public void getId() { } catch (NullPointerException npe) { fail("NPE caught"); } catch (ResourceAllocationException rae) { - throw new CloudRuntimeException(rae); + // this does not work on all plafroms because of the static methods being used in the global lock mechanism + // normally one would + // throw new CloudRuntimeException(rae); + // but we'll ignore this for platforms that can not humour the static bits of the system. } catch (Exception e) { throw new RuntimeException(e); } From eb259a8f53ad231dbca0eab84bf009d813840cf7 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 1 Sep 2022 16:04:54 +0200 Subject: [PATCH 17/19] test reorganise --- .../cloud/resourcelimit/CheckedReservationTest.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index 48fa19a28341..b36918e0a464 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -67,9 +67,9 @@ public void getId() { when(account.getDomainId()).thenReturn(4l); when(quotaLimitLock.lock(anyInt())).thenReturn(true); boolean fail = false; - long id = 0l; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { - id = cr.getId(); + long id = cr.getId(); + assertTrue(id == 1l); } catch (NullPointerException npe) { fail("NPE caught"); } catch (ResourceAllocationException rae) { @@ -80,7 +80,6 @@ public void getId() { } catch (Exception e) { throw new RuntimeException(e); } - assertTrue(id == 1l); } @Test @@ -88,9 +87,9 @@ public void getNoAmount() { when(reservationDao.persist(any())).thenReturn(reservation); when(account.getAccountId()).thenReturn(1l); boolean fail = false; - Long id = 0l; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,-11l, reservationDao, resourceLimitService); ) { - id = cr.getReservedAmount(); + Long amount = cr.getReservedAmount(); + assertTrue(amount == null); } catch (NullPointerException npe) { fail("NPE caught"); } catch (ResourceAllocationException rae) { @@ -98,6 +97,5 @@ public void getNoAmount() { } catch (Exception e) { throw new RuntimeException(e); } - assertTrue(id == null); } } \ No newline at end of file From 08b3e442b502b16382cbd18db909563fbc82e8c0 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 8 Sep 2022 16:38:04 +0200 Subject: [PATCH 18/19] sonar --- .../resourcelimit/CheckedReservation.java | 1 - .../java/com/cloud/vm/UserVmManagerImpl.java | 32 +++++++++++-------- .../resourcelimit/CheckedReservationTest.java | 6 ++-- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java index 83ffb9c5dee9..0075ef1b4f1a 100644 --- a/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java +++ b/server/src/main/java/com/cloud/resourcelimit/CheckedReservation.java @@ -87,7 +87,6 @@ public CheckedReservation(Account account, ResourceType resourceType, Long amoun @NotNull private void setGlobalLock(Account account, ResourceType resourceType) { - final GlobalLock quotaLimitLock; String lockName = String.format("CheckedReservation-%s/%d", account.getDomainId(), resourceType.getOrdinal()); setQuotaLimitLock(GlobalLock.getInternLock(lockName)); } diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index b2040bce04e9..c6487d86f76d 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -4069,15 +4069,15 @@ private UserVm getUncheckedUserVmResource(DataCenter zone, String hostName, Stri profile.setDefaultNic(true); if (!_networkModel.areServicesSupportedInNetwork(network.getId(), new Service[]{Service.UserData})) { if ((userData != null) && (!userData.isEmpty())) { - throw new InvalidParameterValueException("Unable to deploy VM as UserData is provided while deploying the VM, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + throw new InvalidParameterValueException(String.format("Unable to deploy VM as UserData is provided while deploying the VM, but there is no support for %s service in the default network %s/%s.", Service.UserData.getName(), network.getName(), network.getUuid())); } if ((sshPublicKeys != null) && (!sshPublicKeys.isEmpty())) { - throw new InvalidParameterValueException("Unable to deploy VM as SSH keypair is provided while deploying the VM, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + throw new InvalidParameterValueException(String.format("Unable to deploy VM as SSH keypair is provided while deploying the VM, but there is no support for %s service in the default network %s/%s", Service.UserData.getName(), network.getName(), network.getUuid())); } if (template.isEnablePassword()) { - throw new InvalidParameterValueException("Unable to deploy VM as template " + template.getId() + " is password enabled, but there is no support for " + Service.UserData.getName() + " service in the default network " + network.getId()); + throw new InvalidParameterValueException(String.format("Unable to deploy VM as template %s is password enabled, but there is no support for %s service in the default network %s/%s", template.getId(), Service.UserData.getName(), network.getName(), network.getUuid())); } } } @@ -4174,17 +4174,7 @@ private UserVm getUncheckedUserVmResource(DataCenter zone, String hostName, Stri isIso, sshPublicKeys, networkNicMap, id, instanceName, uuidName, hypervisorType, customParameters, dhcpOptionMap, datadiskTemplateToDiskOfferringMap, userVmOVFPropertiesMap, dynamicScalingEnabled, vmType, rootDiskOfferingId, keypairnames); - // Assign instance to the group - try { - if (group != null) { - boolean addToGroup = addInstanceToGroup(Long.valueOf(id), group); - if (!addToGroup) { - throw new CloudRuntimeException("Unable to assign Vm to the group " + group); - } - } - } catch (Exception ex) { - throw new CloudRuntimeException("Unable to assign Vm to the group " + group); - } + assignInstanceToGroup(group, id); return vm; } catch (ResourceAllocationException | CloudRuntimeException e) { throw e; @@ -4194,6 +4184,20 @@ private UserVm getUncheckedUserVmResource(DataCenter zone, String hostName, Stri } } + private void assignInstanceToGroup(String group, long id) { + // Assign instance to the group + try { + if (group != null) { + boolean addToGroup = addInstanceToGroup(Long.valueOf(id), group); + if (!addToGroup) { + throw new CloudRuntimeException("Unable to assign Vm to the group " + group); + } + } + } catch (Exception ex) { + throw new CloudRuntimeException("Unable to assign Vm to the group " + group); + } + } + private long verifyAndGetDiskSize(DiskOfferingVO diskOffering, Long diskSize) { long size = 0l; if (diskOffering == null) { diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index b36918e0a464..a2de46613cf8 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -31,6 +31,8 @@ import org.mockito.Mock; import org.powermock.core.classloader.annotations.PrepareForTest; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -69,7 +71,7 @@ public void getId() { boolean fail = false; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.user_vm,1l, reservationDao, resourceLimitService); ) { long id = cr.getId(); - assertTrue(id == 1l); + assertEquals(1l, id); } catch (NullPointerException npe) { fail("NPE caught"); } catch (ResourceAllocationException rae) { @@ -89,7 +91,7 @@ public void getNoAmount() { boolean fail = false; try (CheckedReservation cr = new CheckedReservation(account, Resource.ResourceType.cpu,-11l, reservationDao, resourceLimitService); ) { Long amount = cr.getReservedAmount(); - assertTrue(amount == null); + assertNull(amount); } catch (NullPointerException npe) { fail("NPE caught"); } catch (ResourceAllocationException rae) { From 336ba6c23fd866d739960d1b48228c1f87f4ba05 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Thu, 8 Sep 2022 17:31:05 +0200 Subject: [PATCH 19/19] assertions/import --- .../cloud/storage/template/LocalTemplateDownloaderTest.java | 6 ++---- .../com/cloud/resourcelimit/CheckedReservationTest.java | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/core/src/test/java/com/cloud/storage/template/LocalTemplateDownloaderTest.java b/core/src/test/java/com/cloud/storage/template/LocalTemplateDownloaderTest.java index 0a5d2f6657d3..482538008019 100644 --- a/core/src/test/java/com/cloud/storage/template/LocalTemplateDownloaderTest.java +++ b/core/src/test/java/com/cloud/storage/template/LocalTemplateDownloaderTest.java @@ -19,7 +19,7 @@ package com.cloud.storage.template; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertTrue; import java.io.File; @@ -33,9 +33,7 @@ public void localTemplateDownloaderTest() throws Exception { String url = new File("pom.xml").toURI().toURL().toString(); TemplateDownloader td = new LocalTemplateDownloader(null, url, System.getProperty("java.io.tmpdir"), TemplateDownloader.DEFAULT_MAX_TEMPLATE_SIZE_IN_BYTES, null); long bytes = td.download(true, null); - if (!(bytes > 0)) { - fail("Failed download"); - } + assertTrue("Failed download", bytes > 0); } } diff --git a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java index a2de46613cf8..a5ec65e913c8 100644 --- a/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/CheckedReservationTest.java @@ -33,7 +33,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt;