Store rejected apps in the database

This moves from throwing an error when an app is rejected due to unmet
requirements (because of contracts) to storing the target with a
`rejected` flag on the database.

The application manager filters rejected apps when calculating steps to
prevent them from affecting the current state. The state engine uses the
rejection info to generate the state report.

Change-type: minor

src/compose/app.ts

@@ -38,6 +38,7 @@ export interface AppConstructOpts {
 	commit?: string;
 	source?: string;
 	isHost?: boolean;
+	isRejected?: boolean;
 
 	services: Service[];
 	volumes: Volume[];
@@ -57,6 +58,7 @@ class AppImpl implements App {
 	public commit?: string;
 	public source?: string;
 	public isHost?: boolean;
+	public isRejected?: boolean;
 	// Services are stored as an array, as at any one time we could have more than one
 	// service for a single service ID running (for example handover)
 	public services: Service[];
@@ -77,6 +79,10 @@ class AppImpl implements App {
 		this.networks = opts.networks;
 		this.isHost = !!opts.isHost;
 
+		if (isTargetState) {
+			this.isRejected = !!opts.isRejected;
+		}
+
 		if (
 			this.networks.find((n) => n.name === 'default') == null &&
 			isTargetState
@@ -1054,6 +1060,7 @@ class AppImpl implements App {
 				appName: app.name,
 				source: app.source,
 				isHost: app.isHost,
+				isRejected: app.rejected,
 				services,
 				volumes,
 				networks,

src/compose/application-manager.ts

@@ -191,8 +191,19 @@ export async function inferNextSteps(
 
 		// We want to remove images before moving on to anything else
 		if (steps.length === 0) {
-			const targetAndCurrent = _.intersection(currentAppIds, targetAppIds);
-			const onlyTarget = _.difference(targetAppIds, currentAppIds);
+			// We only want to modify existing apps for accepted targets
+			const acceptedTargetAppIds = targetAppIds.filter(
+				(id) => !targetApps[id].isRejected,
+			);
+
+			const targetAndCurrent = _.intersection(
+				currentAppIds,
+				acceptedTargetAppIds,
+			);
+			const onlyTarget = _.difference(acceptedTargetAppIds, currentAppIds);
+
+			// We do not want to remove rejected apps, so we compare with the
+			// original target id list
 			const onlyCurrent = _.difference(currentAppIds, targetAppIds);
 
 			// For apps that exist in both current and target state, calculate what we need to
@@ -502,34 +513,27 @@ export async function setTarget(
 	trx: Transaction,
 ) {
 	const setInTransaction = async (
-		$filteredApps: TargetApps,
+		$apps: TargetApps,
+		$rejectedApps: string[],
 		$trx: Transaction,
 	) => {
-		await dbFormat.setApps($filteredApps, source, $trx);
+		await dbFormat.setApps($apps, source, $rejectedApps, $trx);
 		await $trx('app')
 			.where({ source })
 			.whereNotIn(
 				'appId',
-				// Use apps here, rather than filteredApps, to
-				// avoid removing a release from the database
-				// without an application to replace it.
-				// Currently this will only happen if the release
-				// which would replace it fails a contract
-				// validation check
-				Object.values(apps).map(({ id: appId }) => appId),
+				// Delete every appId not in the target list
+				Object.values($apps).map(({ id: appId }) => appId),
 			)
 			.del();
 	};
 
-	// We look at the container contracts here, as if we
-	// cannot run the release, we don't want it to be added
-	// to the database, overwriting the current release. This
-	// is because if we just reject the release, but leave it
-	// in the db, if for any reason the current state stops
-	// running, we won't restart it, leaving the device
-	// useless - The exception to this rule is when the only
-	// failing services are marked as optional, then we
-	// filter those out and add the target state to the database
+	// We look at the container contracts here, apps with failing contract requirements
+	// are stored in the database with a `rejected: true property`, which tells
+	// the inferNextSteps function to ignore them when making changes.
+	//
+	// Apps with optional services with unmet requirements are stored as
+	// `rejected: false`, but services with unmet requirements are removed
 	const contractViolators: contracts.ContractViolators = {};
 	const fulfilledContracts = contracts.validateTargetContracts(apps);
 	const filteredApps = structuredClone(apps);
@@ -538,14 +542,13 @@ export async function setTarget(
 		{ valid, unmetServices, unmetAndOptional },
 	] of Object.entries(fulfilledContracts)) {
 		if (!valid) {
+			// Add the app to the list of contract violators to generate a system
+			// error
 			contractViolators[appUuid] = {
 				appId: apps[appUuid].id,
 				appName: apps[appUuid].name,
 				services: unmetServices.map(({ serviceName }) => serviceName),
 			};
-
-			// Remove the invalid app from the list
-			delete filteredApps[appUuid];
 		} else {
 			// App is valid, but we could still be missing
 			// some optional containers, and need to filter
@@ -563,17 +566,22 @@ export async function setTarget(
 		}
 	}
 
-	await setInTransaction(filteredApps, trx);
+	let rejectedApps: string[] = [];
 	if (!_.isEmpty(contractViolators)) {
-		// TODO: add rejected state for contract violator apps
-		throw new contracts.ContractViolationError(contractViolators);
+		rejectedApps = Object.keys(contractViolators);
+		reportRejectedReleases(contractViolators);
 	}
+	await setInTransaction(filteredApps, rejectedApps, trx);
 }
 
 export async function getTargetApps(): Promise<TargetApps> {
 	return await dbFormat.getTargetJson();
 }
 
+export async function getTargetAppsWithRejections() {
+	return await dbFormat.getTargetWithRejections();
+}
+
 /**
  * This is only used by the API. Do not use as the use of serviceIds is getting
  * deprecated
@@ -778,12 +786,19 @@ function reportOptionalContainers(serviceNames: string[]) {
 		'. ',
 	)}`;
 	log.info(message);
-	return logger.logSystemMessage(
-		message,
-		{},
-		'optionalContainerViolation',
-		true,
+	logger.logSystemMessage(message, {});
+}
+
+function reportRejectedReleases(violators: contracts.ContractViolators) {
+	const appStrings = Object.values(violators).map(
+		({ appName, services }) =>
+			`${appName}: Services with unmet requirements: ${services.join(', ')}`,
 	);
+	const message = `Some releases were rejected due to having unmet requirements:\n  ${appStrings.join(
+		'\n  ',
+	)}`;
+	log.error(message);
+	logger.logSystemMessage(message, { error: true });
 }
 
 /**

src/compose/types/app.ts

@@ -21,6 +21,7 @@ export interface App {
 	commit?: string;
 	source?: string;
 	isHost?: boolean;
+	isRejected?: boolean;
 	// Services are stored as an array, as at any one time we could have more than one
 	// service for a single service ID running (for example handover)
 	services: Service[];

src/compose/types/application-manager.ts

@@ -1,3 +1,5 @@
 import type { App } from './app';
 
 export type InstancedAppState = { [appId: number]: App };
+
+export type AppRelease = { appUuid: string; releaseUuid: string };

src/device-state/db-format.ts

@@ -1,5 +1,3 @@
-import _ from 'lodash';
-
 import type * as db from '../db';
 import * as targetStateCache from './target-state-cache';
 import type { DatabaseApp, DatabaseService } from './target-state-cache';
@@ -7,13 +5,8 @@ import type { DatabaseApp, DatabaseService } from './target-state-cache';
 import { App } from '../compose/app';
 import * as images from '../compose/images';
 
-import type {
-	TargetApp,
-	TargetApps,
-	TargetRelease,
-	TargetService,
-} from '../types/state';
-import type { InstancedAppState } from '../compose/types';
+import type { UUID, TargetApps, TargetRelease, TargetService } from '../types';
+import type { InstancedAppState, AppRelease } from '../compose/types';
 
 type InstancedApp = InstancedAppState[0];
 
@@ -40,16 +33,17 @@ export async function getApps(): Promise<InstancedAppState> {
 export async function setApps(
 	apps: TargetApps,
 	source: string,
+	rejectedApps: UUID[] = [],
 	trx?: db.Transaction,
 ) {
 	const dbApps = Object.keys(apps).map((uuid) => {
 		const { id: appId, ...app } = apps[uuid];
+		const rejected = rejectedApps.includes(uuid);
 
 		// Get the first uuid
-		const [releaseUuid] = Object.keys(app.releases);
-		const release = releaseUuid
-			? app.releases[releaseUuid]
-			: ({} as TargetRelease);
+		const releaseUuid = Object.keys(app.releases).shift();
+		const release =
+			releaseUuid != null ? app.releases[releaseUuid] : ({} as TargetRelease);
 
 		const services = Object.keys(release.services ?? {}).map((serviceName) => {
 			const { id: releaseId } = release;
@@ -77,9 +71,13 @@ export async function setApps(
 			uuid,
 			source,
 			isHost: !!app.is_host,
+			rejected,
 			class: app.class,
 			name: app.name,
-			...(releaseUuid && { releaseId: release.id, commit: releaseUuid }),
+			...(releaseUuid && {
+				releaseId: release.id,
+				commit: releaseUuid,
+			}),
 			services: JSON.stringify(services),
 			networks: JSON.stringify(release.networks ?? {}),
 			volumes: JSON.stringify(release.volumes ?? {}),
@@ -92,67 +90,78 @@ export async function setApps(
 /**
  * Create target state from database state
  */
-export async function getTargetJson(): Promise<TargetApps> {
+export async function getTargetWithRejections(): Promise<{
+	apps: TargetApps;
+	rejections: AppRelease[];
+}> {
 	const dbApps = await getDBEntry();
 
-	return dbApps
-		.map(
-			({
-				source,
-				uuid,
-				releaseId,
-				commit: releaseUuid,
-				...app
-			}): [string, TargetApp] => {
-				const services = (JSON.parse(app.services) as DatabaseService[])
-					.map(
-						({
-							serviceName,
-							serviceId,
-							imageId,
-							...service
-						}): [string, TargetService] => [
-							serviceName,
-							{
-								id: serviceId,
-								image_id: imageId,
-								..._.omit(service, ['appId', 'appUuid', 'commit', 'releaseId']),
-							} as TargetService,
-						],
-					)
-					// Map by serviceName
-					.reduce(
-						(svcs, [serviceName, s]) => ({
-							...svcs,
-							[serviceName]: s,
-						}),
-						{},
-					);
+	const apps: TargetApps = {};
+	const rejections: AppRelease[] = [];
 
-				const releases = releaseUuid
-					? {
-							[releaseUuid]: {
-								id: releaseId,
-								services,
-								networks: JSON.parse(app.networks),
-								volumes: JSON.parse(app.volumes),
-							} as TargetRelease,
-						}
-					: {};
-
-				return [
-					uuid,
+	for (const {
+		source,
+		rejected,
+		uuid,
+		releaseId,
+		commit: releaseUuid,
+		...app
+	} of dbApps) {
+		const services = Object.fromEntries(
+			(JSON.parse(app.services) as DatabaseService[]).map(
+				({
+					serviceName,
+					serviceId,
+					imageId,
+					// Ignore these fields
+					appId: _appId,
+					appUuid: _appUuid,
+					commit: _commit,
+					releaseId: _releaseId,
+					// Use the remainder of the fields for the service description
+					...service
+				}) => [
+					serviceName,
 					{
-						id: app.appId,
-						name: app.name,
-						class: app.class,
-						is_host: !!app.isHost,
-						releases,
-					},
-				];
-			},
-		)
-		.reduce((apps, [uuid, app]) => ({ ...apps, [uuid]: app }), {});
+						id: serviceId,
+						image_id: imageId,
+						...service,
+					} satisfies TargetService,
+				],
+			),
+		);
+
+		const releases =
+			releaseUuid && releaseId
+				? {
+						[releaseUuid]: {
+							id: releaseId,
+							services,
+							networks: JSON.parse(app.networks),
+							volumes: JSON.parse(app.volumes),
+						} satisfies TargetRelease,
+					}
+				: {};
+
+		if (rejected && releaseUuid) {
+			rejections.push({ appUuid: uuid, releaseUuid });
+		}
+
+		apps[uuid] = {
+			id: app.appId,
+			name: app.name,
+			class: app.class,
+			is_host: !!app.isHost,
+			releases,
+		};
+	}
+
+	return { apps, rejections };
+}
+
+export async function getTargetJson(): Promise<TargetApps> {
+	const { apps } = await getTargetWithRejections();
+	return apps;
 }
 
 function getDBEntry(): Promise<DatabaseApp[]>;

src/device-state/index.ts

@@ -26,12 +26,7 @@ import type { InstancedDeviceState } from './target-state';
 import * as TargetState from './target-state';
 export { getTarget, setTarget } from './target-state';
 
-import type {
-	DeviceLegacyState,
-	DeviceState,
-	DeviceReport,
-	AppState,
-} from '../types';
+import type { DeviceLegacyState, DeviceState, DeviceReport } from '../types';
 import type {
 	CompositionStepT,
 	CompositionStepAction,
@@ -366,18 +361,34 @@ export async function getCurrentForReport(
 ): Promise<DeviceState> {
 	const apps = await applicationManager.getState();
 
+	const { apps: targetApps, rejections } =
+		await applicationManager.getTargetAppsWithRejections();
+	const targetAppUuids = Object.keys(targetApps);
+
 	// Fiter current apps by the target state as the supervisor cannot
 	// report on apps for which it doesn't have API permissions
-	const targetAppUuids = Object.keys(await applicationManager.getTargetApps());
-	const appsForReport = Object.keys(apps)
-		.filter((appUuid) => targetAppUuids.includes(appUuid))
-		.reduce(
-			(filteredApps, appUuid) => ({
-				...filteredApps,
-				[appUuid]: apps[appUuid],
-			}),
-			{} as { [appUuid: string]: AppState },
-		);
+	// this step also adds rejected commits for the report
+	const appsForReport = Object.fromEntries(
+		Object.entries(apps).flatMap(([appUuid, app]) => {
+			if (!targetAppUuids.includes(appUuid)) {
+				return [];
+			}
+
+			for (const r of rejections) {
+				if (r.appUuid !== appUuid) {
+					continue;
+				}
+
+				// Add the rejected release to apps for report
+				app.releases[r.releaseUuid] = {
+					update_status: 'rejected',
+					services: {},
+				};
+			}
+
+			return [[appUuid, app]];
+		}),
+	);
 
 	const { uuid, localMode } = await config.getMany(['uuid', 'localMode']);
 

src/device-state/target-state-cache.ts

@@ -8,6 +8,8 @@ import type { TargetAppClass } from '../types';
 // at all, and we can use the below type for both insertion and retrieval.
 export interface DatabaseApp {
 	name: string;
+	// releaseId and commit may be empty as the device could
+	// have been moved to an app without any releases
 	/**
 	 * @deprecated to be removed in target state v4
 	 */
@@ -24,6 +26,7 @@ export interface DatabaseApp {
 	source: string;
 	class: TargetAppClass;
 	isHost: boolean;
+	rejected: boolean;
 }
 
 export type DatabaseService = {

src/migrations/M00012.js

@@ -0,0 +1,10 @@
+export async function up(knex) {
+	// Add a `rejected` field to the target app
+	await knex.schema.table('app', (table) => {
+		table.boolean('rejected').defaultTo(false);
+	});
+}
+
+export function down() {
+	throw new Error('Not implemented');
+}

src/types/state.ts

@@ -127,6 +127,7 @@ const fromType = <T extends object>(name: string) =>
 
 // Alias short string to UUID so code reads more clearly
 export const UUID = ShortString;
+export type UUID = t.TypeOf<typeof UUID>;
 
 /** ***************
  * Current state *

test/integration/compose/application-manager.spec.ts

@@ -1595,6 +1595,97 @@ describe('compose/application-manager', () => {
 			.that.deep.includes({ name: 'main-image' });
 	});
 
+	it('should not calculate steps for a rejected app', async () => {
+		const targetApps = createApps(
+			{
+				services: [
+					await createService({
+						running: true,
+						image: 'main-image-1',
+						appId: 1,
+						appUuid: 'app-one',
+						commit: 'commit-for-app-1',
+					}),
+					await createService({
+						running: true,
+						image: 'main-image-2',
+						appId: 2,
+						appUuid: 'app-two',
+						commit: 'commit-for-app-2',
+					}),
+				],
+				networks: [
+					// Default networks for two apps
+					Network.fromComposeObject('default', 1, 'app-one', {}),
+					Network.fromComposeObject('default', 2, 'app-two', {}),
+				],
+				rejectedAppIds: [1],
+			},
+			true,
+		);
+		const { currentApps, availableImages, downloading, containerIdsByAppId } =
+			createCurrentState({
+				services: [
+					await createService({
+						running: true,
+						image: 'old-image-1',
+						appId: 1,
+						appUuid: 'app-one',
+						commit: 'commit-for-app-0',
+					}),
+				],
+				networks: [
+					// Default networks for two apps
+					Network.fromComposeObject('default', 1, 'app-one', {}),
+					Network.fromComposeObject('default', 2, 'app-two', {}),
+				],
+				images: [
+					createImage({
+						name: 'main-image-1',
+						appId: 1,
+						appUuid: 'app-one',
+						serviceName: 'main',
+						commit: 'commit-for-app-1',
+					}),
+					createImage({
+						name: 'main-image-2',
+						appId: 2,
+						appUuid: 'app-two',
+						serviceName: 'main',
+						commit: 'commit-for-app-2',
+					}),
+				],
+			});
+
+		const steps = await applicationManager.inferNextSteps(
+			currentApps,
+			targetApps,
+			{
+				downloading,
+				availableImages,
+				containerIdsByAppId,
+				// Mock locks taken to avoid takeLock step
+				locksTaken: new LocksTakenMap([{ appId: 2, services: ['main'] }]),
+			},
+		);
+
+		// Expect a start step for both apps
+		expect(
+			steps.filter((s: any) => s.target && s.target.appId === 1),
+		).to.have.lengthOf(0);
+		expect(
+			steps.filter((s: any) => s.image && s.image.appId === 1),
+		).to.have.lengthOf(0);
+		expect(
+			steps.filter(
+				(s: any) =>
+					s.action === 'start' &&
+					s.target.appId === 2 &&
+					s.target.serviceName === 'main',
+			),
+		).to.have.lengthOf(1);
+	});
+
 	it('should correctly generate steps for multiple apps', async () => {
 		const targetApps = createApps(
 			{

test/integration/device-state.spec.ts

@@ -368,6 +368,7 @@ describe('device-state', () => {
 		const app = targetState.local.apps[1234];
 		expect(app).to.have.property('appName').that.equals('superapp');
 		expect(app).to.have.property('commit').that.equals('one');
+		expect(app).to.have.property('isRejected').that.is.false;
 		// Only a single service should be on the target state
 		expect(app).to.have.property('services').that.is.an('array').with.length(1);
 		expect(app.services[0])
@@ -375,7 +376,8 @@ describe('device-state', () => {
 			.that.equals('valid');
 	});
 
-	it('rejects a target state with invalid contract and non optional service', async () => {
+	it('accepts target state with invalid contract setting isRejected to true and resets state when a valid target is received', async () => {
+		// Set the rejected target
 		await expect(
 			deviceState.setTarget({
 				local: {
@@ -424,7 +426,66 @@ describe('device-state', () => {
 					},
 				},
 			} as TargetState),
-		).to.be.rejected;
+		).to.not.be.rejected;
+		const targetState = await deviceState.getTarget();
+		const app = targetState.local.apps[1234];
+		expect(app).to.have.property('appName').that.equals('superapp');
+		expect(app).to.have.property('commit').that.equals('one');
+		expect(app).to.have.property('isRejected').that.is.true;
+
+		// Now set a good target for the same app
+		await deviceState.setTarget({
+			local: {
+				name: 'aDeviceWithDifferentName',
+				config: {},
+				apps: {
+					myapp: {
+						id: 1234,
+						name: 'superapp',
+						class: 'fleet',
+						releases: {
+							two: {
+								id: 2,
+								services: {
+									valid: {
+										id: 23,
+										image_id: 12345,
+										image: 'registry2.resin.io/superapp/valid',
+										environment: {},
+										labels: {},
+									},
+									alsoValid: {
+										id: 24,
+										image_id: 12346,
+										image: 'registry2.resin.io/superapp/afaff',
+										contract: {
+											type: 'sw.container',
+											slug: 'supervisor-version',
+											name: 'Enforce supervisor version',
+											requires: [
+												{
+													type: 'sw.supervisor',
+													version: '>=11.0.0',
+												},
+											],
+										},
+										environment: {},
+										labels: {},
+									},
+								},
+								volumes: {},
+								networks: {},
+							},
+						},
+					},
+				},
+			},
+		} as TargetState);
+
+		const targetState2 = await deviceState.getTarget();
+		const app2 = targetState2.local.apps[1234];
+		expect(app2).to.have.property('commit').that.equals('two');
+		expect(app2).to.have.property('isRejected').that.is.false;
 	});
 
 	// TODO: There is no easy way to test this behaviour with the current

test/lib/state-helper.ts

@@ -85,6 +85,7 @@ export function createApp({
 	isTarget = false,
 	appId = 1,
 	appUuid = 'appuuid',
+	isRejected = false,
 } = {}) {
 	return new App(
 		{
@@ -93,6 +94,7 @@ export function createApp({
 			services,
 			networks,
 			volumes,
+			isRejected,
 		},
 		isTarget,
 	);
@@ -103,6 +105,7 @@ export function createApps(
 		services = [] as Service[],
 		networks = [] as Network[],
 		volumes = [] as Volume[],
+		rejectedAppIds = [] as number[],
 	},
 	target = false,
 ) {
@@ -129,6 +132,7 @@ export function createApps(
 
 	const apps: InstancedAppState = {};
 	for (const appId of allAppIds) {
+		const isRejected = rejectedAppIds.includes(appId);
 		apps[appId] = createApp({
 			services: servicesByAppId[appId] ?? [],
 			networks: networksByAppId[appId] ?? [],
@@ -136,6 +140,7 @@ export function createApps(
 			appId,
 			appUuid: servicesByAppId[appId]?.[0]?.appUuid ?? 'deadbeef',
 			isTarget: target,
+			isRejected,
 		});
 	}