From 956e8301c200ca5e4584e88ccb05e4c6d6ec350c Mon Sep 17 00:00:00 2001 From: Min Chen Date: Tue, 23 Jul 2013 15:06:02 -0700 Subject: [PATCH] CLOUDSTACK-3716: State of expunged volumes are not consistent in volumes table and volume_store_ref. Conflicts: server/src/com/cloud/storage/VolumeManagerImpl.java --- .../api/storage/VolumeDataFactory.java | 4 + .../storage/volume/VolumeDataFactoryImpl.java | 20 +++ .../storage/volume/VolumeObject.java | 8 +- .../storage/volume/VolumeServiceImpl.java | 36 +++++- .../com/cloud/storage/VolumeManagerImpl.java | 115 ++++++++++-------- 5 files changed, 125 insertions(+), 58 deletions(-) diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VolumeDataFactory.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VolumeDataFactory.java index fc65a32c33e..99e3b596071 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VolumeDataFactory.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/VolumeDataFactory.java @@ -18,10 +18,14 @@ */ package org.apache.cloudstack.engine.subsystem.api.storage; +import com.cloud.storage.DataStoreRole; + public interface VolumeDataFactory { VolumeInfo getVolume(long volumeId, DataStore store); VolumeInfo getVolume(DataObject volume, DataStore store); + VolumeInfo getVolume(long volumeId, DataStoreRole storeRole); + VolumeInfo getVolume(long volumeId); } diff --git a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeDataFactoryImpl.java b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeDataFactoryImpl.java index 8d0a5a82bb1..2512c49348c 100644 --- a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeDataFactoryImpl.java +++ b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeDataFactoryImpl.java @@ -51,6 +51,26 @@ public class VolumeDataFactoryImpl implements VolumeDataFactory { return vol; } + @Override + public VolumeInfo getVolume(long volumeId, DataStoreRole storeRole) { + VolumeVO volumeVO = volumeDao.findById(volumeId); + VolumeObject vol = null; + if (storeRole == DataStoreRole.Image) { + VolumeDataStoreVO volumeStore = volumeStoreDao.findByVolume(volumeId); + if (volumeStore != null) { + DataStore store = this.storeMgr.getDataStore(volumeStore.getDataStoreId(), DataStoreRole.Image); + vol = VolumeObject.getVolumeObject(store, volumeVO); + } + } else { + // Primary data store + if (volumeVO.getPoolId() != null) { + DataStore store = this.storeMgr.getDataStore(volumeVO.getPoolId(), DataStoreRole.Primary); + vol = VolumeObject.getVolumeObject(store, volumeVO); + } + } + return vol; + } + @Override public VolumeInfo getVolume(long volumeId) { VolumeVO volumeVO = volumeDao.findById(volumeId); diff --git a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java index 55fc3a64c53..c247f18cc24 100644 --- a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java +++ b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeObject.java @@ -109,7 +109,7 @@ public class VolumeObject implements VolumeInfo { @Override public String get_iScsiName() { - return volumeVO.get_iScsiName(); + return volumeVO.get_iScsiName(); } public void setSize(Long size) { @@ -254,9 +254,8 @@ public class VolumeObject implements VolumeInfo { } if (this.dataStore.getRole() == DataStoreRole.Image) { objectInStoreMgr.update(this, event); - if (this.volumeVO.getState() == Volume.State.Migrating - || this.volumeVO.getState() == Volume.State.Copying - || this.volumeVO.getState() == Volume.State.Uploaded) { + if (this.volumeVO.getState() == Volume.State.Migrating || this.volumeVO.getState() == Volume.State.Copying || this.volumeVO.getState() == Volume.State.Uploaded + || this.volumeVO.getState() == Volume.State.Expunged) { return; } if (event == ObjectInDataStoreStateMachine.Event.CreateOnlyRequested) { @@ -513,6 +512,7 @@ public class VolumeObject implements VolumeInfo { } + @Override public void incRefCount() { if (this.dataStore == null) { return; diff --git a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java index 5919c273bc2..cc73b47e5a2 100644 --- a/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java +++ b/engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java @@ -75,6 +75,7 @@ import com.cloud.storage.StoragePool; import com.cloud.storage.VMTemplateStoragePoolVO; import com.cloud.storage.VMTemplateStorageResourceAssoc; import com.cloud.storage.VMTemplateStorageResourceAssoc.Status; +import com.cloud.storage.Volume.State; import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VMTemplatePoolDao; @@ -210,13 +211,34 @@ public class VolumeServiceImpl implements VolumeService { } } + // check if a volume is expunged on both primary and secondary + private boolean canVolumeBeRemoved(long volumeId) { + VolumeVO vol = volDao.findById(volumeId); + if (vol == null) { + // already removed from volumes table + return false; + } + VolumeDataStoreVO volumeStore = _volumeStoreDao.findByVolume(volumeId); + if (vol.getState() == State.Expunged && volumeStore == null) { + // volume is expunged from primary, as well as on secondary + return true; + } else { + return false; + } + + } + @DB @Override public AsyncCallFuture expungeVolumeAsync(VolumeInfo volume) { AsyncCallFuture future = new AsyncCallFuture(); VolumeApiResult result = new VolumeApiResult(volume); if (volume.getDataStore() == null) { - volDao.remove(volume.getId()); + s_logger.info("Expunge volume with no data store specified"); + if (canVolumeBeRemoved(volume.getId())) { + s_logger.info("Volume " + volume.getId() + " is not referred anywhere, remove it from volumes table"); + volDao.remove(volume.getId()); + } future.complete(result); return future; } @@ -245,7 +267,11 @@ public class VolumeServiceImpl implements VolumeService { } VolumeObject vo = (VolumeObject) volume; - volume.processEvent(Event.ExpungeRequested); + if (volume.getDataStore().getRole() == DataStoreRole.Image) { + volume.processEvent(Event.DestroyRequested); + } else if (volume.getDataStore().getRole() == DataStoreRole.Primary) { + volume.processEvent(Event.ExpungeRequested); + } DeleteVolumeContext context = new DeleteVolumeContext(null, vo, future); AsyncCallbackDispatcher caller = AsyncCallbackDispatcher.create(this); @@ -255,6 +281,7 @@ public class VolumeServiceImpl implements VolumeService { return future; } + public Void deleteVolumeCallback(AsyncCallbackDispatcher callback, DeleteVolumeContext context) { CommandResult result = callback.getResult(); @@ -262,7 +289,10 @@ public class VolumeServiceImpl implements VolumeService { VolumeApiResult apiResult = new VolumeApiResult(vo); if (result.isSuccess()) { vo.processEvent(Event.OperationSuccessed); - volDao.remove(vo.getId()); + if (canVolumeBeRemoved(vo.getId())) { + s_logger.info("Volume " + vo.getId() + " is not referred anywhere, remove it from volumes table"); + volDao.remove(vo.getId()); + } } else { vo.processEvent(Event.OperationFailed); apiResult.setResult(result.getResult()); diff --git a/server/src/com/cloud/storage/VolumeManagerImpl.java b/server/src/com/cloud/storage/VolumeManagerImpl.java index a77efbd8347..f9e04f61252 100644 --- a/server/src/com/cloud/storage/VolumeManagerImpl.java +++ b/server/src/com/cloud/storage/VolumeManagerImpl.java @@ -31,6 +31,10 @@ import java.util.concurrent.ExecutionException; import javax.inject.Inject; import javax.naming.ConfigurationException; +import org.apache.commons.lang.StringUtils; +import org.apache.log4j.Logger; +import org.springframework.stereotype.Component; + import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.command.user.volume.AttachVolumeCmd; import org.apache.cloudstack.api.command.user.volume.CreateVolumeCmd; @@ -69,9 +73,7 @@ import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO; import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreDao; import org.apache.cloudstack.storage.datastore.db.VolumeDataStoreVO; import org.apache.cloudstack.storage.image.datastore.ImageStoreEntity; -import org.apache.commons.lang.StringUtils; -import org.apache.log4j.Logger; -import org.springframework.stereotype.Component; + import com.cloud.agent.AgentManager; import com.cloud.agent.api.Answer; import com.cloud.agent.api.storage.CreateVolumeOVAAnswer; @@ -349,7 +351,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { public VolumeInfo moveVolume(VolumeInfo volume, long destPoolDcId, Long destPoolPodId, Long destPoolClusterId, HypervisorType dataDiskHyperType) - throws ConcurrentOperationException { + throws ConcurrentOperationException { // Find a destination storage pool with the specified criteria DiskOfferingVO diskOffering = _diskOfferingDao.findById(volume @@ -411,7 +413,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { private boolean validateVolume(Account caller, long ownerId, Long zoneId, String volumeName, String url, String format) - throws ResourceAllocationException { + throws ResourceAllocationException { // permission check _accountMgr.checkAccess(caller, null, true, @@ -480,17 +482,17 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { && !url.toLowerCase().endsWith("qcow2.zip") && !url.toLowerCase().endsWith("qcow2.bz2") && !url .toLowerCase().endsWith("qcow2.gz"))) - || (format.equalsIgnoreCase("ova") && (!url.toLowerCase() - .endsWith(".ova") - && !url.toLowerCase().endsWith("ova.zip") - && !url.toLowerCase().endsWith("ova.bz2") && !url - .toLowerCase().endsWith("ova.gz"))) - || (format.equalsIgnoreCase("raw") && (!url.toLowerCase() - .endsWith(".img") && !url.toLowerCase().endsWith("raw")))) { + || (format.equalsIgnoreCase("ova") && (!url.toLowerCase() + .endsWith(".ova") + && !url.toLowerCase().endsWith("ova.zip") + && !url.toLowerCase().endsWith("ova.bz2") && !url + .toLowerCase().endsWith("ova.gz"))) + || (format.equalsIgnoreCase("raw") && (!url.toLowerCase() + .endsWith(".img") && !url.toLowerCase().endsWith("raw")))) { throw new InvalidParameterValueException( "Please specify a valid URL. URL:" + url - + " is an invalid for the format " - + format.toLowerCase()); + + " is an invalid for the format " + + format.toLowerCase()); } UriUtils.validateUrl(url); @@ -905,26 +907,26 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { if (isCustomizedIops != null) { if (isCustomizedIops) { - minIops = cmd.getMinIops(); - maxIops = cmd.getMaxIops(); + minIops = cmd.getMinIops(); + maxIops = cmd.getMaxIops(); - if (minIops == null && maxIops == null) { - minIops = 0L; - maxIops = 0L; - } - else { + if (minIops == null && maxIops == null) { + minIops = 0L; + maxIops = 0L; + } + else { if (minIops == null || minIops <= 0) { throw new InvalidParameterValueException("The min IOPS must be greater than 0."); } - if (maxIops == null) { - maxIops = 0L; - } + if (maxIops == null) { + maxIops = 0L; + } - if (minIops > maxIops) { - throw new InvalidParameterValueException("The min IOPS must be less than or equal to the max IOPS."); - } - } + if (minIops > maxIops) { + throw new InvalidParameterValueException("The min IOPS must be less than or equal to the max IOPS."); + } + } } else { minIops = diskOffering.getMinIops(); @@ -933,10 +935,10 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { } if (!validateVolumeSizeRange(size)) {// convert size from mb to gb - // for validation + // for validation throw new InvalidParameterValueException( "Invalid size for custom volume creation: " + size - + " ,max volume size is:" + _maxVolumeSizeInGb); + + " ,max volume size is:" + _maxVolumeSizeInGb); } } else { // create volume from snapshot Long snapshotId = cmd.getSnapshotId(); @@ -957,7 +959,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { diskOffering = _diskOfferingDao.findById(diskOfferingId); zoneId = snapshotCheck.getDataCenterId(); size = snapshotCheck.getSize(); // ; disk offering is used for tags - // purposes + // purposes // check snapshot permissions _accountMgr.checkAccess(caller, null, true, snapshotCheck); @@ -1311,7 +1313,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { _accountMgr.checkAccess(caller, null, true, volume); - if (volume.getInstanceId() != null) { + if (volume.getInstanceId() != null) { throw new InvalidParameterValueException( "Please specify a volume that is not attached to any VM."); } @@ -1355,9 +1357,20 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { _usageEventDao.persist(usageEvent); } } - AsyncCallFuture future = volService.expungeVolumeAsync(volFactory.getVolume(volume.getId())); - future.get(); - + // expunge volume from primary if volume is on primary + VolumeInfo volOnPrimary = volFactory.getVolume(volume.getId(), DataStoreRole.Primary); + if (volOnPrimary != null) { + s_logger.info("Expunging volume " + volume.getId() + " from primary data store"); + AsyncCallFuture future = volService.expungeVolumeAsync(volOnPrimary); + future.get(); + } + // expunge volume from secondary if volume is on image store + VolumeInfo volOnSecondary = volFactory.getVolume(volume.getId(), DataStoreRole.Image); + if (volOnSecondary != null) { + s_logger.info("Expunging volume " + volume.getId() + " from secondary data store"); + AsyncCallFuture future2 = volService.expungeVolumeAsync(volOnSecondary); + future2.get(); + } } catch (Exception e) { s_logger.warn("Failed to expunge volume:", e); return false; @@ -1644,7 +1657,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { volumeToAttach = _volsDao.findById(volumeToAttach.getId()); if (volumeToAttachStoragePool.isManaged() && - volumeToAttach.getPath() == null) { + volumeToAttach.getPath() == null) { volumeToAttach.setPath(answer.getDisk().getVdiUuid()); _volsDao.update(volumeToAttach.getId(), volumeToAttach); @@ -1656,8 +1669,8 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { // insert record for disk I/O statistics VmDiskStatisticsVO diskstats = _vmDiskStatsDao.findBy(vm.getAccountId(), vm.getDataCenterId(),vm.getId(), volumeToAttach.getId()); if (diskstats == null) { - diskstats = new VmDiskStatisticsVO(vm.getAccountId(), vm.getDataCenterId(),vm.getId(), volumeToAttach.getId()); - _vmDiskStatsDao.persist(diskstats); + diskstats = new VmDiskStatisticsVO(vm.getAccountId(), vm.getDataCenterId(),vm.getId(), volumeToAttach.getId()); + _vmDiskStatsDao.persist(diskstats); } return _volsDao.findById(volumeToAttach.getId()); @@ -2030,7 +2043,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { @DB protected VolumeVO switchVolume(VolumeVO existingVolume, VirtualMachineProfile vm) - throws StorageUnavailableException { + throws StorageUnavailableException { Transaction txn = Transaction.currentTxn(); Long templateIdToUse = null; @@ -2086,7 +2099,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { for (VolumeVO vol : volumesForVm) { if (vol.getVolumeType().equals(Type.ROOT)) { // Destroy volume if not already destroyed - boolean volumeAlreadyDestroyed = (vol.getState() == Volume.State.Destroy || + boolean volumeAlreadyDestroyed = (vol.getState() == Volume.State.Destroy || vol.getState() == Volume.State.Expunged || vol.getState() == Volume.State.Expunging); if (!volumeAlreadyDestroyed) { @@ -2337,14 +2350,14 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { MIGRATE } private static class VolumeTask { - final VolumeTaskType type; - final StoragePoolVO pool; - final VolumeVO volume; - VolumeTask(VolumeTaskType type, VolumeVO volume, StoragePoolVO pool) { - this.type = type; - this.pool = pool; - this.volume = volume; - } + final VolumeTaskType type; + final StoragePoolVO pool; + final VolumeVO volume; + VolumeTask(VolumeTaskType type, VolumeVO volume, StoragePoolVO pool) { + this.type = type; + this.pool = pool; + this.volume = volume; + } } private List getTasks(List vols, Map destVols) throws StorageUnavailableException { @@ -2445,7 +2458,7 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { DataStore destPool = null; if (recreate && (dest.getStorageForDisks() == null || dest - .getStorageForDisks().get(vol) == null)) { + .getStorageForDisks().get(vol) == null)) { destPool = dataStoreMgr.getDataStore(vol.getPoolId(), DataStoreRole.Primary); s_logger.debug("existing pool: " + destPool.getId()); } else { @@ -2602,8 +2615,8 @@ public class VolumeManagerImpl extends ManagerBase implements VolumeManager { .getValue(Config.CustomDiskOfferingMinSize.toString()); _customDiskOfferingMinSize = NumbersUtil.parseInt( _customDiskOfferingMinSizeStr, Integer - .parseInt(Config.CustomDiskOfferingMinSize - .getDefaultValue())); + .parseInt(Config.CustomDiskOfferingMinSize + .getDefaultValue())); String maxVolumeSizeInGbString = _configDao .getValue("storage.max.volume.size");