From ff1f44886007e0bd4e15a76c17a974e381bdff04 Mon Sep 17 00:00:00 2001 From: Girish Ramakrishnan Date: Sat, 21 Sep 2019 19:45:55 -0700 Subject: [PATCH] Fixup repair route * Do not allow scheduling tasks in error state * Only repair is allowed in error state * Use the error object to track what to 'repair' (like the lastState) * If uninstall failed, repair will do uninstall * If move dir failed, repair will do move dir --- src/appdb.js | 6 ++- src/apps.js | 83 ++++++++++++++++++++++++++++++++++------ src/apptask.js | 94 ++++++++++++++++++++++++++++++---------------- src/boxerror.js | 1 + src/routes/apps.js | 17 ++++++++- 5 files changed, 156 insertions(+), 45 deletions(-) diff --git a/src/appdb.js b/src/appdb.js index 690f25845..9160bfac2 100644 --- a/src/appdb.js +++ b/src/appdb.js @@ -458,7 +458,11 @@ function setTask(appId, values, callback) { assert.strictEqual(typeof values, 'object'); assert.strictEqual(typeof callback, 'function'); - updateWithConstraints(appId, values, 'AND taskId IS NULL', callback); + if (values.installationState === 'pending_repair') { // FIXME + updateWithConstraints(appId, values, 'AND taskId IS NULL AND installationState == "error"', callback); + } else { + updateWithConstraints(appId, values, 'AND taskId IS NULL AND installationState != "error"', callback); + } } function getAppStoreIds(callback) { diff --git a/src/apps.js b/src/apps.js index 1dd631d3e..f80cd9922 100644 --- a/src/apps.js +++ b/src/apps.js @@ -646,8 +646,11 @@ function scheduleTask(appId, args, values, callback) { debug(`scheduleTask: task ${taskId} of $${appId} completed`); if (error && (error.code === tasks.ECRASHED || error.code === tasks.ESTOPPED)) { // if task crashed, update the error debug(`Apptask crashed/stopped: ${error.message}`); - const crashed = error.code === tasks.ECRASHED, stopped = error.code === tasks.ESTOPPED; - appdb.update(appId, { installationState: exports.ISTATE_ERROR, error: { reason: BoxError.TASK_ERROR, crashed, stopped, message: error.message }, taskId: null }, NOOP_CALLBACK); + let boxError = new BoxError(BoxError.TASK_ERROR, error.message); + boxError.details.crashed = error.code === tasks.ECRASHED; + boxError.details.stopped = error.code === tasks.ESTOPPED; + boxError.details.task = { args, installationState: values.installationState }; // see also apptask makeTaskError + appdb.update(appId, { installationState: exports.ISTATE_ERROR, error: boxError.toPlainObject(), taskId: null }, NOOP_CALLBACK); } else if (values.installationState !== exports.ISTATE_PENDING_UNINSTALL) { // clear out taskId since it's done appdb.update(appId, { taskId: null }, NOOP_CALLBACK); } @@ -658,6 +661,21 @@ function scheduleTask(appId, args, values, callback) { }); } +function checkAppState(app, state) { + assert.strictEqual(typeof app, 'object'); + assert.strictEqual(typeof state, 'string'); + + if (app.taskId) return new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`); + + if (state === exports.ISTATE_PENDING_REPAIR) { + if (app.installationState !== exports.ISTATE_ERROR) return new AppsError(AppsError.BAD_STATE, 'Not allowed in error state'); + } else { + if (app.installationState === exports.ISTATE_ERROR) return new AppsError(AppsError.BAD_STATE, 'Not allowed in error state'); + } + + return null; +} + function install(data, user, auditSource, callback) { assert(data && typeof data === 'object'); assert(user && typeof user === 'object'); @@ -907,6 +925,9 @@ function setMemoryLimit(appId, memoryLimit, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_RESIZE); + if (error) return callback(error); + error = validateMemoryLimit(app.manifest, memoryLimit); if (error) return callback(error); @@ -929,6 +950,9 @@ function setEnvironment(appId, env, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_RECREATE_CONTAINER); + if (error) return callback(error); + error = validateEnv(env); if (error) return callback(error); @@ -951,6 +975,9 @@ function setDebugMode(appId, debugMode, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_DEBUG); + if (error) return callback(error); + error = validateDebugMode(debugMode); if (error) return callback(error); @@ -973,6 +1000,9 @@ function setMailbox(appId, mailboxName, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_RECREATE_CONTAINER); + if (error) return callback(error); + if (mailboxName) { error = mail.validateName(mailboxName); if (error) return callback(new AppsError(AppsError.BAD_FIELD, error.message, { field: 'mailboxName' })); @@ -1095,6 +1125,9 @@ function setLocation(appId, data, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_LOCATION_CHANGE); + if (error) return callback(error); + let values = { installationState: exports.ISTATE_PENDING_LOCATION_CHANGE, // these are intentionally reset, if not set @@ -1152,6 +1185,9 @@ function setDataDir(appId, dataDir, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); + error = checkAppState(app, exports.ISTATE_PENDING_DATA_DIR_MIGRATION); + if (error) return callback(error); + error = validateDataDir(dataDir); if (error) return callback(error); @@ -1175,7 +1211,9 @@ function update(appId, data, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.ISTATE_PENDING_UPDATE); + if (error) return callback(error); downloadManifest(data.appStoreId, data.manifest, function (error, appStoreId, manifest) { if (error) return callback(error); @@ -1305,12 +1343,25 @@ function repair(appId, data, auditSource, callback) { debug('Will repair app with id:%s', appId); - scheduleTask(appId, { /* task args */ }, { installationState: exports.ISTATE_PENDING_REPAIR }, function (error, result) { + get(appId, function (error, app) { if (error) return callback(error); - eventlog.add(eventlog.ACTION_APP_REPAIR, auditSource, { taskId: result.taskId }); + error = checkAppState(app, exports.ISTATE_PENDING_REPAIR); + if (error) return callback(error); - callback(null, { taskId: result.taskId }); + let values = _.pick(data, 'location', 'domain', 'alternateDomains'); // FIXME: validate + values.installationState = exports.ISTATE_PENDING_REPAIR; + + const restoreConfig = data.backupId ? { backupId: data.backupId, backupFormat: data.backupFormat, oldManifest: app.manifest } : null; // when null, apptask simply reinstalls + const overwriteDns = 'overwriteDns' in data ? data.overwriteDns : false; + + scheduleTask(appId, { restoreConfig, overwriteDns }, values, function (error, result) { + if (error) return callback(error); + + eventlog.add(eventlog.ACTION_APP_REPAIR, auditSource, { taskId: result.taskId, app }); + + callback(null, { taskId: result.taskId }); + }); }); } @@ -1324,7 +1375,9 @@ function restore(appId, data, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.ISTATE_PENDING_RESTORE); + if (error) return callback(error); // for empty or null backupId, use existing manifest to mimic a reinstall var func = data.backupId ? backups.get.bind(null, data.backupId) : function (next) { return next(null, { manifest: app.manifest }); }; @@ -1480,7 +1533,9 @@ function uninstall(appId, auditSource, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.ISTATE_PENDING_UNINSTALL); + if (error) return callback(error); appstore.unpurchaseApp(appId, { appstoreId: app.appStoreId, manifestId: app.manifest.id }, function (error) { if (error && error.reason === AppstoreError.NOT_FOUND) return callback(new AppsError(AppsError.NOT_FOUND)); @@ -1508,7 +1563,9 @@ function start(appId, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.exports.ISTATE_INSTALLED); // FIXME + if (error) return callback(error); scheduleTask(appId, { /* args */ }, { runState: exports.RSTATE_PENDING_START }, callback); }); @@ -1522,7 +1579,9 @@ function stop(appId, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.ISTATE_INSTALLED); // FIXME + if (error) return callback(error); scheduleTask(appId, { /* args */ }, { runState: exports.RSTATE_PENDING_STOP }, callback); }); @@ -1666,7 +1725,9 @@ function backup(appId, callback) { get(appId, function (error, app) { if (error) return callback(error); - if (app.taskId) return callback(new AppsError(AppsError.BAD_STATE, `Not allowed in this app state : ${app.installationState} / ${app.runState}`)); + + error = checkAppState(app, exports.ISTATE_PENDING_BACKUP); + if (error) return callback(error); scheduleTask(appId, { /* args */ }, { installationState: exports.ISTATE_PENDING_BACKUP }, (error, result) => { if (error) return callback(error); diff --git a/src/apptask.js b/src/apptask.js index 8ad261ad3..90d54436e 100644 --- a/src/apptask.js +++ b/src/apptask.js @@ -65,6 +65,15 @@ function debugApp(app) { debug(app.fqdn + ' ' + util.format.apply(util, Array.prototype.slice.call(arguments, 1))); } +function makeTaskError(error, app, args) { + let boxError = error instanceof BoxError ? error : new BoxError(BoxError.UNKNOWN_ERROR, error.message); // until we port everything to BoxError + // we stash the args and not task id because + // a) task table is ephemeral + // b) this is actually the state. if we use taskId, we have to track it properly across failed repairs + boxError.details.task = { args, installationState: app.installationState }; + return boxError.toPlainObject(); +} + // updates the app object and the database function updateApp(app, values, callback) { assert.strictEqual(typeof app, 'object'); @@ -74,7 +83,7 @@ function updateApp(app, values, callback) { debugApp(app, 'updating app with values: %j', values); appdb.update(app.id, values, function (error) { - if (error) return callback(new BoxError(BoxError.INTERNAL_ERROR, error)); + if (error) return callback(new BoxError(BoxError.DATABASE_ERROR, error)); for (var value in values) { app[value] = values[value]; @@ -497,7 +506,7 @@ function install(app, args, progressCallback, callback) { assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); - const isInstalling = app.installationState !== apps.ISTATE_PENDING_RESTORE; // install or clone + const isInstalling = app.installationState !== apps.ISTATE_PENDING_RESTORE; // install or clone or repair const restoreConfig = args.restoreConfig || {}; const overwriteDns = args.overwriteDns; @@ -580,18 +589,19 @@ function install(app, args, progressCallback, callback) { configureReverseProxy.bind(null, app), progressCallback.bind(null, { percent: 100, message: 'Done' }), - updateApp.bind(null, app, { installationState: apps.ISTATE_INSTALLED, health: null }) + updateApp.bind(null, app, { installationState: apps.ISTATE_INSTALLED, error: null, health: null }) ], function seriesDone(error) { if (error) { debugApp(app, 'error installing app: %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); } -function backup(app, progressCallback, callback) { +function backup(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); @@ -613,8 +623,9 @@ function backup(app, progressCallback, callback) { }); } -function create(app, progressCallback, callback) { +function create(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); @@ -638,7 +649,7 @@ function create(app, progressCallback, callback) { ], function seriesDone(error) { if (error) { debugApp(app, 'error creating : %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); @@ -693,18 +704,21 @@ function changeLocation(app, args, progressCallback, callback) { ], function seriesDone(error) { if (error) { debugApp(app, 'error reconfiguring : %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); } -function migrateDataDir(app, oldDataDir, progressCallback, callback) { +function migrateDataDir(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); - assert(oldDataDir === null || typeof oldDataDir === 'string'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); + let oldDataDir = args.oldDataDir; + assert(oldDataDir === null || typeof oldDataDir === 'string'); + async.series([ progressCallback.bind(null, { percent: 10, message: 'Cleaning up old install' }), stopApp.bind(null, app, progressCallback), @@ -737,19 +751,20 @@ function migrateDataDir(app, oldDataDir, progressCallback, callback) { ], function seriesDone(error) { if (error) { debugApp(app, 'error reconfiguring : %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); } // configure is only called for an infra update -function configure(app, oldConfig, progressCallback, callback) { +function configure(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); - assert.strictEqual(typeof oldConfig, 'object'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); + const oldConfig = args.oldConfig; const locationChanged = oldConfig.fqdn !== app.fqdn; const dataDirChanged = oldConfig.dataDir !== app.dataDir; @@ -819,19 +834,20 @@ function configure(app, oldConfig, progressCallback, callback) { ], function seriesDone(error) { if (error) { debugApp(app, 'error reconfiguring : %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); } // nginx configuration is skipped because app.httpPort is expected to be available -function update(app, updateConfig, progressCallback, callback) { +function update(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); - assert.strictEqual(typeof updateConfig, 'object'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); + const updateConfig = args.updateConfig; debugApp(app, `Updating to ${updateConfig.manifest.version}`); // app does not want these addons anymore @@ -920,7 +936,7 @@ function update(app, updateConfig, progressCallback, callback) { updateApp(app, { installationState: apps.ISTATE_INSTALLED, error: null, health: null }, callback.bind(null, error)); } else if (error) { debugApp(app, 'Error updating app: %s', error); - updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message, updateTime: new Date() }, callback.bind(null, error)); + updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } else { if (updateConfig.skipNotification) return callback(null); @@ -929,8 +945,9 @@ function update(app, updateConfig, progressCallback, callback) { }); } -function uninstall(app, progressCallback, callback) { +function uninstall(app, args, progressCallback, callback) { assert.strictEqual(typeof app, 'object'); + assert.strictEqual(typeof args, 'object'); assert.strictEqual(typeof progressCallback, 'function'); assert.strictEqual(typeof callback, 'function'); @@ -973,7 +990,7 @@ function uninstall(app, progressCallback, callback) { ], function seriesDone(error) { if (error) { debugApp(app, 'error uninstalling app: %s', error); - return updateApp(app, { installationState: apps.ISTATE_ERROR, error: error.toPlainObject ? error.toPlainObject() : error.message }, callback.bind(null, error)); + return updateApp(app, { installationState: apps.ISTATE_ERROR, error: makeTaskError(error, app, args) }, callback.bind(null, error)); } callback(null); }); @@ -987,7 +1004,7 @@ function runApp(app, progressCallback, callback) { progressCallback({ message: 'Starting app' }); docker.startContainer(app.containerId, function (error) { - if (error) return callback(error); + if (error) return callback(new BoxError(BoxError.DOCKER_ERROR, `Error starting container: ${error.message}`)); updateApp(app, { runState: apps.RSTATE_RUNNING }, callback); }); @@ -1001,7 +1018,7 @@ function stopApp(app, progressCallback, callback) { progressCallback({ message: 'Stopping app' }); docker.stopContainers(app.id, function (error) { - if (error) return callback(error); + if (error) return callback(new BoxError(BoxError.DOCKER_ERROR, `Error starting container: ${error.message}`)); updateApp(app, { runState: apps.RSTATE_STOPPED, health: null }, callback); }); @@ -1020,21 +1037,34 @@ function run(appId, args, progressCallback, callback) { debugApp(app, 'startTask installationState: %s runState: %s', app.installationState, app.runState); switch (app.installationState) { - case apps.ISTATE_PENDING_INSTALL: return install(app, args, progressCallback, callback); - case apps.ISTATE_PENDING_CONFIGURE: + case apps.ISTATE_PENDING_INSTALL: + case apps.ISTATE_PENDING_CLONE: + case apps.ISTATE_PENDING_RESTORE: + return install(app, args, progressCallback, callback); case apps.ISTATE_PENDING_REPAIR: - return configure(app, args.oldConfig, progressCallback, callback); + if (app.error.task.installationState === apps.ISTATE_PENDING_UNINSTALL) { + return uninstall(app, app.error.task.args, progressCallback, callback); + } else if (app.error.task.installationState === apps.ISTATE_PENDING_DATA_DIR_MIGRATION) { + return migrateDataDir(app, app.error.task.args, progressCallback, callback); + } else { + return install(app, args, progressCallback, callback); + } + case apps.ISTATE_PENDING_CONFIGURE: + return configure(app, args, progressCallback, callback); case apps.ISTATE_PENDING_RECREATE_CONTAINER: case apps.ISTATE_PENDING_RESIZE: case apps.ISTATE_PENDING_DEBUG: - return create(app, progressCallback, callback); - case apps.ISTATE_PENDING_LOCATION_CHANGE: return changeLocation(app, args, progressCallback, callback); - case apps.ISTATE_PENDING_DATA_DIR_MIGRATION: return migrateDataDir(app, args.oldDataDir, progressCallback, callback); - case apps.ISTATE_PENDING_UNINSTALL: return uninstall(app, progressCallback, callback); - case apps.ISTATE_PENDING_CLONE: return install(app, args, progressCallback, callback); - case apps.ISTATE_PENDING_RESTORE: return install(app, args, progressCallback, callback); - case apps.ISTATE_PENDING_UPDATE: return update(app, args.updateConfig, progressCallback, callback); - case apps.ISTATE_PENDING_BACKUP: return backup(app, progressCallback, callback); + return create(app, args, progressCallback, callback); + case apps.ISTATE_PENDING_LOCATION_CHANGE: + return changeLocation(app, args, progressCallback, callback); + case apps.ISTATE_PENDING_DATA_DIR_MIGRATION: + return migrateDataDir(app, args, progressCallback, callback); + case apps.ISTATE_PENDING_UNINSTALL: + return uninstall(app, args, progressCallback, callback); + case apps.ISTATE_PENDING_UPDATE: + return update(app, args, progressCallback, callback); + case apps.ISTATE_PENDING_BACKUP: + return backup(app, args, progressCallback, callback); case apps.ISTATE_INSTALLED: switch (app.runState) { case apps.RSTATE_PENDING_STOP: return stopApp(app, progressCallback, callback); diff --git a/src/boxerror.js b/src/boxerror.js index 6a3d74e02..1cb57f97a 100644 --- a/src/boxerror.js +++ b/src/boxerror.js @@ -46,6 +46,7 @@ BoxError.NETWORK_ERROR = 'Network Error'; BoxError.NOT_FOUND = 'Not found'; BoxError.REVERSEPROXY_ERROR = 'ReverseProxy Error'; BoxError.TASK_ERROR = 'Task Error'; +BoxError.UNKNOWN_ERROR = 'Unknown Error'; // only used for porting BoxError.prototype.toPlainObject = function () { return _.extend({}, { message: this.message, reason: this.reason }, this.details); diff --git a/src/routes/apps.js b/src/routes/apps.js index 164dd6a72..4c7afff29 100644 --- a/src/routes/apps.js +++ b/src/routes/apps.js @@ -368,7 +368,22 @@ function repairApp(req, res, next) { debug('Repair app id:%s', req.params.id); - apps.repair(req.params.id, {}, auditSource.fromRequest(req), function (error, result) { + const data = req.body; + + if (data.backupId && typeof data.backupId !== 'string') return next(new HttpError(400, 'backupId must be string or null')); + if (data.backupFormat && typeof data.backupFormat !== 'string') return next(new HttpError(400, 'backupFormat must be string or null')); + + if (data.location && typeof data.location !== 'string') return next(new HttpError(400, 'location is required')); + if (data.domain && typeof data.domain !== 'string') return next(new HttpError(400, 'domain is required')); + + if ('alternateDomains' in data) { + if (!Array.isArray(data.alternateDomains)) return next(new HttpError(400, 'alternateDomains must be an array')); + if (data.alternateDomains.some(function (d) { return (typeof d.domain !== 'string' || typeof d.subdomain !== 'string'); })) return next(new HttpError(400, 'alternateDomains array must contain objects with domain and subdomain strings')); + } + + if ('overwriteDns' in req.body && typeof req.body.overwriteDns !== 'boolean') return next(new HttpError(400, 'overwriteDns must be boolean')); + + apps.repair(req.params.id, data, auditSource.fromRequest(req), function (error, result) { if (error) return next(toHttpError(error)); next(new HttpSuccess(202, { taskId: result.taskId }));