From 3be06391eb5e66c437db461fcf9ec56e818af291 Mon Sep 17 00:00:00 2001 From: Priyank Parihar Date: Tue, 27 Sep 2016 09:33:58 +0530 Subject: [PATCH] CLOUDSTACK-9607: Preventing template deletion when template is in use.(+ Unit Test and UI) --- .../user/template/DeleteTemplateCmd.java | 6 ++ .../src/com/cloud/vm/dao/VMInstanceDao.java | 8 +++ .../com/cloud/vm/dao/VMInstanceDaoImpl.java | 16 ++++++ .../cloud/template/TemplateManagerImpl.java | 14 +++++ .../template/TemplateManagerImplTest.java | 55 +++++++++++++++++++ ui/scripts/templates.js | 26 ++++++--- 6 files changed, 117 insertions(+), 8 deletions(-) mode change 100644 => 100755 api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java mode change 100644 => 100755 engine/schema/src/com/cloud/vm/dao/VMInstanceDao.java mode change 100644 => 100755 engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java mode change 100644 => 100755 server/src/com/cloud/template/TemplateManagerImpl.java mode change 100644 => 100755 server/test/com/cloud/template/TemplateManagerImplTest.java diff --git a/api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java b/api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java old mode 100644 new mode 100755 index 98d53be836e5..95b3eeee0595 --- a/api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java +++ b/api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java @@ -52,6 +52,9 @@ public class DeleteTemplateCmd extends BaseAsyncCmd { @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "the ID of zone of the template") private Long zoneId; + @Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN, required = false, description = "Force delete a template.", since = "4.9+") + private Boolean forced; + ///////////////////////////////////////////////////// /////////////////// Accessors /////////////////////// ///////////////////////////////////////////////////// @@ -64,6 +67,9 @@ public Long getZoneId() { return zoneId; } + public boolean isForced() { + return (forced != null) ? forced : true; + } ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/engine/schema/src/com/cloud/vm/dao/VMInstanceDao.java b/engine/schema/src/com/cloud/vm/dao/VMInstanceDao.java old mode 100644 new mode 100755 index 3c5024b833e5..69efea42df96 --- a/engine/schema/src/com/cloud/vm/dao/VMInstanceDao.java +++ b/engine/schema/src/com/cloud/vm/dao/VMInstanceDao.java @@ -53,6 +53,14 @@ public interface VMInstanceDao extends GenericDao, StateDao< */ List listByPodId(long podId); + /** + * Lists non-expunged VMs by templateId + * @param templateId + * @return list of VMInstanceVO deployed from the specified template, that are not expunged + */ + public List listNonExpungedByTemplate(long templateId); + + /** * Lists non-expunged VMs by zone ID and templateId * @param zoneId diff --git a/engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java b/engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java old mode 100644 new mode 100755 index 7065350a57ef..6e97d1275a65 --- a/engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java +++ b/engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java @@ -72,6 +72,7 @@ public class VMInstanceDaoImpl extends GenericDaoBase implem protected SearchBuilder IdStatesSearch; protected SearchBuilder AllFieldsSearch; protected SearchBuilder ZoneTemplateNonExpungedSearch; + protected SearchBuilder TemplateNonExpungedSearch; protected SearchBuilder NameLikeSearch; protected SearchBuilder StateChangeSearch; protected SearchBuilder TransitionSearch; @@ -165,6 +166,12 @@ protected void init() { ZoneTemplateNonExpungedSearch.and("state", ZoneTemplateNonExpungedSearch.entity().getState(), Op.NEQ); ZoneTemplateNonExpungedSearch.done(); + + TemplateNonExpungedSearch = createSearchBuilder(); + TemplateNonExpungedSearch.and("template", TemplateNonExpungedSearch.entity().getTemplateId(), Op.EQ); + TemplateNonExpungedSearch.and("state", TemplateNonExpungedSearch.entity().getState(), Op.NEQ); + TemplateNonExpungedSearch.done(); + NameLikeSearch = createSearchBuilder(); NameLikeSearch.and("name", NameLikeSearch.entity().getHostName(), Op.LIKE); NameLikeSearch.done(); @@ -334,6 +341,15 @@ public List listByZoneIdAndType(long zoneId, VirtualMachine.Type t return listBy(sc); } + @Override + public List listNonExpungedByTemplate(long templateId) { + SearchCriteria sc = TemplateNonExpungedSearch.create(); + + sc.setParameters("template", templateId); + sc.setParameters("state", State.Expunging); + return listBy(sc); + } + @Override public List listNonExpungedByZoneAndTemplate(long zoneId, long templateId) { SearchCriteria sc = ZoneTemplateNonExpungedSearch.create(); diff --git a/server/src/com/cloud/template/TemplateManagerImpl.java b/server/src/com/cloud/template/TemplateManagerImpl.java old mode 100644 new mode 100755 index f6494c3b77e2..41f11af68d09 --- a/server/src/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/com/cloud/template/TemplateManagerImpl.java @@ -38,6 +38,7 @@ import com.cloud.utils.DateUtil; import com.cloud.utils.Pair; import com.cloud.utils.EnumUtils; +import com.google.common.base.Joiner; import com.google.gson.Gson; import com.google.gson.GsonBuilder; @@ -1235,6 +1236,19 @@ public boolean deleteTemplate(DeleteTemplateCmd cmd) { throw new InvalidParameterValueException("unable to find template with id " + templateId); } + List vmInstanceVOList; + if(cmd.getZoneId() != null) { + vmInstanceVOList = _vmInstanceDao.listNonExpungedByZoneAndTemplate(cmd.getZoneId(), templateId); + } + else { + vmInstanceVOList = _vmInstanceDao.listNonExpungedByTemplate(templateId); + } + if(!cmd.isForced() && CollectionUtils.isNotEmpty(vmInstanceVOList)) { + final String message = String.format("Unable to delete template with id: %1$s because VM instances: [%2$s] are using it.", templateId, Joiner.on(",").join(vmInstanceVOList)); + s_logger.warn(message); + throw new InvalidParameterValueException(message); + } + _accountMgr.checkAccess(caller, AccessType.OperateEntry, true, template); if (template.getFormat() == ImageFormat.ISO) { diff --git a/server/test/com/cloud/template/TemplateManagerImplTest.java b/server/test/com/cloud/template/TemplateManagerImplTest.java old mode 100644 new mode 100755 index 3039b8bda558..bd21643b4a1a --- a/server/test/com/cloud/template/TemplateManagerImplTest.java +++ b/server/test/com/cloud/template/TemplateManagerImplTest.java @@ -30,6 +30,8 @@ import com.cloud.host.Status; import com.cloud.host.dao.HostDao; import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.Storage; +import com.cloud.storage.TemplateProfile; import com.cloud.projects.ProjectManager; import com.cloud.storage.GuestOSVO; import com.cloud.storage.Snapshot; @@ -59,9 +61,11 @@ import com.cloud.utils.component.ComponentContext; import com.cloud.utils.concurrency.NamedThreadFactory; import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.VMInstanceVO; import com.cloud.vm.dao.UserVmDao; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.api.command.user.template.CreateTemplateCmd; +import org.apache.cloudstack.api.command.user.template.DeleteTemplateCmd; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; @@ -169,6 +173,13 @@ public class TemplateManagerImplTest { @Inject StorageStrategyFactory storageStrategyFactory; + @Inject + VMInstanceDao _vmInstanceDao; + + @Inject + private VMTemplateDao _tmpltDao; + + public class CustomThreadPoolExecutor extends ThreadPoolExecutor { AtomicInteger ai = new AtomicInteger(0); public CustomThreadPoolExecutor(int corePoolSize, int maximumPoolSize, long keepAliveTime, TimeUnit unit, @@ -216,6 +227,50 @@ public void testVerifyTemplateIdOfNonSystemTemplate() { templateManager.verifyTemplateId(1L); } + @Test + public void testForceDeleteTemplate() { + //In this Unit test all the conditions related to "force template delete flag" are tested. + + DeleteTemplateCmd cmd = mock(DeleteTemplateCmd.class); + VMTemplateVO template = mock(VMTemplateVO.class); + TemplateAdapter templateAdapter = mock(TemplateAdapter.class); + TemplateProfile templateProfile = mock(TemplateProfile.class); + + + List vmInstanceVOList = new ArrayList(); + List adapters = new ArrayList(); + adapters.add(templateAdapter); + when(cmd.getId()).thenReturn(0L); + when(_tmpltDao.findById(cmd.getId())).thenReturn(template); + when(cmd.getZoneId()).thenReturn(null); + + when(template.getHypervisorType()).thenReturn(Hypervisor.HypervisorType.None); + when(template.getFormat()).thenReturn(Storage.ImageFormat.VMDK); + templateManager.setTemplateAdapters(adapters); + when(templateAdapter.getName()).thenReturn(TemplateAdapter.TemplateAdapterType.Hypervisor.getName().toString()); + when(templateAdapter.prepareDelete(any(DeleteTemplateCmd.class))).thenReturn(templateProfile); + when(templateAdapter.delete(templateProfile)).thenReturn(true); + + //case 1: When Force delete flag is 'true' but VM instance VO list is empty. + when(cmd.isForced()).thenReturn(true); + templateManager.deleteTemplate(cmd); + + //case 2.1: When Force delete flag is 'false' and VM instance VO list is empty. + when(cmd.isForced()).thenReturn(false); + templateManager.deleteTemplate(cmd); + + //case 2.2: When Force delete flag is 'false' and VM instance VO list is non empty. + when(cmd.isForced()).thenReturn(false); + VMInstanceVO vmInstanceVO = mock(VMInstanceVO.class); + when(vmInstanceVO.getInstanceName()).thenReturn("mydDummyVM"); + vmInstanceVOList.add(vmInstanceVO); + when(_vmInstanceDao.listNonExpungedByTemplate(anyLong())).thenReturn(vmInstanceVOList); + try { + templateManager.deleteTemplate(cmd); + } catch(Exception e) { + assertTrue("Invalid Parameter Value Exception is expected", (e instanceof InvalidParameterValueException)); + } + } @Test public void testPrepareTemplateIsSeeded() { VMTemplateVO mockTemplate = mock(VMTemplateVO.class); diff --git a/ui/scripts/templates.js b/ui/scripts/templates.js index 1ab1b9b09dd9..8a7ec6e33883 100755 --- a/ui/scripts/templates.js +++ b/ui/scripts/templates.js @@ -1526,14 +1526,24 @@ actions: { remove: { label: 'label.action.delete.template', + createForm: { + title: 'label.action.delete.template', + desc: function(args) { + if(args.context.templates[0].crossZones == true) { + return 'message.action.delete.template.for.all.zones'; + } else { + return 'message.action.delete.template'; + } + }, + fields: { + forced: { + label: 'force.delete', + isBoolean: true, + isChecked: false + } + } + }, messages: { - confirm: function(args) { - if(args.context.templates[0].crossZones == true) { - return 'message.action.delete.template.for.all.zones'; - } else { - return 'message.action.delete.template'; - } - }, notification: function(args) { return 'label.action.delete.template'; } @@ -1544,7 +1554,7 @@ queryParams += "&zoneid=" + args.context.zones[0].zoneid; } $.ajax({ - url: createURL(queryParams), + url: createURL(queryParams + "&forced=" + (args.data.forced == "on")), dataType: "json", async: true, success: function(json) {