From c614c6a424f33f582e6ce4a0e7bf4d72f5606bda Mon Sep 17 00:00:00 2001 From: Min Chen Date: Fri, 24 May 2013 11:10:45 -0700 Subject: [PATCH] CLOUDSTACK-2674: Secondary Storage garbage collector failed with NPE in case of S3 storage provider. --- .../storage/image/TemplateServiceImpl.java | 7 ++-- .../com/cloud/storage/StorageManagerImpl.java | 2 +- .../com/cloud/template/TemplateManager.java | 2 +- .../cloud/template/TemplateManagerImpl.java | 35 ++++--------------- 4 files changed, 13 insertions(+), 33 deletions(-) diff --git a/engine/storage/image/src/org/apache/cloudstack/storage/image/TemplateServiceImpl.java b/engine/storage/image/src/org/apache/cloudstack/storage/image/TemplateServiceImpl.java index eb6e44c9c46..fd349710193 100644 --- a/engine/storage/image/src/org/apache/cloudstack/storage/image/TemplateServiceImpl.java +++ b/engine/storage/image/src/org/apache/cloudstack/storage/image/TemplateServiceImpl.java @@ -87,6 +87,7 @@ import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.download.DownloadMonitor; import com.cloud.storage.template.TemplateConstants; import com.cloud.storage.template.TemplateProp; +import com.cloud.template.TemplateManager; import com.cloud.user.AccountManager; import com.cloud.user.ResourceLimitService; import com.cloud.utils.UriUtils; @@ -138,6 +139,8 @@ public class TemplateServiceImpl implements TemplateService { EndPointSelector _epSelector; @Inject ImageDataManager imageMgr; + @Inject + TemplateManager _tmpltMgr; class TemplateOpContext extends AsyncRpcConext { @@ -430,9 +433,7 @@ public class TemplateServiceImpl implements TemplateService { for (String uniqueName : templateInfos.keySet()) { TemplateProp tInfo = templateInfos.get(uniqueName); - List userVmUsingIso = _userVmJoinDao.listActiveByIsoId(tInfo.getId()); - //check if there is any Vm using this ISO. - if (userVmUsingIso == null || userVmUsingIso.isEmpty()) { + if (_tmpltMgr.templateIsDeleteable(tInfo.getId())) { //TODO: we cannot directly call deleteTemplateSync here to reuse delete logic since in this case, our db does not have this template at all. VMTemplateVO template = _templateDao.findById(tInfo.getId()); DeleteTemplateCommand dtCommand = new DeleteTemplateCommand(store.getTO(), tInfo.getInstallPath(), null, null); diff --git a/server/src/com/cloud/storage/StorageManagerImpl.java b/server/src/com/cloud/storage/StorageManagerImpl.java index 0313b491261..cbbbe94abe7 100755 --- a/server/src/com/cloud/storage/StorageManagerImpl.java +++ b/server/src/com/cloud/storage/StorageManagerImpl.java @@ -1133,7 +1133,7 @@ public class StorageManagerImpl extends ManagerBase implements StorageManager, C s_logger.debug("Secondary storage garbage collector found " + destroyedTemplateStoreVOs.size() + " templates to cleanup on secondary storage host: " + store.getName()); for (TemplateDataStoreVO destroyedTemplateStoreVO : destroyedTemplateStoreVOs) { - if (!_tmpltMgr.templateIsDeleteable(destroyedTemplateStoreVO)) { + if (!_tmpltMgr.templateIsDeleteable(destroyedTemplateStoreVO.getTemplateId())) { if (s_logger.isDebugEnabled()) { s_logger.debug("Not deleting template at: " + destroyedTemplateStoreVO); } diff --git a/server/src/com/cloud/template/TemplateManager.java b/server/src/com/cloud/template/TemplateManager.java index 202feefcb3e..af71d30d9e9 100755 --- a/server/src/com/cloud/template/TemplateManager.java +++ b/server/src/com/cloud/template/TemplateManager.java @@ -92,7 +92,7 @@ public interface TemplateManager extends TemplateApiService{ boolean templateIsDeleteable(VMTemplateHostVO templateHostRef); - boolean templateIsDeleteable(TemplateDataStoreVO templateStoreRef); + boolean templateIsDeleteable(long templateId); Pair getAbsoluteIsoPath(long templateId, long dataCenterId); diff --git a/server/src/com/cloud/template/TemplateManagerImpl.java b/server/src/com/cloud/template/TemplateManagerImpl.java index eaf9d250cee..c828773bff4 100755 --- a/server/src/com/cloud/template/TemplateManagerImpl.java +++ b/server/src/com/cloud/template/TemplateManagerImpl.java @@ -945,35 +945,15 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, } @Override - public boolean templateIsDeleteable(TemplateDataStoreVO templateStoreRef) { - VMTemplateVO template = _tmpltDao.findByIdIncludingRemoved(templateStoreRef.getTemplateId()); - long templateId = template.getId(); - ImageStoreVO imageStore = _imageStoreDao.findById(templateStoreRef.getDataStoreId()); - long zoneId = imageStore.getDataCenterId(); - DataCenterVO zone = _dcDao.findById(zoneId); - - // Check if there are VMs running in the template host ref's zone that use the template - List nonExpungedVms = _vmInstanceDao.listNonExpungedByZoneAndTemplate(zoneId, templateId); - - if (!nonExpungedVms.isEmpty()) { - s_logger.debug("Template " + template.getName() + " in zone " + zone.getName() + " is not deleteable because there are non-expunged VMs deployed from this template."); - return false; - } - List userVmUsingIso = _userVmDao.listByIsoId(templateId); - //check if there is any VM using this ISO. + public boolean templateIsDeleteable(long templateId) { + List userVmUsingIso = _userVmJoinDao.listActiveByIsoId(templateId); + //check if there is any Vm using this ISO. We only need to check the case where templateId is an ISO since + // VM can be launched from ISO in secondary storage, while template will always be copied to + // primary storage before deploying VM. if (!userVmUsingIso.isEmpty()) { - s_logger.debug("ISO " + template.getName() + " in zone " + zone.getName() + " is not deleteable because it is attached to " + userVmUsingIso.size() + " VMs"); + s_logger.debug("ISO " + templateId + " is not deleteable because it is attached to " + userVmUsingIso.size() + " VMs"); return false; } - // Check if there are any snapshots for the template in the template host ref's zone - List volumes = _volumeDao.findByTemplateAndZone(templateId, zoneId); - for (VolumeVO volume : volumes) { - List snapshots = _snapshotDao.listByVolumeIdVersion(volume.getId(), "2.1"); - if (!snapshots.isEmpty()) { - s_logger.debug("Template " + template.getName() + " in zone " + zone.getName() + " is not deleteable because there are 2.1 snapshots using this template."); - return false; - } - } return true; } @@ -1153,9 +1133,8 @@ public class TemplateManagerImpl extends ManagerBase implements TemplateManager, throw new InvalidParameterValueException("Please specify a valid iso."); } - List userVmUsingIso = _userVmJoinDao.listActiveByIsoId(templateId); // check if there is any VM using this ISO. - if (userVmUsingIso != null && !userVmUsingIso.isEmpty()) { + if (!templateIsDeleteable(templateId)) { throw new InvalidParameterValueException("Unable to delete iso, as it's used by other vms"); }