address review (#12900): per-management-server backup directory, extract helpers, drop unused provider field

- Backups now go to <location>/infra-backup/<ms-hostname>/<timestamp>, 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 <jmsperu@gmail.com>

Author: jmsperu

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;
         }

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

@@ -190,7 +190,7 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co
     @Override
     public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
         super.configure(name, params);
-        backgroundPollManager.submitTask(new InfrastructureBackupTask(this));
+        backgroundPollManager.submitTask(new InfrastructureBackupTask());
         return true;
     }
 

plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/InfrastructureBackupTaskTest.java

@@ -62,9 +62,10 @@ private static class RecordingTask extends InfrastructureBackupTask {
         final List<String> directoryBackupNames = new ArrayList<>();
         final AtomicInteger retentionCalls = new AtomicInteger(0);
 
-        RecordingTask() {
-            super(null);
-        }
+        final List<String> 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.