Add ability to archive snapshots on primary storage by syed · Pull Request #2585 · apache/cloudstack

syed · 2018-04-19T15:27:44Z

Description

This PR adds an API call named archiveSnapshot which takes in a snapshot residing on primary storage and moves it to secondary storage. The main use case for this relates to the use in Managed storage where the storage supports native snapshots. Because of cost and availability reasons, it is useful to move snapshots that are taken on primary storage (native snapshots) to secondary storage.

The API takes in a snapshotid , creates a new snapshot on secondary storage and deletes the snapshot on primary storage.

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Tested on a local storage cluster (SolidFire) and also added unit tests.

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

@blueorangutan package

blueorangutan · 2018-04-19T15:29:07Z

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

syed · 2018-04-19T15:29:27Z

cc @mike-tutkowski

blueorangutan · 2018-04-19T15:47:23Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1953

rafaelweingartner · 2018-04-19T17:15:13Z

@syed it seems that you have some compilation errors, can you take a look at them?

khos2ow · 2018-04-19T20:13:22Z

        cmdList.add(CreateSnapshotCmd.class);
        cmdList.add(CreateSnapshotFromVMSnapshotCmd.class);
        cmdList.add(DeleteSnapshotCmd.class);
+        cmdList.add(ArchiveSnapshotCmd.class);


@syed I believe you forgot to commit ArchiveSnapshotCmd itself!

@khos2ow You're right. I'll add it now

mike-tutkowski · 2018-04-24T21:54:44Z

    Snapshot createSnapshot(Long volumeId, Long policyId, Long snapshotId, Account snapshotOwner);

+    /**
+     * Archives a snpashot from primary storage to secondary storage.


Minor: Spelling issue on what should be the word "snapshot".

mike-tutkowski · 2018-04-24T21:58:06Z

-                snapObj.processEvent(Snapshot.Event.OperationFailed);
+                // When error archiving an already existing snapshot, emit OperationNotPerformed.
+                // This will ensure that the original snapshot does not get deleted
+                if (origState.equals(Snapshot.State.BackedUp)) {


Just curious @syed: Have you at least manually run a test case where the snapshot process failed and this logic was executed to see if, in fact, the original snapshot remained usable?

@mike-tutkowski Yes. That is how I added this check. In the normal scenario, the original snapshot was getting deleted.

nitin-maharana · 2018-04-25T04:43:53Z

+        }
+
+        SnapshotInfo snapshotOnSecondary = snapshotSrv.backupSnapshot(snapshotOnPrimary);
+        snapshotSrv.deleteSnapshot(snapshotOnPrimary);


@syed, I think it would be better to check the status of deleting the snapshot on the primary storage. Due to any reason, if it fails to delete then the user will be informed with a proper error message. Thanks.

@nitin-maharana That's a good point. I'll catch that here

khos2ow

LGTM

mike-tutkowski · 2018-08-21T20:37:24Z

Closing and re-opening this PR to kick off CI again.

blueorangutan · 2018-08-21T20:38:00Z

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

blueorangutan · 2018-08-21T21:13:01Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2264

DaanHoogland · 2018-08-22T10:54:21Z

@blueorangutan test

blueorangutan · 2018-08-22T10:55:00Z

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

mike-tutkowski · 2018-08-23T20:47:34Z

I have run all of the XenServer-oriented managed storage-related integration tests on this PR. With the exception of two tests (more on that later), all have passed.

These are the two tests that failed:

I plan to investigate this one soon:

test_online_migrate_volume_with_snapshot_from_nfs_storage_to_managed_storage (TestOnlineStorageMigration.TestOnlineStorageMigration) ... === TestName: test_online_migrate_volume_with_snapshot_from_nfs_storage_to_managed_storage | Status : EXCEPTION ===
ERROR
=== TestName: test_online_migrate_volume_with_snapshot_from_nfs_storage_to_managed_storage | Status : EXCEPTION ===
ERROR

This one failed because PR #2761 is not yet merged into master. I plan (just in my sandbox) to merge #2761 into this PR and re-run this test. That way I should be able to see this test pass.

test_02_storage_migrate_root_and_data_disks (TestVMMigrationWithStorage.TestVMMigrationWithStorage) ... === TestName: test_02_storage_migrate_root_and_data_disks | Status : EXCEPTION ===
ERROR

Here are links to all of the tests I ran for this PR:

TestOnlineStorageMigration (second test is one of the failed tests listed above):
results.txt

TestVMMigrationWithStorage (second test is one of the failed tests listed above):
results.txt

TestAddRemoveHosts (all tests passed):
results.txt
results.txt

TestCapacityManagement (both tests passed):
results.txt

TestManagedSystemVMs (both tests passed):
results.txt

TestSnapshots (all tests passed; during the first run, tests 2 - 4 failed due to an environment issue...I corrected the issue and re-ran those tests; the second results file shows them passing):
results.txt
results.txt

TestVMSnapshots (both tests passed):
results.txt

TestVolumes (no resigning; all tests passed):
results.txt

TestVolumes (resigning; all tests passed):
results.txt

mike-tutkowski · 2018-08-23T21:56:16Z

I re-ran test_online_migrate_volume_with_snapshot_from_nfs_storage_to_managed_storage (TestOnlineStorageMigration.TestOnlineStorageMigration), which was one of two tests that failed, and it passed:

results.txt

I also went ahead and ran that test scenario manually and it passed.

blueorangutan · 2018-08-24T00:37:57Z

Trillian test result (tid-2957)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48966 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2585-t2957-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_accounts.py
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 64 look OK, 5 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestTemplateHierarchy>:setup	`Error`	3711.78	test_accounts.py
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80	`Failure`	260.12	test_internal_lb.py
test_02_vpc_privategw_static_routes	`Failure`	208.33	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1276.60	test_privategw_acl.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.52	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	4.27	test_hostha_kvm.py
test_hostha_kvm_host_fencing	`Error`	8.44	test_hostha_kvm.py

mike-tutkowski · 2018-08-24T01:31:45Z

I merged #2761 into this PR in my sandbox and re-ran test_02_storage_migrate_root_and_data_disks (TestVMMigrationWithStorage.TestVMMigrationWithStorage). The test passed.

results.txt

mike-tutkowski · 2018-08-24T01:35:49Z

The tests that failed for this PR do not seem to be likely because of this PR.

That being the case and with all XenServer-oriented managed storage-related integration tests having passed (per above comments), this PR LGTM.

We have two LGTMs, CI passed, and integration tests look reasonable (per previous comments). Merging this PR.

blueorangutan · 2018-08-24T01:37:01Z

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

blueorangutan · 2018-08-24T07:56:27Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2267

yadvr · 2018-08-27T08:32:29Z

@mike-tutkowski this PR was merged without a milestone and please ensure that PRs getting merged have a milestone and use the squash+merge option while merging the PR otherwise it creates a complex git history/graph with merge commits that serves no purpose esp for PRs with a single commit. Thanks.

mike-tutkowski · 2018-09-06T06:02:44Z

I just saw your comment here, @rhtyd - sounds good.

khos2ow suggested changes Apr 19, 2018

View reviewed changes

mike-tutkowski reviewed Apr 24, 2018

View reviewed changes

nitin-maharana reviewed Apr 25, 2018

View reviewed changes

syed force-pushed the upstream-snapshot-archive branch from a2dd376 to 8e80910 Compare May 17, 2018 06:40

Add ability to archive snapshots on primary storage

cd70ede

syed force-pushed the upstream-snapshot-archive branch from 8e80910 to cd70ede Compare May 17, 2018 08:41

khos2ow approved these changes May 17, 2018

View reviewed changes

mike-tutkowski closed this Aug 21, 2018

mike-tutkowski reopened this Aug 21, 2018

mike-tutkowski merged commit 568119d into apache:master Aug 24, 2018

yadvr added this to the 4.12.0.0 milestone Aug 27, 2018

Uh oh!

Conversation

syed commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

blueorangutan commented Apr 19, 2018

Uh oh!

syed commented Apr 19, 2018

Uh oh!

blueorangutan commented Apr 19, 2018

Uh oh!

rafaelweingartner commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khos2ow left a comment

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Aug 21, 2018

Uh oh!

blueorangutan commented Aug 21, 2018

Uh oh!

DaanHoogland commented Aug 22, 2018

Uh oh!

blueorangutan commented Aug 22, 2018

Uh oh!

mike-tutkowski commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mike-tutkowski commented Aug 23, 2018

Uh oh!

blueorangutan commented Aug 24, 2018

Uh oh!

mike-tutkowski commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mike-tutkowski commented Aug 24, 2018

Uh oh!

blueorangutan commented Aug 24, 2018

Uh oh!

blueorangutan commented Aug 24, 2018

Uh oh!

yadvr commented Aug 27, 2018

Uh oh!

mike-tutkowski commented Sep 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

syed commented Apr 19, 2018 •

edited

Loading

mike-tutkowski commented Aug 21, 2018 •

edited

Loading

mike-tutkowski commented Aug 23, 2018 •

edited

Loading

mike-tutkowski commented Aug 24, 2018 •

edited

Loading