rafaelweingartner closed pull request #2808: Fix the problem at #1740 when it
loads all snapshots in the primary storage
URL: https://github.com/apache/cloudstack/pull/2808
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
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.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 @@
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);
}
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.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.utils.exception.CloudRuntimeException;
public class SnapshotDataFactoryImpl implements SnapshotDataFactory {
+
@Inject
- SnapshotDao snapshotDao;
- @Inject
- SnapshotDataStoreDao snapshotStoreDao;
+ private SnapshotDao snapshotDao;
@Inject
- DataStoreManager storeMgr;
+ private SnapshotDataStoreDao snapshotStoreDao;
@Inject
- VolumeDataFactory volumeFactory;
+ private DataStoreManager storeMgr;
@Override
public SnapshotInfo getSnapshot(long snapshotId, DataStore store) {
@@ -66,16 +65,16 @@ public SnapshotInfo getSnapshot(DataObject obj, DataStore
store) {
@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;
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<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);
+ }
+}
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.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 boolean updateState(State currentState, Event
event, State nextState, Dat
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 SnapshotDataStoreVO findLatestSnapshotForVolume(Long
volumeId, DataStoreR
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 SnapshotDataStoreVO findOldestSnapshotForVolume(Long
volumeId, DataStoreR
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 SnapshotDataStoreVO findBySnapshot(long snapshotId,
DataStoreRole role) {
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();
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
