Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: failing backup and restore appsmithctl #35162

Merged
merged 5 commits into from
Aug 6, 2024
Merged

fix: failing backup and restore appsmithctl #35162

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb0509f
fix: failing backup and restore appsmithctl
pratapaprasanna Jul 24, 2024
c32725f
fix: restore db with url
pratapaprasanna Aug 5, 2024
1f89992
feat: optimize how we get dburl
pratapaprasanna Aug 5, 2024
2273ffe
add: fix for import and export db
pratapaprasanna Aug 6, 2024
5ae67ba
add comments
pratapaprasanna Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions deploy/docker/fs/opt/appsmith/utils/bin/backup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function getEncryptionPasswordFromUser(){

async function exportDatabase(destFolder) {
console.log('Exporting database');
await executeMongoDumpCMD(destFolder, process.env.APPSMITH_DB_URL)
await executeMongoDumpCMD(destFolder, utils.getDburl())
console.log('Exporting database done.');
}

Expand All @@ -141,7 +141,7 @@ async function createGitStorageArchive(destFolder) {

async function createManifestFile(path) {
const version = await utils.getCurrentAppsmithVersion()
const manifest_data = { "appsmithVersion": version, "dbName": utils.getDatabaseNameFromMongoURI(process.env.APPSMITH_DB_URL) }
const manifest_data = { "appsmithVersion": version, "dbName": utils.getDatabaseNameFromMongoURI(utils.getDburl()) }
await fsPromises.writeFile(path + '/manifest.json', JSON.stringify(manifest_data));
}

Expand Down Expand Up @@ -259,4 +259,4 @@ module.exports = {
removeOldBackups,
getEncryptionPasswordFromUser,
encryptBackupArchive,
};
};
7 changes: 5 additions & 2 deletions deploy/docker/fs/opt/appsmith/utils/bin/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const APPSMITHCTL_LOG_PATH = "/appsmith-stacks/logs/appsmithctl"

const LAST_ERROR_MAIL_TS = "/appsmith-stacks/data/backup/last-error-mail-ts"

const ENV_PATH = "/appsmith-stacks/configuration/docker.env"

const MIN_REQUIRED_DISK_SPACE_IN_BYTES = 2147483648 // 2GB

const DURATION_BETWEEN_BACKUP_ERROR_MAILS_IN_MILLI_SEC = 21600000 // 6 hrs
Expand All @@ -23,5 +25,6 @@ module.exports = {
APPSMITHCTL_LOG_PATH,
MIN_REQUIRED_DISK_SPACE_IN_BYTES,
DURATION_BETWEEN_BACKUP_ERROR_MAILS_IN_MILLI_SEC,
APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT
}
APPSMITH_DEFAULT_BACKUP_ARCHIVE_LIMIT,
ENV_PATH
}
12 changes: 6 additions & 6 deletions deploy/docker/fs/opt/appsmith/utils/bin/restore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,12 @@ async function extractArchive(backupFilePath, restoreRootPath) {
console.log('Extracting the backup archive completed');
}

async function restoreDatabase(restoreContentsPath) {
async function restoreDatabase(restoreContentsPath, dbUrl) {
console.log('Restoring database...');
const cmd = ['mongorestore', `--uri=${process.env.APPSMITH_DB_URL}`, '--drop', `--archive=${restoreContentsPath}/mongodb-data.gz`, '--gzip']
const cmd = ['mongorestore', `--uri=${dbUrl}`, '--drop', `--archive=${restoreContentsPath}/mongodb-data.gz`, '--gzip']
try {
const fromDbName = await getBackupDatabaseName(restoreContentsPath);
const toDbName = utils.getDatabaseNameFromMongoURI(process.env.APPSMITH_DB_URL);
const toDbName = utils.getDatabaseNameFromMongoURI(dbUrl);
console.log("Restoring database from " + fromDbName + " to " + toDbName)
cmd.push('--nsInclude=*', `--nsFrom=${fromDbName}.*`, `--nsTo=${toDbName}.*`)
} catch (error) {
Expand Down Expand Up @@ -105,10 +105,10 @@ async function restoreDockerEnvFile(restoreContentsPath, backupName, overwriteEn
hideEchoBack: true
});
}
await fsPromises.appendFile(dockerEnvFile, '\nAPPSMITH_ENCRYPTION_PASSWORD=' + encryptionPwd + '\nAPPSMITH_ENCRYPTION_SALT=' + encryptionSalt + '\nAPPSMITH_DB_URL=' + process.env.APPSMITH_DB_URL +
await fsPromises.appendFile(dockerEnvFile, '\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, '\nAPPSMITH_DB_URL=' + process.env.APPSMITH_DB_URL +
await fsPromises.appendFile(dockerEnvFile, '\nAPPSMITH_DB_URL=' + utils.getDburl() +
'\nAPPSMITH_MONGODB_USER=' + process.env.APPSMITH_MONGODB_USER + '\nAPPSMITH_MONGODB_PASSWORD=' + process.env.APPSMITH_MONGODB_PASSWORD ) ;
}
console.log('Restoring docker environment file completed');
Expand Down Expand Up @@ -209,7 +209,7 @@ async function run() {
console.log('****************************************************************');
console.log('Restoring Appsmith instance from the backup at ' + backupFilePath);
utils.stop(['backend', 'rts']);
await restoreDatabase(restoreContentsPath);
await restoreDatabase(restoreContentsPath, utils.getDburl());
await restoreDockerEnvFile(restoreContentsPath, backupName, overwriteEncryptionKeys);
await restoreGitStorageArchive(restoreContentsPath, backupName);
console.log('Appsmith instance successfully restored.');
Expand Down
22 changes: 22 additions & 0 deletions deploy/docker/fs/opt/appsmith/utils/bin/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const shell = require("shelljs");
const fsPromises = require("fs/promises");
const Constants = require("./constants");
const childProcess = require("child_process");
const fs = require('node:fs');
const { ConnectionString } = require("mongodb-connection-string-url");

function showHelp() {
Expand Down Expand Up @@ -31,6 +32,26 @@ function start(apps) {
console.log("Started " + appsStr);
}

function getDburl() {
pratapaprasanna marked this conversation as resolved.
Show resolved Hide resolved
let dbUrl = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a nitpick comment.
It appears to me that dbUrl variable job is to be just eventually returned, i think we can remove this and directly the return the result.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example

function getDburl() {

  try {
    const env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
    for (let i in env_array) {
      if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
       // return early when the desired line is found
       return  env_array[i].toString().split("=")[1];
        
      }
    }
  } catch (err) {
    console.error("Error reading the environment file:", err);
  }
}

  let dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI;
  if (dbEnvUrl && dbEnvUrl !== "undefined") {
   return dbEnvUrl;
  }

 return "";
}

try {
let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
for (let i in env_array) {
if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
dbUrl = env_array[i].toString().split("=")[1];
break; // Break early when the desired line is found
}
}
} catch (err) {
console.error("Error reading the environment file:", err);
}
Comment on lines +37 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid redeclaring variables and optimize the loop.

The dbUrl variable is redeclared, which can be avoided by declaring it once and updating its value conditionally. Additionally, the loop can be optimized by breaking early when the desired line is found.

-  let dbUrl;
-  try {
-    let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
-    for (let i in env_array) {
-      if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
-        dbUrl = env_array[i].toString().split("=")[1];
-        break; // Break early when the desired line is found
-      }
-    }
-  } catch (err) {
-    console.error("Error reading the environment file:", err);
-  }
+  let dbUrl = '';
+  try {
+    let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
+    for (let i in env_array) {
+      if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
+        dbUrl = env_array[i].toString().split("=")[1];
+        break; // Break early when the desired line is found
+      }
+    }
+  } catch (err) {
+    console.error("Error reading the environment file:", err);
+  }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
for (let i in env_array) {
if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
dbUrl = env_array[i].toString().split("=")[1];
break; // Break early when the desired line is found
}
}
} catch (err) {
console.error("Error reading the environment file:", err);
}
let dbUrl = '';
try {
let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
for (let i in env_array) {
if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
dbUrl = env_array[i].toString().split("=")[1];
break; // Break early when the desired line is found
}
}
} catch (err) {
console.error("Error reading the environment file:", err);
}

let dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI;
if (dbEnvUrl && dbEnvUrl !== "undefined") {
dbUrl = dbEnvUrl;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utility functions should generally be pure and env variables should not be a part of it. If you were to drive the utility function the env values as a parameter is okay, however i think you can move this function into a separate file and not be a part of the utility file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this for the time being because of the urgency. I will address this in the next release
thanks @vsvamsi1

return dbUrl;
}

function execCommand(cmd, options) {
return new Promise((resolve, reject) => {
let isPromiseDone = false;
Expand Down Expand Up @@ -174,4 +195,5 @@ module.exports = {
preprocessMongoDBURI,
execCommandSilent,
getDatabaseNameFromMongoURI,
getDburl
};
Loading