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/09/09 09:57:04 UTC

[GitHub] [cloudstack] Pearl1594 opened a new pull request #5428: resource limit: Fix resource limit check on VM start

Pearl1594 opened a new pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428


   ### Description
   
   This PR addresses https://github.com/apache/cloudstack/issues/4155  - wherein resource limits for an account aren't checked when a VM's settings i.e., cpuNumber / memory are updated i.e., VM deployed with custom constrained compute offering. 
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [X] Bug fix (non-breaking change which fixes an issue)
   - [ ] Enhancement (improves an existing feature and functionality)
   - [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
   
   ### Feature/Enhancement Scale or Bug Severity
   
   #### Bug Severity
   
   - [ ] BLOCKER
   - [ ] Critical
   - [ ] Major
   - [X] Minor
   - [ ] Trivial
   
   
   ### Screenshots (if appropriate):
   
   
   ### How Has This Been Tested?
   Set resource limit to a low value for domain admin:
   ![image](https://user-images.githubusercontent.com/10495417/132659607-c5204774-f8cd-4aee-b1d3-4d0766e764ff.png)
   
   Deployed a VM using custom constrained compute offering with cpuNumber = 1 and memory = 100MB
   ![image](https://user-images.githubusercontent.com/10495417/132660024-cc59a692-74c9-4ce5-bc26-234693cc5ca8.png)
   
   Updated VM settings with cpuNumber = 4 (max = 2)  - Failed as expected:
   ![image](https://user-images.githubusercontent.com/10495417/132664596-60eef3be-4aae-453a-9fe8-4db467c5377c.png)
   
   
   Updated VM settings with memory = 1200 (max = 1000) - Failed as expected:
   ![image](https://user-images.githubusercontent.com/10495417/132664300-1c680f5f-b201-4183-ae7e-e0dc17880756.png)
   
   
   
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-919709897


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] rhtyd commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-919709039


   @Pearl1594 can you check and rekick test if necessary 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920562708


   Sure @nvazquez  I'll have a look - but during the previous run - the logs indicated that the hosts were in a wrong state leading to failure in operations. But since the same tests have failed yet again, it seems odd. 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922815993


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1324


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921530940


   <b>Trillian test result (tid-2099)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33796 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2099-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922838132


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1327


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705249470



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       @shwstppr I was wondering the same and I think @weizhouapache may be able to provide some insight here, I didn't quite understand the usage of 'resource.count.running.vms.only' - I have currently aligned it to how the checks have been done for other operations.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918358078


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918148298


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918211799


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707381128



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);
+        }
+
+        if (newCpu > currentCpu) {
+            _resourceLimitMgr.incrementResourceCount(owner.getAccountId(), ResourceType.cpu, newCpu - currentCpu);
+        } else {

Review comment:
       @Pearl1594 
   might newCpu be 0 in line 2584 ?
   ```
   long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918154693


   @Pearl1594 
   fyi, there is a component test test/integration/component/test_resource_count_running_vms.py
   you can update the test if you like.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923647351


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923618220


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707179250



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5056,6 +5091,7 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
             }
         }
 
+        updateResourceCount(owner.getAccountId(), owner.getDomainId());

Review comment:
       @weizhouapache  Do you mean we should move this to updateVM workflow and use `resourceCountIncrement` / `resourceCountIncrement` as opposed to ` updateResourceCount`?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-915977661


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707294037



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       @Pearl1594 
   you can move this to the first check of 'verifyVmLimits', so no need to get current cpu/memory, etc

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2582,8 +2576,8 @@ protected void runInContext() {
     }
 
     private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
-        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER));
-        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY));
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");

Review comment:
       @Pearl1594 
   use NumberUtils.toLong ?

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());

Review comment:
       you can use this instead.
   
           ServiceOfferingVO currentServiceOffering = _offeringDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
           int currentCpu = currentServiceOffering.getCpu();
           int currentMemory = currentServiceOffering.getRamSize();

##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/cloud/entity/api/VirtualMachineEntityImpl.java
##########
@@ -197,7 +197,7 @@ public void delTag() {
     }
 
     @Override
-    public String reserve(DeploymentPlanner plannerToUse, DeploymentPlan plan, ExcludeList exclude, String caller) throws InsufficientCapacityException,
+    public String   reserve(DeploymentPlanner plannerToUse, DeploymentPlan plan, ExcludeList exclude, String caller) throws InsufficientCapacityException,

Review comment:
       @Pearl1594 
   revert this line ?

##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);
+        }
+
+        if (newCpu > currentCpu) {
+            _resourceLimitMgr.incrementResourceCount(owner.getAccountId(), ResourceType.cpu, newCpu - currentCpu);
+        } else {

Review comment:
       is it better to use ?
   ```
   } else if (newCpu > 0 && newCpu < currentCpu) {
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707190470



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2581,38 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER));
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY));
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        if (! VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       if ResoureCountRunningVMsonly is true, it is not needed to check resource limit and update rexsource count.
   
   ```
   if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
       return;
   }
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925538149


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1369


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925542036


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918357055


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1222


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925542534


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-919798720


   @Pearl1594 
   two comments
   (1) can we display the error messge if fails ? for now, it only display "Failed to update VM" on UI
   (2) if vm use constraint offering, can we check the cpu and memory range as well ?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] shwstppr commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
shwstppr commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705246461



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       @Pearl1594 shouldn't this check be done irrespective of the `ResoureCountRunningVMsonly` value.
   Server is trying to start the VM it should simply check resource limit while the limits may vary based on the setting. Or maybe I'm wrong here.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923879879


   @Pearl1594 
   I have created #5486 to fix the issue. let's see if travis passes.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921716087


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1304


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez edited a comment on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez edited a comment on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920232235


   @Pearl1594 can you check if these failures may be related to this PR?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923631408


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1336


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922281403


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918214940


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-916422000


   <b>Trillian test result (tid-1999)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 33673 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t1999-kvm-centos7.zip
   Smoke tests completed. 88 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_02_list_snapshots_with_removed_data_store | `Error` | 51.24 | test_snapshots.py
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-924136278


   <b>Trillian test result (tid-2139)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 36154 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2139-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710981604



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));

Review comment:
       OK, check if _decrementResourceCount()_ calls below are relavent when these values are null.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922281279


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920984818


   <b>Trillian test result (tid-2078)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37584 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2078-kvm-centos7.zip
   Smoke tests completed. 85 look OK, 4 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_add_primary_storage_disabled_host | `Error` | 1.44 | test_primary_storage.py
   test_01_primary_storage_nfs | `Error` | 0.14 | test_primary_storage.py
   ContextSuite context=TestStorageTags>:setup | `Error` | 0.25 | test_primary_storage.py
   test_02_list_snapshots_with_removed_data_store | `Error` | 1.21 | test_snapshots.py
   test_01_secure_vm_migration | `Error` | 167.82 | test_vm_life_cycle.py
   test_02_unsecure_vm_migration | `Error` | 284.66 | test_vm_life_cycle.py
   test_03_secured_to_nonsecured_vm_migration | `Error` | 150.31 | test_vm_life_cycle.py
   test_08_migrate_vm | `Error` | 43.71 | test_vm_life_cycle.py
   test_hostha_enable_ha_when_host_in_maintenance | `Error` | 305.76 | test_hostha_kvm.py
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710816855



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2576,44 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {

Review comment:
       I think you are right @weizhouapache , we'll need to add a check to verify if the modified values for cpuNumber / memory are within the limits of the custom offering. Thanks - I'll add the check




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705269906



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       Oh Ok.. Thanks @weizhouapache 




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921675770


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1301


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710797010



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2576,44 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {

Review comment:
       @Pearl1594 
   do we need to check cpu/memory if vm uses constraint service offering ?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921219987


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922667424


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710974374



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));

Review comment:
       NumberUtil.toLong() -> verifies if the value is null and sets it to 0.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921694089


   Thanks a lot for the review @weizhouapache! 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710953967



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));
+        ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
+        ServiceOfferingVO svcOffering = _serviceOfferingDao.findById(vmInstance.getServiceOfferingId());
+        boolean isDynamic = currentServiceOffering.isDynamic();
+        if (isDynamic) {
+            Map<String, String> customParameters = new HashMap<>();
+            customParameters.put(VmDetailConstants.CPU_NUMBER, String.valueOf(newCpu));
+            customParameters.put(VmDetailConstants.MEMORY, String.valueOf(newMemory));
+            validateCustomParameters(svcOffering, customParameters);
+        }
+        long currentCpu = currentServiceOffering.getCpu();
+        long currentMemory = currentServiceOffering.getRamSize();
+
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());

Review comment:
       can you move this check to the method beginning, this is the first check to be done before proceeding with verifying limits.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923794451


   Sure @DaanHoogland - will keep an eye on it - seems like the following set of tests from that job are failing across other PRs too
   ```
   +--------------------------------------+---------+-------+----------------+
   
   |                 Test                 | Result  | Time  |   Test file    |
   
   +======================================+=========+=======+================+
   
   | test_02_list_publicip_domain_admin   | Failure | 0.146 | test_public_ip |
   
   +--------------------------------------+---------+-------+----------------+
   
   | test_03_list_publicip_user_domain    | Failure | 0.404 | test_public_ip |
   
   +--------------------------------------+---------+-------+----------------+
   
   | test_04_list_publicip_all_subdomains | Failure | 0.397 | test_public_ip |
   
   +--------------------------------------+---------+-------+----------------+
   
   | test_05_list_publicip_user_domain    | Failure | 2.632 | test_public_ip |
   
   +--------------------------------------+---------+-------+----------------+
   ```


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918147677


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 edited a comment on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 edited a comment on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920599702


   @nvazquez The test failures seem to be an env issue - ran the tests manually and they didn't fail:
   ```
   Env is based off this PR:
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# cat /etc/yum.repos.d/cloudstack.repo 
   
   [cloudstack]
   name=cloudstack
   baseurl=http://xxx.xxx.xxx.xxx/cloudstack/pr/5428/centos7/4.16/
   enabled=1
   gpgcheck=0
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-1795-k-M7-pearl-dsilva-advanced-cfg -s -a tags=advanced --hypervisor=kvm /marvin/tests/smoke/test_primary_storage.py 
   
   ==== Marvin Init Started ====
   
   === Marvin Parse Config Successful ===
   
   === Marvin Setting TestData Successful===
   
   ==== Log Folder Path: /marvin/MarvinLogs/Sep_16_2021_04_45_29_84AEE8 All logs will be available here ====
   
   === Marvin Init Logging Successful===
   
   ==== Marvin Init Successful ====
   === TestName: test_01_add_primary_storage_disabled_host | Status : SUCCESS ===
   
   === TestName: test_01_primary_storage_nfs | Status : SUCCESS ===
   
   === TestName: test_01_deploy_vms_storage_tags | Status : SUCCESS ===
   
   === TestName: test_02_edit_primary_storage_tags | Status : SUCCESS ===
   
   === TestName: test_03_migration_options_storage_tags | Status : SUCCESS ===
   
   === Final results are now copied to: /marvin//MarvinLogs/test_primary_storage_P18TBX ===
   
   
   
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# cat /marvin//MarvinLogs/test_vm_life_cycle_TO7697/results.txt
   Test secure VM migration ... === TestName: test_01_secure_vm_migration | Status : SUCCESS ===
   ok
   Test Non-secured VM Migration ... === TestName: test_02_unsecure_vm_migration | Status : SUCCESS ===
   ok
   Test destroy Virtual Machine ... === TestName: test_03_secured_to_nonsecured_vm_migration | Status : SUCCESS ===
   Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
   ok
   
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-1795-k-M7-pearl-dsilva-advanced-cfg -s -a tags=xx --hypervisor=kvm /marvin/tests/smoke/test_hostha_kvm.py 
   
   ==== Marvin Init Started ====
   
   === Marvin Parse Config Successful ===
   
   === Marvin Setting TestData Successful===
   
   ==== Log Folder Path: /marvin/MarvinLogs/Sep_16_2021_05_43_37_3XDYMB All logs will be available here ====
   
   === Marvin Init Logging Successful===
   
   ==== Marvin Init Successful ====
   checkForState:: expected=Ineligible, actual={haenable : True, hastate : 'Ineligible', haprovider : 'kvmhaprovider'}
   === TestName: test_hostha_enable_ha_when_host_in_maintenance | Status : SUCCESS ===
   
   === Final results are now copied to: /marvin//MarvinLogs/test_hostha_kvm_40WQIP ===
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-1795-k-M7-pearl-dsilva-advanced-cfg -s -a tags=xx --hypervisor=kvm /marvin/tests/smoke/test_snapshots.py 
   
   ==== Marvin Init Started ====
   
   === Marvin Parse Config Successful ===
   
   === Marvin Setting TestData Successful===
   
   ==== Log Folder Path: /marvin/MarvinLogs/Sep_16_2021_05_54_35_9B2V8W All logs will be available here ====
   
   === Marvin Init Logging Successful===
   
   ==== Marvin Init Successful ====
   === TestName: test_02_list_snapshots_with_removed_data_store | Status : SUCCESS ===
   
   === Final results are now copied to: /marvin//MarvinLogs/test_snapshots_D2VQCM ===
   
   ```


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925561369


   <b>Trillian Build Failed (tid-2176)<b/>


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r714480347



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,54 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String, String> details) {
+        if (VirtualMachineManager.ResourceCountRunningVMsonly.value()) {

Review comment:
       @Pearl1594 yes, exactly.
   
   otherwise, if ResourceCountRunningVMsonly is true, the validation of customer offering is not performed.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-915978334


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705266075



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       @Pearl1594 @shwstppr 
   if resource.count.running.vms.only is false, the resources (cpu/memory) of the vm have already been added to resource count of domain/account, when we create the vm. so it is not needed to check the resource limitation when we start the vm.
   
   
   
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925586451


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918154244


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1216


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923647170


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923618045


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925588840


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707177109



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,11 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
-            // check if account/domain is with in resource limits to start a new vm
-            ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-        }
+

Review comment:
       Based on my understanding, operations like scaleVM - which could update the vm settings prior starting the VM, and other such operations, have these checks in place, so it may become redundant. However, keeping this check may be safer




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-917997430


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918241948


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1219


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r709163780



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java
##########
@@ -263,13 +264,17 @@ public long getEntityOwnerId() {
     @Override
     public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException {
         CallContext.current().setEventDetails("Vm Id: " + this._uuidMgr.getUuid(VirtualMachine.class, getId()));
-        UserVm result = _userVmService.updateVirtualMachine(this);
-        if (result != null){
-            UserVmResponse response = _responseGenerator.createUserVmResponse(getResponseView(), "virtualmachine", result).get(0);
-            response.setResponseName(getCommandName());
-            setResponseObject(response);
-        } else {
-            throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update vm");
+        try {
+            UserVm result = _userVmService.updateVirtualMachine(this);
+            if (result != null) {
+                UserVmResponse response = _responseGenerator.createUserVmResponse(getResponseView(), "virtualmachine", result).get(0);
+                response.setResponseName(getCommandName());
+                setResponseObject(response);
+            } else {
+                throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update vm");
+            }
+        } catch (Exception e) {

Review comment:
       this thrown exception will be caught immediately.
   can we be more specific in the catch, if not out of pokemon principle, at least to not catch the `ServerApiException`




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti merged pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti merged pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428


   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705269906



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       Oh Ok.. Thanks @weizhouapache . Updated it!




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918333165


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923429450


   <b>Trillian test result (tid-2127)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 37481 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2127-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922391364


   <b>Trillian test result (tid-2119)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 34198 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2119-kvm-centos7.zip
   Smoke tests completed. 88 look OK, 1 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_nic | `Error` | 138.15 | test_nic.py
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923530771


   Hi @Pearl1594 can you fix the conflicts?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920599702


   @nvazquez The test failures seem to be an env issue - ran the tests manually and they didn't fail:
   ```
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-1795-k-M7-pearl-dsilva-advanced-cfg -s -a tags=advanced --hypervisor=kvm /marvin/tests/smoke/test_primary_storage.py 
   
   ==== Marvin Init Started ====
   
   === Marvin Parse Config Successful ===
   
   === Marvin Setting TestData Successful===
   
   ==== Log Folder Path: /marvin/MarvinLogs/Sep_16_2021_04_45_29_84AEE8 All logs will be available here ====
   
   === Marvin Init Logging Successful===
   
   ==== Marvin Init Successful ====
   === TestName: test_01_add_primary_storage_disabled_host | Status : SUCCESS ===
   
   === TestName: test_01_primary_storage_nfs | Status : SUCCESS ===
   
   === TestName: test_01_deploy_vms_storage_tags | Status : SUCCESS ===
   
   === TestName: test_02_edit_primary_storage_tags | Status : SUCCESS ===
   
   === TestName: test_03_migration_options_storage_tags | Status : SUCCESS ===
   
   === Final results are now copied to: /marvin//MarvinLogs/test_primary_storage_P18TBX ===
   
   
   
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# cat /marvin//MarvinLogs/test_vm_life_cycle_TO7697/results.txt
   Test secure VM migration ... === TestName: test_01_secure_vm_migration | Status : SUCCESS ===
   ok
   Test Non-secured VM Migration ... === TestName: test_02_unsecure_vm_migration | Status : SUCCESS ===
   ok
   Test destroy Virtual Machine ... === TestName: test_03_secured_to_nonsecured_vm_migration | Status : SUCCESS ===
   Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
   ok
   
   
   
   [root@ref-trl-1795-k-M7-pearl-dsilva-marvin ~]# nosetests --with-xunit --xunit-file=results.xml --with-marvin --marvin-config=/marvin/ref-trl-1795-k-M7-pearl-dsilva-advanced-cfg -s -a tags=xx --hypervisor=kvm /marvin/tests/smoke/test_hostha_kvm.py 
   
   ==== Marvin Init Started ====
   
   === Marvin Parse Config Successful ===
   
   === Marvin Setting TestData Successful===
   
   ==== Log Folder Path: /marvin/MarvinLogs/Sep_16_2021_05_43_37_3XDYMB All logs will be available here ====
   
   === Marvin Init Logging Successful===
   
   ==== Marvin Init Successful ====
   checkForState:: expected=Ineligible, actual={haenable : True, hastate : 'Ineligible', haprovider : 'kvmhaprovider'}
   === TestName: test_hostha_enable_ha_when_host_in_maintenance | Status : SUCCESS ===
   
   === Final results are now copied to: /marvin//MarvinLogs/test_hostha_kvm_40WQIP ===
   
   ```


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r714469371



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,54 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String, String> details) {
+        if (VirtualMachineManager.ResourceCountRunningVMsonly.value()) {

Review comment:
       @Pearl1594 
   I think we need to move line 2589-2591 to line 2609.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922877052


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921443415


   @blueorangutan test keepEnv


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r714476892



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,54 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String, String> details) {
+        if (VirtualMachineManager.ResourceCountRunningVMsonly.value()) {

Review comment:
       Just to confirm before making the change - @weizhouapache this is to ensure that before updating the VM settings for cpuNumber/ memory - we validate it is within the limits of the custom offering it uses (if it uses)?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r714480347



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,54 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String, String> details) {
+        if (VirtualMachineManager.ResourceCountRunningVMsonly.value()) {

Review comment:
       @Pearl1594 yes, exactly.
   
   otherwise, if ResourceCountRunningVMsonly is true, the validation of customer offering is not executed.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920191158


   <b>Trillian test result (tid-2050)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 38983 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2050-kvm-centos7.zip
   Smoke tests completed. 85 look OK, 4 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_add_primary_storage_disabled_host | `Error` | 0.66 | test_primary_storage.py
   test_01_primary_storage_nfs | `Error` | 0.15 | test_primary_storage.py
   ContextSuite context=TestStorageTags>:setup | `Error` | 0.28 | test_primary_storage.py
   test_02_list_snapshots_with_removed_data_store | `Error` | 1.19 | test_snapshots.py
   test_01_secure_vm_migration | `Error` | 168.52 | test_vm_life_cycle.py
   test_02_unsecure_vm_migration | `Error` | 279.48 | test_vm_life_cycle.py
   test_03_secured_to_nonsecured_vm_migration | `Error` | 146.06 | test_vm_life_cycle.py
   test_08_migrate_vm | `Error` | 44.87 | test_vm_life_cycle.py
   test_hostha_enable_ha_when_host_in_maintenance | `Error` | 302.81 | test_hostha_kvm.py
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918797929


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707190936



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5056,6 +5091,7 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
             }
         }
 
+        updateResourceCount(owner.getAccountId(), owner.getDomainId());

Review comment:
       @Pearl1594 yes.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-917999121


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918159153


   Thanks for the review @weizhouapache I'll address your comments and have a look at the component test.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710938020



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,52 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));
+        ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
+        ServiceOfferingVO svcOffering = _serviceOfferingDao.findById(vmInstance.getServiceOfferingId());
+        boolean isDynamic = currentServiceOffering.isDynamic();
+        if (isDynamic) {
+            Map<String, String> customParameters = new HashMap<>();
+            customParameters.put(VmDetailConstants.CPU_NUMBER, String.valueOf(newCpu));
+            customParameters.put(VmDetailConstants.MEMORY, String.valueOf(newMemory));
+            validateCustomParameters(svcOffering, customParameters);
+        }
+        long currentCpu = currentServiceOffering.getCpu();
+        long currentMemory = currentServiceOffering.getRamSize();
+
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);

Review comment:
       Updated @weizhouapache 
   Looks like this now:
   If we exceed the threshold set for a domain:
   ![image](https://user-images.githubusercontent.com/10495417/133767651-71cba77a-a11d-42e4-89ad-20be187d4a44.png)
   
   If we exceed the limit enforced by custom offering:
   ![image](https://user-images.githubusercontent.com/10495417/133767723-9236ecf9-7ed3-4939-8406-898d747dbc5b.png)
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925525970


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707466582



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);
+        }
+
+        if (newCpu > currentCpu) {
+            _resourceLimitMgr.incrementResourceCount(owner.getAccountId(), ResourceType.cpu, newCpu - currentCpu);
+        } else {

Review comment:
       Ideally, it shouldn't be zero - and the UI handles the check, but via cmk it is possible to pass 0 as the cpu number or any other parameter, and it fails eventually during deployment for example on VMWare:
   ```
   2021-09-13 15:42:13,633 INFO  [c.c.h.v.u.VmwareHelper] (DirectAgent-3:ctx-9536c4ac 10.0.35.165, job-288/job-291, cmd: StartCommand) (logid:9ffa5f01) [ignored]failed to get message for exception: A specified parameter was not correct: configSpec.numCPUs
   2021-09-13 15:42:13,633 WARN  [c.c.h.v.r.VmwareResource] (DirectAgent-3:ctx-9536c4ac 10.0.35.165, job-288/job-291, cmd: StartCommand) (logid:9ffa5f01) StartCommand failed due to Exception: java.lang.RuntimeException
   Message: A specified parameter was not correct: configSpec.numCPUs
   
   java.lang.RuntimeException: A specified parameter was not correct: configSpec.numCPUs
   	at com.cloud.hypervisor.vmware.util.VmwareClient.waitForTask(VmwareClient.java:426)
   	at com.cloud.hypervisor.vmware.mo.VirtualMachineMO.configureVm(VirtualMachineMO.java:1142)
   	at com.cloud.hypervisor.vmware.resource.VmwareResource.execute(VmwareResource.java:2390)
   	at com.cloud.hypervisor.vmware.resource.VmwareResource.executeRequest(VmwareResource.java:555)
   ```
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918065992


   Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1213


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-919711305


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921686412


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705266075



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,10 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+        if (!VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
             // check if account/domain is with in resource limits to start a new vm
             ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
+            resourceLimitCheck(owner, vm.isDisplayVm(), Long.valueOf(offering.getCpu()), Long.valueOf(offering.getRamSize()));

Review comment:
       @Pearl1594 @shwstppr 
   if resource.count.running.vms.only is false, the resources (cpu/memory) of the vm have already been added to resource count of domain/account, when we create the vm. so it is not needed to check the resource limitation when we start the vm.
   
   https://github.com/apache/cloudstack/blob/f6073052aa3d592d6d737990a95c44e702908d59/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L4246-L4247
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921835189


   <b>Trillian test result (tid-2103)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 34663 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2103-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922687875


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1322


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922877857


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921860713


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921647633


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921443918


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922794439


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921721203


   updated @sureshanaparti 


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710955284



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));

Review comment:
       does _details_ contains both these params, or can be null?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-915955156


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921884485


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1308


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710950549



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {

Review comment:
       ```suggestion
       private void verifyVmLimits(UserVmVO vmInstance, Map<String, String> details) {
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710938020



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,52 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));
+        ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
+        ServiceOfferingVO svcOffering = _serviceOfferingDao.findById(vmInstance.getServiceOfferingId());
+        boolean isDynamic = currentServiceOffering.isDynamic();
+        if (isDynamic) {
+            Map<String, String> customParameters = new HashMap<>();
+            customParameters.put(VmDetailConstants.CPU_NUMBER, String.valueOf(newCpu));
+            customParameters.put(VmDetailConstants.MEMORY, String.valueOf(newMemory));
+            validateCustomParameters(svcOffering, customParameters);
+        }
+        long currentCpu = currentServiceOffering.getCpu();
+        long currentMemory = currentServiceOffering.getRamSize();
+
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);

Review comment:
       Update @weizhouapache 
   Looks like this now:
   If we exceed the threshold set for a domain:
   ![image](https://user-images.githubusercontent.com/10495417/133767651-71cba77a-a11d-42e4-89ad-20be187d4a44.png)
   
   If we exceed the limit enforced by custom offering:
   ![image](https://user-images.githubusercontent.com/10495417/133767723-9236ecf9-7ed3-4939-8406-898d747dbc5b.png)
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707173823



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -5056,6 +5091,7 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
             }
         }
 
+        updateResourceCount(owner.getAccountId(), owner.getDomainId());

Review comment:
       this might take longer time if there are many accounts.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707172826



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,11 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
-            // check if account/domain is with in resource limits to start a new vm
-            ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-        }
+

Review comment:
       do we need to keep this ?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918332575


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707345405



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2575,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER) != null ? details.get(VmDetailConstants.CPU_NUMBER) : "0");
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY) != null ? details.get(VmDetailConstants.MEMORY) : "0");
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);
+        }
+
+        if (newCpu > currentCpu) {
+            _resourceLimitMgr.incrementResourceCount(owner.getAccountId(), ResourceType.cpu, newCpu - currentCpu);
+        } else {

Review comment:
       @weizhouapache not sure if this is required - please do correct me if I am wrong, but cpuNumber can never be set to 0 and in case of equality i.e., currentCpu = newCpu , the delta would be 0, hence effectively not causing any change in resource count. 




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710951856



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       ```suggestion
           if (VirtualMachineManager.ResourceCountRunningVMsOnly.value()) {
   ```
   
   typo ^^^ (not related to this PR)




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710924262



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,52 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
+            return;
+        }
+
+        long newCpu = NumberUtils.toLong(details.get(VmDetailConstants.CPU_NUMBER));
+        long newMemory = NumberUtils.toLong(details.get(VmDetailConstants.MEMORY));
+        ServiceOfferingVO currentServiceOffering = _serviceOfferingDao.findByIdIncludingRemoved(vmInstance.getId(), vmInstance.getServiceOfferingId());
+        ServiceOfferingVO svcOffering = _serviceOfferingDao.findById(vmInstance.getServiceOfferingId());
+        boolean isDynamic = currentServiceOffering.isDynamic();
+        if (isDynamic) {
+            Map<String, String> customParameters = new HashMap<>();
+            customParameters.put(VmDetailConstants.CPU_NUMBER, String.valueOf(newCpu));
+            customParameters.put(VmDetailConstants.MEMORY, String.valueOf(newMemory));
+            validateCustomParameters(svcOffering, customParameters);
+        }
+        long currentCpu = currentServiceOffering.getCpu();
+        long currentMemory = currentServiceOffering.getRamSize();
+
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        try {
+            if (newCpu > currentCpu) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.cpu, newCpu - currentCpu);
+            }
+            if (newMemory > currentMemory) {
+                _resourceLimitMgr.checkResourceLimit(owner, ResourceType.memory, newMemory - currentMemory);
+            }
+        } catch (ResourceAllocationException e) {
+            throw new CloudRuntimeException("Failed to update VM settings", e);

Review comment:
       @Pearl1594 use InvalidParameterValueException instead of CloudRuntimeException ?
   this can be improved 
   
   ![Screenshot from 2021-09-17 12-00-28](https://user-images.githubusercontent.com/57355700/133764958-280760c4-bc33-4fc4-9f00-e4214b1363ef.png)
   

##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java
##########
@@ -263,8 +264,13 @@ public long getEntityOwnerId() {
     @Override
     public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException {
         CallContext.current().setEventDetails("Vm Id: " + this._uuidMgr.getUuid(VirtualMachine.class, getId()));
-        UserVm result = _userVmService.updateVirtualMachine(this);
-        if (result != null){
+        UserVm result = null;
+        try {
+            result = _userVmService.updateVirtualMachine(this);
+        } catch (CloudRuntimeException e) {
+            throw new CloudRuntimeException(String.format("Failed to update VM, due to %s: ", e.getLocalizedMessage()));

Review comment:
       @Pearl1594 
   maybe move "%s" after ":" ?
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921011735


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922817391


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923805862


   > Sure @DaanHoogland - will keep an eye on it - seems like the following set of tests from that job are failing across other PRs too
   > 
   > ```
   > +--------------------------------------+---------+-------+----------------+
   > 
   > |                 Test                 | Result  | Time  |   Test file    |
   > 
   > +======================================+=========+=======+================+
   > 
   > | test_02_list_publicip_domain_admin   | Failure | 0.146 | test_public_ip |
   > 
   > +--------------------------------------+---------+-------+----------------+
   > 
   > | test_03_list_publicip_user_domain    | Failure | 0.404 | test_public_ip |
   > 
   > +--------------------------------------+---------+-------+----------------+
   > 
   > | test_04_list_publicip_all_subdomains | Failure | 0.397 | test_public_ip |
   > 
   > +--------------------------------------+---------+-------+----------------+
   > 
   > | test_05_list_publicip_user_domain    | Failure | 2.632 | test_public_ip |
   > 
   > +--------------------------------------+---------+-------+----------------+
   > ```
   
   @Pearl1594 
   this seems to be caused by #5464 . I will look into it.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918753410


   <b>Trillian test result (tid-2023)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 36371 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2023-kvm-centos7.zip
   Smoke tests completed. 85 look OK, 4 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_add_primary_storage_disabled_host | `Error` | 0.58 | test_primary_storage.py
   test_01_primary_storage_nfs | `Error` | 0.13 | test_primary_storage.py
   ContextSuite context=TestStorageTags>:setup | `Error` | 0.22 | test_primary_storage.py
   test_02_list_snapshots_with_removed_data_store | `Error` | 1.16 | test_snapshots.py
   test_01_secure_vm_migration | `Error` | 156.17 | test_vm_life_cycle.py
   test_02_unsecure_vm_migration | `Error` | 269.80 | test_vm_life_cycle.py
   test_03_secured_to_nonsecured_vm_migration | `Error` | 145.78 | test_vm_life_cycle.py
   test_08_migrate_vm | `Error` | 43.76 | test_vm_life_cycle.py
   test_hostha_enable_ha_when_host_in_maintenance | `Error` | 302.71 | test_hostha_kvm.py
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923807172


   Alright @DaanHoogland Thanks!


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922817476


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-925525362


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922817317


   BO failure seems to be jenkins related - re-kicking packaging


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r711794272



##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVMCmd.java
##########
@@ -263,8 +264,13 @@ public long getEntityOwnerId() {
     @Override
     public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException {
         CallContext.current().setEventDetails("Vm Id: " + this._uuidMgr.getUuid(VirtualMachine.class, getId()));
-        UserVm result = _userVmService.updateVirtualMachine(this);
-        if (result != null){
+        UserVm result = null;
+        try {
+            result = _userVmService.updateVirtualMachine(this);
+        } catch (CloudRuntimeException e) {
+            throw new CloudRuntimeException(String.format("Failed to update VM, due to: %s", e.getLocalizedMessage()));

Review comment:
       ```suggestion
               throw new CloudRuntimeException(String.format("Failed to update VM, due to: %s", e.getLocalizedMessage()), e);
   ```




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920559014


   @nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918797602


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921010489


   @blueorangutan test keepEnv


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez removed a comment on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez removed a comment on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920232235


   @Pearl1594 can you check if these failures may be related to this PR?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921013175


   <b>Trillian Build Failed (tid-2093)<b/>


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r705323315



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -4927,11 +4927,10 @@ public void finalizeStop(VirtualMachineProfile profile, Answer answer) {
         if (owner.getState() == Account.State.disabled) {
             throw new PermissionDeniedException("The owner of " + vm + " is disabled: " + vm.getAccountId());
         }
-        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {
-            // check if account/domain is with in resource limits to start a new vm
-            ServiceOfferingVO offering = _serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId());
-            resourceLimitCheck(owner, vm.isDisplayVm(), new Long(offering.getCpu()), new Long(offering.getRamSize()));
-        }
+

Review comment:
       @Pearl1594 
   I think we need to check resource limitation and update resource count when update the vm setting.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-926097724


   <b>Trillian test result (tid-2178)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 40219 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5428-t2178-kvm-centos7.zip
   Smoke tests completed. 89 look OK, 0 have errors
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707177569



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2581,38 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER));
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY));
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        if (! VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       Sorry @weizhouapache I didn't quite understand what you mean here.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921646820


   @blueorangutan package


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922667627


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921220604


   @sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r707172495



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2575,6 +2581,38 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        Long newCpu = Long.valueOf(details.get(VmDetailConstants.CPU_NUMBER));
+        Long newMemory = Long.valueOf(details.get(VmDetailConstants.MEMORY));
+        long currentCpu = 0L;
+        long currentMemory = 0L;
+        List<UserVmDetailVO> vmDetails = userVmDetailsDao.listDetails(vmInstance.getId());
+        for (UserVmDetailVO detailVO : vmDetails) {
+            if (VmDetailConstants.CPU_NUMBER.equals(detailVO.getName())) {
+                currentCpu = Long.parseLong(detailVO.getValue());
+            }
+            if (VmDetailConstants.MEMORY.equals(detailVO.getName())) {
+                currentMemory = Long.parseLong(detailVO.getValue());
+            }
+        }
+        Account owner = _accountDao.findById(vmInstance.getAccountId());
+        if (owner == null) {
+            throw new InvalidParameterValueException("The owner of " + vmInstance + " does not exist: " + vmInstance.getAccountId());
+        }
+        if (! VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       move to first check ?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920558923


   @blueorangutan test


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#discussion_r710954438



##########
File path: server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
##########
@@ -2584,6 +2585,53 @@ protected void runInContext() {
         }
     }
 
+    private void verifyVmLimits(UserVmVO vmInstance, Map<String,String> details) {
+        if (VirtualMachineManager.ResoureCountRunningVMsonly.value()) {

Review comment:
       @sureshanaparti hmm, it was introduced by my pr #3760
   I did not see it ......




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] DaanHoogland commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923791803


   @Pearl1594 the resource limit test in travis job 15 failed. I restarted it, can you keep an eye on it?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-915972135


   Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1195


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-920232235


   @Pearl1594 can you check if this failures may be related to this PR?


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-915955532


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921687005


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] Pearl1594 commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
Pearl1594 commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-923881691


   > @Pearl1594
   > I have created #5486 to fix the issue. let's see if travis passes.
   Thanks @weizhouapache!


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-922795173


   @Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] nvazquez closed pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
nvazquez closed pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428


   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-918358488


   @Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [cloudstack] blueorangutan commented on pull request #5428: resource limit: Fix resource limit check on VM start

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5428:
URL: https://github.com/apache/cloudstack/pull/5428#issuecomment-921861486


   @DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org