CE-110 markedForGC is ignored so delete the templates in the task that finds them

Author: DaanHoogland

plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/CleanupFullyClonedTemplatesTask.java

@@ -22,6 +22,8 @@
 import com.cloud.hypervisor.HypervisorGuru;
 import com.cloud.storage.VMTemplateStoragePoolVO;
 import com.cloud.storage.dao.VMTemplatePoolDao;
+import com.cloud.template.TemplateManager;
+import com.cloud.template.VirtualMachineTemplate;
 import com.cloud.vm.UserVmCloneSettingVO;
 import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.dao.UserVmCloneSettingDao;
@@ -46,19 +48,22 @@ public class CleanupFullyClonedTemplatesTask extends ManagedContextRunnable {
     private TemplateJoinDao templateDao;
     private VMInstanceDao vmInstanceDao;
     private UserVmCloneSettingDao cloneSettingDao;
+    private TemplateManager templateManager;
 
     private Thread mine;
 
     CleanupFullyClonedTemplatesTask(PrimaryDataStoreDao primaryStorageDao,
                                     VMTemplatePoolDao templateDataStoreDao,
                                     TemplateJoinDao templateDao,
                                     VMInstanceDao vmInstanceDao,
-                                    UserVmCloneSettingDao cloneSettingDao) {
+                                    UserVmCloneSettingDao cloneSettingDao,
+                                    TemplateManager templateManager) {
         this.primaryStorageDao = primaryStorageDao;
         this.templateDataStoreDao = templateDataStoreDao;
         this.templateDao = templateDao;
         this.vmInstanceDao = vmInstanceDao;
         this.cloneSettingDao = cloneSettingDao;
+        this.templateManager = templateManager;
         if(s_logger.isDebugEnabled()) {
             s_logger.debug("new task created: " + this);
         }
@@ -97,7 +102,9 @@ private void queryPoolForTemplates(StoragePoolVO pool, long zoneId) {
             }
             List<VMTemplateStoragePoolVO> templatePrimaryDataStoreVOS = templateDataStoreDao.listByPoolId(pool.getId());
             for (VMTemplateStoragePoolVO templateMapping : templatePrimaryDataStoreVOS) {
-                checkTemplateForRemoval(zoneId, templateMapping);
+                if (canRemoveTemplateFromZone(zoneId, templateMapping)) {
+                    templateManager.evictTemplateFromStoragePool(templateMapping);
+                }
             }
         } else {
             if(s_logger.isDebugEnabled()) {
@@ -106,22 +113,23 @@ private void queryPoolForTemplates(StoragePoolVO pool, long zoneId) {
         }
     }
 
-    private void checkTemplateForRemoval(long zoneId, VMTemplateStoragePoolVO templateMapping) {
+    private boolean canRemoveTemplateFromZone(long zoneId, VMTemplateStoragePoolVO templateMapping) {
         if (!templateMapping.getMarkedForGC()) {
             if(s_logger.isDebugEnabled()) {
                 s_logger.debug(mine.getName() + " is checking template with id " + templateMapping.getTemplateId() + " for deletion from pool with id " + templateMapping.getPoolId());
             }
 
-            TemplateJoinVO templateJoinVO = templateDao.findByIdIncludingRemoved(templateMapping.getPoolId());
+            TemplateJoinVO templateJoinVO = templateDao.findByIdIncludingRemoved(templateMapping.getTemplateId());
             //  check if these are deleted (not removed is null)
-            if (templateJoinVO.getRemoved() != null) { // meaning it is removed!
+            if (VirtualMachineTemplate.State.Inactive.equals(templateJoinVO.getTemplateState())) { // meaning it is removed!
                 //  see if we can find vms using it with user_vm_clone_setting != full
-                markForGcAsNeeded(templateMapping, zoneId);
+                return markedForGc(templateMapping, zoneId);
             }
         }
+        return false;
     }
 
-    private void markForGcAsNeeded(VMTemplateStoragePoolVO templateMapping, long zoneId) {
+    private boolean markedForGc(VMTemplateStoragePoolVO templateMapping, long zoneId) {
         boolean used = false;
         List<VMInstanceVO> vms = vmInstanceDao.listNonExpungedByZoneAndTemplate(zoneId, templateMapping.getTemplateId());
         for (VMInstanceVO vm : vms) {
@@ -130,6 +138,7 @@ private void markForGcAsNeeded(VMTemplateStoragePoolVO templateMapping, long zon
                 // VolumeOrchestrator or UserVmManagerImpl depending on version
                 if (VolumeOrchestrator.UserVmCloneType.linked.equals(VolumeOrchestrator.UserVmCloneType.valueOf(cloneSetting.getCloneType()))) {
                     used = true;
+                    break;
                 }
             } catch (Exception e) {
                 s_logger.error("failed to retrieve vm clone setting for vm " + vm.toString());
@@ -139,13 +148,11 @@ private void markForGcAsNeeded(VMTemplateStoragePoolVO templateMapping, long zon
             }
         }
         if (!used) {
-            if(s_logger.isInfoEnabled()) {
-                s_logger.info(mine.getName() + " is marking template with id " + templateMapping.getTemplateId() + " for gc in pool with id " + templateMapping.getPoolId());
-            }
+            s_logger.info(mine.getName() + " is marking template with id " + templateMapping.getTemplateId() + " for gc in pool with id " + templateMapping.getPoolId());
             // else
             //  mark it for removal from primary store
             templateMapping.setMarkedForGC(true);
-            templateDataStoreDao.persist(templateMapping);
         }
+        return !used;
     }
 }

plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/manager/VmwareManagerImpl.java

@@ -79,6 +79,7 @@
 import com.cloud.storage.StorageLayer;
 import com.cloud.storage.StorageManager;
 import com.cloud.storage.dao.VMTemplatePoolDao;
+import com.cloud.template.TemplateManager;
 import com.cloud.utils.FileUtil;
 import com.cloud.utils.NumbersUtil;
 import com.cloud.utils.Pair;
@@ -181,6 +182,8 @@ public class VmwareManagerImpl extends ManagerBase implements VmwareManager, Vmw
     private VMInstanceDao vmInstanceDao;
     @Inject
     private UserVmCloneSettingDao cloneSettingDao;
+    @Inject
+    private TemplateManager templateManager;
 
     private String _mountParent;
     private StorageLayer _storage;
@@ -1309,6 +1312,7 @@ private Runnable getCleanupFullyClonedTemplatesTask() {
                 templateDataStoreDao,
                 templateDao,
                 vmInstanceDao,
-                cloneSettingDao);
+                cloneSettingDao,
+                templateManager);
     }
 }

plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java

@@ -50,6 +50,7 @@
 import com.cloud.server.ConfigurationServer;
 import com.cloud.storage.ImageStoreDetailsUtil;
 import com.cloud.storage.dao.VMTemplatePoolDao;
+import com.cloud.template.TemplateManager;
 import com.cloud.user.Account;
 import com.cloud.user.AccountManager;
 import com.cloud.user.AccountService;
@@ -453,23 +454,32 @@ public ImageStoreDetailsDao imageStoreDetailsDao() {
         public VMTemplatePoolDao templateDataStoreDao() {
             return Mockito.mock(VMTemplatePoolDao.class);
         }
+
         @Bean
         public TemplateJoinDao templateDao() {
             return Mockito.mock(TemplateJoinDao.class);
         }
+
         @Bean
         public VMInstanceDao vmInstanceDao() {
             return Mockito.mock(VMInstanceDao.class);
         }
+
         @Bean
         public UserVmCloneSettingDao cloneSettingDao() {
             return Mockito.mock(UserVmCloneSettingDao.class);
         }
+
         @Bean
         public PrimaryDataStoreDao primaryStorageDao() {
             return Mockito.mock(PrimaryDataStoreDao.class);
         }
 
+        @Bean
+        public TemplateManager templateManager() {
+            return Mockito.mock(TemplateManager.class);
+        }
+
         public static class Library implements TypeFilter {
 
             @Override