Skip to content

Commit 2c05b63

Browse files
harikrishna-patnalaharikrishna-patnala
authored
kvm: Fix for Revert volume snapshot (apache#6527)
This PR fixes the issue apache#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.
1 parent d530573 commit 2c05b63

File tree

5 files changed

+134
-11
lines changed

5 files changed

+134
-11
lines changed

‎engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
3737
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
3838
import org.apache.cloudstack.engine.subsystem.api.storage.StorageCacheManager;
39+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
3940
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
4041
import org.apache.cloudstack.framework.async.AsyncCallFuture;
4142
import org.apache.cloudstack.framework.async.AsyncCallbackDispatcher;
@@ -79,6 +80,8 @@ public class SnapshotServiceImpl implements SnapshotService {
7980
StorageCacheManager _cacheMgr;
8081
@Inject
8182
private SnapshotDetailsDao _snapshotDetailsDao;
83+
@Inject
84+
VolumeDataFactory volFactory;
8285

8386
static private class CreateSnapshotContext<T> extends AsyncRpcContext<T> {
8487
final SnapshotInfo snapshot;
@@ -428,11 +431,15 @@ public boolean deleteSnapshot(SnapshotInfo snapInfo) {
428431

429432
@Override
430433
public boolean revertSnapshot(SnapshotInfo snapshot) {
434+
PrimaryDataStore store = null;
431435
SnapshotInfo snapshotOnPrimaryStore = _snapshotFactory.getSnapshot(snapshot.getId(), DataStoreRole.Primary);
432436
if (snapshotOnPrimaryStore == null) {
433-
throw new CloudRuntimeException("Cannot find an entry for snapshot " + snapshot.getId() + " on primary storage pools");
437+
s_logger.warn("Cannot find an entry for snapshot " + snapshot.getId() + " on primary storage pools, searching with volume's primary storage pool");
438+
VolumeInfo volumeInfo = volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
439+
store = (PrimaryDataStore)volumeInfo.getDataStore();
440+
} else {
441+
store = (PrimaryDataStore)snapshotOnPrimaryStore.getDataStore();
434442
}
435-
PrimaryDataStore store = (PrimaryDataStore)snapshotOnPrimaryStore.getDataStore();
436443

437444
AsyncCallFuture<SnapshotResult> future = new AsyncCallFuture<SnapshotResult>();
438445
RevertSnapshotContext<CommandResult> context = new RevertSnapshotContext<CommandResult>(null, snapshot, future);

‎engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImplTest.java

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.cloudstack.storage.snapshot;
20+
21+
import com.cloud.storage.DataStoreRole;
22+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
23+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreDriver;
24+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
25+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
26+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
27+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
28+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
29+
import org.apache.cloudstack.framework.async.AsyncCallFuture;
30+
import org.apache.cloudstack.framework.async.AsyncCallbackDispatcher;
31+
import org.apache.cloudstack.storage.command.CommandResult;
32+
import org.junit.Assert;
33+
import org.junit.Before;
34+
import org.junit.Test;
35+
import org.junit.runner.RunWith;
36+
import org.mockito.InjectMocks;
37+
import org.mockito.Mock;
38+
import org.mockito.Mockito;
39+
import org.mockito.MockitoAnnotations;
40+
import org.mockito.Spy;
41+
import org.powermock.api.mockito.PowerMockito;
42+
import org.powermock.core.classloader.annotations.PrepareForTest;
43+
import org.powermock.modules.junit4.PowerMockRunner;
44+
import org.springframework.test.context.ContextConfiguration;
45+
import org.springframework.test.context.support.AnnotationConfigContextLoader;
46+
47+
@RunWith(PowerMockRunner.class)
48+
@PrepareForTest({SnapshotServiceImpl.class})
49+
@ContextConfiguration(loader = AnnotationConfigContextLoader.class)
50+
public class SnapshotServiceImplTest {
51+
52+
@Spy
53+
@InjectMocks
54+
private SnapshotServiceImpl snapshotService = new SnapshotServiceImpl();
55+
56+
@Mock
57+
VolumeDataFactory volFactory;
58+
59+
@Mock
60+
SnapshotDataFactory _snapshotFactory;
61+
62+
@Mock
63+
AsyncCallFuture<SnapshotResult> futureMock;
64+
65+
@Mock
66+
AsyncCallbackDispatcher<SnapshotServiceImpl, CommandResult> caller;
67+
68+
@Before
69+
public void testSetUp() throws Exception {
70+
MockitoAnnotations.initMocks(this);
71+
}
72+
73+
@Test
74+
public void testRevertSnapshotWithNoPrimaryStorageEntry() throws Exception {
75+
SnapshotInfo snapshot = Mockito.mock(SnapshotInfo.class);
76+
VolumeInfo volumeInfo = Mockito.mock(VolumeInfo.class);
77+
78+
Mockito.when(snapshot.getId()).thenReturn(1L);
79+
Mockito.when(snapshot.getVolumeId()).thenReturn(1L);
80+
Mockito.when(_snapshotFactory.getSnapshot(1L, DataStoreRole.Primary)).thenReturn(null);
81+
Mockito.when(volFactory.getVolume(1L, DataStoreRole.Primary)).thenReturn(volumeInfo);
82+
83+
PrimaryDataStore store = Mockito.mock(PrimaryDataStore.class);
84+
Mockito.when(volumeInfo.getDataStore()).thenReturn(store);
85+
86+
PrimaryDataStoreDriver driver = Mockito.mock(PrimaryDataStoreDriver.class);
87+
Mockito.when(store.getDriver()).thenReturn(driver);
88+
Mockito.doNothing().when(driver).revertSnapshot(snapshot, null, caller);
89+
90+
SnapshotResult result = Mockito.mock(SnapshotResult.class);
91+
PowerMockito.whenNew(AsyncCallFuture.class).withNoArguments().thenReturn(futureMock);
92+
Mockito.when(futureMock.get()).thenReturn(result);
93+
Mockito.when(result.isFailed()).thenReturn(false);
94+
95+
Assert.assertEquals(true, snapshotService.revertSnapshot(snapshot));
96+
}
97+
98+
}

‎plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -178,20 +178,24 @@ protected void revertVolumeToSnapshot(SnapshotObjectTO snapshotOnPrimaryStorage,
178178
* and the snapshot path from the primary storage.
179179
*/
180180
protected Pair<String, SnapshotObjectTO> getSnapshot(SnapshotObjectTO snapshotOnPrimaryStorage, SnapshotObjectTO snapshotOnSecondaryStorage,
181-
KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool kvmStoragePoolSecondary){
182-
String snapshotPath = snapshotOnPrimaryStorage.getPath();
183-
184-
if (Files.exists(Paths.get(snapshotPath))) {
185-
return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
181+
KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool kvmStoragePoolSecondary) {
182+
String snapshotPath = null;
183+
if (snapshotOnPrimaryStorage != null) {
184+
snapshotPath = snapshotOnPrimaryStorage.getPath();
185+
if (Files.exists(Paths.get(snapshotPath))) {
186+
return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
187+
}
186188
}
187189

188190
if (kvmStoragePoolSecondary == null) {
189191
throw new CloudRuntimeException(String.format("Snapshot [%s] does not exists on secondary storage, unable to revert volume [%s] to it.",
190192
snapshotOnSecondaryStorage, snapshotOnSecondaryStorage.getVolume()));
191193
}
192194

193-
s_logger.trace(String.format("Snapshot [%s] does not exists on primary storage [%s], searching snapshot [%s] on secondary storage [%s].", snapshotOnPrimaryStorage,
194-
kvmStoragePoolPrimary, snapshotOnSecondaryStorage, kvmStoragePoolSecondary));
195+
if (s_logger.isTraceEnabled()) {
196+
s_logger.trace(String.format("Snapshot does not exists on primary storage [%s], searching snapshot [%s] on secondary storage [%s].",
197+
kvmStoragePoolPrimary, snapshotOnSecondaryStorage, kvmStoragePoolSecondary));
198+
}
195199

196200
String snapshotPathOnSecondaryStorage = snapshotOnSecondaryStorage.getPath();
197201

‎plugins/storage/volume/datera/src/main/java/org/apache/cloudstack/storage/datastore/driver/DateraPrimaryDataStoreDriver.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1536,6 +1536,7 @@ private void deleteTemplate(TemplateInfo templateInfo, long storagePoolId)
15361536

15371537
/**
15381538
* Revert snapshot for a volume
1539+
*
15391540
* @param snapshotInfo Information about volume snapshot
15401541
* @param snapshotOnPrimaryStore Not used
15411542
* @throws CloudRuntimeException

‎plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.apache.cloudstack.engine.subsystem.api.storage.StorageAction;
4242
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
4343
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
44+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
4445
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
4546
import org.apache.cloudstack.framework.async.AsyncCompletionCallback;
4647
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
@@ -121,6 +122,8 @@ public Map<String, String> getCapabilities() {
121122
TemplateManager templateManager;
122123
@Inject
123124
TemplateDataFactory templateDataFactory;
125+
@Inject
126+
VolumeDataFactory volFactory;
124127

125128
@Override
126129
public DataTO getTO(DataObject data) {
@@ -379,11 +382,21 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCm
379382

380383
@Override
381384
public void revertSnapshot(SnapshotInfo snapshot, SnapshotInfo snapshotOnPrimaryStore, AsyncCompletionCallback<CommandResult> callback) {
382-
RevertSnapshotCommand cmd = new RevertSnapshotCommand((SnapshotObjectTO)snapshot.getTO(), (SnapshotObjectTO)snapshotOnPrimaryStore.getTO());
385+
SnapshotObjectTO dataOnPrimaryStorage = null;
386+
if (snapshotOnPrimaryStore != null) {
387+
dataOnPrimaryStorage = (SnapshotObjectTO)snapshotOnPrimaryStore.getTO();
388+
}
389+
RevertSnapshotCommand cmd = new RevertSnapshotCommand((SnapshotObjectTO)snapshot.getTO(), dataOnPrimaryStorage);
383390

384391
CommandResult result = new CommandResult();
385392
try {
386-
EndPoint ep = epSelector.select(snapshotOnPrimaryStore);
393+
EndPoint ep = null;
394+
if (snapshotOnPrimaryStore != null) {
395+
ep = epSelector.select(snapshotOnPrimaryStore);
396+
} else {
397+
VolumeInfo volumeInfo = volFactory.getVolume(snapshot.getVolumeId(), DataStoreRole.Primary);
398+
ep = epSelector.select(volumeInfo);
399+
}
387400
if ( ep == null ){
388401
String errMsg = "No remote endpoint to send RevertSnapshotCommand, check if host or ssvm is down?";
389402
s_logger.error(errMsg);

0 commit comments

Comments
 (0)