From 73d89aa4e69a7b16e0c0317d35435b8a2d90da3e Mon Sep 17 00:00:00 2001 From: jmsperu Date: Fri, 27 Mar 2026 01:18:43 +0300 Subject: [PATCH 1/6] NAS backup: add infrastructure backup (database, configs, certs) Adds automated backup of CloudStack infrastructure to NAS storage: - MySQL databases (cloud + cloud_usage if enabled) - Management server configuration (/etc/cloudstack/management/) - Agent configuration (/etc/cloudstack/agent/) - SSL certificates and keystores Backup runs daily via BackgroundPollManager, configurable via global settings: - nas.infra.backup.enabled (default: false) - nas.infra.backup.location (NAS mount path) - nas.infra.backup.retention (default: 7 backup sets) - nas.infra.backup.include.usage.db (default: true) Backups are stored in the NAS backup storage under infra-backup/ with automatic retention management. Uses mysqldump --single-transaction for hot database backup (no table locks, InnoDB consistent snapshot). Database credentials are read from /etc/cloudstack/management/db.properties. --- .../backup/InfrastructureBackupTask.java | 273 ++++++++++++++++++ .../cloudstack/backup/NASBackupProvider.java | 53 +++- 2 files changed, 325 insertions(+), 1 deletion(-) create mode 100644 plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java new file mode 100644 index 000000000000..99634be62429 --- /dev/null +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -0,0 +1,273 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.backup; + +import org.apache.cloudstack.managed.context.ManagedContextRunnable; +import org.apache.cloudstack.poll.BackgroundPollTask; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.LogManager; + +import java.io.BufferedReader; +import java.io.File; +import java.io.FileReader; +import java.io.IOException; +import java.time.LocalDateTime; +import java.time.format.DateTimeFormatter; +import java.util.Arrays; +import java.util.Comparator; +import java.util.Properties; +import java.util.concurrent.TimeUnit; + +/** + * Scheduled task that backs up CloudStack infrastructure to NAS storage: + * + * + * Database credentials are read from /etc/cloudstack/management/db.properties. + * Backups are stored under {nasBackupPath}/infra-backup/{timestamp}/ with + * automatic retention management. + */ +public class InfrastructureBackupTask extends ManagedContextRunnable implements BackgroundPollTask { + + private static final Logger LOG = LogManager.getLogger(InfrastructureBackupTask.class); + + private static final String DB_PROPERTIES_PATH = "/etc/cloudstack/management/db.properties"; + private static final String MANAGEMENT_CONFIG_PATH = "/etc/cloudstack/management"; + private static final String AGENT_CONFIG_PATH = "/etc/cloudstack/agent"; + private static final String SSL_CERT_PATH = "/etc/cloudstack/management/cert"; + + /** 24 hours in milliseconds */ + private static final long DAILY_INTERVAL_MS = 86400L * 1000L; + + private final NASBackupProvider provider; + + public InfrastructureBackupTask(NASBackupProvider provider) { + this.provider = provider; + } + + @Override + public Long getDelay() { + return DAILY_INTERVAL_MS; + } + + @Override + protected void runInContext() { + if (!Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupEnabled.value())) { + LOG.debug("Infrastructure backup is disabled (nas.infra.backup.enabled=false)"); + return; + } + + String nasBackupPath = NASBackupProvider.NASInfraBackupLocation.value(); + if (nasBackupPath == null || nasBackupPath.isEmpty()) { + LOG.error("Infrastructure backup location not configured (nas.infra.backup.location is empty)"); + return; + } + + int retentionCount = NASBackupProvider.NASInfraBackupRetention.value(); + boolean includeUsageDb = Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupUsageDb.value()); + + String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss")); + String backupDir = nasBackupPath + "/infra-backup/" + timestamp; + + LOG.info("Starting infrastructure backup to {}", backupDir); + + try { + File dir = new File(backupDir); + if (!dir.mkdirs()) { + LOG.error("Failed to create backup directory: {}", backupDir); + return; + } + + // Read database credentials from db.properties + Properties dbProps = loadDbProperties(); + if (dbProps == null) { + LOG.error("Failed to load database properties from {}", DB_PROPERTIES_PATH); + return; + } + + String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); + String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); + String dbPassword = dbProps.getProperty("db.cloud.password", ""); + + // 1. Backup CloudStack database + backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); + + // 2. Backup usage database if enabled + if (includeUsageDb) { + String usageHost = dbProps.getProperty("db.usage.host", dbHost); + String usageUser = dbProps.getProperty("db.usage.username", dbUser); + String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); + backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); + } + + // 3. Backup management server configs + backupDirectory(MANAGEMENT_CONFIG_PATH, backupDir, "management-config"); + + // 4. Backup agent configs (if present on this host) + File agentDir = new File(AGENT_CONFIG_PATH); + if (agentDir.exists()) { + backupDirectory(AGENT_CONFIG_PATH, backupDir, "agent-config"); + } + + // 5. Backup SSL certificates + File sslDir = new File(SSL_CERT_PATH); + if (sslDir.exists()) { + backupDirectory(SSL_CERT_PATH, backupDir, "ssl-certs"); + } + + // 6. Cleanup old backups based on retention policy + cleanupOldBackups(nasBackupPath, retentionCount); + + LOG.info("Infrastructure backup completed successfully: {}", backupDir); + + } catch (Exception e) { + LOG.error("Infrastructure backup failed: {}", e.getMessage(), e); + } + } + + private Properties loadDbProperties() { + File propsFile = new File(DB_PROPERTIES_PATH); + if (!propsFile.exists()) { + LOG.warn("Database properties file not found: {}", DB_PROPERTIES_PATH); + return null; + } + + Properties props = new Properties(); + try (BufferedReader reader = new BufferedReader(new FileReader(propsFile))) { + props.load(reader); + return props; + } catch (IOException e) { + LOG.error("Failed to read database properties: {}", e.getMessage()); + return null; + } + } + + private void backupDatabase(String dbName, String backupDir, String timestamp, + String dbHost, String dbUser, String dbPassword) { + String dumpFile = backupDir + "/" + dbName + "-" + timestamp + ".sql.gz"; + + // Use --single-transaction for InnoDB hot backup (no table locks, consistent snapshot) + String[] cmd = {"/bin/bash", "-c", + String.format("mysqldump --single-transaction --routines --triggers --events " + + "-h '%s' -u '%s' -p'%s' '%s' | gzip > '%s'", + dbHost, dbUser, dbPassword, dbName, dumpFile)}; + + try { + ProcessBuilder pb = new ProcessBuilder(cmd); + pb.redirectErrorStream(true); + Process process = pb.start(); + boolean completed = process.waitFor(300, TimeUnit.SECONDS); + + if (!completed) { + process.destroyForcibly(); + LOG.error("Database backup timed out for {}", dbName); + return; + } + + if (process.exitValue() != 0) { + LOG.error("Database backup failed for {} with exit code {}", dbName, process.exitValue()); + return; + } + + File dump = new File(dumpFile); + LOG.info("Database {} backed up: {} ({} bytes)", dbName, dumpFile, dump.length()); + + } catch (IOException | InterruptedException e) { + LOG.error("Failed to backup database {}: {}", dbName, e.getMessage()); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + } + } + + private void backupDirectory(String sourcePath, String backupDir, String archiveName) { + File source = new File(sourcePath); + if (!source.exists() || !source.isDirectory()) { + LOG.debug("Directory {} does not exist, skipping", sourcePath); + return; + } + + String tarFile = backupDir + "/" + archiveName + ".tar.gz"; + String[] cmd = {"/bin/bash", "-c", + String.format("tar czf '%s' -C '%s' .", tarFile, sourcePath)}; + + try { + ProcessBuilder pb = new ProcessBuilder(cmd); + pb.redirectErrorStream(true); + Process process = pb.start(); + boolean completed = process.waitFor(60, TimeUnit.SECONDS); + + if (completed && process.exitValue() == 0) { + LOG.info("Directory {} backed up to {}", sourcePath, tarFile); + } else { + if (!completed) { + process.destroyForcibly(); + } + LOG.warn("Directory backup failed for {} (exit code: {})", + sourcePath, completed ? process.exitValue() : "timeout"); + } + } catch (IOException | InterruptedException e) { + LOG.error("Failed to backup directory {}: {}", sourcePath, e.getMessage()); + if (e instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + } + } + + private void cleanupOldBackups(String nasBackupPath, int retentionCount) { + File infraDir = new File(nasBackupPath + "/infra-backup"); + if (!infraDir.exists()) { + return; + } + + File[] backups = infraDir.listFiles(File::isDirectory); + if (backups == null || backups.length <= retentionCount) { + return; + } + + // Sort by name (timestamp-based), oldest first + Arrays.sort(backups, Comparator.comparing(File::getName)); + + int toDelete = backups.length - retentionCount; + for (int i = 0; i < toDelete; i++) { + LOG.info("Removing old infrastructure backup: {}", backups[i].getName()); + deleteDirectory(backups[i]); + } + } + + private void deleteDirectory(File dir) { + File[] files = dir.listFiles(); + if (files != null) { + for (File file : files) { + if (file.isDirectory()) { + deleteDirectory(file); + } else { + if (!file.delete()) { + LOG.warn("Failed to delete file: {}", file.getAbsolutePath()); + } + } + } + } + if (!dir.delete()) { + LOG.warn("Failed to delete directory: {}", dir.getAbsolutePath()); + } + } +} diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index a72920fbae88..f861cf9a93dc 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -60,6 +60,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager; import org.apache.cloudstack.framework.config.ConfigKey; import org.apache.cloudstack.framework.config.Configurable; +import org.apache.cloudstack.poll.BackgroundPollManager; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; @@ -69,6 +70,7 @@ import org.apache.logging.log4j.LogManager; import javax.inject.Inject; +import javax.naming.ConfigurationException; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Collections; @@ -127,6 +129,41 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co ConfigKey.Scope.Zone, BackupFrameworkEnabled.key()); + static final ConfigKey NASInfraBackupEnabled = new ConfigKey<>("Advanced", Boolean.class, + "nas.infra.backup.enabled", + "false", + "Enable automated infrastructure backup (database, configs) to NAS storage. " + + "When enabled, the management server will perform a daily backup of the CloudStack " + + "database, configuration files, and SSL certificates to the configured NAS location.", + true, + ConfigKey.Scope.Global, + BackupFrameworkEnabled.key()); + + static final ConfigKey NASInfraBackupLocation = new ConfigKey<>("Advanced", String.class, + "nas.infra.backup.location", + "", + "NAS mount path where infrastructure backups are stored (e.g. /mnt/nas-backup). " + + "Backups will be written to {location}/infra-backup/{timestamp}/.", + true, + ConfigKey.Scope.Global, + BackupFrameworkEnabled.key()); + + static final ConfigKey NASInfraBackupRetention = new ConfigKey<>("Advanced", Integer.class, + "nas.infra.backup.retention", + "7", + "Number of infrastructure backup sets to retain. Older backups are automatically removed.", + true, + ConfigKey.Scope.Global, + BackupFrameworkEnabled.key()); + + static final ConfigKey NASInfraBackupUsageDb = new ConfigKey<>("Advanced", Boolean.class, + "nas.infra.backup.include.usage.db", + "true", + "Include the cloud_usage database in infrastructure backup.", + true, + ConfigKey.Scope.Global, + BackupFrameworkEnabled.key()); + @Inject private BackupDao backupDao; @@ -184,6 +221,16 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co @Inject private AlertManager alertManager; + @Inject + private BackgroundPollManager backgroundPollManager; + + @Override + public boolean configure(String name, Map params) throws ConfigurationException { + super.configure(name, params); + backgroundPollManager.submitTask(new InfrastructureBackupTask(this)); + return true; + } + private Long getClusterIdFromRootVolume(VirtualMachine vm) { VolumeVO rootVolume = volumeDao.getInstanceRootVolume(vm.getId()); StoragePoolVO rootDiskPool = primaryDataStoreDao.findById(rootVolume.getPoolId()); @@ -1283,7 +1330,11 @@ public ConfigKey[] getConfigKeys() { NASBackupRestoreMountTimeout, NASBackupFullEvery, NASBackupIncrementalEnabled, - NASBackupQuiesceAgentTimeout + NASBackupQuiesceAgentTimeout, + NASInfraBackupEnabled, + NASInfraBackupLocation, + NASInfraBackupRetention, + NASInfraBackupUsageDb }; } From 23569c22c0ebb317807efdcb36c379775a918733 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Sat, 23 May 2026 01:11:21 +0300 Subject: [PATCH 2/6] 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. --- .../backup/InfrastructureBackupTask.java | 80 ++++-- .../cloudstack/backup/NASBackupProvider.java | 27 +- .../backup/InfrastructureBackupTaskTest.java | 236 ++++++++++++++++++ 3 files changed, 312 insertions(+), 31 deletions(-) create mode 100644 plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java index 99634be62429..25c086430fb0 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -68,26 +68,48 @@ public Long getDelay() { return DAILY_INTERVAL_MS; } + /** Indirection so tests can override without standing up the ConfigDepot. */ + protected boolean isEnabled() { + return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupEnabled.value()); + } + + protected String getBackupLocation() { + return NASBackupProvider.NASInfraBackupLocation.value(); + } + + protected int getRetentionCount() { + return NASBackupProvider.NASInfraBackupRetention.value(); + } + + protected boolean isDatabaseIncluded() { + return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupIncludeDatabase.value()); + } + + protected boolean isUsageDbIncluded() { + return Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupUsageDb.value()); + } + @Override protected void runInContext() { - if (!Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupEnabled.value())) { + if (!isEnabled()) { LOG.debug("Infrastructure backup is disabled (nas.infra.backup.enabled=false)"); return; } - String nasBackupPath = NASBackupProvider.NASInfraBackupLocation.value(); + String nasBackupPath = getBackupLocation(); if (nasBackupPath == null || nasBackupPath.isEmpty()) { LOG.error("Infrastructure backup location not configured (nas.infra.backup.location is empty)"); return; } - int retentionCount = NASBackupProvider.NASInfraBackupRetention.value(); - boolean includeUsageDb = Boolean.TRUE.equals(NASBackupProvider.NASInfraBackupUsageDb.value()); + int retentionCount = getRetentionCount(); + boolean includeDatabase = isDatabaseIncluded(); + boolean includeUsageDb = isUsageDbIncluded(); String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss")); String backupDir = nasBackupPath + "/infra-backup/" + timestamp; - LOG.info("Starting infrastructure backup to {}", backupDir); + LOG.info("Starting infrastructure backup to {} (database included: {})", backupDir, includeDatabase); try { File dir = new File(backupDir); @@ -96,26 +118,30 @@ protected void runInContext() { return; } - // Read database credentials from db.properties - Properties dbProps = loadDbProperties(); - if (dbProps == null) { - LOG.error("Failed to load database properties from {}", DB_PROPERTIES_PATH); - return; - } - - String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); - String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); - String dbPassword = dbProps.getProperty("db.cloud.password", ""); + // 1 & 2. Database backup — opt-in via nas.infra.backup.include.database. + // Production deployments typically run their own mysqldump cron jobs and disable this; + // it exists for small/edge deployments wanting unified DR on the same NAS as VM backups. + if (includeDatabase) { + Properties dbProps = loadDbProperties(); + if (dbProps == null) { + LOG.error("Database backup requested but failed to load properties from {} — skipping DB component", DB_PROPERTIES_PATH); + } else { + String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); + String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); + String dbPassword = dbProps.getProperty("db.cloud.password", ""); - // 1. Backup CloudStack database - backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); + backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); - // 2. Backup usage database if enabled - if (includeUsageDb) { - String usageHost = dbProps.getProperty("db.usage.host", dbHost); - String usageUser = dbProps.getProperty("db.usage.username", dbUser); - String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); - backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); + if (includeUsageDb) { + String usageHost = dbProps.getProperty("db.usage.host", dbHost); + String usageUser = dbProps.getProperty("db.usage.username", dbUser); + String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); + backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); + } + } + } else { + LOG.debug("Database backup skipped (nas.infra.backup.include.database=false). " + + "Manage DB backups externally for production deployments."); } // 3. Backup management server configs @@ -143,7 +169,7 @@ protected void runInContext() { } } - private Properties loadDbProperties() { + protected Properties loadDbProperties() { File propsFile = new File(DB_PROPERTIES_PATH); if (!propsFile.exists()) { LOG.warn("Database properties file not found: {}", DB_PROPERTIES_PATH); @@ -160,7 +186,7 @@ private Properties loadDbProperties() { } } - private void backupDatabase(String dbName, String backupDir, String timestamp, + protected void backupDatabase(String dbName, String backupDir, String timestamp, String dbHost, String dbUser, String dbPassword) { String dumpFile = backupDir + "/" + dbName + "-" + timestamp + ".sql.gz"; @@ -198,7 +224,7 @@ private void backupDatabase(String dbName, String backupDir, String timestamp, } } - private void backupDirectory(String sourcePath, String backupDir, String archiveName) { + protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { File source = new File(sourcePath); if (!source.exists() || !source.isDirectory()) { LOG.debug("Directory {} does not exist, skipping", sourcePath); @@ -232,7 +258,7 @@ private void backupDirectory(String sourcePath, String backupDir, String archive } } - private void cleanupOldBackups(String nasBackupPath, int retentionCount) { + protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { File infraDir = new File(nasBackupPath + "/infra-backup"); if (!infraDir.exists()) { return; diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index f861cf9a93dc..d8656095b437 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -132,9 +132,26 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co static final ConfigKey NASInfraBackupEnabled = new ConfigKey<>("Advanced", Boolean.class, "nas.infra.backup.enabled", "false", - "Enable automated infrastructure backup (database, configs) to NAS storage. " + - "When enabled, the management server will perform a daily backup of the CloudStack " + - "database, configuration files, and SSL certificates to the configured NAS location.", + "Enable automated infrastructure backup to NAS storage. When enabled, the management " + + "server will perform a daily backup of CloudStack configuration files and SSL " + + "certificates to the configured NAS location. The CloudStack database is NOT included " + + "by default — for production deployments, manage database backups externally (e.g. via " + + "a cron job running mysqldump). To opt in to bundling the database with this backup " + + "(useful only for small / edge / single-MS deployments without separate ops tooling), " + + "also set nas.infra.backup.include.database=true.", + true, + ConfigKey.Scope.Global, + BackupFrameworkEnabled.key()); + + static final ConfigKey NASInfraBackupIncludeDatabase = new ConfigKey<>("Advanced", Boolean.class, + "nas.infra.backup.include.database", + "false", + "Include the CloudStack database in the daily infrastructure backup. Defaults to false " + + "because production deployments typically manage DB backups via external tooling (e.g. " + + "cron + mysqldump, replication, dedicated backup appliance) and are better served " + + "doing so. Only set true when you want one-knob disaster recovery for a small/edge " + + "deployment and the same NAS that already holds your VM backups is an acceptable " + + "target. Has no effect unless nas.infra.backup.enabled is also true.", true, ConfigKey.Scope.Global, BackupFrameworkEnabled.key()); @@ -159,7 +176,8 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co static final ConfigKey NASInfraBackupUsageDb = new ConfigKey<>("Advanced", Boolean.class, "nas.infra.backup.include.usage.db", "true", - "Include the cloud_usage database in infrastructure backup.", + "Also include the cloud_usage database when the CloudStack database is being backed " + + "up. Has no effect unless nas.infra.backup.include.database is also true.", true, ConfigKey.Scope.Global, BackupFrameworkEnabled.key()); @@ -1332,6 +1350,7 @@ public ConfigKey[] getConfigKeys() { NASBackupIncrementalEnabled, NASBackupQuiesceAgentTimeout, NASInfraBackupEnabled, + NASInfraBackupIncludeDatabase, NASInfraBackupLocation, NASInfraBackupRetention, NASInfraBackupUsageDb diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java new file mode 100644 index 000000000000..7c8e3b02b2f3 --- /dev/null +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package org.apache.cloudstack.backup; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +public class InfrastructureBackupTaskTest { + + private Path tmpRoot; + + @Before + public void setUp() throws IOException { + tmpRoot = Files.createTempDirectory("cs-infra-backup-test-"); + } + + @After + public void tearDown() { + if (tmpRoot != null) { + deleteRecursively(tmpRoot.toFile()); + } + } + + /** Captures backupDatabase/backupDirectory invocations instead of shelling out. */ + private static class RecordingTask extends InfrastructureBackupTask { + boolean enabled = true; + String location; + int retention = 7; + boolean databaseIncluded = false; + boolean usageDbIncluded = true; + Properties dbProps; + final List databaseBackupNames = new ArrayList<>(); + final List directoryBackupNames = new ArrayList<>(); + final AtomicInteger retentionCalls = new AtomicInteger(0); + + RecordingTask() { + super(null); + } + + @Override + protected boolean isEnabled() { return enabled; } + @Override + protected String getBackupLocation() { return location; } + @Override + protected int getRetentionCount() { return retention; } + @Override + protected boolean isDatabaseIncluded() { return databaseIncluded; } + @Override + protected boolean isUsageDbIncluded() { return usageDbIncluded; } + + @Override + protected Properties loadDbProperties() { + return dbProps; + } + + @Override + protected void backupDatabase(String dbName, String backupDir, String timestamp, + String dbHost, String dbUser, String dbPassword) { + databaseBackupNames.add(dbName); + } + + @Override + protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { + directoryBackupNames.add(archiveName); + } + + @Override + protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { + retentionCalls.incrementAndGet(); + } + } + + private static Properties stubDbProps() { + Properties props = new Properties(); + props.setProperty("db.cloud.host", "localhost"); + props.setProperty("db.cloud.username", "cloud"); + props.setProperty("db.cloud.password", "secret"); + return props; + } + + @Test + public void doesNothingWhenMasterSwitchDisabled() { + RecordingTask task = new RecordingTask(); + task.enabled = false; + task.location = tmpRoot.toString(); + + task.runInContext(); + + Assert.assertTrue("no DB backups when feature disabled", task.databaseBackupNames.isEmpty()); + Assert.assertTrue("no dir backups when feature disabled", task.directoryBackupNames.isEmpty()); + Assert.assertEquals("no retention when feature disabled", 0, task.retentionCalls.get()); + } + + @Test + public void doesNothingWhenLocationEmpty() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = ""; + + task.runInContext(); + + Assert.assertTrue(task.databaseBackupNames.isEmpty()); + Assert.assertTrue(task.directoryBackupNames.isEmpty()); + Assert.assertEquals(0, task.retentionCalls.get()); + } + + @Test + public void skipsDatabaseByDefault() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.databaseIncluded = false; // default + task.dbProps = stubDbProps(); + + task.runInContext(); + + Assert.assertTrue("DB component is opt-in: no DB backups by default", + task.databaseBackupNames.isEmpty()); + // Configs+certs still attempted (they're skipped silently if dirs absent) + Assert.assertEquals("retention still runs", 1, task.retentionCalls.get()); + } + + @Test + public void backsUpCloudDbWhenIncludeDatabaseTrue() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.usageDbIncluded = false; // suppress cloud_usage + task.dbProps = stubDbProps(); + + task.runInContext(); + + Assert.assertEquals("only cloud DB backed up", 1, task.databaseBackupNames.size()); + Assert.assertEquals("cloud", task.databaseBackupNames.get(0)); + } + + @Test + public void backsUpBothDbsWhenUsageEnabled() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.usageDbIncluded = true; + task.dbProps = stubDbProps(); + + task.runInContext(); + + Assert.assertEquals("both DBs backed up", 2, task.databaseBackupNames.size()); + Assert.assertEquals("cloud", task.databaseBackupNames.get(0)); + Assert.assertEquals("cloud_usage", task.databaseBackupNames.get(1)); + } + + @Test + public void usageDbFlagIgnoredWhenDatabaseExcluded() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.databaseIncluded = false; // master DB gate off + task.usageDbIncluded = true; // should be ignored + task.dbProps = stubDbProps(); + + task.runInContext(); + + Assert.assertTrue("usage DB requires include.database=true", + task.databaseBackupNames.isEmpty()); + } + + @Test + public void skipsDbBackupWhenPropertiesUnreadable() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.dbProps = null; // simulate missing/unreadable db.properties + + task.runInContext(); + + Assert.assertTrue("no DB backups attempted when props can't be loaded", + task.databaseBackupNames.isEmpty()); + Assert.assertEquals("retention still runs (configs+certs path unaffected)", + 1, task.retentionCalls.get()); + } + + @Test + public void retentionRunsOnEverySuccessfulPass() { + RecordingTask task = new RecordingTask(); + task.enabled = true; + task.location = tmpRoot.toString(); + task.retention = 3; + task.databaseIncluded = false; + + task.runInContext(); + + Assert.assertEquals(1, task.retentionCalls.get()); + } + + @Test + public void dailyIntervalIs24Hours() { + InfrastructureBackupTask task = new RecordingTask(); + Assert.assertEquals(Long.valueOf(86_400_000L), task.getDelay()); + } + + private void deleteRecursively(File f) { + File[] kids = f.listFiles(); + if (kids != null) { + for (File k : kids) deleteRecursively(k); + } + f.delete(); + } +} From 4c288ba669a46880a3ef88682a5443337f40d63d Mon Sep 17 00:00:00 2001 From: jmsperu Date: Sun, 21 Jun 2026 20:48:57 +0300 Subject: [PATCH 3/6] fix(backup): harden infrastructure backup per review (#12900) Address review feedback on #12900 (with unit tests proving the deletion logic): - backupDatabase: invoke mysqldump without a shell and pass credentials via a 0600 --defaults-extra-file, so db.properties values can't be shell-interpreted (no command injection) and the password no longer appears in the process list; gzip the dump in-process. - cleanupOldBackups: clamp negative retentionCount to avoid ArrayIndexOutOfBounds. - deleteDirectory: use Files.walkFileTree (no symlink following) so a symlink in the NAS tree cannot cause deletion outside the backup path. - runInContext: guard with a cluster-wide GlobalLock so only one management server runs the infra backup at a time. Lock acquisition is an overridable seam (acquireRunLock/releaseRunLock) so unit tests need no DB-backed lock. - create the backup dir only when it does not already exist. Extend InfrastructureBackupTaskTest with retention/delete coverage: negative- retention clamp, retention keeps newest/deletes oldest, and symlink-escape safety (the last two fail against the pre-fix code). Existing orchestration tests are preserved. --- .../backup/InfrastructureBackupTask.java | 141 +++++++++++++++--- .../backup/InfrastructureBackupTaskTest.java | 94 ++++++++++++ 2 files changed, 213 insertions(+), 22 deletions(-) diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java index 25c086430fb0..9c66f23c97c6 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -16,6 +16,8 @@ // under the License. package org.apache.cloudstack.backup; +import com.cloud.utils.db.GlobalLock; + import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.poll.BackgroundPollTask; import org.apache.logging.log4j.Logger; @@ -23,14 +25,28 @@ import java.io.BufferedReader; import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; import java.io.FileReader; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.PosixFilePermission; import java.time.LocalDateTime; import java.time.format.DateTimeFormatter; import java.util.Arrays; import java.util.Comparator; +import java.util.EnumSet; import java.util.Properties; import java.util.concurrent.TimeUnit; +import java.util.zip.GZIPOutputStream; /** * Scheduled task that backs up CloudStack infrastructure to NAS storage: @@ -111,9 +127,18 @@ protected void runInContext() { LOG.info("Starting infrastructure backup to {} (database included: {})", backupDir, includeDatabase); + // Cluster-wide lock: in multi-management-server deployments only one MS should run the + // infrastructure backup at a time, otherwise they would write/delete concurrently in the + // same NAS infra-backup path (duplicate backups, retention races, accidental deletes). + GlobalLock lock = acquireRunLock(); + if (lock == null) { + LOG.info("Another management server is performing the infrastructure backup; skipping this run"); + return; + } + try { File dir = new File(backupDir); - if (!dir.mkdirs()) { + if (!dir.exists() && !dir.mkdirs()) { LOG.error("Failed to create backup directory: {}", backupDir); return; } @@ -166,6 +191,24 @@ protected void runInContext() { } catch (Exception e) { LOG.error("Infrastructure backup failed: {}", e.getMessage(), e); + } finally { + releaseRunLock(lock); + } + } + + /** + * Acquire the cluster-wide run lock so only one management server performs the infrastructure + * backup at a time. Returns null if the lock can't be taken (another MS holds it). Overridable + * so unit tests can exercise runInContext without standing up the DB-backed lock manager. + */ + protected GlobalLock acquireRunLock() { + GlobalLock lock = GlobalLock.getInternLock("infra-backup"); + return (lock != null && lock.lock(5)) ? lock : null; + } + + protected void releaseRunLock(GlobalLock lock) { + if (lock != null) { + lock.unlock(); } } @@ -190,15 +233,23 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, String dbHost, String dbUser, String dbPassword) { String dumpFile = backupDir + "/" + dbName + "-" + timestamp + ".sql.gz"; - // Use --single-transaction for InnoDB hot backup (no table locks, consistent snapshot) - String[] cmd = {"/bin/bash", "-c", - String.format("mysqldump --single-transaction --routines --triggers --events " + - "-h '%s' -u '%s' -p'%s' '%s' | gzip > '%s'", - dbHost, dbUser, dbPassword, dbName, dumpFile)}; - + File defaultsFile = null; + File rawDump = null; try { - ProcessBuilder pb = new ProcessBuilder(cmd); - pb.redirectErrorStream(true); + // Pass credentials via a 0600 --defaults-extra-file rather than the command line, so the + // password never appears in the process list, and invoke mysqldump WITHOUT a shell so that + // values read from db.properties cannot be interpreted as shell metacharacters (no injection). + defaultsFile = writeMysqlDefaultsFile(dbHost, dbUser, dbPassword); + + rawDump = new File(backupDir, dbName + "-" + timestamp + ".sql"); + // --single-transaction = InnoDB hot backup (no table locks, consistent snapshot) + ProcessBuilder pb = new ProcessBuilder( + "mysqldump", + "--defaults-extra-file=" + defaultsFile.getAbsolutePath(), + "--single-transaction", "--routines", "--triggers", "--events", + dbName); + pb.redirectOutput(ProcessBuilder.Redirect.to(rawDump)); + pb.redirectError(ProcessBuilder.Redirect.INHERIT); Process process = pb.start(); boolean completed = process.waitFor(300, TimeUnit.SECONDS); @@ -213,6 +264,12 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, return; } + // Compress in-process (no shell pipeline) into the final .sql.gz. + try (InputStream in = new FileInputStream(rawDump); + OutputStream out = new GZIPOutputStream(new FileOutputStream(dumpFile))) { + in.transferTo(out); + } + File dump = new File(dumpFile); LOG.info("Database {} backed up: {} ({} bytes)", dbName, dumpFile, dump.length()); @@ -221,7 +278,37 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, if (e instanceof InterruptedException) { Thread.currentThread().interrupt(); } + } finally { + if (rawDump != null && rawDump.exists() && !rawDump.delete()) { + LOG.warn("Failed to delete temporary dump file: {}", rawDump.getAbsolutePath()); + } + if (defaultsFile != null && defaultsFile.exists() && !defaultsFile.delete()) { + LOG.warn("Failed to delete temporary mysql defaults file: {}", defaultsFile.getAbsolutePath()); + } + } + } + + /** + * Writes a temporary MySQL option file (restricted to mode 0600 where supported) holding the + * connection credentials, so mysqldump can read them via {@code --defaults-extra-file}. Keeping + * the password out of the argument list prevents it leaking through the process list. + */ + private File writeMysqlDefaultsFile(String host, String user, String password) throws IOException { + Path tmp = Files.createTempFile("cs-infra-mysqldump-", ".cnf"); + try { + Files.setPosixFilePermissions(tmp, + EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)); + } catch (UnsupportedOperationException ignored) { + // Non-POSIX filesystem; createTempFile already restricts access to the owner. + } + String content = "[client]\n" + + "host=" + (host == null ? "" : host) + "\n" + + "user=" + (user == null ? "" : user) + "\n" + + "password=" + (password == null ? "" : password) + "\n"; + try (Writer w = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8)) { + w.write(content); } + return tmp.toFile(); } protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { @@ -259,6 +346,11 @@ protected void backupDirectory(String sourcePath, String backupDir, String archi } protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { + // A negative retention (misconfiguration) would make toDelete exceed backups.length below and + // throw ArrayIndexOutOfBoundsException; clamp it so we never compute a delete count > available. + if (retentionCount < 0) { + retentionCount = 0; + } File infraDir = new File(nasBackupPath + "/infra-backup"); if (!infraDir.exists()) { return; @@ -280,20 +372,25 @@ protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { } private void deleteDirectory(File dir) { - File[] files = dir.listFiles(); - if (files != null) { - for (File file : files) { - if (file.isDirectory()) { - deleteDirectory(file); - } else { - if (!file.delete()) { - LOG.warn("Failed to delete file: {}", file.getAbsolutePath()); - } + try { + // walkFileTree does NOT follow symbolic links by default: a symlink placed inside the + // backup tree (NAS shares are often writable by multiple systems) is removed as a link, + // its target is never descended into or deleted — so a delete can't escape the tree. + Files.walkFileTree(dir.toPath(), new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + Files.deleteIfExists(file); + return FileVisitResult.CONTINUE; } - } - } - if (!dir.delete()) { - LOG.warn("Failed to delete directory: {}", dir.getAbsolutePath()); + + @Override + public FileVisitResult postVisitDirectory(Path visited, IOException exc) throws IOException { + Files.deleteIfExists(visited); + return FileVisitResult.CONTINUE; + } + }); + } catch (IOException e) { + LOG.warn("Failed to delete directory {}: {}", dir.getAbsolutePath(), e.getMessage()); } } } diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java index 7c8e3b02b2f3..19ebf9ea288b 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java @@ -16,17 +16,21 @@ // under the License. package org.apache.cloudstack.backup; +import com.cloud.utils.db.GlobalLock; + import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Properties; import java.util.concurrent.atomic.AtomicInteger; import org.junit.After; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; @@ -93,6 +97,17 @@ protected void backupDirectory(String sourcePath, String backupDir, String archi protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { retentionCalls.incrementAndGet(); } + + @Override + protected GlobalLock acquireRunLock() { + // in-memory interned lock only; never touches the DB-backed lock manager in unit tests + return GlobalLock.getInternLock("infra-backup-test"); + } + + @Override + protected void releaseRunLock(GlobalLock lock) { + // no-op: the DB lock was never actually acquired in tests + } } private static Properties stubDbProps() { @@ -226,6 +241,85 @@ public void dailyIntervalIs24Hours() { Assert.assertEquals(Long.valueOf(86_400_000L), task.getDelay()); } + // ---- retention / delete logic (exercises the REAL cleanupOldBackups + deleteDirectory, + // which RecordingTask above stubs out) ---- + + private final InfrastructureBackupTask realTask = new InfrastructureBackupTask(null); + + private File infraBackupDir() { + return new File(tmpRoot.toFile(), "infra-backup"); + } + + private File makeBackup(String name) throws IOException { + File dir = new File(infraBackupDir(), name); + Assert.assertTrue(dir.mkdirs()); + Assert.assertTrue(new File(dir, "dump.sql.gz").createNewFile()); + return dir; + } + + private String[] remainingBackups() { + File[] dirs = infraBackupDir().listFiles(File::isDirectory); + if (dirs == null) { + return new String[0]; + } + return Arrays.stream(dirs).map(File::getName).sorted().toArray(String[]::new); + } + + @Test + public void cleanupClampsNegativeRetentionInsteadOfThrowing() throws IOException { + makeBackup("20240101-000000"); + makeBackup("20240102-000000"); + makeBackup("20240103-000000"); + + // A negative retention (misconfiguration) previously made toDelete (= count - retention) + // larger than the array length and threw ArrayIndexOutOfBoundsException. It must now clamp + // to 0 and simply remove everything, without throwing. + realTask.cleanupOldBackups(tmpRoot.toString(), -5); + + Assert.assertEquals(0, remainingBackups().length); + } + + @Test + public void cleanupKeepsNewestAndDeletesOldest() throws IOException { + makeBackup("20240101-000000"); + makeBackup("20240102-000000"); + makeBackup("20240103-000000"); + makeBackup("20240104-000000"); + makeBackup("20240105-000000"); + + realTask.cleanupOldBackups(tmpRoot.toString(), 2); + + Assert.assertArrayEquals(new String[] {"20240104-000000", "20240105-000000"}, remainingBackups()); + } + + @Test + public void deleteDoesNotFollowSymlinkOutOfTheBackupTree() throws IOException { + // A file living OUTSIDE the backup tree, reachable only via a symlink inside an old backup. + File outside = new File(tmpRoot.toFile(), "outside"); + Assert.assertTrue(outside.mkdirs()); + File precious = new File(outside, "precious.txt"); + Assert.assertTrue(precious.createNewFile()); + + File oldest = makeBackup("20240101-000000"); + makeBackup("20240102-000000"); + makeBackup("20240103-000000"); + + Path link = new File(oldest, "escape").toPath(); + try { + Files.createSymbolicLink(link, outside.toPath()); + } catch (IOException | UnsupportedOperationException e) { + Assume.assumeNoException("Filesystem does not support symbolic links", e); + } + + // Retain only the newest; the two oldest (including the one holding the symlink) are deleted. + realTask.cleanupOldBackups(tmpRoot.toString(), 1); + + Assert.assertArrayEquals(new String[] {"20240103-000000"}, remainingBackups()); + // The symlink target and its contents MUST survive — the delete must not follow the link out. + Assert.assertTrue("symlink target directory must survive", outside.exists()); + Assert.assertTrue("file behind the symlink must survive", precious.exists()); + } + private void deleteRecursively(File f) { File[] kids = f.listFiles(); if (kids != null) { From 8b132ad0d9ed4a09448af1aed90eb3d8375fdb23 Mon Sep 17 00:00:00 2001 From: James Peru Date: Mon, 24 Aug 2026 16:52:28 +0300 Subject: [PATCH 4/6] address review (#12900): per-management-server backup directory, extract helpers, drop unused provider field - Backups now go to /infra-backup//, so the configs and certificates of different management servers never overwrite each other and the retention count applies per server. The cluster-wide GlobalLock still serialises runs. - backupDatabases() and backupDirectoryIfPresent() replace the numbered inline steps; the include-database rationale is javadoc. - InfrastructureBackupTask no longer takes an unused NASBackupProvider. Signed-off-by: James Peru --- .../backup/InfrastructureBackupTask.java | 120 ++++++++++-------- .../cloudstack/backup/NASBackupProvider.java | 2 +- .../backup/InfrastructureBackupTaskTest.java | 43 ++++++- 3 files changed, 106 insertions(+), 59 deletions(-) diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java index 9c66f23c97c6..3f6c5cbb55f0 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -20,6 +20,7 @@ import org.apache.cloudstack.managed.context.ManagedContextRunnable; import org.apache.cloudstack.poll.BackgroundPollTask; +import org.apache.cloudstack.utils.identity.ManagementServerNode; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; @@ -32,6 +33,8 @@ import java.io.InputStream; import java.io.OutputStream; import java.io.Writer; +import java.net.InetAddress; +import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; import java.nio.file.FileVisitResult; import java.nio.file.Files; @@ -73,12 +76,6 @@ public class InfrastructureBackupTask extends ManagedContextRunnable implements /** 24 hours in milliseconds */ private static final long DAILY_INTERVAL_MS = 86400L * 1000L; - private final NASBackupProvider provider; - - public InfrastructureBackupTask(NASBackupProvider provider) { - this.provider = provider; - } - @Override public Long getDelay() { return DAILY_INTERVAL_MS; @@ -123,7 +120,8 @@ protected void runInContext() { boolean includeUsageDb = isUsageDbIncluded(); String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss")); - String backupDir = nasBackupPath + "/infra-backup/" + timestamp; + String infraBackupRoot = nasBackupPath + "/infra-backup/" + getManagementServerLabel(); + String backupDir = infraBackupRoot + "/" + timestamp; LOG.info("Starting infrastructure backup to {} (database included: {})", backupDir, includeDatabase); @@ -143,49 +141,11 @@ protected void runInContext() { return; } - // 1 & 2. Database backup — opt-in via nas.infra.backup.include.database. - // Production deployments typically run their own mysqldump cron jobs and disable this; - // it exists for small/edge deployments wanting unified DR on the same NAS as VM backups. - if (includeDatabase) { - Properties dbProps = loadDbProperties(); - if (dbProps == null) { - LOG.error("Database backup requested but failed to load properties from {} — skipping DB component", DB_PROPERTIES_PATH); - } else { - String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); - String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); - String dbPassword = dbProps.getProperty("db.cloud.password", ""); - - backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); - - if (includeUsageDb) { - String usageHost = dbProps.getProperty("db.usage.host", dbHost); - String usageUser = dbProps.getProperty("db.usage.username", dbUser); - String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); - backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); - } - } - } else { - LOG.debug("Database backup skipped (nas.infra.backup.include.database=false). " + - "Manage DB backups externally for production deployments."); - } - - // 3. Backup management server configs + backupDatabases(backupDir, timestamp, includeDatabase, includeUsageDb); backupDirectory(MANAGEMENT_CONFIG_PATH, backupDir, "management-config"); - - // 4. Backup agent configs (if present on this host) - File agentDir = new File(AGENT_CONFIG_PATH); - if (agentDir.exists()) { - backupDirectory(AGENT_CONFIG_PATH, backupDir, "agent-config"); - } - - // 5. Backup SSL certificates - File sslDir = new File(SSL_CERT_PATH); - if (sslDir.exists()) { - backupDirectory(SSL_CERT_PATH, backupDir, "ssl-certs"); - } - - // 6. Cleanup old backups based on retention policy - cleanupOldBackups(nasBackupPath, retentionCount); + backupDirectoryIfPresent(AGENT_CONFIG_PATH, backupDir, "agent-config"); + backupDirectoryIfPresent(SSL_CERT_PATH, backupDir, "ssl-certs"); + cleanupOldBackups(infraBackupRoot, retentionCount); LOG.info("Infrastructure backup completed successfully: {}", backupDir); @@ -196,6 +156,63 @@ protected void runInContext() { } } + /** + * Name of the sub-directory that keeps this management server's backups apart from those of the + * other servers in the cluster. Management configs and certificates are per server, so they must + * not overwrite each other, and the retention count applies per server. Uses the host name and + * falls back to the management server id. + */ + protected String getManagementServerLabel() { + String label = null; + try { + label = InetAddress.getLocalHost().getHostName(); + } catch (UnknownHostException e) { + LOG.debug("Could not determine the local host name for the infrastructure backup directory: {}", e.getMessage()); + } + if (label == null || label.isBlank()) { + label = "ms-" + ManagementServerNode.getManagementServerId(); + } + return label.replaceAll("[^A-Za-z0-9._-]", "_"); + } + + /** + * Dumps the cloud database, and the usage database when requested, into {@code backupDir}. + * The database component is opt-in ({@code nas.infra.backup.include.database}): production + * deployments typically run their own mysqldump jobs and leave it off; it exists for small and + * edge deployments that want unified disaster recovery on the same NAS as their VM backups. + */ + protected void backupDatabases(String backupDir, String timestamp, boolean includeDatabase, boolean includeUsageDb) { + if (!includeDatabase) { + LOG.debug("Database backup skipped (nas.infra.backup.include.database=false). " + + "Manage DB backups externally for production deployments."); + return; + } + Properties dbProps = loadDbProperties(); + if (dbProps == null) { + LOG.error("Database backup requested but failed to load properties from {}, skipping DB component", DB_PROPERTIES_PATH); + return; + } + String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); + String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); + String dbPassword = dbProps.getProperty("db.cloud.password", ""); + + backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); + + if (includeUsageDb) { + String usageHost = dbProps.getProperty("db.usage.host", dbHost); + String usageUser = dbProps.getProperty("db.usage.username", dbUser); + String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); + backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); + } + } + + /** Archives {@code sourcePath} like {@link #backupDirectory} but silently skips it when it does not exist on this server. */ + protected void backupDirectoryIfPresent(String sourcePath, String backupDir, String archiveName) { + if (new File(sourcePath).exists()) { + backupDirectory(sourcePath, backupDir, archiveName); + } + } + /** * Acquire the cluster-wide run lock so only one management server performs the infrastructure * backup at a time. Returns null if the lock can't be taken (another MS holds it). Overridable @@ -345,13 +362,14 @@ protected void backupDirectory(String sourcePath, String backupDir, String archi } } - protected void cleanupOldBackups(String nasBackupPath, int retentionCount) { + /** Keeps the newest {@code retentionCount} backups under {@code infraBackupRoot} (this server's directory) and deletes the rest. */ + protected void cleanupOldBackups(String infraBackupRoot, int retentionCount) { // A negative retention (misconfiguration) would make toDelete exceed backups.length below and // throw ArrayIndexOutOfBoundsException; clamp it so we never compute a delete count > available. if (retentionCount < 0) { retentionCount = 0; } - File infraDir = new File(nasBackupPath + "/infra-backup"); + File infraDir = new File(infraBackupRoot); if (!infraDir.exists()) { return; } diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index d8656095b437..fa8e3a162a43 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -245,7 +245,7 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co @Override public boolean configure(String name, Map params) throws ConfigurationException { super.configure(name, params); - backgroundPollManager.submitTask(new InfrastructureBackupTask(this)); + backgroundPollManager.submitTask(new InfrastructureBackupTask()); return true; } diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java index 19ebf9ea288b..54f7451894c3 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java @@ -62,9 +62,10 @@ private static class RecordingTask extends InfrastructureBackupTask { final List directoryBackupNames = new ArrayList<>(); final AtomicInteger retentionCalls = new AtomicInteger(0); - RecordingTask() { - super(null); - } + final List directoryBackupDirs = new ArrayList<>(); + + @Override + protected String getManagementServerLabel() { return "ms-a"; } @Override protected boolean isEnabled() { return enabled; } @@ -91,6 +92,7 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, @Override protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { directoryBackupNames.add(archiveName); + directoryBackupDirs.add(backupDir); } @Override @@ -235,6 +237,33 @@ public void retentionRunsOnEverySuccessfulPass() { Assert.assertEquals(1, task.retentionCalls.get()); } + @Test + public void backupsAreWrittenUnderThisManagementServersDirectory() { + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + task.runInContext(); + Assert.assertFalse(task.directoryBackupDirs.isEmpty()); + for (String dir : task.directoryBackupDirs) { + Assert.assertTrue(dir, dir.startsWith(tmpRoot + "/infra-backup/ms-a/")); + } + } + + @Test + public void retentionIsCountedPerManagementServer() throws IOException { + File msA = new File(infraBackupDir(), "ms-a"); + File msB = new File(infraBackupDir(), "ms-b"); + for (String name : new String[] {"20240101-000000", "20240102-000000", "20240103-000000"}) { + Assert.assertTrue(new File(msA, name).mkdirs()); + } + Assert.assertTrue(new File(msB, "20240101-000000").mkdirs()); + + realTask.cleanupOldBackups(msA.toString(), 2); + + Assert.assertArrayEquals(new String[] {"20240102-000000", "20240103-000000"}, + Arrays.stream(msA.listFiles(File::isDirectory)).map(File::getName).sorted().toArray(String[]::new)); + Assert.assertEquals(1, msB.listFiles(File::isDirectory).length); + } + @Test public void dailyIntervalIs24Hours() { InfrastructureBackupTask task = new RecordingTask(); @@ -244,7 +273,7 @@ public void dailyIntervalIs24Hours() { // ---- retention / delete logic (exercises the REAL cleanupOldBackups + deleteDirectory, // which RecordingTask above stubs out) ---- - private final InfrastructureBackupTask realTask = new InfrastructureBackupTask(null); + private final InfrastructureBackupTask realTask = new InfrastructureBackupTask(); private File infraBackupDir() { return new File(tmpRoot.toFile(), "infra-backup"); @@ -274,7 +303,7 @@ public void cleanupClampsNegativeRetentionInsteadOfThrowing() throws IOException // A negative retention (misconfiguration) previously made toDelete (= count - retention) // larger than the array length and threw ArrayIndexOutOfBoundsException. It must now clamp // to 0 and simply remove everything, without throwing. - realTask.cleanupOldBackups(tmpRoot.toString(), -5); + realTask.cleanupOldBackups(infraBackupDir().toString(), -5); Assert.assertEquals(0, remainingBackups().length); } @@ -287,7 +316,7 @@ public void cleanupKeepsNewestAndDeletesOldest() throws IOException { makeBackup("20240104-000000"); makeBackup("20240105-000000"); - realTask.cleanupOldBackups(tmpRoot.toString(), 2); + realTask.cleanupOldBackups(infraBackupDir().toString(), 2); Assert.assertArrayEquals(new String[] {"20240104-000000", "20240105-000000"}, remainingBackups()); } @@ -312,7 +341,7 @@ public void deleteDoesNotFollowSymlinkOutOfTheBackupTree() throws IOException { } // Retain only the newest; the two oldest (including the one holding the symlink) are deleted. - realTask.cleanupOldBackups(tmpRoot.toString(), 1); + realTask.cleanupOldBackups(infraBackupDir().toString(), 1); Assert.assertArrayEquals(new String[] {"20240103-000000"}, remainingBackups()); // The symlink target and its contents MUST survive — the delete must not follow the link out. From b1baa57bcb64877a312f612980025e966cd1a124 Mon Sep 17 00:00:00 2001 From: James Peru Date: Tue, 6 Oct 2026 17:56:49 +0300 Subject: [PATCH 5/6] address review (#12900): read DB credentials through DbProperties, actionable error for an unwritable backup location - loadDbProperties() now uses DbProperties.getDbProperties(), which decrypts ENC(...) values exactly as the management server does for its own pool; the raw file read handed mysqldump the encrypted string (Access denied, exit 2) - a backup directory that cannot be created now logs which user the management server runs as, that it needs write access to nas.infra.backup.location, and a chown example; the location setting's description states the requirement - tests: encrypted password reaches mysqldump decrypted, unreadable db.properties skips only the DB component, unwritable location stops the run --- .../backup/InfrastructureBackupTask.java | 48 ++++++++---- .../cloudstack/backup/NASBackupProvider.java | 4 +- .../backup/InfrastructureBackupTaskTest.java | 77 ++++++++++++++++++- 3 files changed, 110 insertions(+), 19 deletions(-) diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java index 3f6c5cbb55f0..e19325df61e8 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.backup; +import com.cloud.utils.db.DbProperties; import com.cloud.utils.db.GlobalLock; import org.apache.cloudstack.managed.context.ManagedContextRunnable; @@ -24,11 +25,9 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.LogManager; -import java.io.BufferedReader; import java.io.File; import java.io.FileInputStream; import java.io.FileOutputStream; -import java.io.FileReader; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -60,7 +59,8 @@ *
  • SSL certificates and keystores
    • * * - * Database credentials are read from /etc/cloudstack/management/db.properties. + * Database credentials come from the management server's db.properties (via {@link DbProperties}, + * which decrypts ENC(...) values when db.properties encryption is in use). * Backups are stored under {nasBackupPath}/infra-backup/{timestamp}/ with * automatic retention management. */ @@ -68,7 +68,6 @@ public class InfrastructureBackupTask extends ManagedContextRunnable implements private static final Logger LOG = LogManager.getLogger(InfrastructureBackupTask.class); - private static final String DB_PROPERTIES_PATH = "/etc/cloudstack/management/db.properties"; private static final String MANAGEMENT_CONFIG_PATH = "/etc/cloudstack/management"; private static final String AGENT_CONFIG_PATH = "/etc/cloudstack/agent"; private static final String SSL_CERT_PATH = "/etc/cloudstack/management/cert"; @@ -137,7 +136,7 @@ protected void runInContext() { try { File dir = new File(backupDir); if (!dir.exists() && !dir.mkdirs()) { - LOG.error("Failed to create backup directory: {}", backupDir); + LOG.error(describeDirectoryCreationFailure(backupDir, nasBackupPath)); return; } @@ -156,6 +155,19 @@ protected void runInContext() { } } + /** + * Error text for a backup directory that could not be created. The usual cause is that the location + * (typically a NAS mount) is owned by root while the management server runs as an unprivileged user, + * so the message names that user and the fix. + */ + protected String describeDirectoryCreationFailure(String backupDir, String location) { + String user = System.getProperty("user.name", "cloud"); + return String.format("Failed to create infrastructure backup directory %s. The management server runs as user '%s', " + + "which needs write access to %s (nas.infra.backup.location); for example, run 'chown %s %s' on the management " + + "server after mounting the NAS there. Also check that the NAS is mounted at that path.", + backupDir, user, location, user, location); + } + /** * Name of the sub-directory that keeps this management server's backups apart from those of the * other servers in the cluster. Management configs and certificates are per server, so they must @@ -189,7 +201,7 @@ protected void backupDatabases(String backupDir, String timestamp, boolean inclu } Properties dbProps = loadDbProperties(); if (dbProps == null) { - LOG.error("Database backup requested but failed to load properties from {}, skipping DB component", DB_PROPERTIES_PATH); + LOG.error("Database backup requested but the management server's db.properties could not be loaded, skipping DB component"); return; } String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); @@ -229,19 +241,21 @@ protected void releaseRunLock(GlobalLock lock) { } } + /** + * The management server's database properties, already decrypted: when db.properties holds ENC(...) + * values, reading the file directly would hand mysqldump the encrypted string and fail with + * "Access denied". {@link DbProperties} applies the same decryption the server uses for its own pool. + */ protected Properties loadDbProperties() { - File propsFile = new File(DB_PROPERTIES_PATH); - if (!propsFile.exists()) { - LOG.warn("Database properties file not found: {}", DB_PROPERTIES_PATH); - return null; - } - - Properties props = new Properties(); - try (BufferedReader reader = new BufferedReader(new FileReader(propsFile))) { - props.load(reader); + try { + Properties props = DbProperties.getDbProperties(); + if (props == null || props.isEmpty()) { + LOG.warn("The management server's db.properties is empty or could not be found"); + return null; + } return props; - } catch (IOException e) { - LOG.error("Failed to read database properties: {}", e.getMessage()); + } catch (RuntimeException e) { + LOG.error("Failed to load database properties: {}", e.getMessage()); return null; } } diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index fa8e3a162a43..0b46c27d5cff 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -160,7 +160,9 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co "nas.infra.backup.location", "", "NAS mount path where infrastructure backups are stored (e.g. /mnt/nas-backup). " + - "Backups will be written to {location}/infra-backup/{timestamp}/.", + "Backups will be written to {location}/infra-backup/{management-server-host}/{timestamp}/. " + + "The path must be writable by the user the management server runs as (normally 'cloud'), " + + "e.g. chown cloud:cloud on the mounted path; a root-owned mount makes every run fail.", true, ConfigKey.Scope.Global, BackupFrameworkEnabled.key()); diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java index 54f7451894c3..82cc4b1d3b05 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java @@ -16,6 +16,8 @@ // under the License. package org.apache.cloudstack.backup; +import com.cloud.utils.crypt.EncryptionSecretKeyChecker; +import com.cloud.utils.db.DbProperties; import com.cloud.utils.db.GlobalLock; import java.io.File; @@ -33,6 +35,8 @@ import org.junit.Assume; import org.junit.Before; import org.junit.Test; +import org.mockito.MockedStatic; +import org.mockito.Mockito; public class InfrastructureBackupTaskTest { @@ -58,6 +62,8 @@ private static class RecordingTask extends InfrastructureBackupTask { boolean databaseIncluded = false; boolean usageDbIncluded = true; Properties dbProps; + boolean useRealDbPropertiesLoader = false; + final List databaseBackupPasswords = new ArrayList<>(); final List databaseBackupNames = new ArrayList<>(); final List directoryBackupNames = new ArrayList<>(); final AtomicInteger retentionCalls = new AtomicInteger(0); @@ -80,13 +86,14 @@ private static class RecordingTask extends InfrastructureBackupTask { @Override protected Properties loadDbProperties() { - return dbProps; + return useRealDbPropertiesLoader ? super.loadDbProperties() : dbProps; } @Override protected void backupDatabase(String dbName, String backupDir, String timestamp, String dbHost, String dbUser, String dbPassword) { databaseBackupNames.add(dbName); + databaseBackupPasswords.add(dbPassword); } @Override @@ -349,6 +356,74 @@ public void deleteDoesNotFollowSymlinkOutOfTheBackupTree() throws IOException { Assert.assertTrue("file behind the symlink must survive", precious.exists()); } + @Test + public void encryptedDbPasswordIsDecryptedBeforeMysqldump() { + // db.properties with db.cloud.encryption.type=file: the password is stored as ENC(...). + EncryptionSecretKeyChecker.initEncryptor("unit-test-key"); + try { + Properties onDisk = stubDbProps(); + onDisk.setProperty("db.cloud.password", "ENC(" + EncryptionSecretKeyChecker.getEncryptor().encrypt("s3cret") + ")"); + Properties loadedByServer = new Properties(); + loadedByServer.putAll(onDisk); + EncryptionSecretKeyChecker.decryptAnyProperties(loadedByServer); + + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.usageDbIncluded = true; + task.useRealDbPropertiesLoader = true; + try (MockedStatic dbProperties = Mockito.mockStatic(DbProperties.class)) { + dbProperties.when(DbProperties::getDbProperties).thenReturn(loadedByServer); + task.runInContext(); + } + + Assert.assertEquals(Arrays.asList("cloud", "cloud_usage"), task.databaseBackupNames); + Assert.assertEquals("mysqldump must get the decrypted password, not the ENC(...) value", + Arrays.asList("s3cret", "s3cret"), task.databaseBackupPasswords); + } finally { + EncryptionSecretKeyChecker.resetEncryptor(); + } + } + + @Test + public void missingDbPropertiesSkipsOnlyTheDatabase() { + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.useRealDbPropertiesLoader = true; + try (MockedStatic dbProperties = Mockito.mockStatic(DbProperties.class)) { + dbProperties.when(DbProperties::getDbProperties).thenThrow(new IllegalStateException("Failed to load db.properties")); + task.runInContext(); + } + + Assert.assertTrue(task.databaseBackupNames.isEmpty()); + Assert.assertEquals("configs, certs and retention still run", 1, task.retentionCalls.get()); + } + + @Test + public void unwritableLocationStopsTheRunWithAnActionableMessage() { + Assume.assumeFalse("root can write anywhere", "root".equals(System.getProperty("user.name"))); + File readOnly = tmpRoot.toFile(); + Assert.assertTrue(readOnly.setWritable(false)); + try { + RecordingTask task = new RecordingTask(); + task.location = readOnly.getAbsolutePath(); + + task.runInContext(); + + Assert.assertTrue("nothing is archived when the directory cannot be created", task.directoryBackupNames.isEmpty()); + Assert.assertEquals(0, task.retentionCalls.get()); + } finally { + readOnly.setWritable(true); + } + + String message = new InfrastructureBackupTask().describeDirectoryCreationFailure("/mnt/nas-backup/infra-backup/ms-a/20260909-130348", "/mnt/nas-backup"); + String user = System.getProperty("user.name"); + Assert.assertTrue(message, message.contains("runs as user '" + user + "'")); + Assert.assertTrue(message, message.contains("write access to /mnt/nas-backup")); + Assert.assertTrue(message, message.contains("chown " + user + " /mnt/nas-backup")); + } + private void deleteRecursively(File f) { File[] kids = f.listFiles(); if (kids != null) { From b99a0c25e93d3dfc12d571947d6e38326eab5f34 Mon Sep 17 00:00:00 2001 From: James Peru Date: Wed, 21 Oct 2026 08:55:54 +0300 Subject: [PATCH 6/6] address review (#12900): only complete backups count for retention, owner-only backup files - every step now reports success; a run where a requested DB dump or any archive failed is removed and never triggers retention, so a week of failures can no longer delete the good backups - a complete run writes a BACKUP_COMPLETE marker last, and retention counts only marked sets; unmarked sets (crash leftovers, or backups from before this change) are removed only once a full window of complete sets exists and only when older than the oldest kept one - backup directories are created 0700 and files (dumps, archives, marker) 0600 at creation time, so there is no window where they are readable by others; existing infra-backup directories are tightened to 0700, and a file system without POSIX permissions logs a warning - tests: failed dump or archive skips retention and removes the partial set, a week of failed runs keeps every good backup, retention ignores incomplete sets, owner-only modes on directories, marker and config archive Signed-off-by: James Peru --- .../backup/InfrastructureBackupTask.java | 217 +++++++++++++---- .../backup/InfrastructureBackupTaskTest.java | 219 +++++++++++++++++- 2 files changed, 389 insertions(+), 47 deletions(-) diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java index e19325df61e8..969673299f2b 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/InfrastructureBackupTask.java @@ -38,15 +38,21 @@ import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.time.LocalDateTime; import java.time.format.DateTimeFormatter; import java.util.Arrays; import java.util.Comparator; import java.util.EnumSet; +import java.util.List; import java.util.Properties; +import java.util.Set; +import java.util.stream.Collectors; import java.util.concurrent.TimeUnit; import java.util.zip.GZIPOutputStream; @@ -61,8 +67,12 @@ * * Database credentials come from the management server's db.properties (via {@link DbProperties}, * which decrypts ENC(...) values when db.properties encryption is in use). - * Backups are stored under {nasBackupPath}/infra-backup/{timestamp}/ with - * automatic retention management. + * Backups are stored under {nasBackupPath}/infra-backup/{ms}/{timestamp}/ with + * automatic retention management. A backup set only counts once every requested component succeeded, + * which is recorded by a {@value #COMPLETION_MARKER} file; a failed run is removed and never triggers + * retention, so failures cannot push good backups out. Everything is created owner-only (directories + * 0700, files 0600) because the archives hold db.properties and the encryption key, and the NAS is + * usually mounted on the hosts as well. */ public class InfrastructureBackupTask extends ManagedContextRunnable implements BackgroundPollTask { @@ -72,6 +82,12 @@ public class InfrastructureBackupTask extends ManagedContextRunnable implements private static final String AGENT_CONFIG_PATH = "/etc/cloudstack/agent"; private static final String SSL_CERT_PATH = "/etc/cloudstack/management/cert"; + /** Written last into a backup set, only when every requested component succeeded. */ + protected static final String COMPLETION_MARKER = "BACKUP_COMPLETE"; + + private static final Set OWNER_ONLY_DIR = PosixFilePermissions.fromString("rwx------"); + private static final Set OWNER_ONLY_FILE = PosixFilePermissions.fromString("rw-------"); + /** 24 hours in milliseconds */ private static final long DAILY_INTERVAL_MS = 86400L * 1000L; @@ -133,28 +149,117 @@ protected void runInContext() { return; } + File dir = new File(backupDir); + boolean complete = false; try { - File dir = new File(backupDir); - if (!dir.exists() && !dir.mkdirs()) { - LOG.error(describeDirectoryCreationFailure(backupDir, nasBackupPath)); + try { + createBackupDirectories(nasBackupPath, infraBackupRoot, backupDir); + } catch (IOException e) { + LOG.error("{} ({})", describeDirectoryCreationFailure(backupDir, nasBackupPath), e.toString()); return; } - backupDatabases(backupDir, timestamp, includeDatabase, includeUsageDb); - backupDirectory(MANAGEMENT_CONFIG_PATH, backupDir, "management-config"); - backupDirectoryIfPresent(AGENT_CONFIG_PATH, backupDir, "agent-config"); - backupDirectoryIfPresent(SSL_CERT_PATH, backupDir, "ssl-certs"); - cleanupOldBackups(infraBackupRoot, retentionCount); - - LOG.info("Infrastructure backup completed successfully: {}", backupDir); - + // Run every step even after a failure so one run reports all problems. + boolean ok = backupDatabases(backupDir, timestamp, includeDatabase, includeUsageDb); + ok &= backupDirectory(MANAGEMENT_CONFIG_PATH, backupDir, "management-config"); + ok &= backupDirectoryIfPresent(AGENT_CONFIG_PATH, backupDir, "agent-config"); + ok &= backupDirectoryIfPresent(SSL_CERT_PATH, backupDir, "ssl-certs"); + complete = ok && writeCompletionMarker(backupDir, timestamp); + + if (complete) { + // Retention only after a complete set exists, and it only counts complete sets. + cleanupOldBackups(infraBackupRoot, retentionCount); + LOG.info("Infrastructure backup completed successfully: {}", backupDir); + } } catch (Exception e) { LOG.error("Infrastructure backup failed: {}", e.getMessage(), e); } finally { + if (!complete && dir.exists()) { + LOG.error("Infrastructure backup {} is incomplete; removing it and skipping retention so existing backups are kept", backupDir); + deleteDirectory(dir); + } releaseRunLock(lock); } } + /** + * Creates {@code infra-backup//} under the configured location, each level owner-only + * (0700). The permissions are applied at creation, so there is no moment where the directory is open + * to other users. Existing {@code infra-backup} and per-server directories, for example from an older + * version, are tightened to 0700 as well, which also closes access to backups written before. + */ + protected void createBackupDirectories(String location, String infraBackupRoot, String backupDir) throws IOException { + Path locationPath = Paths.get(location); + if (!Files.isDirectory(locationPath)) { + Files.createDirectories(locationPath); + } + createOwnerOnlyDirectory(Paths.get(infraBackupRoot).getParent()); + createOwnerOnlyDirectory(Paths.get(infraBackupRoot)); + createOwnerOnlyDirectory(Paths.get(backupDir)); + } + + /** Creates {@code dir} with mode 0700, or restricts it to 0700 when it already exists. */ + protected void createOwnerOnlyDirectory(Path dir) throws IOException { + if (Files.isDirectory(dir)) { + restrictToOwner(dir, OWNER_ONLY_DIR); + return; + } + try { + Files.createDirectory(dir, ownerOnly(OWNER_ONLY_DIR)); + } catch (UnsupportedOperationException e) { + Files.createDirectory(dir); + warnNonPosix(dir); + } + } + + /** Creates an empty file with mode 0600 (replacing any existing one), so later writes never widen access. */ + protected Path createOwnerOnlyFile(Path file) throws IOException { + Files.deleteIfExists(file); + try { + return Files.createFile(file, ownerOnly(OWNER_ONLY_FILE)); + } catch (UnsupportedOperationException e) { + Files.createFile(file); + warnNonPosix(file); + return file; + } + } + + private static FileAttribute> ownerOnly(Set perms) { + return PosixFilePermissions.asFileAttribute(perms); + } + + private void restrictToOwner(Path path, Set perms) { + try { + Files.setPosixFilePermissions(path, perms); + } catch (UnsupportedOperationException e) { + warnNonPosix(path); + } catch (IOException e) { + LOG.warn("Could not restrict {} to its owner: {}. Check that only the management server user can read the infrastructure backups.", + path, e.getMessage()); + } + } + + private void warnNonPosix(Path path) { + LOG.warn("The file system of {} does not support POSIX permissions, so the infrastructure backup cannot be made owner-only there. " + + "The backups include db.properties and the encryption key; restrict access to nas.infra.backup.location on the NAS itself.", path); + } + + /** Marks {@code backupDir} as a complete set; only marked sets count towards retention. */ + protected boolean writeCompletionMarker(String backupDir, String timestamp) { + try { + Path marker = createOwnerOnlyFile(Paths.get(backupDir, COMPLETION_MARKER)); + Files.write(marker, (timestamp + "\n").getBytes(StandardCharsets.UTF_8)); + return true; + } catch (IOException e) { + LOG.error("Failed to write the completion marker in {}: {}", backupDir, e.getMessage()); + return false; + } + } + + protected static boolean isCompleteBackup(File backup) { + return new File(backup, COMPLETION_MARKER).isFile(); + } + /** * Error text for a backup directory that could not be created. The usual cause is that the location * (typically a NAS mount) is owned by root while the management server runs as an unprivileged user, @@ -192,37 +297,41 @@ protected String getManagementServerLabel() { * The database component is opt-in ({@code nas.infra.backup.include.database}): production * deployments typically run their own mysqldump jobs and leave it off; it exists for small and * edge deployments that want unified disaster recovery on the same NAS as their VM backups. + * + * @return false when a requested dump could not be made */ - protected void backupDatabases(String backupDir, String timestamp, boolean includeDatabase, boolean includeUsageDb) { + protected boolean backupDatabases(String backupDir, String timestamp, boolean includeDatabase, boolean includeUsageDb) { if (!includeDatabase) { LOG.debug("Database backup skipped (nas.infra.backup.include.database=false). " + "Manage DB backups externally for production deployments."); - return; + return true; } Properties dbProps = loadDbProperties(); if (dbProps == null) { - LOG.error("Database backup requested but the management server's db.properties could not be loaded, skipping DB component"); - return; + LOG.error("Database backup requested but the management server's db.properties could not be loaded"); + return false; } String dbHost = dbProps.getProperty("db.cloud.host", "localhost"); String dbUser = dbProps.getProperty("db.cloud.username", "cloud"); String dbPassword = dbProps.getProperty("db.cloud.password", ""); - backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); + boolean ok = backupDatabase("cloud", backupDir, timestamp, dbHost, dbUser, dbPassword); if (includeUsageDb) { String usageHost = dbProps.getProperty("db.usage.host", dbHost); String usageUser = dbProps.getProperty("db.usage.username", dbUser); String usagePassword = dbProps.getProperty("db.usage.password", dbPassword); - backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); + ok &= backupDatabase("cloud_usage", backupDir, timestamp, usageHost, usageUser, usagePassword); } + return ok; } /** Archives {@code sourcePath} like {@link #backupDirectory} but silently skips it when it does not exist on this server. */ - protected void backupDirectoryIfPresent(String sourcePath, String backupDir, String archiveName) { + protected boolean backupDirectoryIfPresent(String sourcePath, String backupDir, String archiveName) { if (new File(sourcePath).exists()) { - backupDirectory(sourcePath, backupDir, archiveName); + return backupDirectory(sourcePath, backupDir, archiveName); } + return true; } /** @@ -260,7 +369,8 @@ protected Properties loadDbProperties() { } } - protected void backupDatabase(String dbName, String backupDir, String timestamp, + /** Dumps {@code dbName} into an owner-only {@code .sql.gz}; returns false when the dump failed. */ + protected boolean backupDatabase(String dbName, String backupDir, String timestamp, String dbHost, String dbUser, String dbPassword) { String dumpFile = backupDir + "/" + dbName + "-" + timestamp + ".sql.gz"; @@ -272,7 +382,7 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, // values read from db.properties cannot be interpreted as shell metacharacters (no injection). defaultsFile = writeMysqlDefaultsFile(dbHost, dbUser, dbPassword); - rawDump = new File(backupDir, dbName + "-" + timestamp + ".sql"); + rawDump = createOwnerOnlyFile(Paths.get(backupDir, dbName + "-" + timestamp + ".sql")).toFile(); // --single-transaction = InnoDB hot backup (no table locks, consistent snapshot) ProcessBuilder pb = new ProcessBuilder( "mysqldump", @@ -287,15 +397,16 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, if (!completed) { process.destroyForcibly(); LOG.error("Database backup timed out for {}", dbName); - return; + return false; } if (process.exitValue() != 0) { LOG.error("Database backup failed for {} with exit code {}", dbName, process.exitValue()); - return; + return false; } - // Compress in-process (no shell pipeline) into the final .sql.gz. + // Compress in-process (no shell pipeline) into the final .sql.gz, created owner-only first. + createOwnerOnlyFile(Paths.get(dumpFile)); try (InputStream in = new FileInputStream(rawDump); OutputStream out = new GZIPOutputStream(new FileOutputStream(dumpFile))) { in.transferTo(out); @@ -303,12 +414,14 @@ protected void backupDatabase(String dbName, String backupDir, String timestamp, File dump = new File(dumpFile); LOG.info("Database {} backed up: {} ({} bytes)", dbName, dumpFile, dump.length()); + return true; } catch (IOException | InterruptedException e) { LOG.error("Failed to backup database {}: {}", dbName, e.getMessage()); if (e instanceof InterruptedException) { Thread.currentThread().interrupt(); } + return false; } finally { if (rawDump != null && rawDump.exists() && !rawDump.delete()) { LOG.warn("Failed to delete temporary dump file: {}", rawDump.getAbsolutePath()); @@ -342,11 +455,15 @@ private File writeMysqlDefaultsFile(String host, String user, String password) t return tmp.toFile(); } - protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { + /** + * Archives {@code sourcePath} into an owner-only {@code .tar.gz}. Returns false when tar + * failed; a missing source directory is skipped and is not a failure. + */ + protected boolean backupDirectory(String sourcePath, String backupDir, String archiveName) { File source = new File(sourcePath); if (!source.exists() || !source.isDirectory()) { LOG.debug("Directory {} does not exist, skipping", sourcePath); - return; + return true; } String tarFile = backupDir + "/" + archiveName + ".tar.gz"; @@ -354,6 +471,8 @@ protected void backupDirectory(String sourcePath, String backupDir, String archi String.format("tar czf '%s' -C '%s' .", tarFile, sourcePath)}; try { + // tar truncates the existing file and keeps its 0600 mode + createOwnerOnlyFile(Paths.get(tarFile)); ProcessBuilder pb = new ProcessBuilder(cmd); pb.redirectErrorStream(true); Process process = pb.start(); @@ -361,22 +480,30 @@ protected void backupDirectory(String sourcePath, String backupDir, String archi if (completed && process.exitValue() == 0) { LOG.info("Directory {} backed up to {}", sourcePath, tarFile); - } else { - if (!completed) { - process.destroyForcibly(); - } - LOG.warn("Directory backup failed for {} (exit code: {})", - sourcePath, completed ? process.exitValue() : "timeout"); + return true; } + if (!completed) { + process.destroyForcibly(); + } + LOG.error("Directory backup failed for {} (exit code: {})", + sourcePath, completed ? process.exitValue() : "timeout"); + return false; } catch (IOException | InterruptedException e) { LOG.error("Failed to backup directory {}: {}", sourcePath, e.getMessage()); if (e instanceof InterruptedException) { Thread.currentThread().interrupt(); } + return false; } } - /** Keeps the newest {@code retentionCount} backups under {@code infraBackupRoot} (this server's directory) and deletes the rest. */ + /** + * Keeps the newest {@code retentionCount} complete backups under {@code infraBackupRoot} (this server's + * directory) and deletes older complete ones. Sets without the completion marker never count, so failed + * or partial runs cannot push good backups out. Unmarked sets (left by a crash, or written before the + * marker existed) are removed only once a full window of complete sets exists, and only when they are + * older than the oldest complete set that is kept. + */ protected void cleanupOldBackups(String infraBackupRoot, int retentionCount) { // A negative retention (misconfiguration) would make toDelete exceed backups.length below and // throw ArrayIndexOutOfBoundsException; clamp it so we never compute a delete count > available. @@ -389,17 +516,29 @@ protected void cleanupOldBackups(String infraBackupRoot, int retentionCount) { } File[] backups = infraDir.listFiles(File::isDirectory); - if (backups == null || backups.length <= retentionCount) { + if (backups == null) { return; } // Sort by name (timestamp-based), oldest first Arrays.sort(backups, Comparator.comparing(File::getName)); + List complete = Arrays.stream(backups).filter(InfrastructureBackupTask::isCompleteBackup).collect(Collectors.toList()); + if (complete.isEmpty() || complete.size() < retentionCount) { + return; + } - int toDelete = backups.length - retentionCount; + int toDelete = complete.size() - retentionCount; for (int i = 0; i < toDelete; i++) { - LOG.info("Removing old infrastructure backup: {}", backups[i].getName()); - deleteDirectory(backups[i]); + LOG.info("Removing old infrastructure backup: {}", complete.get(i).getName()); + deleteDirectory(complete.get(i)); + } + + String oldestKept = complete.get(Math.min(toDelete, complete.size() - 1)).getName(); + for (File backup : backups) { + if (!complete.contains(backup) && backup.getName().compareTo(oldestKept) < 0) { + LOG.info("Removing incomplete infrastructure backup: {}", backup.getName()); + deleteDirectory(backup); + } } } diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java index 82cc4b1d3b05..a63fb0834c09 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java @@ -24,10 +24,13 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermissions; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Properties; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import org.junit.After; @@ -69,6 +72,8 @@ private static class RecordingTask extends InfrastructureBackupTask { final AtomicInteger retentionCalls = new AtomicInteger(0); final List directoryBackupDirs = new ArrayList<>(); + final Set failingDatabases = new HashSet<>(); + final Set failingArchives = new HashSet<>(); @Override protected String getManagementServerLabel() { return "ms-a"; } @@ -90,16 +95,18 @@ protected Properties loadDbProperties() { } @Override - protected void backupDatabase(String dbName, String backupDir, String timestamp, + protected boolean backupDatabase(String dbName, String backupDir, String timestamp, String dbHost, String dbUser, String dbPassword) { databaseBackupNames.add(dbName); databaseBackupPasswords.add(dbPassword); + return !failingDatabases.contains(dbName); } @Override - protected void backupDirectory(String sourcePath, String backupDir, String archiveName) { + protected boolean backupDirectory(String sourcePath, String backupDir, String archiveName) { directoryBackupNames.add(archiveName); directoryBackupDirs.add(backupDir); + return !failingArchives.contains(archiveName); } @Override @@ -216,7 +223,7 @@ public void usageDbFlagIgnoredWhenDatabaseExcluded() { } @Test - public void skipsDbBackupWhenPropertiesUnreadable() { + public void unreadablePropertiesFailTheRunWhenDatabaseRequested() { RecordingTask task = new RecordingTask(); task.enabled = true; task.location = tmpRoot.toString(); @@ -227,8 +234,9 @@ public void skipsDbBackupWhenPropertiesUnreadable() { Assert.assertTrue("no DB backups attempted when props can't be loaded", task.databaseBackupNames.isEmpty()); - Assert.assertEquals("retention still runs (configs+certs path unaffected)", - 1, task.retentionCalls.get()); + Assert.assertEquals("a run without the requested DB dump must not trigger retention", + 0, task.retentionCalls.get()); + Assert.assertEquals(0, backupSetsOf("ms-a").length); } @Test @@ -260,9 +268,9 @@ public void retentionIsCountedPerManagementServer() throws IOException { File msA = new File(infraBackupDir(), "ms-a"); File msB = new File(infraBackupDir(), "ms-b"); for (String name : new String[] {"20240101-000000", "20240102-000000", "20240103-000000"}) { - Assert.assertTrue(new File(msA, name).mkdirs()); + markComplete(new File(msA, name)); } - Assert.assertTrue(new File(msB, "20240101-000000").mkdirs()); + markComplete(new File(msB, "20240101-000000")); realTask.cleanupOldBackups(msA.toString(), 2); @@ -287,12 +295,206 @@ private File infraBackupDir() { } private File makeBackup(String name) throws IOException { + File dir = makeIncompleteBackup(name); + Assert.assertTrue(new File(dir, InfrastructureBackupTask.COMPLETION_MARKER).createNewFile()); + return dir; + } + + /** A set without the completion marker: a failed run left behind by a crash, or one written before the marker existed. */ + private File makeIncompleteBackup(String name) throws IOException { File dir = new File(infraBackupDir(), name); Assert.assertTrue(dir.mkdirs()); Assert.assertTrue(new File(dir, "dump.sql.gz").createNewFile()); return dir; } + private static void markComplete(File dir) throws IOException { + Assert.assertTrue(dir.mkdirs()); + Assert.assertTrue(new File(dir, InfrastructureBackupTask.COMPLETION_MARKER).createNewFile()); + } + + private File[] backupSetsOf(String ms) { + File[] sets = new File(infraBackupDir(), ms).listFiles(File::isDirectory); + return sets == null ? new File[0] : sets; + } + + private static String mode(File f) throws IOException { + return PosixFilePermissions.toString(Files.getPosixFilePermissions(f.toPath())); + } + + private void assumePosix() { + Assume.assumeTrue("needs a POSIX file system", + tmpRoot.getFileSystem().supportedFileAttributeViews().contains("posix")); + } + + // ---- review: a failed run must never count, and never trigger retention ---- + + @Test + public void successfulRunMarksTheSetComplete() { + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + + task.runInContext(); + + File[] sets = backupSetsOf("ms-a"); + Assert.assertEquals(1, sets.length); + Assert.assertTrue(InfrastructureBackupTask.isCompleteBackup(sets[0])); + Assert.assertEquals(1, task.retentionCalls.get()); + } + + @Test + public void failedDatabaseDumpSkipsRetentionAndRemovesThePartialSet() { + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + task.databaseIncluded = true; + task.dbProps = stubDbProps(); + task.failingDatabases.add("cloud_usage"); + + task.runInContext(); + + Assert.assertEquals(0, task.retentionCalls.get()); + Assert.assertEquals("the partial set is removed", 0, backupSetsOf("ms-a").length); + } + + @Test + public void failedArchiveSkipsRetentionAndRemovesThePartialSet() { + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + task.failingArchives.add("management-config"); + + task.runInContext(); + + Assert.assertEquals(0, task.retentionCalls.get()); + Assert.assertEquals(0, backupSetsOf("ms-a").length); + } + + @Test + public void aWeekOfFailedRunsKeepsEveryGoodBackup() throws IOException { + File msA = new File(infraBackupDir(), "ms-a"); + for (String name : new String[] {"20240101-000000", "20240102-000000", "20240103-000000"}) { + markComplete(new File(msA, name)); + } + // real retention this time, so the test sees what a failed run would delete + RecordingTask task = new RecordingTask() { + @Override + protected void cleanupOldBackups(String root, int retentionCount) { + retentionCalls.incrementAndGet(); + realTask.cleanupOldBackups(root, retentionCount); + } + }; + task.location = tmpRoot.toString(); + task.retention = 3; + task.failingArchives.add("management-config"); + + for (int day = 0; day < 7; day++) { + task.runInContext(); + } + + Assert.assertEquals(0, task.retentionCalls.get()); + Assert.assertEquals(3, backupSetsOf("ms-a").length); + } + + @Test + public void cleanupCountsOnlyCompleteSets() throws IOException { + makeBackup("20240101-000000"); + makeBackup("20240102-000000"); + makeBackup("20240103-000000"); + makeIncompleteBackup("20240104-000000"); + makeIncompleteBackup("20240105-000000"); + makeIncompleteBackup("20240106-000000"); + + realTask.cleanupOldBackups(infraBackupDir().toString(), 3); + + Assert.assertEquals("incomplete sets do not push complete ones out", 6, remainingBackups().length); + } + + @Test + public void cleanupRemovesIncompleteSetsOlderThanTheKeptWindow() throws IOException { + makeIncompleteBackup("20240101-000000"); + makeBackup("20240102-000000"); + makeIncompleteBackup("20240103-000000"); + makeBackup("20240104-000000"); + makeBackup("20240105-000000"); + makeIncompleteBackup("20240106-000000"); + + realTask.cleanupOldBackups(infraBackupDir().toString(), 2); + + Assert.assertArrayEquals(new String[] {"20240104-000000", "20240105-000000", "20240106-000000"}, remainingBackups()); + } + + @Test + public void unmarkedSetsFromBeforeTheUpgradeStayUntilAFullWindowIsComplete() throws IOException { + for (int day = 1; day <= 7; day++) { + makeIncompleteBackup("2024010" + day + "-000000"); + } + makeBackup("20240108-000000"); + + realTask.cleanupOldBackups(infraBackupDir().toString(), 7); + + Assert.assertEquals(8, remainingBackups().length); + } + + // ---- review: backups hold db.properties and the key, so they are owner-only ---- + + @Test + public void backupDirectoriesAndMarkerAreOwnerOnly() throws IOException { + assumePosix(); + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + + task.runInContext(); + + File set = backupSetsOf("ms-a")[0]; + Assert.assertEquals("rwx------", mode(infraBackupDir())); + Assert.assertEquals("rwx------", mode(new File(infraBackupDir(), "ms-a"))); + Assert.assertEquals("rwx------", mode(set)); + Assert.assertEquals("rw-------", mode(new File(set, InfrastructureBackupTask.COMPLETION_MARKER))); + } + + @Test + public void existingBackupDirectoriesAreTightened() throws IOException { + assumePosix(); + File msA = new File(infraBackupDir(), "ms-a"); + Assert.assertTrue(msA.mkdirs()); + Files.setPosixFilePermissions(infraBackupDir().toPath(), PosixFilePermissions.fromString("rwxr-xr-x")); + Files.setPosixFilePermissions(msA.toPath(), PosixFilePermissions.fromString("rwxr-xr-x")); + RecordingTask task = new RecordingTask(); + task.location = tmpRoot.toString(); + + task.runInContext(); + + Assert.assertEquals("rwx------", mode(infraBackupDir())); + Assert.assertEquals("rwx------", mode(msA)); + } + + @Test + public void configArchiveIsOwnerOnly() throws IOException { + assumePosix(); + File source = new File(tmpRoot.toFile(), "management"); + Assert.assertTrue(source.mkdirs()); + Files.write(new File(source, "db.properties").toPath(), "db.cloud.password=secret\n".getBytes()); + File set = new File(tmpRoot.toFile(), "set"); + Assert.assertTrue(set.mkdirs()); + + Assert.assertTrue(realTask.backupDirectory(source.getPath(), set.getPath(), "management-config")); + + File archive = new File(set, "management-config.tar.gz"); + Assert.assertTrue(archive.length() > 0); + Assert.assertEquals("rw-------", mode(archive)); + } + + @Test + public void ownerOnlyFileIsCreatedWithoutAWiderWindow() throws IOException { + assumePosix(); + File file = new File(tmpRoot.toFile(), "cloud-20240101-000000.sql.gz"); + Assert.assertTrue(file.createNewFile()); + Files.setPosixFilePermissions(file.toPath(), PosixFilePermissions.fromString("rw-r--r--")); + + realTask.createOwnerOnlyFile(file.toPath()); + + Assert.assertEquals("rw-------", mode(file)); + } + private String[] remainingBackups() { File[] dirs = infraBackupDir().listFiles(File::isDirectory); if (dirs == null) { @@ -397,7 +599,8 @@ public void missingDbPropertiesSkipsOnlyTheDatabase() { } Assert.assertTrue(task.databaseBackupNames.isEmpty()); - Assert.assertEquals("configs, certs and retention still run", 1, task.retentionCalls.get()); + Assert.assertFalse("configs are still attempted so the log shows every problem", task.directoryBackupNames.isEmpty()); + Assert.assertEquals("retention is skipped", 0, task.retentionCalls.get()); } @Test