From 6a31cacba5556ba29275bad12dcaffb7b474baac Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Thu, 28 Nov 2024 13:05:31 +0530 Subject: [PATCH] chore: Modular backup implementation (#37715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backup implementation changed to be modular, and made up of separate pieces. Each piece (link in a chain) is responsible for one component/functionality of all the data that's being backed up. This PR introduces the framework for this modularization. The next PR will finish migration to that architecture. ## Automation /test sanity ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 75c1d787c874ff1dd398b7e7228d062a2c66c141 > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Thu, 28 Nov 2024 07:23:30 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a modular backup system with `BackupState`, `DiskSpaceLink`, `EncryptionLink`, and `ManifestLink` classes to manage backup operations more efficiently. - Added command-line argument support for the backup command, enhancing flexibility. - Improved user interaction during backup restoration with prompts for encryption passwords. - **Bug Fixes** - Enhanced error handling and clarity in the restoration process, particularly for manifest file reading. - **Documentation** - Updated test structure to reflect new directory organization and improved focus on backup cleanup logic. --- .../rts/src/ctl/backup/BackupState.ts | 25 +++ .../rts/src/ctl/{ => backup}/backup.test.ts | 90 ++++------ .../src/ctl/{backup.ts => backup/index.ts} | 159 ++++++------------ .../rts/src/ctl/backup/links/DiskSpaceLink.ts | 11 ++ .../src/ctl/backup/links/EncryptionLink.ts | 36 ++++ .../rts/src/ctl/backup/links/ManifestLink.ts | 22 +++ .../rts/src/ctl/backup/links/index.ts | 13 ++ app/client/packages/rts/src/ctl/index.ts | 2 +- app/client/packages/rts/src/ctl/restore.ts | 57 +++---- deploy/docker/fs/opt/bin/ctl | 5 +- 10 files changed, 232 insertions(+), 188 deletions(-) create mode 100644 app/client/packages/rts/src/ctl/backup/BackupState.ts rename app/client/packages/rts/src/ctl/{ => backup}/backup.test.ts (79%) rename app/client/packages/rts/src/ctl/{backup.ts => backup/index.ts} (70%) create mode 100644 app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts create mode 100644 app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts create mode 100644 app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts create mode 100644 app/client/packages/rts/src/ctl/backup/links/index.ts diff --git a/app/client/packages/rts/src/ctl/backup/BackupState.ts b/app/client/packages/rts/src/ctl/backup/BackupState.ts new file mode 100644 index 0000000000..182756ab29 --- /dev/null +++ b/app/client/packages/rts/src/ctl/backup/BackupState.ts @@ -0,0 +1,25 @@ +import { getTimeStampInISO } from "./index"; + +export class BackupState { + readonly args: string[]; + readonly initAt: string = getTimeStampInISO(); + readonly errors: string[] = []; + + backupRootPath: string = ""; + archivePath: string = ""; + + encryptionPassword: string = ""; + + constructor(args: string[]) { + this.args = args; + + // We seal `this` so that no link in the chain can "add" new properties to the state. This is intentional. If any + // link wants to save data in the `BackupState`, which shouldn't even be needed in most cases, it should do so by + // explicitly declaring a property in this class. No surprises. + Object.seal(this); + } + + isEncryptionEnabled() { + return !!this.encryptionPassword; + } +} diff --git a/app/client/packages/rts/src/ctl/backup.test.ts b/app/client/packages/rts/src/ctl/backup/backup.test.ts similarity index 79% rename from app/client/packages/rts/src/ctl/backup.test.ts rename to app/client/packages/rts/src/ctl/backup/backup.test.ts index 4dddd337b5..fd4d12b730 100644 --- a/app/client/packages/rts/src/ctl/backup.test.ts +++ b/app/client/packages/rts/src/ctl/backup/backup.test.ts @@ -1,15 +1,14 @@ -jest.mock("./utils", () => ({ - ...jest.requireActual("./utils"), +import fsPromises from "fs/promises"; +import * as backup from "."; +import * as Constants from "../constants"; +import * as utils from "../utils"; +import readlineSync from "readline-sync"; + +jest.mock("../utils", () => ({ + ...jest.requireActual("../utils"), execCommand: jest.fn().mockImplementation(async (a) => a.join(" ")), })); -import * as backup from "./backup"; -import * as Constants from "./constants"; -import os from "os"; -import fsPromises from "fs/promises"; -import * as utils from "./utils"; -import readlineSync from "readline-sync"; - describe("Backup Tests", () => { test("Timestamp string in ISO format", () => { console.log(backup.getTimeStampInISO()); @@ -46,14 +45,6 @@ describe("Backup Tests", () => { ); }); - it("Generates t", async () => { - os.tmpdir = jest.fn().mockReturnValue("temp/dir"); - fsPromises.mkdtemp = jest.fn().mockImplementation((a) => a); - const res = await backup.generateBackupRootPath(); - - expect(res).toBe("temp/dir/appsmithctl-backup-"); - }); - test("Test backup contents path generation", () => { const root = "/rootDir"; const timestamp = "0000-00-0T00-00-00.00Z"; @@ -136,67 +127,60 @@ describe("Backup Tests", () => { }); test("Cleanup Backups when limit is 4 and there are 5 files", async () => { - const backupArchivesLimit = 4; - - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); + fsPromises.rm = jest.fn().mockImplementation(); const backupFiles = ["file1", "file2", "file3", "file4", "file5"]; - const expectedBackupFiles = ["file2", "file3", "file4", "file5"]; - const res = await backup.removeOldBackups(backupFiles, backupArchivesLimit); - console.log(res); + await backup.removeOldBackups(backupFiles, 4); - expect(res).toEqual(expectedBackupFiles); + expect(fsPromises.rm).toHaveBeenCalledTimes(1); + expect(fsPromises.rm).toHaveBeenCalledWith( + Constants.BACKUP_PATH + "/file1", + ); }); test("Cleanup Backups when limit is 2 and there are 5 files", async () => { - const backupArchivesLimit = 2; + fsPromises.rm = jest.fn().mockImplementation(); + const backupFiles = ["file1", "file4", "file3", "file2", "file5"]; - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); - const backupFiles = ["file1", "file2", "file3", "file4", "file5"]; - const expectedBackupFiles = ["file4", "file5"]; - const res = await backup.removeOldBackups(backupFiles, backupArchivesLimit); + await backup.removeOldBackups(backupFiles, 2); - console.log(res); - - expect(res).toEqual(expectedBackupFiles); + expect(fsPromises.rm).toHaveBeenCalledTimes(3); + expect(fsPromises.rm).toHaveBeenCalledWith( + Constants.BACKUP_PATH + "/file1", + ); + expect(fsPromises.rm).toHaveBeenCalledWith( + Constants.BACKUP_PATH + "/file2", + ); + expect(fsPromises.rm).toHaveBeenCalledWith( + Constants.BACKUP_PATH + "/file3", + ); }); test("Cleanup Backups when limit is 4 and there are 4 files", async () => { - const backupArchivesLimit = 4; - - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); + fsPromises.rm = jest.fn().mockImplementation(); const backupFiles = ["file1", "file2", "file3", "file4"]; - const expectedBackupFiles = ["file1", "file2", "file3", "file4"]; - const res = await backup.removeOldBackups(backupFiles, backupArchivesLimit); - console.log(res); + await backup.removeOldBackups(backupFiles, 4); - expect(res).toEqual(expectedBackupFiles); + expect(fsPromises.rm).not.toHaveBeenCalled(); }); test("Cleanup Backups when limit is 4 and there are 2 files", async () => { - const backupArchivesLimit = 4; - - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); + fsPromises.rm = jest.fn().mockImplementation(); const backupFiles = ["file1", "file2"]; - const expectedBackupFiles = ["file1", "file2"]; - const res = await backup.removeOldBackups(backupFiles, backupArchivesLimit); - console.log(res); + await backup.removeOldBackups(backupFiles, 4); - expect(res).toEqual(expectedBackupFiles); + expect(fsPromises.rm).not.toHaveBeenCalled(); }); test("Cleanup Backups when limit is 2 and there is 1 file", async () => { - const backupArchivesLimit = 4; - - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); + fsPromises.rm = jest.fn().mockImplementation(); const backupFiles = ["file1"]; - const expectedBackupFiles = ["file1"]; - const res = await backup.removeOldBackups(backupFiles, backupArchivesLimit); - console.log(res); - expect(res).toEqual(expectedBackupFiles); + await backup.removeOldBackups(backupFiles, 4); + + expect(fsPromises.rm).not.toHaveBeenCalled(); }); test("Cleanup Backups when limit is 2 and there is no file", async () => { diff --git a/app/client/packages/rts/src/ctl/backup.ts b/app/client/packages/rts/src/ctl/backup/index.ts similarity index 70% rename from app/client/packages/rts/src/ctl/backup.ts rename to app/client/packages/rts/src/ctl/backup/index.ts index 1ae5661eb1..8fa2c35358 100644 --- a/app/client/packages/rts/src/ctl/backup.ts +++ b/app/client/packages/rts/src/ctl/backup/index.ts @@ -1,64 +1,50 @@ import fsPromises from "fs/promises"; import path from "path"; import os from "os"; -import * as utils from "./utils"; -import * as Constants from "./constants"; -import * as logger from "./logger"; -import * as mailer from "./mailer"; -import tty from "tty"; +import * as utils from "../utils"; +import * as Constants from "../constants"; +import * as logger from "../logger"; +import * as mailer from "../mailer"; import readlineSync from "readline-sync"; +import { DiskSpaceLink } from "./links/DiskSpaceLink"; +import type { Link } from "./links"; +import { EncryptionLink, ManifestLink } from "./links"; +import { BackupState } from "./BackupState"; -const command_args = process.argv.slice(3); - -class BackupState { - readonly initAt: string = getTimeStampInISO(); - readonly errors: string[] = []; - - backupRootPath: string = ""; - archivePath: string = ""; - - encryptionPassword: string = ""; - - isEncryptionEnabled() { - return !!this.encryptionPassword; - } -} - -export async function run() { +export async function run(args: string[]) { await utils.ensureSupervisorIsRunning(); - const state: BackupState = new BackupState(); + const state: BackupState = new BackupState(args); + + const chain: Link[] = [ + new DiskSpaceLink(), + new ManifestLink(state), + new EncryptionLink(state), + ]; try { // PRE-BACKUP - const availSpaceInBytes: number = - await getAvailableBackupSpaceInBytes("/appsmith-stacks"); - - checkAvailableBackupSpace(availSpaceInBytes); - - if ( - !command_args.includes("--non-interactive") && - tty.isatty((process.stdout as any).fd) - ) { - state.encryptionPassword = getEncryptionPasswordFromUser(); + for (const link of chain) { + await link.preBackup?.(); } - state.backupRootPath = await generateBackupRootPath(); - const backupContentsPath: string = getBackupContentsPath( - state.backupRootPath, - state.initAt, + // BACKUP + state.backupRootPath = await fsPromises.mkdtemp( + path.join(os.tmpdir(), "appsmithctl-backup-"), ); - // BACKUP - await fsPromises.mkdir(backupContentsPath); + await exportDatabase(state.backupRootPath); - await exportDatabase(backupContentsPath); + await createGitStorageArchive(state.backupRootPath); - await createGitStorageArchive(backupContentsPath); + await exportDockerEnvFile( + state.backupRootPath, + state.isEncryptionEnabled(), + ); - await createManifestFile(backupContentsPath); - - await exportDockerEnvFile(backupContentsPath, state.isEncryptionEnabled()); + for (const link of chain) { + await link.doBackup?.(); + } state.archivePath = await createFinalArchive( state.backupRootPath, @@ -66,27 +52,13 @@ export async function run() { ); // POST-BACKUP - if (state.isEncryptionEnabled()) { - const encryptedArchivePath = await encryptBackupArchive( - state.archivePath, - state.encryptionPassword, - ); + for (const link of chain) { + await link.postBackup?.(); + } - await logger.backup_info( - "Finished creating an encrypted a backup archive at " + - encryptedArchivePath, - ); + console.log("Post-backup done. Final archive at", state.archivePath); - if (state.archivePath != null) { - await fsPromises.rm(state.archivePath, { - recursive: true, - force: true, - }); - } - } else { - await logger.backup_info( - "Finished creating a backup archive at " + state.archivePath, - ); + if (!state.isEncryptionEnabled()) { console.log( "********************************************************* IMPORTANT!!! *************************************************************", ); @@ -110,7 +82,7 @@ export async function run() { process.exitCode = 1; await logger.backup_error(err.stack); - if (command_args.includes("--error-mail")) { + if (state.args.includes("--error-mail")) { const currentTS = new Date().getTime(); const lastMailTS = await utils.getLastBackupErrorMailSentInMilliSec(); @@ -123,6 +95,14 @@ export async function run() { await utils.updateLastBackupErrorMailSentInMilliSec(currentTS); } } + + // Delete the archive, if exists, since its existence may mislead the user. + if (state.archivePath != null) { + await fsPromises.rm(state.archivePath, { + recursive: true, + force: true, + }); + } } finally { if (state.backupRootPath != null) { await fsPromises.rm(state.backupRootPath, { @@ -131,15 +111,6 @@ export async function run() { }); } - if (state.isEncryptionEnabled()) { - if (state.archivePath != null) { - await fsPromises.rm(state.archivePath, { - recursive: true, - force: true, - }); - } - } - await postBackupCleanup(); process.exit(); } @@ -222,19 +193,6 @@ async function createGitStorageArchive(destFolder: string) { console.log("Created git-storage archive"); } -async function createManifestFile(path: string) { - const version = await utils.getCurrentAppsmithVersion(); - const manifest_data = { - appsmithVersion: version, - dbName: utils.getDatabaseNameFromMongoURI(utils.getDburl()), - }; - - await fsPromises.writeFile( - path + "/manifest.json", - JSON.stringify(manifest_data), - ); -} - async function exportDockerEnvFile( destFolder: string, encryptArchive: boolean, @@ -291,19 +249,15 @@ async function createFinalArchive(destFolder: string, timestamp: string) { } async function postBackupCleanup() { - console.log("Starting the cleanup task after taking a backup."); + console.log("Starting cleanup."); const backupArchivesLimit = getBackupArchiveLimit( parseInt(process.env.APPSMITH_BACKUP_ARCHIVE_LIMIT, 10), ); const backupFiles = await utils.listLocalBackupFiles(); - while (backupFiles.length > backupArchivesLimit) { - const fileName = backupFiles.shift(); + await removeOldBackups(backupFiles, backupArchivesLimit); - await fsPromises.rm(Constants.BACKUP_PATH + "/" + fileName); - } - - console.log("Cleanup task completed."); + console.log("Cleanup completed."); } export async function executeCopyCMD(srcFolder: string, destFolder: string) { @@ -323,10 +277,6 @@ export function getGitRoot(gitRoot?: string | undefined) { return gitRoot; } -export async function generateBackupRootPath() { - return fsPromises.mkdtemp(path.join(os.tmpdir(), "appsmithctl-backup-")); -} - export function getBackupContentsPath( backupRootPath: string, timestamp: string, @@ -359,13 +309,14 @@ export async function removeOldBackups( backupFiles: string[], backupArchivesLimit: number, ) { - while (backupFiles.length > backupArchivesLimit) { - const fileName = backupFiles.shift(); - - await fsPromises.rm(Constants.BACKUP_PATH + "/" + fileName); - } - - return backupFiles; + return Promise.all( + backupFiles + .sort() + .reverse() + .slice(backupArchivesLimit) + .map((file) => Constants.BACKUP_PATH + "/" + file) + .map(async (file) => fsPromises.rm(file)), + ); } export function getTimeStampInISO() { diff --git a/app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts b/app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts new file mode 100644 index 0000000000..8ab16efe7e --- /dev/null +++ b/app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts @@ -0,0 +1,11 @@ +import { checkAvailableBackupSpace, getAvailableBackupSpaceInBytes } from ".."; +import type { Link } from "."; + +export class DiskSpaceLink implements Link { + async preBackup() { + const availSpaceInBytes: number = + await getAvailableBackupSpaceInBytes("/appsmith-stacks"); + + checkAvailableBackupSpace(availSpaceInBytes); + } +} diff --git a/app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts b/app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts new file mode 100644 index 0000000000..086c7fc0c8 --- /dev/null +++ b/app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts @@ -0,0 +1,36 @@ +import type { Link } from "./index"; +import tty from "tty"; +import fsPromises from "fs/promises"; +import { encryptBackupArchive, getEncryptionPasswordFromUser } from "../index"; +import type { BackupState } from "../BackupState"; + +export class EncryptionLink implements Link { + constructor(private readonly state: BackupState) {} + + async preBackup() { + if ( + !this.state.args.includes("--non-interactive") && + tty.isatty((process.stdout as any).fd) + ) { + this.state.encryptionPassword = getEncryptionPasswordFromUser(); + } + } + + async postBackup() { + if (!this.state.isEncryptionEnabled()) { + return; + } + + const unencryptedArchivePath = this.state.archivePath; + + this.state.archivePath = await encryptBackupArchive( + unencryptedArchivePath, + this.state.encryptionPassword, + ); + + await fsPromises.rm(unencryptedArchivePath, { + recursive: true, + force: true, + }); + } +} diff --git a/app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts b/app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts new file mode 100644 index 0000000000..16998203ac --- /dev/null +++ b/app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts @@ -0,0 +1,22 @@ +import type { Link } from "./index"; +import type { BackupState } from "../BackupState"; +import * as utils from "../../utils"; +import fsPromises from "fs/promises"; +import path from "path"; + +export class ManifestLink implements Link { + constructor(private readonly state: BackupState) {} + + async doBackup() { + const version = await utils.getCurrentAppsmithVersion(); + const manifestData = { + appsmithVersion: version, + dbName: utils.getDatabaseNameFromMongoURI(utils.getDburl()), + }; + + await fsPromises.writeFile( + path.join(this.state.backupRootPath, "/manifest.json"), + JSON.stringify(manifestData, null, 2), + ); + } +} diff --git a/app/client/packages/rts/src/ctl/backup/links/index.ts b/app/client/packages/rts/src/ctl/backup/links/index.ts new file mode 100644 index 0000000000..eea6f31bcd --- /dev/null +++ b/app/client/packages/rts/src/ctl/backup/links/index.ts @@ -0,0 +1,13 @@ +export interface Link { + // Called before the backup folder is created. + preBackup?(): Promise; + + // Called after backup folder is created. Expected to copy/create any backup files in the backup folder. + doBackup?(): Promise; + + // Called after backup archive is created. The archive location is available now. + postBackup?(): Promise; +} + +export { EncryptionLink } from "./EncryptionLink"; +export { ManifestLink } from "./ManifestLink"; diff --git a/app/client/packages/rts/src/ctl/index.ts b/app/client/packages/rts/src/ctl/index.ts index 9d25417aac..b333d27cdc 100755 --- a/app/client/packages/rts/src/ctl/index.ts +++ b/app/client/packages/rts/src/ctl/index.ts @@ -51,7 +51,7 @@ if (["export-db", "export_db", "ex"].includes(command)) { ) { check_replica_set.exec(); } else if (["backup"].includes(command)) { - backup.run(); + backup.run(process.argv.slice(3)); } else if (["restore"].includes(command)) { restore.run(); } else if ( diff --git a/app/client/packages/rts/src/ctl/restore.ts b/app/client/packages/rts/src/ctl/restore.ts index 22e386b783..a32d27564c 100644 --- a/app/client/packages/rts/src/ctl/restore.ts +++ b/app/client/packages/rts/src/ctl/restore.ts @@ -61,14 +61,15 @@ async function decryptArchive( encryptedFilePath: string, backupFilePath: string, ) { - console.log("Enter the password to decrypt the backup archive:"); - for (const attempt of [1, 2, 3]) { if (attempt > 1) { console.log("Retry attempt", attempt); } - const decryptionPwd = readlineSync.question("", { hideEchoBack: true }); + const decryptionPwd = readlineSync.question( + "Enter the password to decrypt the backup archive: ", + { hideEchoBack: true }, + ); try { await utils.execCommandSilent([ @@ -150,15 +151,15 @@ async function restoreDockerEnvFile( let encryptionSalt = process.env.APPSMITH_ENCRYPTION_SALT; await utils.execCommand([ - "mv", + "cp", dockerEnvFile, dockerEnvFile + "." + backupName, ]); - await utils.execCommand([ - "cp", + + let dockerEnvContent = await fsPromises.readFile( restoreContentsPath + "/docker.env", - dockerEnvFile, - ]); + "utf8", + ); if (overwriteEncryptionKeys) { if (encryptionPwd && encryptionSalt) { @@ -202,31 +203,29 @@ async function restoreDockerEnvFile( ); } - await fsPromises.appendFile( - dockerEnvFile, + dockerEnvContent += "\nAPPSMITH_ENCRYPTION_PASSWORD=" + - encryptionPwd + - "\nAPPSMITH_ENCRYPTION_SALT=" + - encryptionSalt + - "\nAPPSMITH_DB_URL=" + - utils.getDburl() + - "\nAPPSMITH_MONGODB_USER=" + - process.env.APPSMITH_MONGODB_USER + - "\nAPPSMITH_MONGODB_PASSWORD=" + - process.env.APPSMITH_MONGODB_PASSWORD, - ); - } else { - await fsPromises.appendFile( - dockerEnvFile, + encryptionPwd + + "\nAPPSMITH_ENCRYPTION_SALT=" + + encryptionSalt + "\nAPPSMITH_DB_URL=" + - updatedbUrl + - "\nAPPSMITH_MONGODB_USER=" + - process.env.APPSMITH_MONGODB_USER + - "\nAPPSMITH_MONGODB_PASSWORD=" + - process.env.APPSMITH_MONGODB_PASSWORD, - ); + utils.getDburl() + + "\nAPPSMITH_MONGODB_USER=" + + process.env.APPSMITH_MONGODB_USER + + "\nAPPSMITH_MONGODB_PASSWORD=" + + process.env.APPSMITH_MONGODB_PASSWORD; + } else { + dockerEnvContent += + "\nAPPSMITH_DB_URL=" + + updatedbUrl + + "\nAPPSMITH_MONGODB_USER=" + + process.env.APPSMITH_MONGODB_USER + + "\nAPPSMITH_MONGODB_PASSWORD=" + + process.env.APPSMITH_MONGODB_PASSWORD; } + await fsPromises.writeFile(dockerEnvFile, dockerEnvContent, "utf8"); + console.log("Restoring docker environment file completed"); } diff --git a/deploy/docker/fs/opt/bin/ctl b/deploy/docker/fs/opt/bin/ctl index a6ba5037d4..1d1f3fc83d 100644 --- a/deploy/docker/fs/opt/bin/ctl +++ b/deploy/docker/fs/opt/bin/ctl @@ -3,4 +3,7 @@ # Do NOT change working directory with `cd`, so that the command being run has access to the working directory where # the command was invoked by the user. -exec node /opt/appsmith/rts/bundle/ctl/index.js "$@" +exec node \ + --enable-source-maps \ + /opt/appsmith/rts/bundle/ctl/index.js \ + "$@"