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
2025-12-16 10:32:34 +01:00 · 2018-08-17 08:33:25 -03:00 · 2018-08-17 08:33:25 -03:00 · 21c0225921
commit 21c0225921
parent bd7a09b5d9
4 changed files with 182 additions and 58 deletions
--- 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<SnapshotDataStoreVO, Long>,
-        StateDao<ObjectInDataStoreStateMachine.State, ObjectInDataStoreStateMachine.Event, DataObjectInStore> {
+StateDao<ObjectInDataStoreStateMachine.State, ObjectInDataStoreStateMachine.Event, DataObjectInStore> {

    List<SnapshotDataStoreVO> listByStoreId(long id, DataStoreRole role);

@ -63,5 +63,10 @@ public interface SnapshotDataStoreDao extends GenericDao<SnapshotDataStoreVO, Lo

    SnapshotDataStoreVO findByVolume(long volumeId, DataStoreRole role);

+    /**
+     * List all snapshots in 'snapshot_store_ref' by volume and data store role. Therefore, it is possible to list all snapshots that are in the primary storage or in the secondary storage.
+     */
+    List<SnapshotDataStoreVO> listAllByVolumeAndDataStore(long volumeId, DataStoreRole role);
+
    List<SnapshotDataStoreVO> listByState(ObjectInDataStoreStateMachine.State... states);
 }
--- 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<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole role) {
-
-        SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByVolume(volumeId, role);
-        if (snapshotStore == null) {
+        List<SnapshotDataStoreVO> allSnapshotsFromVolumeAndDataStore = snapshotStoreDao.listAllByVolumeAndDataStore(volumeId, role);
+        if (CollectionUtils.isEmpty(allSnapshotsFromVolumeAndDataStore)) {
            return new ArrayList<>();
        }
-        DataStore store = storeMgr.getDataStore(snapshotStore.getDataStoreId(), role);
-        List<SnapshotVO> volSnapShots = snapshotDao.listByVolumeId(volumeId);
        List<SnapshotInfo> 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;
--- 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
@ -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<SnapshotInfo> 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<SnapshotDataStoreVO> 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<SnapshotInfo> 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);
+    }
+}
--- 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<SnapshotDataStoreVO, Long> implements SnapshotDataStoreDao {
@ -176,33 +178,33 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
            if (dbVol != null) {
                StringBuilder str = new StringBuilder("Unable to update ").append(dataObj.toString());
                str.append(": DB Data={id=")
-                    .append(dbVol.getId())
-                    .append("; state=")
-                    .append(dbVol.getState())
-                    .append("; updatecount=")
-                    .append(dbVol.getUpdatedCount())
-                    .append(";updatedTime=")
-                    .append(dbVol.getUpdated());
+                .append(dbVol.getId())
+                .append("; state=")
+                .append(dbVol.getState())
+                .append("; updatecount=")
+                .append(dbVol.getUpdatedCount())
+                .append(";updatedTime=")
+                .append(dbVol.getUpdated());
                str.append(": New Data={id=")
-                    .append(dataObj.getId())
-                    .append("; state=")
-                    .append(nextState)
-                    .append("; event=")
-                    .append(event)
-                    .append("; updatecount=")
-                    .append(dataObj.getUpdatedCount())
-                    .append("; updatedTime=")
-                    .append(dataObj.getUpdated());
+                .append(dataObj.getId())
+                .append("; state=")
+                .append(nextState)
+                .append("; event=")
+                .append(event)
+                .append("; updatecount=")
+                .append(dataObj.getUpdatedCount())
+                .append("; updatedTime=")
+                .append(dataObj.getUpdated());
                str.append(": stale Data={id=")
-                    .append(dataObj.getId())
-                    .append("; state=")
-                    .append(currentState)
-                    .append("; event=")
-                    .append(event)
-                    .append("; updatecount=")
-                    .append(oldUpdated)
-                    .append("; updatedTime=")
-                    .append(oldUpdatedTime);
+                .append(dataObj.getId())
+                .append("; state=")
+                .append(currentState)
+                .append("; event=")
+                .append(event)
+                .append("; updatecount=")
+                .append(oldUpdated)
+                .append("; updatedTime=")
+                .append(oldUpdatedTime);
            } else {
                s_logger.debug("Unable to update objectIndatastore: id=" + dataObj.getId() + ", as there is no such object exists in the database anymore");
            }
@ -254,7 +256,7 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
        TransactionLegacy txn = TransactionLegacy.currentTxn();
        try (
                PreparedStatement pstmt = txn.prepareStatement(findLatestSnapshot);
-            ){
+                ){
            pstmt.setString(1, role.toString());
            pstmt.setLong(2, volumeId);
            try (ResultSet rs = pstmt.executeQuery();) {
@ -275,7 +277,7 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
        TransactionLegacy txn = TransactionLegacy.currentTxn();
        try (
                PreparedStatement pstmt = txn.prepareStatement(findOldestSnapshot);
-            ){
+                ){
            pstmt.setString(1, role.toString());
            pstmt.setLong(2, volumeId);
            try (ResultSet rs = pstmt.executeQuery();) {
@ -318,6 +320,14 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
        return findOneBy(sc);
    }

+    @Override
+    public List<SnapshotDataStoreVO> listAllByVolumeAndDataStore(long volumeId, DataStoreRole role) {
+        SearchCriteria<SnapshotDataStoreVO> 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<SnapshotDataStoreVO> sc = volumeSearch.create();