Skip to content

CLOUDSTACK-9570: Bug in listSnapshots for snapshots with deleted data stores #1735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nvazquez wants to merge 1 commit into from
Closed

CLOUDSTACK-9570: Bug in listSnapshots for snapshots with deleted data stores #1735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions api/src/org/apache/cloudstack/api/command/user/snapshot/ListSnapshotsCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,10 @@ public void execute() {
List<SnapshotResponse> snapshotResponses = new ArrayList<SnapshotResponse>();
for (Snapshot snapshot : result.first()) {
SnapshotResponse snapshotResponse = _responseGenerator.createSnapshotResponse(snapshot);
snapshotResponse.setObjectName("snapshot");
snapshotResponses.add(snapshotResponse);
if (snapshotResponse != null) {
snapshotResponse.setObjectName("snapshot");
Copy link

@anshul1886 anshul1886 Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is any issue observed here for which null check is added?

Copy link
Contributor Author

@nvazquez nvazquez Jan 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is added as we introduced a case in createSnapshotResponse where null is returned, and in that case we shouldn't add the response to the response list

Copy link

@anshul1886 anshul1886 Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for null case when that can happen?

Copy link
Member

@mike-tutkowski mike-tutkowski Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few comments.

If we implement those comments, then null should not be a problem here (in other words, the code in this class could remain unchanged).

snapshotResponses.add(snapshotResponse);
}
}
response.setResponses(snapshotResponses, result.second());
response.setResponseName(getCommandName());
Expand Down
5 changes: 2 additions & 3 deletions .../org/apache/cloudstack/storage/datastore/manager/PrimaryDataStoreProviderManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -56,7 +55,7 @@ public void config() {

@Override
public PrimaryDataStore getPrimaryDataStore(long dataStoreId) {
StoragePoolVO dataStoreVO = dataStoreDao.findById(dataStoreId);
StoragePoolVO dataStoreVO = dataStoreDao.findByIdIncludingRemoved(dataStoreId);
Copy link
Member

@mike-tutkowski mike-tutkowski Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is getPrimaryDataStore(long) called from many places? If so, it might be a bit risky to change this from findById to findByIdIncludingRemoved unless we are pretty sure all of the calling code is OK with that change.

if (dataStoreVO == null) {
throw new CloudRuntimeException("Unable to locate datastore with id " + dataStoreId);
}
Expand Down
21 changes: 13 additions & 8 deletions server/src/com/cloud/api/ApiResponseHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -493,7 +494,9 @@ public SnapshotResponse createSnapshotResponse(Snapshot snapshot) {
snapshotInfo = (SnapshotInfo)snapshot;
} else {
DataStoreRole dataStoreRole = getDataStoreRole(snapshot, _snapshotStoreDao, _dataStoreMgr);
Copy link
Member

@mike-tutkowski mike-tutkowski Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the default should be DataStoreRole.Image for getDataStoreRole and so getDataStoreRole should probably never return null.


if (dataStoreRole == null){
return null;
}
snapshotInfo = snapshotfactory.getSnapshot(snapshot.getId(), dataStoreRole);
}

Expand Down Expand Up @@ -526,16 +529,18 @@ public static DataStoreRole getDataStoreRole(Snapshot snapshot, SnapshotDataStor
}
Copy link
Member

@mike-tutkowski mike-tutkowski Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility here is that we could simply still try to retrieve "dataStore" and then perform this check:

     DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);

     if (dataStore == null) {
         return DataStoreRole.Image;
      }

If "dataStore" equals null, then it was removed, which should only be something that happened when unmanaged storage is being used (thus when the the snapshot resides on secondary storage).


long storagePoolId = snapshotStore.getDataStoreId();
DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);
if (snapshotStore.getState() != null && ! snapshotStore.getState().equals(ObjectInDataStoreStateMachine.State.Destroyed)) {
Copy link

@anshul1886 anshul1886 Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out by @mike-tutkowski store cannot be destroyed if there are snapshots. Also that enum is meant for snapshot (and entities like that) and not for snapshot store.

Copy link
Member

@mike-tutkowski mike-tutkowski Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For managed storage, that is true. For unmanaged storage, one should be able to delete the primary storage from which a snapshot was created and the snapshot should be able to remain (since the snapshot, in this case, is on secondary storage).

DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);

Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();
Map<String, String> 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;
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,10 +517,10 @@ public Pair<List<? extends Snapshot>, Integer> listSnapshots(ListSnapshotsCmd cm
List<Long> ids = getIdsListFromCmd(cmd.getId(), cmd.getIds());

Ternary<Long, Boolean, ListProjectResourcesCriteria> domainIdRecursiveListProject = new Ternary<Long, Boolean, ListProjectResourcesCriteria>(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();
Copy link

@anshul1886 anshul1886 Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvazquez What is changed here?

Copy link
Contributor Author

@nvazquez nvazquez Jan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anshul1886, at the beggining of each line there's one space added, to be consistent with indentation

Copy link

@anshul1886 anshul1886 Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These type of changes should not be done on unrelated file as it makes following changes difficult.


Filter searchFilter = new Filter(SnapshotVO.class, "created", false, cmd.getStartIndex(), cmd.getPageSizeVal());
SearchBuilder<SnapshotVO> sb = _snapshotDao.createSearchBuilder();
Expand Down
140 changes: 136 additions & 4 deletions test/integration/smoke/test_snapshots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
"""
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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