Merge pull request #2065 from balena-os/simplify-getting-images-for-cleanup

Simplify getting images for cleanup

src/compose/images.ts

@@ -465,72 +465,71 @@ export const save = async (image: Image): Promise<void> => {
 	await markAsSupervised(image);
 };
 
-async function getImagesForCleanup(): Promise<string[]> {
+// TODO: remove after we agree on what to do for Supervisor image cleanup after hup
-	const images: string[] = [];
+const getSupervisorRepos = (imageName: string) => {
+	const supervisorRepos = new Set<string>();
+	supervisorRepos.add(imageName);
+	// If we're on the new balena/ARCH-supervisor image, add legacy image.
+	// If the image name is legacy, the `replace` will have no effect.
+	supervisorRepos.add(imageName.replace(/^balena/, 'resin'));
+	return [...supervisorRepos];
+};
 
-	const supervisorImageInfo = dockerUtils.getRegistryAndName(
+// TODO: same as above, we no longer use tags to identify supervisors
+const isSupervisorRepoTag = ({
+	repoTag,
+	svRepos,
+	svTag,
+}: {
+	repoTag: string;
+	svRepos: ReturnType<typeof getSupervisorRepos>;
+	svTag?: string;
+}) => {
+	const { imageName, tagName } = dockerUtils.getRegistryAndName(repoTag);
+	return svRepos.some((repo) => imageName === repo) && tagName !== svTag;
+};
+
+async function getImagesForCleanup(): Promise<Array<Docker.ImageInfo['Id']>> {
+	// Get Supervisor image metadata for exclusion from image cleanup
+	const { imageName: svImage, tagName: svTag } = dockerUtils.getRegistryAndName(
 		constants.supervisorImage,
 	);
-	const [supervisorImage, usedImageIds] = await Promise.all([
+	const svRepos = getSupervisorRepos(svImage);
-		docker.getImage(constants.supervisorImage).inspect(),
-		db
-			.models('image')
-			.select('dockerImageId')
-			.then((vals) => vals.map((img: Image) => img.dockerImageId)),
-	]);
 
-	// TODO: remove after we agree on what to do for
+	const usedImageIds: string[] = await db
-	// supervisor image cleanup after hup
+		.models('image')
-	const supervisorRepos = [supervisorImageInfo.imageName];
+		.select('dockerImageId')
-	// If we're on the new balena/ARCH-supervisor image
+		.then((vals) => vals.map(({ dockerImageId }: Image) => dockerImageId));
-	if (_.startsWith(supervisorImageInfo.imageName, 'balena/')) {
-		supervisorRepos.push(
-			supervisorImageInfo.imageName.replace(/^balena/, 'resin'),
-		);
-	}
-
-	// TODO: same as above, we no longer use tags to identify supervisors
-	const isSupervisorRepoTag = ({
-		imageName,
-		tagName,
-	}: {
-		imageName: string;
-		tagName?: string;
-	}) => {
-		return (
-			_.some(supervisorRepos, (repo) => imageName === repo) &&
-			tagName !== supervisorImageInfo.tagName
-		);
-	};
 
 	const dockerImages = await docker.listImages({ digests: true });
+
+	const imagesToCleanup = new Set<Docker.ImageInfo['Id']>();
 	for (const image of dockerImages) {
 		// Cleanup should remove truly dangling images (i.e dangling and with no digests)
-		if (isDangling(image) && !_.includes(usedImageIds, image.Id)) {
+		if (isDangling(image) && !usedImageIds.includes(image.Id)) {
-			images.push(image.Id);
+			imagesToCleanup.add(image.Id);
-		} else if (!_.isEmpty(image.RepoTags) && image.Id !== supervisorImage.Id) {
+			continue;
-			// We also remove images from the supervisor repository with a different tag
+		}
-			for (const tag of image.RepoTags) {
+
-				const imageNameComponents = dockerUtils.getRegistryAndName(tag);
+		// We also remove images from the Supervisor repository with a different tag
-				// If
+		for (const repoTag of image.RepoTags || []) {
-				if (isSupervisorRepoTag(imageNameComponents)) {
+			if (isSupervisorRepoTag({ repoTag, svRepos, svTag })) {
-					images.push(image.Id);
+				imagesToCleanup.add(image.Id);
-				}
 			}
 		}
 	}
 
-	return _(images)
+	return [...imagesToCleanup].filter(
-		.uniq()
+		(image) =>
-		.filter(
+			imageCleanupFailures[image] == null ||
-			(image) =>
+			Date.now() - imageCleanupFailures[image] >
-				imageCleanupFailures[image] == null ||
+				constants.imageCleanupErrorIgnoreTimeout,
-				Date.now() - imageCleanupFailures[image] >
+	);
-					constants.imageCleanupErrorIgnoreTimeout,
-		)
-		.value();
 }
 
+export const isCleanupNeeded = async () =>
+	(await getImagesForCleanup()).length > 0;
+
 // Look for an image in the engine with registry/image as reference (tag)
 // for images with deltas this should return unless there is some inconsistency
 // and the tag was deleted.
@@ -606,10 +605,6 @@ export async function inspectByName(imageName: string) {
 	);
 }
 
-export async function isCleanupNeeded() {
-	return !_.isEmpty(await getImagesForCleanup());
-}
-
 export async function cleanup() {
 	const images = await getImagesForCleanup();
 	for (const image of images) {

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

@@ -1,6 +1,5 @@
 import { expect } from 'chai';
 import * as sinon from 'sinon';
-import { stub } from 'sinon';
 import * as Docker from 'dockerode';
 import App from '~/src/compose/app';
 import * as applicationManager from '~/src/compose/application-manager';
@@ -14,6 +13,7 @@ import { ServiceComposeConfig } from '~/src/compose/types/service';
 import Volume from '~/src/compose/volume';
 import { InstancedAppState } from '~/src/types/state';
 import * as config from '~/src/config';
+import { createDockerImage } from '~/test-lib/docker-helper';
 
 const DEFAULT_NETWORK = Network.fromComposeObject('default', 1, 'appuuid', {});
 
@@ -171,9 +171,6 @@ function createCurrentState({
 // the app spec, remove that redundancy to simplify the tests
 describe('compose/application-manager', () => {
 	before(async () => {
-		// Stub methods that depend on external dependencies
-		stub(imageManager, 'isCleanupNeeded');
-
 		// Service.fromComposeObject gets api keys from the database
 		// which also depend on the local mode. This ensures the database
 		// is initialized. This can be removed when ApplicationManager and Service
@@ -182,17 +179,10 @@ describe('compose/application-manager', () => {
 	});
 
 	beforeEach(async () => {
-		// Do not check for cleanup images by default
-		(imageManager.isCleanupNeeded as sinon.SinonStub).resolves(false);
 		// Set up network by default
 		await networkManager.ensureSupervisorNetwork();
 	});
 
-	after(() => {
-		// Restore stubs
-		(imageManager.isCleanupNeeded as sinon.SinonStub).restore();
-	});
-
 	afterEach(async () => {
 		// Delete any created networks
 		const docker = new Docker();
@@ -927,8 +917,21 @@ describe('compose/application-manager', () => {
 	});
 
 	it('should infer a cleanup step when a cleanup is required', async () => {
-		// Stub the image manager function
+		// Create a dangling image; this is done by building an image again with
-		(imageManager.isCleanupNeeded as sinon.SinonStub).resolves(true);
+		// some slightly different metadata, leaving the old image with no metadata.
+		const docker = new Docker();
+		const dockerImageIdOne = await createDockerImage(
+			'some-image:some-tag',
+			['io.balena.testing=1'],
+			docker,
+		);
+		const dockerImageIdTwo = await createDockerImage(
+			'some-image:some-tag',
+			['io.balena.testing=2'],
+			docker,
+		);
+		// Remove the tagged image, leaving only the dangling image
+		await docker.getImage(dockerImageIdTwo).remove();
 
 		const targetApps = createApps(
 			{
@@ -958,6 +961,8 @@ describe('compose/application-manager', () => {
 			action: 'cleanup',
 		});
 		expect(nextSteps).to.have.lengthOf(0);
+
+		await docker.getImage(dockerImageIdOne).remove();
 	});
 
 	it('should infer that an image should be removed if it is no longer referenced in current or target state (only target)', async () => {
@@ -1188,7 +1193,7 @@ describe('compose/application-manager', () => {
 		).to.have.lengthOf(1);
 	});
 
-	describe('getting applications current state', () => {
+	describe("getting application's current state", () => {
 		let getImagesState: sinon.SinonStub;
 		let getServicesState: sinon.SinonStub;