This is an automated email from the ASF dual-hosted git repository.
rafael pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/master by this push:
new 21c0225 Fix the problem at #1740 when it loads all snapshots in the
primary storage (#2808)
21c0225 is described below
commit 21c0225921a4b9dd8ba2fb823629ab7b6a23ee34
Author: Rafael Weingärtner <[email protected]>
AuthorDate: Fri Aug 17 08:33:25 2018 -0300
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
---
.../storage/datastore/db/SnapshotDataStoreDao.java | 7 +-
.../storage/snapshot/SnapshotDataFactoryImpl.java | 23 +++--
.../snapshot/SnapshotDataFactoryImplTest.java | 110 +++++++++++++++++++++
.../storage/image/db/SnapshotDataStoreDaoImpl.java | 100 ++++++++++---------
4 files changed, 182 insertions(+), 58 deletions(-)
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 20cff56..50f311a 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<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);
}
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 ad58f42..11d61df 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;
- @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 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;
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 0000000..aad339b
--- /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 c3e48b9..6ca6b23 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<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();) {
@@ -319,6 +321,14 @@ public class SnapshotDataStoreDaoImpl extends
GenericDaoBase<SnapshotDataStoreVO
}
@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();
sc.setParameters("volume_id", volumeId);
