Fix of revert RBD snapshots by slavkap · Pull Request #5544 · apache/cloudstack

slavkap · 2021-10-01T09:42:19Z

Description

If a snapshot is taken only on Primary storage with the option "snapshot.backup.to.secondary" disabled, and after this when you enable the option the revert will fail. Added check if there isn't a snapshot on Secondary storage to check for it on Primary.

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)

Bug Severity

How Has This Been Tested?

4.15 and main with KVM hypervisors

set snapshot.backup.to.secondary to false
take one or more snapshots
set snapshot.backup.to.secondary to true
try to revert the snapshot. The operation will fail with "snapshot [snapshot ID] does not exists in data store"

DaanHoogland · 2021-10-01T14:44:03Z

@slavkap code looks good, but shouldn't this extra check only occurs if snapshot.backup.to.secondary is set to false?

slavkap · 2021-10-02T06:39:41Z

@DaanHoogland, thanks for the review! If you ask me, the right solution is first to check if the snapshot is on the primary storage and, if it's not there, to check on secondary. From what I saw in all of the storage plugins, the snapshots are kept on primary storage always. For me checking the value of snapshot.backup.to.secondary is just a limitation. I think that this method should be modified or removed:

 private DataStoreRole getDataStoreRole(Snapshot snapshot, SnapshotDataStoreDao snapshotStoreDao, DataStoreManager dataStoreMgr) {
    SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySnapshot(snapshot.getId(), DataStoreRole.Primary);

    if (snapshotStore == null) {
        return DataStoreRole.Image;
    }

    long storagePoolId = snapshotStore.getDataStoreId();
    DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);

    Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();

    if (mapCapabilities != null) {
        String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString());
        Boolean supportsStorageSystemSnapshots = new Boolean(value);

        if (supportsStorageSystemSnapshots) {
            return DataStoreRole.Primary;
        }
    }

    StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePoolId);
    if (storagePoolVO.getPoolType() == StoragePoolType.RBD && !BackupSnapshotAfterTakingSnapshot.value()) {
        return DataStoreRole.Primary;
    }

    return DataStoreRole.Image;
}

yadvr · 2021-10-02T09:40:56Z

@blueorangutan package

blueorangutan · 2021-10-02T09:42:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-10-02T10:18:14Z

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

yadvr · 2021-10-03T09:10:05Z

@blueorangutan test

blueorangutan · 2021-10-03T09:11:02Z

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

blueorangutan · 2021-10-03T18:12:39Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2021-10-04T08:25:17Z

@DaanHoogland, thanks for the review! If you ask me, the right solution is first to check if the snapshot is on the primary storage and, if it's not there, to check on secondary. From what I saw in all of the storage plugins, the snapshots are kept on primary storage always. For me checking the value of snapshot.backup.to.secondary is just a limitation. I think that this method should be modified or removed:

 private DataStoreRole getDataStoreRole(Snapshot snapshot, SnapshotDataStoreDao snapshotStoreDao, DataStoreManager dataStoreMgr) {
    SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findBySnapshot(snapshot.getId(), DataStoreRole.Primary);

    if (snapshotStore == null) {
        return DataStoreRole.Image;
    }

    long storagePoolId = snapshotStore.getDataStoreId();
    DataStore dataStore = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);

    Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();

    if (mapCapabilities != null) {
        String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString());
        Boolean supportsStorageSystemSnapshots = new Boolean(value);

        if (supportsStorageSystemSnapshots) {
            return DataStoreRole.Primary;
        }
    }

    StoragePoolVO storagePoolVO = _storagePoolDao.findById(storagePoolId);
    if (storagePoolVO.getPoolType() == StoragePoolType.RBD && !BackupSnapshotAfterTakingSnapshot.value()) {
        return DataStoreRole.Primary;
    }

    return DataStoreRole.Image;
}

@slavkap , are you doing / planning that? or creating a ticket for it?

yadvr · 2021-10-05T05:56:50Z

@sureshanaparti @nvazquez @DaanHoogland what's the concensus here, is this good to merge?

sureshanaparti · 2021-10-05T07:07:50Z

@sureshanaparti @nvazquez @DaanHoogland what's the concensus here, is this good to merge?

not yet @rhtyd

sureshanaparti · 2021-10-06T07:18:55Z

@blueorangutan package

blueorangutan · 2021-10-06T07:20:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

slavkap · 2021-10-06T07:20:51Z

@sureshanaparti, I have to make some changes in the unit tests, that's why I converted it as a draft

blueorangutan · 2021-10-06T07:30:44Z

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian. SL-JID 1518

slavkap · 2021-10-06T11:24:56Z

@blueorangutan package

blueorangutan · 2021-10-06T11:26:02Z

@slavkap a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

sureshanaparti · 2021-10-06T11:35:20Z

@slavkap can you re-target to 'main' pls.

blueorangutan · 2021-10-06T12:03:36Z

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

yadvr · 2021-11-15T10:11:16Z

Hi @slavkap should this be targetted for 4.17 or 4.16.1?

slavkap · 2021-11-15T10:40:58Z

Hi @rhtyd, I guess that it will be better to get in 4.16.1. Later today I'll change the target branch

sureshanaparti · 2022-02-08T11:26:36Z

@blueorangutan test centos7 vmware-67u3

blueorangutan · 2022-02-08T11:27:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + vmware-67u3) has been kicked to run smoke tests

DaanHoogland

cltgm

blueorangutan · 2022-02-08T21:47:35Z

Trillian test result (tid-3230)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36034 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5544-t3230-vmware-67u3.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2022-02-09T07:30:16Z

@blueorangutan test keepEnv

blueorangutan · 2022-02-09T07:32:03Z

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

yadvr

LGTM but needs manual testing.

sureshanaparti · 2022-02-09T10:45:29Z

Verified snapshot operations with PowerFlex and NFS. Minor suggestions on error messages. rest all LGTM.

If snapshot is taken only on Primary storage with the option "snapshot.backup.to.secondary" set to true, when you set this option to false the revert will fail. Added check if the snapshot is not on Secondary to check for it on Primary

Will check first if the snapshot is on Primary storage, if not will return Image as data store

sureshanaparti · 2022-02-09T13:50:36Z

@blueorangutan package

blueorangutan · 2022-02-09T13:51:02Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-09T14:45:11Z

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

sureshanaparti · 2022-02-09T14:52:28Z

@blueorangutan test centos7 xcpng82

blueorangutan · 2022-02-09T14:53:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + xcpng82) has been kicked to run smoke tests

blueorangutan · 2022-02-10T04:38:54Z

Trillian test result (tid-3267)
Environment: xcpng82 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48294 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5544-t3267-xcpng82.zip
Smoke tests completed. 91 look OK, 1 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_08_arping_in_ssvm	`Failure`	5.17	test_diagnostics.py

sureshanaparti · 2022-02-10T04:50:21Z

Trillian test result (tid-3267) Environment: xcpng82 (x2), Advanced Networking with Mgmt server 7 Total time taken: 48294 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5544-t3267-xcpng82.zip Smoke tests completed. 91 look OK, 1 have errors Only failed tests results shown below:

Test Result Time (s) Test File
test_08_arping_in_ssvm Failure 5.17 test_diagnostics.py

test failure ^^^ is unrelated to the PR changes.

* Fix of revert RBD snapshots If snapshot is taken only on Primary storage with the option "snapshot.backup.to.secondary" set to true, when you set this option to false the revert will fail. Added check if the snapshot is not on Secondary to check for it on Primary * Check if snapshot is on primary storage Will check first if the snapshot is on Primary storage, if not will return Image as data store * Fix unit tests * removed unused method's params * Formatted error message and added the snapshot ID to it * Return to the old logic, the fix will only apply to RBD * Formatted Exception's messages

yadvr added this to the 4.16.0.0 milestone Oct 4, 2021

sureshanaparti reviewed Oct 4, 2021

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java

nvazquez modified the milestones: 4.16.0.0, 4.16.1.0 Oct 5, 2021

slavkap marked this pull request as draft October 6, 2021 06:12

sureshanaparti added the needs-testing label Oct 6, 2021

slavkap marked this pull request as ready for review October 6, 2021 11:24

slavkap changed the base branch from 4.15 to main October 6, 2021 12:54

yadvr changed the base branch from main to 4.16 November 15, 2021 14:04

sureshanaparti requested a review from harikrishna-patnala February 8, 2022 08:16

DaanHoogland approved these changes Feb 8, 2022

View reviewed changes

yadvr approved these changes Feb 9, 2022

View reviewed changes

sureshanaparti reviewed Feb 9, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java Outdated

sureshanaparti reviewed Feb 9, 2022

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java Outdated

slavkap added 7 commits February 9, 2022 14:08

Fix of revert RBD snapshots

f72263d

If snapshot is taken only on Primary storage with the option "snapshot.backup.to.secondary" set to true, when you set this option to false the revert will fail. Added check if the snapshot is not on Secondary to check for it on Primary

Check if snapshot is on primary storage

d86f383

Will check first if the snapshot is on Primary storage, if not will return Image as data store

Fix unit tests

8a238cf

removed unused method's params

04cc48f

Formatted error message and added the snapshot ID to it

3c4ef89

Return to the old logic, the fix will only apply to RBD

709e042

Formatted Exception's messages

d4e0ecc

slavkap force-pushed the fix_revertsnapshot_not_on_secondary branch from 34568ca to d4e0ecc Compare February 9, 2022 12:17

sureshanaparti approved these changes Feb 10, 2022

View reviewed changes

sureshanaparti removed the needs-testing label Feb 10, 2022

sureshanaparti merged commit 4ffb949 into apache:4.16 Feb 10, 2022

Uh oh!

Conversation

slavkap commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Bug Severity

How Has This Been Tested?

Uh oh!

DaanHoogland commented Oct 1, 2021

Uh oh!

slavkap commented Oct 2, 2021

Uh oh!

yadvr commented Oct 2, 2021

Uh oh!

blueorangutan commented Oct 2, 2021

Uh oh!

blueorangutan commented Oct 2, 2021

Uh oh!

yadvr commented Oct 3, 2021

Uh oh!

blueorangutan commented Oct 3, 2021

Uh oh!

blueorangutan commented Oct 3, 2021

Uh oh!

DaanHoogland commented Oct 4, 2021

Uh oh!

Uh oh!

yadvr commented Oct 5, 2021

Uh oh!

sureshanaparti commented Oct 5, 2021

Uh oh!

sureshanaparti commented Oct 6, 2021

Uh oh!

blueorangutan commented Oct 6, 2021

Uh oh!

slavkap commented Oct 6, 2021

Uh oh!

blueorangutan commented Oct 6, 2021

Uh oh!

slavkap commented Oct 6, 2021

Uh oh!

blueorangutan commented Oct 6, 2021

Uh oh!

sureshanaparti commented Oct 6, 2021

Uh oh!

blueorangutan commented Oct 6, 2021

Uh oh!

yadvr commented Nov 15, 2021

Uh oh!

slavkap commented Nov 15, 2021

Uh oh!

sureshanaparti commented Feb 8, 2022

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Feb 8, 2022

Uh oh!

sureshanaparti commented Feb 9, 2022

Uh oh!

blueorangutan commented Feb 9, 2022

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sureshanaparti commented Feb 9, 2022

Uh oh!

sureshanaparti commented Feb 9, 2022

Uh oh!

blueorangutan commented Feb 9, 2022

Uh oh!

blueorangutan commented Feb 9, 2022

Uh oh!

sureshanaparti commented Feb 9, 2022

Uh oh!

slavkap commented Oct 1, 2021 •

edited

Loading