Move reboot/shutdown to actions and related tests to integration

Signed-off-by: Christina Ying Wang <christina@balena.io>

src/device-api/actions.ts

@@ -310,6 +310,22 @@ export const getLegacyService = async (appId: number) => {
 	return (await getCurrentApp(appId, BadRequestError)).services[0];
 };
 
+/**
+ * Executes a device state action such as reboot, shutdown, or noop
+ * Used by:
+ * - POST /v1/reboot
+ * - POST /v1/shutdown
+ * - actions.executeServiceAction
+ */
+export const executeDeviceAction = async (
+	step: Parameters<typeof deviceState.executeStepAction>[0],
+	force: boolean = false,
+) => {
+	return await deviceState.executeStepAction(step, {
+		force,
+	});
+};
+
 /**
  * Executes a composition step action on a service.
  * isLegacy indicates that the action is being called from a legacy (v1) endpoint,
@@ -371,12 +387,12 @@ export const executeServiceAction = async ({
 	});
 
 	// Execute action on service
-	return await applicationManager.executeStep(
+	return await executeDeviceAction(
 		generateStep(action, {
 			current: currentService,
 			target: targetService,
 			wait: true,
 		}),
-		{ force },
+		force,
 	);
 };

src/device-api/v1.ts

@@ -1,5 +1,6 @@
 import * as express from 'express';
 import * as _ from 'lodash';
+import type { Response } from 'express';
 
 import * as actions from './actions';
 import { AuthorizedRequest } from './api-keys';
@@ -87,29 +88,24 @@ const handleLegacyServiceAction = (action: CompositionStepAction) => {
 router.post('/v1/apps/:appId/stop', handleLegacyServiceAction('stop'));
 router.post('/v1/apps/:appId/start', handleLegacyServiceAction('start'));
 
-const rebootOrShutdown = async (
+const handleDeviceAction = (action: deviceState.DeviceStateStepTarget) => {
-	req: express.Request,
+	return async (req: AuthorizedRequest, res: Response) => {
-	res: express.Response,
+		const force = checkTruthy(req.body.force);
-	action: deviceState.DeviceStateStepTarget,
-) => {
-	const override = await config.get('lockOverride');
-	const force = checkTruthy(req.body.force) || override;
 		try {
-		const response = await deviceState.executeStepAction({ action }, { force });
+			await actions.executeDeviceAction({ action }, force);
-		res.status(202).json(response);
+			return res.status(202).send({ Data: 'OK', Error: null });
-	} catch (e: any) {
+		} catch (e: unknown) {
 			const status = e instanceof UpdatesLockedError ? 423 : 500;
-		res.status(status).json({
+			return res.status(status).json({
 				Data: '',
-			Error: (e != null ? e.message : undefined) || e || 'Unknown error',
+				Error: (e as Error)?.message ?? e ?? 'Unknown error',
 			});
 		}
+	};
 };
 
-router.post('/v1/reboot', (req, res) => rebootOrShutdown(req, res, 'reboot'));
+router.post('/v1/reboot', handleDeviceAction('reboot'));
-router.post('/v1/shutdown', (req, res) =>
+router.post('/v1/shutdown', handleDeviceAction('shutdown'));
-	rebootOrShutdown(req, res, 'shutdown'),
-);
 
 router.get('/v1/apps/:appId', async (req: AuthorizedRequest, res, next) => {
 	const appId = checkInt(req.params.appId);

src/device-state.ts

@@ -89,11 +89,7 @@ export type DeviceStateStepTarget = 'reboot' | 'shutdown' | 'noop';
 
 type PossibleStepTargets = CompositionStepAction | DeviceStateStepTarget;
 type DeviceStateStep<T extends PossibleStepTargets> =
-	| {
+	| { action: DeviceStateStepTarget }
-			action: 'reboot';
-	  }
-	| { action: 'shutdown' }
-	| { action: 'noop' }
 	| CompositionStepT<T extends CompositionStepAction ? T : never>
 	| deviceConfig.ConfigStep;
 
@@ -564,8 +560,8 @@ export async function shutdown({
 	});
 }
 
-export async function executeStepAction<T extends PossibleStepTargets>(
+export async function executeStepAction(
-	step: DeviceStateStep<T>,
+	step: DeviceStateStep<PossibleStepTargets>,
 	{
 		force,
 		initial,
@@ -586,19 +582,12 @@ export async function executeStepAction<T extends PossibleStepTargets>(
 			case 'reboot':
 				// There isn't really a way that these methods can fail,
 				// and if they do, we wouldn't know about it until after
-				// the response has been sent back to the API. Just return
+				// the response has been sent back to the API.
-				// "OK" for this and the below action
 				await shutdown({ force, reboot: true });
-				return {
+				return;
-					Data: 'OK',
-					Error: null,
-				};
 			case 'shutdown':
 				await shutdown({ force, reboot: false });
-				return {
+				return;
-					Data: 'OK',
-					Error: null,
-				};
 			case 'noop':
 				return;
 			default:
@@ -607,8 +596,8 @@ export async function executeStepAction<T extends PossibleStepTargets>(
 	}
 }
 
-export async function applyStep<T extends PossibleStepTargets>(
+export async function applyStep(
-	step: DeviceStateStep<T>,
+	step: DeviceStateStep<PossibleStepTargets>,
 	{
 		force,
 		initial,
@@ -623,12 +612,12 @@ export async function applyStep<T extends PossibleStepTargets>(
 		return;
 	}
 	try {
-		const stepResult = await executeStepAction(step, {
+		await executeStepAction(step, {
 			force,
 			initial,
 			skipLock,
 		});
-		emitAsync('step-completed', null, step, stepResult || undefined);
+		emitAsync('step-completed', null, step);
 	} catch (e: any) {
 		emitAsync('step-error', e, step);
 		throw e;

test/integration/device-api/actions.spec.ts

@@ -1,5 +1,5 @@
 import { expect } from 'chai';
-import { stub, SinonStub } from 'sinon';
+import { stub, SinonStub, spy, SinonSpy } from 'sinon';
 import * as Docker from 'dockerode';
 import * as request from 'supertest';
 import { setTimeout } from 'timers/promises';
@@ -7,6 +7,7 @@ import { setTimeout } from 'timers/promises';
 import * as deviceState from '~/src/device-state';
 import * as deviceApi from '~/src/device-api';
 import * as actions from '~/src/device-api/actions';
+import * as dbus from '~/lib/dbus';
 import { cleanupDocker } from '~/test-lib/docker-helper';
 
 describe('regenerates API keys', () => {
@@ -111,13 +112,13 @@ describe('manages application lifecycle', () => {
 		appId?: number;
 		serviceNames?: string[];
 	}) => {
-		const { name, config } = await getSupervisorTarget();
+		const { name, config: svConfig } = await getSupervisorTarget();
 		return {
 			local: {
 				// We don't want to change name or config as this may result in
 				// unintended reboots. We just want to test state changes in containers.
 				name,
-				config,
+				config: svConfig,
 				apps:
 					serviceCount === 0
 						? {}
@@ -616,3 +617,25 @@ describe('manages application lifecycle', () => {
 		});
 	});
 });
+
+describe('reboots or shuts down device', () => {
+	before(async () => {
+		spy(dbus, 'reboot');
+		spy(dbus, 'shutdown');
+	});
+
+	after(() => {
+		(dbus.reboot as SinonSpy).restore();
+		(dbus.shutdown as SinonSpy).restore();
+	});
+
+	it('reboots device', async () => {
+		await actions.executeDeviceAction({ action: 'reboot' });
+		expect(dbus.reboot as SinonSpy).to.have.been.called;
+	});
+
+	it('shuts down device', async () => {
+		await actions.executeDeviceAction({ action: 'shutdown' });
+		expect(dbus.shutdown as SinonSpy).to.have.been.called;
+	});
+});

test/integration/device-api/v1.spec.ts

@@ -496,4 +496,144 @@ describe('device-api/v1', () => {
 				.expect(503);
 		});
 	});
+
+	describe('POST /v1/reboot', () => {
+		let executeDeviceActionStub: SinonStub;
+		beforeEach(() => {
+			executeDeviceActionStub = stub(actions, 'executeDeviceAction').resolves();
+		});
+		afterEach(async () => executeDeviceActionStub.restore());
+
+		it('validates data from request body', async () => {
+			// Parses force: false
+			await request(api)
+				.post('/v1/reboot')
+				.send({ force: false })
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'reboot',
+				},
+				false,
+			);
+			executeDeviceActionStub.resetHistory();
+
+			// Parses force: true
+			await request(api)
+				.post('/v1/reboot')
+				.send({ force: true })
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'reboot',
+				},
+				true,
+			);
+			executeDeviceActionStub.resetHistory();
+
+			// Defaults to force: false
+			await request(api)
+				.post('/v1/reboot')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'reboot',
+				},
+				false,
+			);
+		});
+
+		it('responds with 202 if request successful', async () => {
+			await request(api)
+				.post('/v1/reboot')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(202);
+		});
+
+		it('responds with 423 if there are update locks', async () => {
+			executeDeviceActionStub.throws(new UpdatesLockedError());
+			await request(api)
+				.post('/v1/reboot')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(423);
+		});
+
+		it('responds with 500 for other errors that occur during reboot', async () => {
+			executeDeviceActionStub.throws(new Error());
+			await request(api)
+				.post('/v1/reboot')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(500);
+		});
+	});
+
+	describe('POST /v1/shutdown', () => {
+		let executeDeviceActionStub: SinonStub;
+		beforeEach(() => {
+			executeDeviceActionStub = stub(actions, 'executeDeviceAction').resolves();
+		});
+		afterEach(async () => executeDeviceActionStub.restore());
+
+		it('validates data from request body', async () => {
+			// Parses force: false
+			await request(api)
+				.post('/v1/shutdown')
+				.send({ force: false })
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'shutdown',
+				},
+				false,
+			);
+			executeDeviceActionStub.resetHistory();
+
+			// Parses force: true
+			await request(api)
+				.post('/v1/shutdown')
+				.send({ force: true })
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'shutdown',
+				},
+				true,
+			);
+			executeDeviceActionStub.resetHistory();
+
+			// Defaults to force: false
+			await request(api)
+				.post('/v1/shutdown')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`);
+			expect(executeDeviceActionStub).to.have.been.calledWith(
+				{
+					action: 'shutdown',
+				},
+				false,
+			);
+		});
+
+		it('responds with 202 if request successful', async () => {
+			await request(api)
+				.post('/v1/shutdown')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(202);
+		});
+
+		it('responds with 423 if there are update locks', async () => {
+			executeDeviceActionStub.throws(new UpdatesLockedError());
+			await request(api)
+				.post('/v1/shutdown')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(423);
+		});
+
+		it('responds with 500 for other errors that occur during shutdown', async () => {
+			executeDeviceActionStub.throws(new Error());
+			await request(api)
+				.post('/v1/shutdown')
+				.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
+				.expect(500);
+		});
+	});
 });

test/legacy/41-device-api-v1.spec.ts

@@ -164,291 +164,6 @@ describe('SupervisorAPI [V1 Endpoints]', () => {
 		});
 	});
 
-	describe('POST /v1/reboot', () => {
-		let rebootMock: SinonStub;
-		before(() => {
-			rebootMock = stub(dbus, 'reboot').resolves((() => void 0) as any);
-		});
-
-		after(() => {
-			rebootMock.restore();
-		});
-
-		afterEach(() => {
-			rebootMock.resetHistory();
-		});
-
-		it('should return 202 and reboot if no locks are set', async () => {
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/reboot')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(202);
-
-					expect(response.body).to.have.property('Data').that.is.not.empty;
-					expect(rebootMock).to.have.been.calledOnce;
-				},
-			);
-		});
-
-		it('should return 423 and reject the reboot if no locks are set', async () => {
-			stub(updateLock, 'lock').callsFake(async (__, opts, fn) => {
-				if (opts.force) {
-					return fn();
-				}
-				throw new UpdatesLockedError('Updates locked');
-			});
-
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/reboot')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(423);
-
-					expect(updateLock.lock).to.be.calledOnce;
-					expect(response.body).to.have.property('Error').that.is.not.empty;
-					expect(rebootMock).to.not.have.been.called;
-				},
-			);
-
-			(updateLock.lock as SinonStub).restore();
-		});
-
-		it('should return 202 and reboot if force is set to true', async () => {
-			stub(updateLock, 'lock').callsFake(async (__, opts, fn) => {
-				if (opts.force) {
-					return fn();
-				}
-				throw new UpdatesLockedError('Updates locked');
-			});
-
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/reboot')
-						.send({ force: true })
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(202);
-
-					expect(updateLock.lock).to.be.calledOnce;
-					expect(response.body).to.have.property('Data').that.is.not.empty;
-					expect(rebootMock).to.have.been.calledOnce;
-				},
-			);
-
-			(updateLock.lock as SinonStub).restore();
-		});
-	});
-
-	describe('POST /v1/shutdown', () => {
-		let shutdownMock: SinonStub;
-		before(() => {
-			shutdownMock = stub(dbus, 'shutdown').resolves((() => void 0) as any);
-		});
-
-		after(async () => {
-			shutdownMock.restore();
-		});
-
-		it('should return 202 and shutdown if no locks are set', async () => {
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/shutdown')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(202);
-
-					expect(response.body).to.have.property('Data').that.is.not.empty;
-					expect(shutdownMock).to.have.been.calledOnce;
-				},
-			);
-
-			shutdownMock.resetHistory();
-		});
-
-		it('should lock all applications before trying to shutdown', async () => {
-			// Setup 2 applications running
-			const twoContainers = [
-				mockedAPI.mockService({
-					containerId: 'abc123',
-					appId: 1000,
-					releaseId: 55555,
-				}),
-				mockedAPI.mockService({
-					containerId: 'def456',
-					appId: 2000,
-					releaseId: 77777,
-				}),
-			];
-			const twoImages = [
-				mockedAPI.mockImage({
-					appId: 1000,
-				}),
-				mockedAPI.mockImage({
-					appId: 2000,
-				}),
-			];
-			appMock.mockManagers(twoContainers, [], []);
-			appMock.mockImages([], false, twoImages);
-
-			const lockSpy = spy(updateLock, 'lock');
-			await mockedDockerode.testWithData(
-				{ containers: twoContainers, images: twoImages },
-				async () => {
-					const response = await request
-						.post('/v1/shutdown')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(202);
-
-					expect(lockSpy.callCount).to.equal(1);
-					// Check that lock was passed both application Ids
-					expect(lockSpy.lastCall.args[0]).to.deep.equal([1000, 2000]);
-					expect(response.body).to.have.property('Data').that.is.not.empty;
-					expect(shutdownMock).to.have.been.calledOnce;
-				},
-			);
-
-			shutdownMock.resetHistory();
-			lockSpy.restore();
-		});
-
-		it('should return 423 and reject the reboot if locks are set', async () => {
-			stub(updateLock, 'lock').callsFake(async (__, opts, fn) => {
-				if (opts.force) {
-					return fn();
-				}
-				throw new UpdatesLockedError('Updates locked');
-			});
-
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/shutdown')
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(423);
-
-					expect(updateLock.lock).to.be.calledOnce;
-					expect(response.body).to.have.property('Error').that.is.not.empty;
-					expect(shutdownMock).to.not.have.been.called;
-				},
-			);
-
-			(updateLock.lock as SinonStub).restore();
-		});
-
-		it('should return 202 and shutdown if force is set to true', async () => {
-			stub(updateLock, 'lock').callsFake(async (__, opts, fn) => {
-				if (opts.force) {
-					return fn();
-				}
-				throw new UpdatesLockedError('Updates locked');
-			});
-
-			// Setup single container application
-			const container = mockedAPI.mockService({
-				containerId: 'abc123',
-				appId: 2,
-				releaseId: 77777,
-			});
-			const image = mockedAPI.mockImage({
-				appId: 2,
-			});
-			appMock.mockManagers([container], [], []);
-			appMock.mockImages([], false, [image]);
-
-			await mockedDockerode.testWithData(
-				{ containers: [container], images: [image] },
-				async () => {
-					const response = await request
-						.post('/v1/shutdown')
-						.send({ force: true })
-						.set('Accept', 'application/json')
-						.set('Authorization', `Bearer ${await deviceApi.getGlobalApiKey()}`)
-						.expect(202);
-
-					expect(updateLock.lock).to.be.calledOnce;
-					expect(response.body).to.have.property('Data').that.is.not.empty;
-					expect(shutdownMock).to.have.been.calledOnce;
-				},
-			);
-
-			(updateLock.lock as SinonStub).restore();
-		});
-	});
-
 	describe('POST /v1/update', () => {
 		let configStub: SinonStub;
 		let targetUpdateSpy: SinonSpy;