Add back ability to disable backup of snapshot to secondary by nathanejohnson · Pull Request #3122 · apache/cloudstack

nathanejohnson · 2019-01-07T15:38:05Z

Description

The snapshot.backup.rightafter configuration variable was removed by:

SHA: 6bb0ca2

This adds it back, though named snapshot.backup.to.secondary now instead.

This global parameter, once set, will allow you to prevent automatic backups of
snapshots to secondary storage, unless they're actually needed.

Fixes #3096

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)

How Has This Been Tested?

tested this in our 4.11 lab environment

wido · 2019-01-08T11:04:26Z

-                if (backupedSnapshot != null) {
-                    snapshotStrategy.postSnapshotCreation(snapshotOnPrimary);
+                if(s_logger.isDebugEnabled()) {
+                    s_logger.debug("skipping backup of snapshot to secondary due to configuration");


Is it clear from this log message which snapshot we are talking about?

wido · 2019-01-08T11:04:39Z

+                    s_logger.debug("skipping backup of snapshot to secondary due to configuration");
+                }
+                if (!snapshotOnPrimary.markBackedUp()) {
+                    throw new CloudRuntimeException("Can't mark snapshot as backed up!");


Same applies here. Maybe add the snapshot name, volume ID, etc?

rafaelweingartner · 2019-01-15T17:40:50Z

    }

+    @Override
+    public boolean markBackedUp() {


Why returning a boolean here?

I mean, you capture a checked exception here, and then you return false. Then, in the method where you use this markBackedUp, you will check if the return is false, and then you throw a runtime exception. Why not capture the checked exception and throw the runtime here?

Also, what about unit test cases?

Mainly I just couldn't throw a NoTransitionException because it would create a circular dependency. CloudRuntimeException would work though, since as you say that's ultimately what ends up happening in the one place this gets called (for now). I have no problem making this change.

I am +1 on the @rafaelweingartner idea. Throwing a CloudRuntimeException would be better than the boolean approach.

rafaelweingartner · 2019-01-15T17:42:48Z

            SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot);
-            if (payload.getAsyncBackup()) {
-                backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS);
+            boolean backupFlag = BackupSnapshotAfterTakingSnapshot.value() == null ||


Could you rename this variable to something more descriptive?

rafaelweingartner · 2019-01-15T17:43:23Z

+                    BackupSnapshotAfterTakingSnapshot.value();
+
+            if (backupFlag) {
+                if (payload.getAsyncBackup()) {


can you extract lines 1130-1136 to a method? This will also allow you to document it, and then to unit test it.

This isn't my code, but I'm not entirely sure how to even test things in backupSnapshotExecutor. That is inited by the config() method, and that's not called from the existing unit tests. Any thoughts?

nathanejohnson · 2019-01-16T19:07:08Z

@wido I hope I've addressed your concerns. @rafaelweingartner see above regarding the unit test suggestion

SHA: 6bb0ca2 This adds it back, though named snapshot.backup.to.secondary now instead. This global parameter, once set, will allow you to prevent automatic backups of snapshots to secondary storage, unless they're actually needed. Fixes apache#3096

wido

Code LGTM

rafaelweingartner · 2019-01-16T23:03:04Z

+            processEvent(Event.OperationNotPerformed);
+        } catch (NoTransitionException ex) {
+            s_logger.error("no transition error: ", ex);
+            throw new CloudRuntimeException("Error marking snapshot backed up: " +


Can you maintain the history of the stack of the exception that is re-thrown here?
I mean, it is a matter of using throw new CloudRuntimeException(<message>, <exception>)

@nathanejohnson I see this point as critical. Doing as @rafaelweingartner proposed would allow having the full stack on the log, which helps on debugging.
Could you please address this point? Thanks! Overall LGTM.

Sign in to view

            SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot);
-            if (payload.getAsyncBackup()) {
-                backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS);
+            boolean backupSnapToSecondary = BackupSnapshotAfterTakingSnapshot.value() == null ||


rafaelweingartner · 2019-01-16T23:04:14Z

        return snapshot;
    }

+    protected void backupSnapshotToSecondary(boolean asyncBackup, SnapshotStrategy snapshotStrategy, SnapshotInfo snapshotOnPrimary) {


Would you mind documenting this method, and then creating unit test cases for it?

GabrielBrascher · 2019-01-18T16:08:49Z

@blueorangutan package

blueorangutan · 2019-01-18T16:09:21Z

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

blueorangutan · 2019-01-18T16:29:06Z

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

GabrielBrascher · 2019-01-18T16:31:47Z

@blueorangutan test

blueorangutan · 2019-01-18T16:32:21Z

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

GabrielBrascher · 2019-01-18T18:32:31Z

This PR is almost good to go, thanks @nathanejohnson!
I see that we have some minor changes requested by @rafaelweingartner. Can you please address them @nathanejohnson?

blueorangutan · 2019-01-18T22:22:17Z

Trillian test result (tid-3337)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 19575 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3122-t3337-kvm-centos7.zip
Smoke tests completed. 68 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Correção no estado da VM ao criar um _backup_ com o host `Down` ou `Disconnected` Closes #3122 See merge request scclouds/scclouds!1314

nathanejohnson changed the title ~~The snapshot.backup.rightafter configuration variable was removed by:~~ Add back ability to disable backup of snapshot to secondary Jan 7, 2019

wido requested changes Jan 8, 2019

View reviewed changes

wido added this to the 4.12.0.0 milestone Jan 8, 2019

wido added component:management-server component:primary-storage labels Jan 8, 2019

rafaelweingartner modified the milestones: 4.12.0.0, 4.11.3.0 Jan 9, 2019

rafaelweingartner requested changes Jan 15, 2019

View reviewed changes

nathanejohnson force-pushed the bug/disablesnapshotbackup branch from 816a356 to ca27bbc Compare January 16, 2019 19:01

nathanejohnson added 2 commits January 16, 2019 13:30

updates per review

4372205

nathanejohnson force-pushed the bug/disablesnapshotbackup branch from ca27bbc to 4372205 Compare January 16, 2019 19:31

wido approved these changes Jan 16, 2019

View reviewed changes

rafaelweingartner reviewed Jan 16, 2019

View reviewed changes

GabrielBrascher approved these changes Feb 4, 2019

View reviewed changes

GabrielBrascher merged commit bf805d1 into apache:4.11 Feb 4, 2019

GabrielBrascher pushed a commit that referenced this pull request Feb 4, 2019

Fix conflict and merge forward PR #3122 from 4.11 to master (4.12)

460d312

winterhazel pushed a commit that referenced this pull request Jan 28, 2026

Merge branch 'Issues3122' into '4.20.0.0-scclouds'

6feaadf

Correção no estado da VM ao criar um _backup_ com o host `Down` ou `Disconnected` Closes #3122 See merge request scclouds/scclouds!1314

Conversation

nathanejohnson commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

How Has This Been Tested?

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!

Choose a reason for hiding this comment

Uh oh!

nathanejohnson commented Jan 16, 2019

Uh oh!

wido left a comment

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!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jan 18, 2019

Uh oh!

blueorangutan commented Jan 18, 2019

Uh oh!

blueorangutan commented Jan 18, 2019

Uh oh!

GabrielBrascher commented Jan 18, 2019

Uh oh!

blueorangutan commented Jan 18, 2019

Uh oh!

GabrielBrascher commented Jan 18, 2019

Uh oh!

blueorangutan commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nathanejohnson commented Jan 7, 2019 •

edited

Loading