From 21c0225921a4b9dd8ba2fb823629ab7b6a23ee34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Weing=C3=A4rtner?= Date: Fri, 17 Aug 2018 08:33:25 -0300 Subject: [PATCH] Fix the problem at #1740 when it loads all snapshots in the primary storage (#2808) * Fix the problem at #1740 when it loads all snapshots in the primary storage While checking a problem with @mike-tutkowski at PR #1740, we noticed that the method `org.apache.cloudstack.storage.snapshot.SnapshotDataFactoryImpl.getSnapshots(long, DataStoreRole)` was not loading the snapshots in `snapshot_store_ref` table according to their storage role. Instead, it would return a list of all snapshots (even if they are not in the storage role sent as a parameter) saying that they are in the storage that was sent as parameter. * Add unit test --- .../datastore/db/SnapshotDataStoreDao.java | 7 +- .../snapshot/SnapshotDataFactoryImpl.java | 23 ++-- .../snapshot/SnapshotDataFactoryImplTest.java | 110 ++++++++++++++++++ .../image/db/SnapshotDataStoreDaoImpl.java | 100 +++++++++------- 4 files changed, 182 insertions(+), 58 deletions(-) create mode 100644 engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImplTest.java diff --git a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java index 20cff5671b7..50f311a4eaa 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java @@ -26,7 +26,7 @@ import com.cloud.utils.db.GenericDao; import com.cloud.utils.fsm.StateDao; public interface SnapshotDataStoreDao extends GenericDao, - StateDao { +StateDao { List listByStoreId(long id, DataStoreRole role); @@ -63,5 +63,10 @@ public interface SnapshotDataStoreDao extends GenericDao listAllByVolumeAndDataStore(long volumeId, DataStoreRole role); + List listByState(ObjectInDataStoreStateMachine.State... states); } diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java index ad58f42a97a..11d61df3d7f 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java @@ -28,9 +28,9 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; -import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; +import org.apache.commons.collections.CollectionUtils; import com.cloud.storage.DataStoreRole; import com.cloud.storage.SnapshotVO; @@ -38,14 +38,13 @@ import com.cloud.storage.dao.SnapshotDao; import com.cloud.utils.exception.CloudRuntimeException; public class SnapshotDataFactoryImpl implements SnapshotDataFactory { + @Inject - SnapshotDao snapshotDao; + private SnapshotDao snapshotDao; @Inject - SnapshotDataStoreDao snapshotStoreDao; + private SnapshotDataStoreDao snapshotStoreDao; @Inject - DataStoreManager storeMgr; - @Inject - VolumeDataFactory volumeFactory; + private DataStoreManager storeMgr; @Override public SnapshotInfo getSnapshot(long snapshotId, DataStore store) { @@ -66,16 +65,16 @@ public class SnapshotDataFactoryImpl implements SnapshotDataFactory { @Override public List getSnapshots(long volumeId, DataStoreRole role) { - - SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByVolume(volumeId, role); - if (snapshotStore == null) { + List allSnapshotsFromVolumeAndDataStore = snapshotStoreDao.listAllByVolumeAndDataStore(volumeId, role); + if (CollectionUtils.isEmpty(allSnapshotsFromVolumeAndDataStore)) { return new ArrayList<>(); } - DataStore store = storeMgr.getDataStore(snapshotStore.getDataStoreId(), role); - List volSnapShots = snapshotDao.listByVolumeId(volumeId); List infos = new ArrayList<>(); - for(SnapshotVO snapshot: volSnapShots) { + for (SnapshotDataStoreVO snapshotDataStoreVO : allSnapshotsFromVolumeAndDataStore) { + DataStore store = storeMgr.getDataStore(snapshotDataStoreVO.getDataStoreId(), role); + SnapshotVO snapshot = snapshotDao.findById(snapshotDataStoreVO.getSnapshotId()); SnapshotObject info = SnapshotObject.getSnapshotObject(snapshot, store); + infos.add(info); } return infos; diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImplTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImplTest.java new file mode 100644 index 00000000000..aad339bb8ff --- /dev/null +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImplTest.java @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cloudstack.storage.snapshot; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; +import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import com.cloud.storage.DataStoreRole; +import com.cloud.storage.SnapshotVO; +import com.cloud.storage.dao.SnapshotDao; +import com.cloud.utils.component.ComponentContext; + +@RunWith(PowerMockRunner.class) +public class SnapshotDataFactoryImplTest { + + @Spy + @InjectMocks + private SnapshotDataFactoryImpl snapshotDataFactoryImpl = new SnapshotDataFactoryImpl(); + + @Mock + private SnapshotDataStoreDao snapshotStoreDaoMock; + @Mock + private DataStoreManager dataStoreManagerMock; + @Mock + private SnapshotDao snapshotDaoMock; + + private long volumeMockId = 1; + + @Test + public void getSnapshotsByVolumeAndDataStoreTestNoSnapshotDataStoreVOFound() { + Mockito.doReturn(new ArrayList<>()).when(snapshotStoreDaoMock).listAllByVolumeAndDataStore(volumeMockId, DataStoreRole.Primary); + + List snapshots = snapshotDataFactoryImpl.getSnapshots(volumeMockId, DataStoreRole.Primary); + + Assert.assertTrue(snapshots.isEmpty()); + } + + @Test + @PrepareForTest({ComponentContext.class}) + public void getSnapshotsByVolumeAndDataStoreTest() { + PowerMockito.mockStatic(ComponentContext.class); + PowerMockito.when(ComponentContext.inject(SnapshotObject.class)).thenReturn(new SnapshotObject()); + + SnapshotDataStoreVO snapshotDataStoreVoMock = Mockito.mock(SnapshotDataStoreVO.class); + Mockito.doReturn(volumeMockId).when(snapshotDataStoreVoMock).getVolumeId(); + + long snapshotId = 1223; + long dataStoreId = 34567; + Mockito.doReturn(snapshotId).when(snapshotDataStoreVoMock).getSnapshotId(); + Mockito.doReturn(dataStoreId).when(snapshotDataStoreVoMock).getDataStoreId(); + + SnapshotVO snapshotVoMock = Mockito.mock(SnapshotVO.class); + Mockito.doReturn(snapshotId).when(snapshotVoMock).getId(); + + DataStoreRole dataStoreRole = DataStoreRole.Primary; + DataStore dataStoreMock = Mockito.mock(DataStore.class); + Mockito.doReturn(dataStoreId).when(dataStoreMock).getId(); + Mockito.doReturn(dataStoreRole).when(dataStoreMock).getRole(); + + List snapshotDataStoreVOs = new ArrayList<>(); + snapshotDataStoreVOs.add(snapshotDataStoreVoMock); + + Mockito.doReturn(snapshotDataStoreVOs).when(snapshotStoreDaoMock).listAllByVolumeAndDataStore(volumeMockId, dataStoreRole); + Mockito.doReturn(dataStoreMock).when(dataStoreManagerMock).getDataStore(dataStoreId, dataStoreRole); + Mockito.doReturn(snapshotVoMock).when(snapshotDaoMock).findById(snapshotId); + + List snapshots = snapshotDataFactoryImpl.getSnapshots(volumeMockId, dataStoreRole); + + Assert.assertEquals(1, snapshots.size()); + + SnapshotInfo snapshotInfo = snapshots.get(0); + Assert.assertEquals(dataStoreMock, snapshotInfo.getDataStore()); + Assert.assertEquals(snapshotVoMock, ((SnapshotObject)snapshotInfo).getSnapshotVO()); + + PowerMockito.verifyStatic(); + ComponentContext.inject(SnapshotObject.class); + } +} diff --git a/engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java b/engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java index c3e48b9b2f3..6ca6b239b6f 100644 --- a/engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java +++ b/engine/storage/src/main/java/org/apache/cloudstack/storage/image/db/SnapshotDataStoreDaoImpl.java @@ -16,14 +16,17 @@ // under the License. package org.apache.cloudstack.storage.image.db; -import com.cloud.storage.DataStoreRole; -import com.cloud.utils.db.DB; -import com.cloud.utils.db.GenericDaoBase; -import com.cloud.utils.db.SearchBuilder; -import com.cloud.utils.db.SearchCriteria; -import com.cloud.utils.db.SearchCriteria.Op; -import com.cloud.utils.db.TransactionLegacy; -import com.cloud.utils.db.UpdateBuilder; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.Map; + +import javax.inject.Inject; +import javax.naming.ConfigurationException; + import org.apache.cloudstack.engine.subsystem.api.storage.DataObjectInStore; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event; @@ -33,19 +36,18 @@ import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; +import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.DataStoreRole; import com.cloud.storage.SnapshotVO; import com.cloud.storage.dao.SnapshotDao; -import javax.inject.Inject; -import com.cloud.hypervisor.Hypervisor; -import java.util.ArrayList; +import com.cloud.utils.db.DB; import com.cloud.utils.db.Filter; -import javax.naming.ConfigurationException; -import java.sql.PreparedStatement; -import java.sql.ResultSet; -import java.sql.SQLException; -import java.util.Date; -import java.util.List; -import java.util.Map; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import com.cloud.utils.db.SearchCriteria.Op; +import com.cloud.utils.db.TransactionLegacy; +import com.cloud.utils.db.UpdateBuilder; @Component public class SnapshotDataStoreDaoImpl extends GenericDaoBase implements SnapshotDataStoreDao { @@ -176,33 +178,33 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase listAllByVolumeAndDataStore(long volumeId, DataStoreRole role) { + SearchCriteria sc = volumeSearch.create(); + sc.setParameters("volume_id", volumeId); + sc.setParameters("store_role", role); + return listBy(sc); + } + @Override public SnapshotDataStoreVO findByVolume(long volumeId, DataStoreRole role) { SearchCriteria sc = volumeSearch.create();