[KVM] Fix for Revert volume snapshot by harikrishna-patnala · Pull Request #6527 · apache/cloudstack

harikrishna-patnala · 2022-07-04T10:42:42Z

Description

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.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Create a VM and stop the VM on a KVM environment
Create snapshot of the volume
migrate VM with volume or volume to a different storage.
Create another snapshot of the same volume
Performed revert operations on these two snapshots and they are successful

acs-robot · 2022-07-04T10:43:46Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-07-04T10:44:03Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

harikrishna-patnala · 2022-07-04T10:45:02Z

@blueorangutan package

acs-robot · 2022-07-04T10:45:27Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-07-04T10:46:02Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

harikrishna-patnala · 2022-07-05T01:48:32Z

@blueorangutan package

blueorangutan · 2022-07-05T01:49:02Z

@harikrishna-patnala a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-07-05T02:26:04Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3706

harikrishna-patnala · 2022-07-05T03:27:15Z

@blueorangutan test

blueorangutan · 2022-07-05T03:28:03Z

@harikrishna-patnala a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

…t is not there

blueorangutan · 2022-07-05T05:19:54Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6527 (SL-JID-1903)

DaanHoogland

clgtm

acs-robot · 2022-07-05T09:45:02Z

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan · 2022-07-05T09:46:02Z

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2022-07-05T09:51:40Z

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6527 (SL-JID-1905)

harikrishna-patnala · 2022-07-05T10:01:08Z

@blueorangutan package

blueorangutan · 2022-07-05T10:02:02Z

@harikrishna-patnala a Jenkins job has been kicked to build packages. It will be bundled with

SystemVM template(s). I'll keep you posted as I make progress.

sonarqubecloud · 2022-07-05T10:34:50Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

blueorangutan · 2022-07-05T10:43:22Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3710

harikrishna-patnala · 2022-07-05T10:57:32Z

@blueorangutan test

blueorangutan · 2022-07-05T10:58:02Z

@harikrishna-patnala a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-07-05T14:09:36Z

Trillian test result (tid-4427)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36996 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6527-t4427-kvm-centos7.zip
Smoke tests completed. 98 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2022-07-05T21:36:12Z

Trillian test result (tid-4429)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36972 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6527-t4429-kvm-centos7.zip
Smoke tests completed. 98 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

yadvr · 2022-07-06T03:53:35Z

-            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();


@harikrishna-patnala should we check for subsequent cases where it's not found again or this is null?

We are not looking for snapshotOnPrimaryStore again. We are just trying to get the appropriate datastore driver to run the command. We are using volume's info here if snapshot on primary os not found. So need to check again.

yadvr · 2022-07-06T03:54:22Z

-        if (Files.exists(Paths.get(snapshotPath))) {
-            return new Pair<>(snapshotPath, snapshotOnPrimaryStorage);
+            KVMStoragePool kvmStoragePoolPrimary, KVMStoragePool kvmStoragePoolSecondary) {
+        String snapshotPath = null;


If snapshotOnPrimaryStorage is null, then this is never set?

No need to set or use it. If the snapshot on primary is not there then we will get it from secondary storage.

yadvr · 2022-07-06T03:55:04Z

    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) {


same as above if snapshotOnPrimaryStore is null then dataOnPrimaryStorage is never set? (or is it that this will never be null?)

It can be null. We are handling the same case where snapshotOnPrimary is null.
Following is the complete flow

Look for snapshotOnPrimary entry if exists

If exists, use that entry to find the appropriate driver to execute revertSnapshotCommand

If not found, use volume information to find the appropriate driver to execute revertSnapshotCommand

While executing the revertSnapshotCommand, if snapshotOnPrimary exists, use it to revert the volume

If snapshotOnPrimary does not exists, use the snapshot from secondary storage to revert the volume

yadvr

LGTM but left some comments. @harikrishna-patnala can you advise how this was tested, is there an existing smoketest or was it manually tested?

harikrishna-patnala · 2022-07-06T06:29:37Z

LGTM but left some comments. @harikrishna-patnala can you advise how this was tested, is there an existing smoketest or was it manually tested?

For revert volume to snapshot I did not see any smoke tests, I've tested it manually and added the steps that I've tested in the description.

Create a VM and stop the VM on a KVM environment
Create snapshot of the volume
migrate VM with volume or volume to a different storage.
Create another snapshot of the same volume
Performed revert operations on these two snapshots and they are successful

To write tests to address the revert operation and the issue here, we have to write the exact same flow as above.

harikrishna-patnala · 2022-07-06T10:29:23Z

@nvazquez @Pearl1594 will you be able to help me test this PR please.

Pearl1594

Code LGTM

Pearl1594

Tested fix. LGTM - Was able to revert to snapshot taken before migration of volume.

boring-cyborg Bot added component:kvm component:primary-storage component:storage component:UI component:vmware labels Jul 4, 2022

harikrishna-patnala removed component:vmware component:UI labels Jul 4, 2022

harikrishna-patnala added this to the 4.17.1.0 milestone Jul 4, 2022

boring-cyborg Bot added component:UI component:vmware labels Jul 4, 2022

[KVM] Fix revert volume snapshot after volume migration

ae7693f

harikrishna-patnala force-pushed the KVMrevertVolumeSnapshotFix branch from c8b6fd2 to ae7693f Compare July 4, 2022 10:44

harikrishna-patnala removed component:vmware component:UI labels Jul 4, 2022

apache deleted a comment from blueorangutan Jul 4, 2022

harikrishna-patnala added the status:needs-testing label Jul 4, 2022

harikrishna-patnala requested review from Pearl1594 and nvazquez July 5, 2022 03:39

Added unit test case to handle when primary storage entry for snapsho…

a8e5829

…t is not there

DaanHoogland approved these changes Jul 5, 2022

View reviewed changes

Comment thread ...main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertSnapshotCommandWrapper.java Outdated

Fixed a trace message

cfcfeb6

yadvr reviewed Jul 6, 2022

View reviewed changes

yadvr approved these changes Jul 6, 2022

View reviewed changes

yadvr assigned Pearl1594, vladimirpetrov and borisstoyanov Jul 8, 2022

Pearl1594 approved these changes Jul 19, 2022

View reviewed changes

Pearl1594 approved these changes Jul 20, 2022

View reviewed changes

yadvr changed the base branch from main to 4.17 July 20, 2022 06:03

yadvr merged commit 2c05b63 into apache:4.17 Jul 20, 2022

yadvr deleted the KVMrevertVolumeSnapshotFix branch July 20, 2022 06:04

yadvr mentioned this pull request Jul 20, 2022

Revert volume to snapshot failures #6209

Closed

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Uh oh!

Conversation

harikrishna-patnala commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

acs-robot commented Jul 4, 2022

Uh oh!

blueorangutan commented Jul 4, 2022

Uh oh!

harikrishna-patnala commented Jul 4, 2022

Uh oh!

acs-robot commented Jul 4, 2022

Uh oh!

blueorangutan commented Jul 4, 2022

Uh oh!

harikrishna-patnala commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

harikrishna-patnala commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acs-robot commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

harikrishna-patnala commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

sonarqubecloud Bot commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

harikrishna-patnala commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

blueorangutan commented Jul 5, 2022

Uh oh!

yadvr Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

yadvr Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

yadvr Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

harikrishna-patnala commented Jul 4, 2022 •

edited

Loading

harikrishna-patnala Jul 6, 2022 •

edited

Loading