You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/04/07 08:14:49 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4422: Count resources of virtual routers cpu/memory
DaanHoogland commented on a change in pull request #4422:
URL: https://github.com/apache/cloudstack/pull/4422#discussion_r608427716
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -996,7 +1052,51 @@ public long calculateMemoryForAccount(long accountId) {
for (UserVmJoinVO vm : userVms) {
ramtotal += Long.valueOf(vm.getRamSize());
}
- return ramtotal;
+
+ ServiceOffering defaultRouterOffering = null;
+ String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
+ if (globalRouterOffering != null) {
+ defaultRouterOffering = _serviceOfferingDao.findByUuid(globalRouterOffering);
+ }
+ if (defaultRouterOffering == null) {
+ defaultRouterOffering = _serviceOfferingDao.findByName(ServiceOffering.routerDefaultOffUniqueName);
+ }
Review comment:
no reason to grow this method for this bit.
```suggestion
ServiceOffering defaultRouterOffering = getDefaultServiceOfferingOrSomeOtherNewName();
```
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -972,11 +980,59 @@ public long countCpusForAccount(long accountId) {
for (UserVmJoinVO vm : userVms) {
cputotal += Long.valueOf(vm.getCpu());
}
- return cputotal;
+
+ ServiceOffering defaultRouterOffering = null;
+ String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
+ if (globalRouterOffering != null) {
+ defaultRouterOffering = _serviceOfferingDao.findByUuid(globalRouterOffering);
+ }
+ if (defaultRouterOffering == null) {
+ defaultRouterOffering = _serviceOfferingDao.findByName(ServiceOffering.routerDefaultOffUniqueName);
+ }
+ GenericSearchBuilder<ServiceOfferingVO, SumCount> cpuSearch = _serviceOfferingDao.createSearchBuilder(SumCount.class);
+ cpuSearch.select("sum", Func.SUM, cpuSearch.entity().getCpu());
+ cpuSearch.select("count", Func.COUNT, (Object[])null);
+ SearchBuilder<VMInstanceVO> join1 = _vmDao.createSearchBuilder();
+ join1.and("accountId", join1.entity().getAccountId(), Op.EQ);
+ join1.and("type", join1.entity().getType(), Op.IN);
+ join1.and("state", join1.entity().getState(), SearchCriteria.Op.NIN);
+ join1.and("displayVm", join1.entity().isDisplayVm(), Op.EQ);
+ cpuSearch.join("offerings", join1, cpuSearch.entity().getId(), join1.entity().getServiceOfferingId(), JoinBuilder.JoinType.INNER);
+ cpuSearch.done();
+
+ SearchCriteria<SumCount> sc = cpuSearch.create();
+ sc.setJoinParameters("offerings", "accountId", accountId);
+ sc.setJoinParameters("offerings", "type", VirtualMachine.Type.DomainRouter); // domain routers
+
+ if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging, State.Stopped});
+ } else {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging});
+ }
+ sc.setJoinParameters("offerings", "displayVm", 1);
+ List<SumCount> cpus = _serviceOfferingDao.customSearch(sc, null);
Review comment:
should be in `ServiceOfferingDao`, no reason to grow this method for this code.
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -996,7 +1052,51 @@ public long calculateMemoryForAccount(long accountId) {
for (UserVmJoinVO vm : userVms) {
ramtotal += Long.valueOf(vm.getRamSize());
}
- return ramtotal;
+
+ ServiceOffering defaultRouterOffering = null;
+ String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
+ if (globalRouterOffering != null) {
+ defaultRouterOffering = _serviceOfferingDao.findByUuid(globalRouterOffering);
+ }
+ if (defaultRouterOffering == null) {
+ defaultRouterOffering = _serviceOfferingDao.findByName(ServiceOffering.routerDefaultOffUniqueName);
+ }
+ GenericSearchBuilder<ServiceOfferingVO, SumCount> memorySearch = _serviceOfferingDao.createSearchBuilder(SumCount.class);
+ memorySearch.select("sum", Func.SUM, memorySearch.entity().getRamSize());
+ memorySearch.select("count", Func.COUNT, (Object[])null);
+ SearchBuilder<VMInstanceVO> join1 = _vmDao.createSearchBuilder();
+ join1.and("accountId", join1.entity().getAccountId(), Op.EQ);
+ join1.and("type", join1.entity().getType(), Op.IN);
+ join1.and("state", join1.entity().getState(), SearchCriteria.Op.NIN);
+ join1.and("displayVm", join1.entity().isDisplayVm(), Op.EQ);
+ memorySearch.join("offerings", join1, memorySearch.entity().getId(), join1.entity().getServiceOfferingId(), JoinBuilder.JoinType.INNER);
+ memorySearch.done();
+
+ SearchCriteria<SumCount> sc = memorySearch.create();
+ sc.setJoinParameters("offerings", "accountId", accountId);
+ sc.setJoinParameters("offerings", "type", VirtualMachine.Type.DomainRouter); // domain routers
+ sc.setJoinParameters("offerings", "displayVm", 1);
+
+ if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging, State.Stopped});
+ } else {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging});
+ }
+ sc.setJoinParameters("offerings", "displayVm", 1);
+ List<SumCount> memory = _serviceOfferingDao.customSearch(sc, null);
Review comment:
this bit seems like it should be a member method of `ServiceOfferringDao`, or activated by flag in an existing method in `ServiceOfferringDao`
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -972,11 +980,59 @@ public long countCpusForAccount(long accountId) {
for (UserVmJoinVO vm : userVms) {
cputotal += Long.valueOf(vm.getCpu());
}
- return cputotal;
+
+ ServiceOffering defaultRouterOffering = null;
+ String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
+ if (globalRouterOffering != null) {
+ defaultRouterOffering = _serviceOfferingDao.findByUuid(globalRouterOffering);
+ }
+ if (defaultRouterOffering == null) {
+ defaultRouterOffering = _serviceOfferingDao.findByName(ServiceOffering.routerDefaultOffUniqueName);
+ }
Review comment:
```suggestion
ServiceOffering defaultRouterOffering = getDefaultServiceOfferingOrSomeOtherNewName();
```
this happens below as well, for memory.
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -1368,6 +1382,131 @@ private void logBootModeParameters(Map<VirtualMachineProfile.Param, Object> para
}
}
+ /**
Review comment:
`incrementVrResourceCount` (60 lines) and `decrementVrResourceCount` (38 lines) are big to start with and a lot of comment. Each comment could be the javadoc for a new method instead of having just these two.
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -972,11 +980,59 @@ public long countCpusForAccount(long accountId) {
for (UserVmJoinVO vm : userVms) {
cputotal += Long.valueOf(vm.getCpu());
}
- return cputotal;
+
+ ServiceOffering defaultRouterOffering = null;
+ String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
+ if (globalRouterOffering != null) {
+ defaultRouterOffering = _serviceOfferingDao.findByUuid(globalRouterOffering);
+ }
+ if (defaultRouterOffering == null) {
+ defaultRouterOffering = _serviceOfferingDao.findByName(ServiceOffering.routerDefaultOffUniqueName);
+ }
+ GenericSearchBuilder<ServiceOfferingVO, SumCount> cpuSearch = _serviceOfferingDao.createSearchBuilder(SumCount.class);
+ cpuSearch.select("sum", Func.SUM, cpuSearch.entity().getCpu());
+ cpuSearch.select("count", Func.COUNT, (Object[])null);
+ SearchBuilder<VMInstanceVO> join1 = _vmDao.createSearchBuilder();
+ join1.and("accountId", join1.entity().getAccountId(), Op.EQ);
+ join1.and("type", join1.entity().getType(), Op.IN);
+ join1.and("state", join1.entity().getState(), SearchCriteria.Op.NIN);
+ join1.and("displayVm", join1.entity().isDisplayVm(), Op.EQ);
+ cpuSearch.join("offerings", join1, cpuSearch.entity().getId(), join1.entity().getServiceOfferingId(), JoinBuilder.JoinType.INNER);
+ cpuSearch.done();
+
+ SearchCriteria<SumCount> sc = cpuSearch.create();
+ sc.setJoinParameters("offerings", "accountId", accountId);
+ sc.setJoinParameters("offerings", "type", VirtualMachine.Type.DomainRouter); // domain routers
+
+ if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging, State.Stopped});
+ } else {
+ sc.setJoinParameters("offerings", "state", new Object[] {State.Destroyed, State.Error, State.Expunging});
+ }
+ sc.setJoinParameters("offerings", "displayVm", 1);
+ List<SumCount> cpus = _serviceOfferingDao.customSearch(sc, null);
+ if (cpus != null) {
+ // Calculate the VR CPU resource count depending on the global setting
+ if (VirtualMachineManager.ResourceCountRouters.valueIn(domain.getId())) {
+ cputotal += cpus.get(0).sum;
+
+ // Get the diff of current router offering with default offering
+ if (VirtualMachineManager.ResourceCountRoutersType.valueIn(domain.getId())
+ .equalsIgnoreCase(VirtualMachineManager.COUNT_DELTA_VR_RESOURCES)) {
+ cputotal = cputotal - cpus.get(0).count * defaultRouterOffering.getCpu();
+ }
+ }
+ return cputotal;
+ } else {
+ return cputotal;
+ }
Review comment:
this also is the same as below with the exception of the naming
maybe create a
private long totaliseResourceCount(List<SumCount> resourceCounts){...}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org