This is an automated email from the ASF dual-hosted git repository.
rohit pushed a commit to branch 4.17
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.17 by this push:
new 2c05b63495 kvm: Fix for Revert volume snapshot (#6527)
2c05b63495 is described below
commit 2c05b6349548d8795326225190c7d782d6a767be
Author: Harikrishna <[email protected]>
AuthorDate: Wed Jul 20 11:34:02 2022 +0530
kvm: Fix for Revert volume snapshot (#6527)
This PR fixes the issue #6209 where the snapshot revert operation fails
after certain volume operations like Migrate VM with volume / migrate volume /
reinstall VM.
The root cause of the issue after these volume operations, the primary
storage entry is getting deleted for that volume. We have fixed it here to get
the primary datastore entry wrt volume and continue the operation.
---
.../storage/snapshot/SnapshotServiceImpl.java | 11 ++-
.../storage/snapshot/SnapshotServiceImplTest.java | 98 ++++++++++++++++++++++
.../LibvirtRevertSnapshotCommandWrapper.java | 18 ++--
.../driver/DateraPrimaryDataStoreDriver.java | 1 +
.../CloudStackPrimaryDataStoreDriverImpl.java | 17 +++-
5 files changed, 134 insertions(+), 11 deletions(-)
diff --git
a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
index b8788fbef9..4d106f5c4f 100644
---
a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
+++
b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
@@ -36,6 +36,7 @@ import
org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
import org.apache.cloudstack.engine.subsystem.api.storage.StorageCacheManager;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.framework.async.AsyncCallFuture;
import org.apache.cloudstack.framework.async.AsyncCallbackDispatcher;
@@ -79,6 +80,8 @@ public class SnapshotServiceImpl implements SnapshotService {
StorageCacheManager _cacheMgr;
@Inject
private SnapshotDetailsDao _snapshotDetailsDao;
+ @Inject
+ VolumeDataFactory volFactory;
static private class CreateSnapshotContext<T> extends AsyncRpcContext<T> {
final SnapshotInfo snapshot;
@@ -428,11 +431,15 @@ public class SnapshotServiceImpl implements
SnapshotService {
@Override
public boolean revertSnapshot(SnapshotInfo snapshot) {
+ PrimaryDataStore store = null;
SnapshotInfo snapshotOnPrimaryStore =
_snapshotFactory.getSnapshot(snapshot.getId(), DataStoreRole.Primary);
if (snapshotOnPrimaryStore == null) {
- throw new CloudRuntimeException("Cannot find an entry for snapshot
" + snapshot.getId() + " on primary storage pools");
+ s_logger.warn("Cannot find an entry for snapshot " +
snapshot.getId() + " on primary storage pools, searching with volume's primary
storage pool");
+ VolumeInfo volumeInfo =
volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
+ store = (PrimaryDataStore)volumeInfo.getDataStore();
+ } else {
+ store = (PrimaryDataStore)snapshotOnPrimaryStore.getDataStore();
}
- PrimaryDataStore store =
(PrimaryDataStore)snapshotOnPrimaryStore.getDataStore();
AsyncCallFuture<SnapshotResult> future = new
AsyncCallFuture<SnapshotResult>();
RevertSnapshotContext<CommandResult> context = new
RevertSnapshotContext<CommandResult>(null, snapshot, future);
diff --git
a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImplTest.java
b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImplTest.java
new file mode 100644
index 0000000000..ec5c35501b
--- /dev/null
+++
b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImplTest.java
@@ -0,0 +1,98 @@
+/*
+ * 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 com.cloud.storage.DataStoreRole;
+import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
+import
org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
+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.SnapshotResult;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
+import org.apache.cloudstack.framework.async.AsyncCallFuture;
+import org.apache.cloudstack.framework.async.AsyncCallbackDispatcher;
+import org.apache.cloudstack.storage.command.CommandResult;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.springframework.test.context.ContextConfiguration;
+import org.springframework.test.context.support.AnnotationConfigContextLoader;
+
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({SnapshotServiceImpl.class})
+@ContextConfiguration(loader = AnnotationConfigContextLoader.class)
+public class SnapshotServiceImplTest {
+
+ @Spy
+ @InjectMocks
+ private SnapshotServiceImpl snapshotService = new SnapshotServiceImpl();
+
+ @Mock
+ VolumeDataFactory volFactory;
+
+ @Mock
+ SnapshotDataFactory _snapshotFactory;
+
+ @Mock
+ AsyncCallFuture<SnapshotResult> futureMock;
+
+ @Mock
+ AsyncCallbackDispatcher<SnapshotServiceImpl, CommandResult> caller;
+
+ @Before
+ public void testSetUp() throws Exception {
+ MockitoAnnotations.initMocks(this);
+ }
+
+ @Test
+ public void testRevertSnapshotWithNoPrimaryStorageEntry() throws Exception
{
+ SnapshotInfo snapshot = Mockito.mock(SnapshotInfo.class);
+ VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class);
+
+ Mockito.when(snapshot.getId()).thenReturn(1L);
+ Mockito.when(snapshot.getVolumeId()).thenReturn(1L);
+ Mockito.when(_snapshotFactory.getSnapshot(1L,
DataStoreRole.Primary)).thenReturn(null);
+ Mockito.when(volFactory.getVolume(1L,
DataStoreRole.Primary)).thenReturn(volumeInfo);
+
+ PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);
+ Mockito.when(volumeInfo.getDataStore()).thenReturn(store);
+
+ PrimaryDataStoreDriver driver =
Mockito.mock(PrimaryDataStoreDriver.class);
+ Mockito.when(store.getDriver()).thenReturn(driver);
+ Mockito.doNothing().when(driver).revertSnapshot(snapshot, null,
caller);
+
+ SnapshotResult result = Mockito.mock(SnapshotResult.class);
+
PowerMockito.whenNew(AsyncCallFuture.class).withNoArguments().thenReturn(futureMock);
+ Mockito.when(futureMock.get()).thenReturn(result);
+ Mockito.when(result.isFailed()).thenReturn(false);
+
+ Assert.assertEquals(true, snapshotService.revertSnapshot(snapshot));
+ }
+
+}
diff --git
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
index 92e737e528..4071c1bcb4 100644
---
a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
+++
b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java
@@ -178,11 +178,13 @@ public class LibvirtRevertSnapshotCommandWrapper extends
CommandWrapper<RevertSn
* and the snapshot path from the primary storage.
*/
protected Pair<String, SnapshotObjectTO> getSnapshot(SnapshotObjectTO
snapshotOnPrimaryStorage, SnapshotObjectTO snapshotOnSecondaryStorage,
- KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool
kvmStoragePoolSecondary){
- String snapshotPath = snapshotOnPrimaryStorage.getPath();
-
- if (Files.exists(Paths.get(snapshotPath))) {
- return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
+ KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool
kvmStoragePoolSecondary) {
+ String snapshotPath = null;
+ if (snapshotOnPrimaryStorage != null) {
+ snapshotPath = snapshotOnPrimaryStorage.getPath();
+ if (Files.exists(Paths.get(snapshotPath))) {
+ return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
+ }
}
if (kvmStoragePoolSecondary == null) {
@@ -190,8 +192,10 @@ public class LibvirtRevertSnapshotCommandWrapper extends
CommandWrapper<RevertSn
snapshotOnSecondaryStorage,
snapshotOnSecondaryStorage.getVolume()));
}
- s_logger.trace(String.format("Snapshot [%s] does not exists on primary
storage [%s], searching snapshot [%s] on secondary storage [%s].",
snapshotOnPrimaryStorage,
- kvmStoragePoolPrimary, snapshotOnSecondaryStorage,
kvmStoragePoolSecondary));
+ if (s_logger.isTraceEnabled()) {
+ s_logger.trace(String.format("Snapshot does not exists on primary
storage [%s], searching snapshot [%s] on secondary storage [%s].",
+ kvmStoragePoolPrimary, snapshotOnSecondaryStorage,
kvmStoragePoolSecondary));
+ }
String snapshotPathOnSecondaryStorage =
snapshotOnSecondaryStorage.getPath();
diff --git
a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java
b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java
index 0002dbe1c1..703f9a1e4e 100644
---
a/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java
+++
b/plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java
@@ -1536,6 +1536,7 @@ public class DateraPrimaryDataStoreDriver implements
PrimaryDataStoreDriver {
/**
* Revert snapshot for a volume
+ *
* @param snapshotInfo Information about volume snapshot
* @param snapshotOnPrimaryStore Not used
* @throws CloudRuntimeException
diff --git
a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
index 7c8b5fb22c..0f7b7b4fc3 100644
---
a/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
+++
b/plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
@@ -41,6 +41,7 @@ import
org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
import org.apache.cloudstack.engine.subsystem.api.storage.StorageAction;
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
+import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.framework.async.AsyncCompletionCallback;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -121,6 +122,8 @@ public class CloudStackPrimaryDataStoreDriverImpl
implements PrimaryDataStoreDri
TemplateManager templateManager;
@Inject
TemplateDataFactory templateDataFactory;
+ @Inject
+ VolumeDataFactory volFactory;
@Override
public DataTO getTO(DataObject data) {
@@ -379,11 +382,21 @@ public class CloudStackPrimaryDataStoreDriverImpl
implements PrimaryDataStoreDri
@Override
public void revertSnapshot(SnapshotInfo snapshot, SnapshotInfo
snapshotOnPrimaryStore, AsyncCompletionCallback<CommandResult> callback) {
- RevertSnapshotCommand cmd = new
RevertSnapshotCommand((SnapshotObjectTO)snapshot.getTO(),
(SnapshotObjectTO)snapshotOnPrimaryStore.getTO());
+ SnapshotObjectTO dataOnPrimaryStorage = null;
+ if (snapshotOnPrimaryStore != null) {
+ dataOnPrimaryStorage =
(SnapshotObjectTO)snapshotOnPrimaryStore.getTO();
+ }
+ RevertSnapshotCommand cmd = new
RevertSnapshotCommand((SnapshotObjectTO)snapshot.getTO(), dataOnPrimaryStorage);
CommandResult result = new CommandResult();
try {
- EndPoint ep = epSelector.select(snapshotOnPrimaryStore);
+ EndPoint ep = null;
+ if (snapshotOnPrimaryStore != null) {
+ ep = epSelector.select(snapshotOnPrimaryStore);
+ } else {
+ VolumeInfo volumeInfo =
volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
+ ep = epSelector.select(volumeInfo);
+ }
if ( ep == null ){
String errMsg = "No remote endpoint to send
RevertSnapshotCommand, check if host or ssvm is down?";
s_logger.error(errMsg);
