Skip to content

Commit 432c049

Browse files
jmsperujmsperu
committed
fix(backup): make infrastructure DB backup opt-in, add unit tests
Addresses @weizhouapache's review: by default, production deployments should manage CloudStack DB backups via existing external tooling (cron + mysqldump, replication, etc), not via the in-process backup task. The DB component is now gated on a new explicit ConfigKey that defaults to false. - New ConfigKey: nas.infra.backup.include.database (Boolean, default false, Global scope). When false, the InfrastructureBackupTask runs only the configs + certs path (where there is no reasonable external alternative). - nas.infra.backup.enabled description rewritten to make the new split clear and to steer production users toward the cron-job pattern for the DB. - nas.infra.backup.include.usage.db description updated to clarify it has no effect unless include.database is also true. - New InfrastructureBackupTaskTest with 9 tests covering the gating decisions: master switch off, empty location, DB-skipped-by-default, both DBs when usage enabled, usage flag ignored when DB excluded, props unreadable, retention always runs, daily interval. Addresses codecov coverage gap on this PR (143 untested lines was the original report). - backupDatabase/backupDirectory/cleanupOldBackups + loadDbProperties + the ConfigKey accessors made protected so tests can override the side-effecting paths without standing up ProcessBuilder.
1 parent 6a8e1d1 commit 432c049

3 files changed

Lines changed: 312 additions & 31 deletions

File tree

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java‎

Lines changed: 53 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -68,26 +68,48 @@ public Long getDelay() {
6868
return DAILY_INTERVAL_MS;
6969
}
7070

71+
/** Indirection so tests can override without standing up the ConfigDepot. */
72+
protected boolean isEnabled() {
73+
return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupEnabled.value());
74+
}
75+
76+
protected String getBackupLocation() {
77+
return NASBackupProvider.NASInfraBackupLocation.value();
78+
}
79+
80+
protected int getRetentionCount() {
81+
return NASBackupProvider.NASInfraBackupRetention.value();
82+
}
83+
84+
protected boolean isDatabaseIncluded() {
85+
return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupIncludeDatabase.value());
86+
}
87+
88+
protected boolean isUsageDbIncluded() {
89+
return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupUsageDb.value());
90+
}
91+
7192
@Override
7293
protected void runInContext() {
73-
if (!Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupEnabled.value())) {
94+
if (!isEnabled()) {
7495
LOG.debug("Infrastructure backup is disabled (nas.infra.backup.enabled=false)");
7596
return;
7697
}
7798

78-
String nasBackupPath = NASBackupProvider.NASInfraBackupLocation.value();
99+
String nasBackupPath = getBackupLocation();
79100
if (nasBackupPath == null || nasBackupPath.isEmpty()) {
80101
LOG.error("Infrastructure backup location not configured (nas.infra.backup.location is empty)");
81102
return;
82103
}
83104

84-
int retentionCount = NASBackupProvider.NASInfraBackupRetention.value();
85-
boolean includeUsageDb = Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupUsageDb.value());
105+
int retentionCount = getRetentionCount();
106+
boolean includeDatabase = isDatabaseIncluded();
107+
boolean includeUsageDb = isUsageDbIncluded();
86108

87109
String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss"));
88110
String backupDir = nasBackupPath + "/infra-backup/" + timestamp;
89111

90-
LOG.info("Starting infrastructure backup to {}", backupDir);
112+
LOG.info("Starting infrastructure backup to {} (database included: {})", backupDir, includeDatabase);
91113

92114
try {
93115
File dir = new File(backupDir);
@@ -96,26 +118,30 @@ protected void runInContext() {
96118
return;
97119
}
98120

99-
// Read database credentials from db.properties
100-
Properties dbProps = loadDbProperties();
101-
if (dbProps == null) {
102-
LOG.error("Failed to load database properties from {}", DB_PROPERTIES_PATH);
103-
return;
104-
}
105-
106-
String dbHost = dbProps.getProperty("db.cloud.host", "localhost");
107-
String dbUser = dbProps.getProperty("db.cloud.username", "cloud");
108-
String dbPassword = dbProps.getProperty("db.cloud.password", "");
121+
// 1 & 2. Database backup — opt-in via nas.infra.backup.include.database.
122+
// Production deployments typically run their own mysqldump cron jobs and disable this;
123+
// it exists for small/edge deployments wanting unified DR on the same NAS as VM backups.
124+
if (includeDatabase) {
125+
Properties dbProps = loadDbProperties();
126+
if (dbProps == null) {
127+
LOG.error("Database backup requested but failed to load properties from {} — skipping DB component", DB_PROPERTIES_PATH);
128+
} else {
129+
String dbHost = dbProps.getProperty("db.cloud.host", "localhost");
130+
String dbUser = dbProps.getProperty("db.cloud.username", "cloud");
131+
String dbPassword = dbProps.getProperty("db.cloud.password", "");
109132

110-
// 1. Backup CloudStack database
111-
backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword);
133+
backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword);
112134

113-
// 2. Backup usage database if enabled
114-
if (includeUsageDb) {
115-
String usageHost = dbProps.getProperty("db.usage.host", dbHost);
116-
String usageUser = dbProps.getProperty("db.usage.username", dbUser);
117-
String usagePassword = dbProps.getProperty("db.usage.password", dbPassword);
118-
backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword);
135+
if (includeUsageDb) {
136+
String usageHost = dbProps.getProperty("db.usage.host", dbHost);
137+
String usageUser = dbProps.getProperty("db.usage.username", dbUser);
138+
String usagePassword = dbProps.getProperty("db.usage.password", dbPassword);
139+
backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword);
140+
}
141+
}
142+
} else {
143+
LOG.debug("Database backup skipped (nas.infra.backup.include.database=false). " +
144+
"Manage DB backups externally for production deployments.");
119145
}
120146

121147
// 3. Backup management server configs
@@ -143,7 +169,7 @@ protected void runInContext() {
143169
}
144170
}
145171

146-
private Properties loadDbProperties() {
172+
protected Properties loadDbProperties() {
147173
File propsFile = new File(DB_PROPERTIES_PATH);
148174
if (!propsFile.exists()) {
149175
LOG.warn("Database properties file not found: {}", DB_PROPERTIES_PATH);
@@ -160,7 +186,7 @@ private Properties loadDbProperties() {
160186
}
161187
}
162188

163-
private void backupDatabase(String dbName, String backupDir, String timestamp,
189+
protected void backupDatabase(String dbName, String backupDir, String timestamp,
164190
String dbHost, String dbUser, String dbPassword) {
165191
String dumpFile = backupDir + "/" + dbName + "-" + timestamp + ".sql.gz";
166192

@@ -198,7 +224,7 @@ private void backupDatabase(String dbName, String backupDir, String timestamp,
198224
}
199225
}
200226

201-
private void backupDirectory(String sourcePath, String backupDir, String archiveName) {
227+
protected void backupDirectory(String sourcePath, String backupDir, String archiveName) {
202228
File source = new File(sourcePath);
203229
if (!source.exists() || !source.isDirectory()) {
204230
LOG.debug("Directory {} does not exist, skipping", sourcePath);
@@ -232,7 +258,7 @@ private void backupDirectory(String sourcePath, String backupDir, String archive
232258
}
233259
}
234260

235-
private void cleanupOldBackups(String nasBackupPath, int retentionCount) {
261+
protected void cleanupOldBackups(String nasBackupPath, int retentionCount) {
236262
File infraDir = new File(nasBackupPath + "/infra-backup");
237263
if (!infraDir.exists()) {
238264
return;

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,26 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
8989
static final ConfigKey<Boolean> NASInfraBackupEnabled = new ConfigKey<>("Advanced", Boolean.class,
9090
"nas.infra.backup.enabled",
9191
"false",
92-
"Enable automated infrastructure backup (database, configs) to NAS storage. " +
93-
"When enabled, the management server will perform a daily backup of the CloudStack " +
94-
"database, configuration files, and SSL certificates to the configured NAS location.",
92+
"Enable automated infrastructure backup to NAS storage. When enabled, the management " +
93+
"server will perform a daily backup of CloudStack configuration files and SSL " +
94+
"certificates to the configured NAS location. The CloudStack database is NOT included " +
95+
"by default — for production deployments, manage database backups externally (e.g. via " +
96+
"a cron job running mysqldump). To opt in to bundling the database with this backup " +
97+
"(useful only for small / edge / single-MS deployments without separate ops tooling), " +
98+
"also set nas.infra.backup.include.database=true.",
99+
true,
100+
ConfigKey.Scope.Global,
101+
BackupFrameworkEnabled.key());
102+
103+
static final ConfigKey<Boolean> NASInfraBackupIncludeDatabase = new ConfigKey<>("Advanced", Boolean.class,
104+
"nas.infra.backup.include.database",
105+
"false",
106+
"Include the CloudStack database in the daily infrastructure backup. Defaults to false " +
107+
"because production deployments typically manage DB backups via external tooling (e.g. " +
108+
"cron + mysqldump, replication, dedicated backup appliance) and are better served " +
109+
"doing so. Only set true when you want one-knob disaster recovery for a small/edge " +
110+
"deployment and the same NAS that already holds your VM backups is an acceptable " +
111+
"target. Has no effect unless nas.infra.backup.enabled is also true.",
95112
true,
96113
ConfigKey.Scope.Global,
97114
BackupFrameworkEnabled.key());
@@ -116,7 +133,8 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
116133
static final ConfigKey<Boolean> NASInfraBackupUsageDb = new ConfigKey<>("Advanced", Boolean.class,
117134
"nas.infra.backup.include.usage.db",
118135
"true",
119-
"Include the cloud_usage database in infrastructure backup.",
136+
"Also include the cloud_usage database when the CloudStack database is being backed " +
137+
"up. Has no effect unless nas.infra.backup.include.database is also true.",
120138
true,
121139
ConfigKey.Scope.Global,
122140
BackupFrameworkEnabled.key());
@@ -643,6 +661,7 @@ public ConfigKey<?>[] getConfigKeys() {
643661
return new ConfigKey[]{
644662
NASBackupRestoreMountTimeout,
645663
NASInfraBackupEnabled,
664+
NASInfraBackupIncludeDatabase,
646665
NASInfraBackupLocation,
647666
NASInfraBackupRetention,
648667
NASInfraBackupUsageDb

0 commit comments

Comments
 (0)