From ed87d662053e93135747158d5a0d409f0fef0cb6 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Thu, 6 Oct 2016 15:28:20 -0300 Subject: [PATCH] CLOUDSTACK-9570: Bug in listSnapshots for snapshots with deleted data stores --- .../user/snapshot/ListSnapshotsCmd.java | 6 +- .../PrimaryDataStoreProviderManagerImpl.java | 5 +- .../src/com/cloud/api/ApiResponseHelper.java | 21 ++- .../storage/snapshot/SnapshotManagerImpl.java | 8 +- test/integration/smoke/test_snapshots.py | 140 +++++++++++++++++- 5 files changed, 159 insertions(+), 21 deletions(-) diff --git a/api/src/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java b/api/src/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java index 97bb187dd525..22fee2209b0f 100644 --- a/api/src/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java +++ b/api/src/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java @@ -115,8 +115,10 @@ public void execute() { List snapshotResponses = new ArrayList(); for (Snapshot snapshot : result.first()) { SnapshotResponse snapshotResponse = _responseGenerator.createSnapshotResponse(snapshot); - snapshotResponse.setObjectName("snapshot"); - snapshotResponses.add(snapshotResponse); + if (snapshotResponse != null) { + snapshotResponse.setObjectName("snapshot"); + snapshotResponses.add(snapshotResponse); + } } response.setResponses(snapshotResponses, result.second()); response.setResponseName(getCommandName()); diff --git a/engine/storage/volume/src/org/apache/cloudstack/storage/datastore/manager/PrimaryDataStoreProviderManagerImpl.java b/engine/storage/volume/src/org/apache/cloudstack/storage/datastore/manager/PrimaryDataStoreProviderManagerImpl.java index 49bcb5b69811..da5eb68d181e 100644 --- a/engine/storage/volume/src/org/apache/cloudstack/storage/datastore/manager/PrimaryDataStoreProviderManagerImpl.java +++ b/engine/storage/volume/src/org/apache/cloudstack/storage/datastore/manager/PrimaryDataStoreProviderManagerImpl.java @@ -24,8 +24,6 @@ import javax.annotation.PostConstruct; import javax.inject.Inject; -import org.springframework.stereotype.Component; - import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProviderManager; import org.apache.cloudstack.engine.subsystem.api.storage.HypervisorHostListener; @@ -35,6 +33,7 @@ import org.apache.cloudstack.storage.datastore.PrimaryDataStoreProviderManager; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.springframework.stereotype.Component; import com.cloud.storage.StorageManager; import com.cloud.utils.exception.CloudRuntimeException; @@ -56,7 +55,7 @@ public void config() { @Override public PrimaryDataStore getPrimaryDataStore(long dataStoreId) { - StoragePoolVO dataStoreVO = dataStoreDao.findById(dataStoreId); + StoragePoolVO dataStoreVO = dataStoreDao.findByIdIncludingRemoved(dataStoreId); if (dataStoreVO == null) { throw new CloudRuntimeException("Unable to locate datastore with id " + dataStoreId); } diff --git a/server/src/com/cloud/api/ApiResponseHelper.java b/server/src/com/cloud/api/ApiResponseHelper.java index 6a7f0abde1e3..2cf90e1530cc 100644 --- a/server/src/com/cloud/api/ApiResponseHelper.java +++ b/server/src/com/cloud/api/ApiResponseHelper.java @@ -142,6 +142,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; +import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; import org.apache.cloudstack.framework.jobs.AsyncJob; @@ -493,7 +494,9 @@ public SnapshotResponse createSnapshotResponse(Snapshot snapshot) { snapshotInfo = (SnapshotInfo)snapshot; } else { DataStoreRole dataStoreRole = getDataStoreRole(snapshot, _snapshotStoreDao, _dataStoreMgr); - + if (dataStoreRole == null){ + return null; + } snapshotInfo = snapshotfactory.getSnapshot(snapshot.getId(), dataStoreRole); } @@ -526,16 +529,18 @@ public static DataStoreRole getDataStoreRole(Snapshot snapshot, SnapshotDataStor } long storagePoolId = snapshotStore.getDataStoreId(); - DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary); + if (snapshotStore.getState() != null && ! snapshotStore.getState().equals(ObjectInDataStoreStateMachine.State.Destroyed)) { + DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary); - Map mapCapabilities = dataStore.getDriver().getCapabilities(); + Map mapCapabilities = dataStore.getDriver().getCapabilities(); - if (mapCapabilities != null) { - String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString()); - Boolean supportsStorageSystemSnapshots = new Boolean(value); + if (mapCapabilities != null) { + String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString()); + Boolean supportsStorageSystemSnapshots = Boolean.valueOf(value); - if (supportsStorageSystemSnapshots) { - return DataStoreRole.Primary; + if (supportsStorageSystemSnapshots) { + return DataStoreRole.Primary; + } } } diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 9a363be7688a..9344f62cd88b 100644 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -517,10 +517,10 @@ public Pair, Integer> listSnapshots(ListSnapshotsCmd cm List ids = getIdsListFromCmd(cmd.getId(), cmd.getIds()); Ternary domainIdRecursiveListProject = new Ternary(cmd.getDomainId(), cmd.isRecursive(), null); - _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, cmd.listAll(), false); - Long domainId = domainIdRecursiveListProject.first(); - Boolean isRecursive = domainIdRecursiveListProject.second(); - ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); + _accountMgr.buildACLSearchParameters(caller, id, cmd.getAccountName(), cmd.getProjectId(), permittedAccounts, domainIdRecursiveListProject, cmd.listAll(), false); + Long domainId = domainIdRecursiveListProject.first(); + Boolean isRecursive = domainIdRecursiveListProject.second(); + ListProjectResourcesCriteria listProjectResourcesCriteria = domainIdRecursiveListProject.third(); Filter searchFilter = new Filter(SnapshotVO.class, "created", false, cmd.getStartIndex(), cmd.getPageSizeVal()); SearchBuilder sb = _snapshotDao.createSearchBuilder(); diff --git a/test/integration/smoke/test_snapshots.py b/test/integration/smoke/test_snapshots.py index 638b66cac406..81fbf8b0b294 100644 --- a/test/integration/smoke/test_snapshots.py +++ b/test/integration/smoke/test_snapshots.py @@ -24,15 +24,19 @@ Account, Template, ServiceOffering, - Snapshot) + Snapshot, + StoragePool, + Volume) from marvin.lib.common import (get_domain, get_template, get_zone, + get_pod, list_volumes, - list_snapshots) + list_snapshots, + list_storage_pools, + list_clusters) from marvin.lib.decoratorGenerators import skipTestIf - class Templates: """Test data for templates """ @@ -95,6 +99,7 @@ def setUpClass(cls): # Get Zone, Domain and templates cls.domain = get_domain(cls.apiclient) cls.zone = get_zone(cls.apiclient, testClient.getZoneForTests()) + cls.pod = get_pod(cls.apiclient, cls.zone.id) cls.services['mode'] = cls.zone.networktype cls.hypervisorNotSupported = False @@ -140,7 +145,6 @@ def setUpClass(cls): mode=cls.services["mode"] ) - cls._cleanup.append(cls.virtual_machine) cls._cleanup.append(cls.service_offering) cls._cleanup.append(cls.account) cls._cleanup.append(cls.template) @@ -255,3 +259,131 @@ def test_01_snapshot_root_disk(self): self.assertTrue(is_snapshot_on_nfs( self.apiclient, self.dbclient, self.config, self.zone.id, snapshot.id)) return + + @skipTestIf("hypervisorNotSupported") + @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true") + def test_02_list_snapshots_with_removed_data_store(self): + """Test listing volume snapshots with removed data stores + """ + + # 1) Create new Primary Storage + clusters = list_clusters( + self.apiclient, + zoneid=self.zone.id + ) + assert isinstance(clusters,list) and len(clusters)>0 + + storage = StoragePool.create(self.apiclient, + self.services["nfs"], + clusterid=clusters[0].id, + zoneid=self.zone.id, + podid=self.pod.id + ) + self.assertEqual( + storage.state, + 'Up', + "Check primary storage state" + ) + self.assertEqual( + storage.type, + 'NetworkFilesystem', + "Check storage pool type" + ) + storage_pools_response = list_storage_pools(self.apiclient, + id=storage.id) + self.assertEqual( + isinstance(storage_pools_response, list), + True, + "Check list response returns a valid list" + ) + self.assertNotEqual( + len(storage_pools_response), + 0, + "Check list Hosts response" + ) + storage_response = storage_pools_response[0] + self.assertEqual( + storage_response.id, + storage.id, + "Check storage pool ID" + ) + self.assertEqual( + storage.type, + storage_response.type, + "Check storage pool type " + ) + + # 2) Migrate VM ROOT volume to new Primary Storage + volumes = list_volumes( + self.apiclient, + virtualmachineid=self.virtual_machine_with_disk.id, + type='ROOT', + listall=True + ) + Volume.migrate(self.apiclient, + storageid=storage.id, + volumeid=volumes[0].id, + livemigrate="true" + ) + + volume_response = list_volumes( + self.apiclient, + id=volumes[0].id, + ) + self.assertNotEqual( + len(volume_response), + 0, + "Check list Volumes response" + ) + volume_migrated = volume_response[0] + self.assertEqual( + volume_migrated.storageid, + storage.id, + "Check volume storage id" + ) + self.cleanup.append(self.virtual_machine_with_disk) + self.cleanup.append(storage) + + # 3) Take snapshot of VM ROOT volume + snapshot = Snapshot.create( + self.apiclient, + volume_migrated.id, + account=self.account.name, + domainid=self.account.domainid + ) + self.debug("Snapshot created: ID - %s" % snapshot.id) + + # 4) Delete VM and created Primery Storage + cleanup_resources(self.apiclient, self.cleanup) + + # 5) List snapshot and verify it gets properly listed although Primary Storage was removed + snapshot_response = Snapshot.list( + self.apiclient, + id=snapshot.id + ) + self.assertNotEqual( + len(snapshot_response), + 0, + "Check list Snapshot response" + ) + self.assertEqual( + snapshot_response[0].id, + snapshot.id, + "Check snapshot id" + ) + + # 6) Delete snapshot and verify it gets properly deleted (should not be listed) + self.cleanup = [snapshot] + cleanup_resources(self.apiclient, self.cleanup) + + snapshot_response_2 = Snapshot.list( + self.apiclient, + id=snapshot.id + ) + self.assertEqual( + snapshot_response_2, + None, + "Check list Snapshot response" + ) + + return \ No newline at end of file