From 509410bba83376eee9edb4bf536d6311d69ace4e Mon Sep 17 00:00:00 2001 From: Nathan Johnson Date: Wed, 19 Dec 2018 14:17:47 -0600 Subject: [PATCH 1/2] The snapshot.backup.rightafter configuration variable was removed by: SHA: 6bb0ca2f854 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 --- .../subsystem/api/storage/SnapshotInfo.java | 2 ++ .../storage/snapshot/SnapshotObject.java | 11 +++++++++ .../storage/snapshot/SnapshotManager.java | 3 +++ .../storage/snapshot/SnapshotManagerImpl.java | 24 ++++++++++++++----- 4 files changed, 34 insertions(+), 6 deletions(-) diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java index 5541b362547a..cc27b20a9422 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java @@ -42,4 +42,6 @@ public interface SnapshotInfo extends DataObject, Snapshot { boolean isRevertable(); long getPhysicalSize(); + + boolean markBackedUp(); } diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java index 23e1650ffa0c..4ea1dd148d9e 100644 --- a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java +++ b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java @@ -149,6 +149,17 @@ public long getPhysicalSize() { return physicalSize; } + @Override + public boolean markBackedUp() { + try { + processEvent(Event.OperationNotPerformed); + } catch (NoTransitionException ex) { + s_logger.error("no transition error: ", ex); + return false; + } + return true; + } + @Override public VolumeInfo getBaseVolume() { return volFactory.getVolume(snapshot.getVolumeId()); diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManager.java b/server/src/com/cloud/storage/snapshot/SnapshotManager.java index f3557d0a670b..407ffa302135 100644 --- a/server/src/com/cloud/storage/snapshot/SnapshotManager.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManager.java @@ -56,6 +56,9 @@ public interface SnapshotManager extends Configurable { public static final ConfigKey BackupRetryInterval = new ConfigKey(Integer.class, "backup.retry.interval", "Advanced", "300", "Time in seconds between retries in backing up snapshot to secondary", false, ConfigKey.Scope.Global, null); + public static final ConfigKey BackupSnapshotAfterTakingSnapshot = new ConfigKey(Boolean.class, "snapshot.backup.to.secondary", "Snapshots", "true", + "Indicates whether to always backup primary storage snapshot to secondary storage", false, ConfigKey.Scope.Global, null); + void deletePoliciesForVolume(Long volumeId); /** diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index bd49c05f43e2..5416049b69c6 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -206,7 +206,7 @@ public String getConfigComponentName() { @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {BackupRetryAttempts, BackupRetryInterval, SnapshotHourlyMax, SnapshotDailyMax, SnapshotMonthlyMax, SnapshotWeeklyMax, usageSnapshotSelection}; + return new ConfigKey[] {BackupRetryAttempts, BackupRetryInterval, SnapshotHourlyMax, SnapshotDailyMax, SnapshotMonthlyMax, SnapshotWeeklyMax, usageSnapshotSelection, BackupSnapshotAfterTakingSnapshot}; } @Override @@ -1123,12 +1123,24 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc } SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot); - if (payload.getAsyncBackup()) { - backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS); + boolean backupFlag = BackupSnapshotAfterTakingSnapshot.value() == null || + BackupSnapshotAfterTakingSnapshot.value(); + + if (backupFlag) { + if (payload.getAsyncBackup()) { + backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS); + } else { + SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary); + if (backupedSnapshot != null) { + snapshotStrategy.postSnapshotCreation(snapshotOnPrimary); + } + } } else { - SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary); - if (backupedSnapshot != null) { - snapshotStrategy.postSnapshotCreation(snapshotOnPrimary); + if(s_logger.isDebugEnabled()) { + 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!"); } } From 43722053db5a4d8a517dc90193e696a3400a10d5 Mon Sep 17 00:00:00 2001 From: Nathan Johnson Date: Wed, 16 Jan 2019 13:01:19 -0600 Subject: [PATCH 2/2] updates per review --- .../subsystem/api/storage/SnapshotInfo.java | 3 +- .../storage/snapshot/SnapshotObject.java | 6 ++-- .../storage/snapshot/SnapshotManagerImpl.java | 30 ++++++++++--------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java index cc27b20a9422..ef72afc5b777 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java @@ -17,6 +17,7 @@ package org.apache.cloudstack.engine.subsystem.api.storage; import com.cloud.storage.Snapshot; +import com.cloud.utils.exception.CloudRuntimeException; public interface SnapshotInfo extends DataObject, Snapshot { SnapshotInfo getParent(); @@ -43,5 +44,5 @@ public interface SnapshotInfo extends DataObject, Snapshot { long getPhysicalSize(); - boolean markBackedUp(); + void markBackedUp() throws CloudRuntimeException; } diff --git a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java index 4ea1dd148d9e..65d6fa52e667 100644 --- a/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java +++ b/engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java @@ -150,14 +150,14 @@ public long getPhysicalSize() { } @Override - public boolean markBackedUp() { + public void markBackedUp() throws CloudRuntimeException{ try { processEvent(Event.OperationNotPerformed); } catch (NoTransitionException ex) { s_logger.error("no transition error: ", ex); - return false; + throw new CloudRuntimeException("Error marking snapshot backed up: " + + this.snapshot.getId() + " " + ex.getMessage()); } - return true; } @Override diff --git a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java index 5416049b69c6..07eb072e82c8 100755 --- a/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -1123,25 +1123,16 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc } SnapshotInfo snapshotOnPrimary = snapshotStrategy.takeSnapshot(snapshot); - boolean backupFlag = BackupSnapshotAfterTakingSnapshot.value() == null || + boolean backupSnapToSecondary = BackupSnapshotAfterTakingSnapshot.value() == null || BackupSnapshotAfterTakingSnapshot.value(); - if (backupFlag) { - if (payload.getAsyncBackup()) { - backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS); - } else { - SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary); - if (backupedSnapshot != null) { - snapshotStrategy.postSnapshotCreation(snapshotOnPrimary); - } - } + if (backupSnapToSecondary) { + backupSnapshotToSecondary(payload.getAsyncBackup(), snapshotStrategy, snapshotOnPrimary); } else { if(s_logger.isDebugEnabled()) { - 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!"); + s_logger.debug("skipping backup of snapshot " + snapshotId + " to secondary due to configuration"); } + snapshotOnPrimary.markBackedUp(); } try { @@ -1183,6 +1174,17 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc return snapshot; } + protected void backupSnapshotToSecondary(boolean asyncBackup, SnapshotStrategy snapshotStrategy, SnapshotInfo snapshotOnPrimary) { + if (asyncBackup) { + backupSnapshotExecutor.schedule(new BackupSnapshotTask(snapshotOnPrimary, snapshotBackupRetries - 1, snapshotStrategy), 0, TimeUnit.SECONDS); + } else { + SnapshotInfo backupedSnapshot = snapshotStrategy.backupSnapshot(snapshotOnPrimary); + if (backupedSnapshot != null) { + snapshotStrategy.postSnapshotCreation(snapshotOnPrimary); + } + } + } + protected class BackupSnapshotTask extends ManagedContextRunnable { SnapshotInfo snapshot; int attempts;