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 2020/10/07 07:10:22 UTC

[GitHub] [cloudstack] Spaceman1984 opened a new pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

Spaceman1984 opened a new pull request #4389:
URL: https://github.com/apache/cloudstack/pull/4389


   …e garbage collection
   
   ## Description
   <!--- Describe your changes in detail -->
   Fixes: #4292 
   <!-- 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: # -->
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] 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)
   
   ## Screenshots (if appropriate):
   
   ## How Has This Been Tested?
   <!-- 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. -->
   This has been tested by changing the storage cleanup global var to 30 seconds, creating a single instance from a template, removing the instance and checking if the template gets removed from Primary Storage.
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/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.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 unsupported parameters provided. Supported mgmt server os are: `centos6, centos7, centos8, ubuntu`. Supported hypervisors are: `kvm-centos6, kvm-centos7, kvm-centos8, kvm-ubuntu, xenserver-71, xenserver-65sp1, vmware-67u3, vmware-65u2, vmware-60u2, vmware-55u3, xcpng76, xcpng80, xcpng81, xenserver-74, xcpng74`


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @blueorangutan test centos7 vmware-67u3


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @blueorangutan test centos7  vmware-67u3


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       Yes, it did @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.

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       Yes, in this case, better not to mark the VM as template while spooling it from the secondary storage. So, no need to mark it back to VM for deletion.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] DaanHoogland commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 as this is not a draft-pr and there are two lgtm; is this good to go or does it need more testing? (cc @borisstoyanov )


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) 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.

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



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       The error happened for all templates I tested, even systemvm. I speculate that it is referring to the internal command that vmware calls when you click on the delete button from the UI.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       If _destroy()_ works only when marked as a VM, then the same should be ensured across the usages of vmMO _destroy()_




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


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

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       @Spaceman1984 why to mark as VirtualMachine before destroying it ? will the destroy operation fail without 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.

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



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @andrijapanicsb, I agree with @nvazquez. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   <b>Trillian test result (tid-2906)</b>
   Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 40035 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4389-t2906-vmware-67u3.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
   Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
   Smoke tests completed. 81 look OK, 1 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | `Failure` | 409.45 | test_vpc_redundant.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.

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



[GitHub] [cloudstack] Spaceman1984 commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


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

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



[GitHub] [cloudstack] nvazquez commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   In my opinion, if the record has been already removed from the table `template_spool_ref` by the GC then CloudStack loses track of the template (need manual delete). However, templates which are referenced in `template_spool_ref` can be marked as templates in vSphere so when are not used the GC can remove them


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


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

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



[GitHub] [cloudstack] rhtyd merged pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       The error was:
   Unable to delete template with snapshots - convert the template to a Virtual Machine first.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] DaanHoogland commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 as this is not a draft-pr and there are two lgtm; is this good to go or does it need more testing? (cc @borisstoyanov )


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -406,6 +406,10 @@ public void markAsTemplate() throws Exception {
         _context.getService().markAsTemplate(_mor);
     }
 
+    public void markAsVirtualMachine(ManagedObjectReference resourcePool, ManagedObjectReference host) throws Exception {
+        _context.getService().markAsVirtualMachine(_mor, pool, host);

Review comment:
       Fixed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] andrijapanicsb commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 do you want to advise, in form of the official documentation AND here - how one can fix his env, by marking the existing "template VMs" (which are just VMs in vSphere) as real VMware templates in vSphere - to ensure a proper GC next time for all the old garbage
   
   If I understood this PR correctly, this should be possible - and should allow for an automatical garbage cleanup of tons of older "template VMs" from vSphere (i.e. this PR, afaik, will work only when a new VM is deployed for a VERY first time from a template, as we mark that template VM as a template in vSphere)
   
   cc @rhtyd and @nvazquez for opinion


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] nvazquez commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       Looks good. I tried `vmMo.removeAllSnapshots();` before the destroy but fails. I wonder if templates GC worked before not being marked as templates.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -406,6 +406,10 @@ public void markAsTemplate() throws Exception {
         _context.getService().markAsTemplate(_mor);
     }
 
+    public void markAsVirtualMachine(ManagedObjectReference pool, ManagedObjectReference host) throws Exception {

Review comment:
       ```suggestion
       public void markAsVirtualMachine(ManagedObjectReference resourcePool, ManagedObjectReference host) throws Exception {
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   @Spaceman1984 unsupported parameters provided. Supported mgmt server os are: `centos6, centos7, centos8, ubuntu`. Supported hypervisors are: `kvm-centos6, kvm-centos7, kvm-centos8, kvm-ubuntu, xenserver-71, xenserver-65sp1, vmware-67u3, vmware-65u2, vmware-60u2, vmware-55u3, xcpng76, xcpng80, xcpng81, xenserver-74, xcpng74`


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] Spaceman1984 commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       > Do we/CloudStack check that template is not used by any of the VMs, prior to removing it. 
   
   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.

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       I see delete/destory operation works for VM & Template in the vCenter. so, I think _destroyTask()_ API can be used with the template mor as well, no need to explicitly mark as VM. What are the errors seen in the logs for failed destroy task without marking as 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.

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



[GitHub] [cloudstack] blueorangutan commented on pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2143


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java
##########
@@ -406,6 +406,10 @@ public void markAsTemplate() throws Exception {
         _context.getService().markAsTemplate(_mor);
     }
 
+    public void markAsVirtualMachine(ManagedObjectReference resourcePool, ManagedObjectReference host) throws Exception {
+        _context.getService().markAsVirtualMachine(_mor, pool, host);

Review comment:
       _pool_ => _resourcePool_




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



[GitHub] [cloudstack] rhtyd commented on a change in pull request #4389: Fixed vm-templates not being removed from primary storage with storag…

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



##########
File path: plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/vmware/resource/VmwareResource.java
##########
@@ -6785,6 +6785,7 @@ public Answer execute(DestroyCommand cmd) {
                 if (s_logger.isInfoEnabled()) {
                     s_logger.info("Destroy template volume " + vol.getPath());
                 }
+                vmMo.markAsVirtualMachine(hyperHost.getHyperHostOwnerResourcePool(), hyperHost.getMor());

Review comment:
       @Spaceman1984 @sureshanaparti - in case of linked clones, it makes sense (the error). Do we/CloudStack check that template is not used by any of the VMs, prior to removing it. In that case it perfectly makes sense, if we don't delete the template from primary storage. Thoughts?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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