You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ro...@apache.org on 2022/07/20 06:04:08 UTC
[cloudstack] branch 4.17 updated: kvm: Fix for Revert volume snapshot (#6527)
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 <ha...@gmail.com>
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);