From 2c05b6349548d8795326225190c7d782d6a767be Mon Sep 17 00:00:00 2001 From: Harikrishna Date: Wed, 20 Jul 2022 11:34:02 +0530 Subject: [PATCH] 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 ++- .../snapshot/SnapshotServiceImplTest.java | 98 +++++++++++++++++++ .../LibvirtRevertSnapshotCommandWrapper.java | 18 ++-- .../driver/DateraPrimaryDataStoreDriver.java | 1 + .../CloudStackPrimaryDataStoreDriverImpl.java | 17 +++- 5 files changed, 134 insertions(+), 11 deletions(-) create mode 100644 engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/snapshot/SnapshotServiceImplTest.java 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 b8788fbef98..4d106f5c4f6 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 extends AsyncRpcContext { 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 future = new AsyncCallFuture(); RevertSnapshotContext context = new RevertSnapshotContext(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 00000000000..ec5c35501bb --- /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 futureMock; + + @Mock + AsyncCallbackDispatcher 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 92e737e528e..4071c1bcb45 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 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 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);