From 3385bfa180a8b3e6f30a91168e384adb3fb907df Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Thu, 2 Mar 2023 17:29:17 -0300 Subject: [PATCH 1/5] Remove releaseId from container name The release id is not really necessary for reporting the current state since v3 (it has been replaced by `releaseUuid`) and is only kept for backwards compatibility for some supervisor API endpoints. Until now, the `releaseId` was part o the container name, leading to longer names than desired. This change removes the value from the container name and modifies queries to look for the value in the image database. Change-type: minor Relates-to: #2077 --- src/compose/application-manager.ts | 18 +++++++++++++++--- src/compose/images.ts | 2 ++ src/compose/service-manager.ts | 5 ++--- src/compose/service.ts | 13 +++++++++---- src/device-api/actions.ts | 10 ++++++++++ test/integration/device-api/actions.spec.ts | 1 - test/integration/state-engine.spec.ts | 18 +++++++++--------- 7 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 051d65bb8..8596484ff 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -350,6 +350,7 @@ export async function getCurrentApps(): Promise { ); s.imageName = imageForService?.name ?? s.imageName; + s.releaseId = imageForService?.releaseId ?? s.releaseId; return s; }); @@ -695,7 +696,10 @@ function saveAndRemoveImages( availableWithoutIds, (image) => !_.some(currentImages.concat(targetImages), (imageInUse) => { - return _.isEqual(image, _.omit(imageInUse, ['dockerImageId', 'id'])); + return _.isEqual( + _.omit(image, ['releaseId']), + _.omit(imageInUse, ['dockerImageId', 'id', 'releaseId']), + ); }), ); @@ -840,10 +844,18 @@ export async function getLegacyState() { if (apps[appId].services == null) { apps[appId].services = {}; } + + // Get releaseId from the list of images if it can be found + service.releaseId = images.find( + (img) => + img.serviceName === service.serviceName && + img.commit === service.commit, + )?.releaseId; + // We only send commit if all services have the same release, and it matches the target release if (releaseId == null) { - ({ releaseId } = service); - } else if (releaseId !== service.releaseId) { + releaseId = service.releaseId; + } else if (service.releaseId != null && releaseId !== service.releaseId) { releaseId = false; } if (imageId == null) { diff --git a/src/compose/images.ts b/src/compose/images.ts index 8aa1d7f66..8fcdc1ff0 100644 --- a/src/compose/images.ts +++ b/src/compose/images.ts @@ -180,6 +180,8 @@ export function imageFromService(service: ServiceInfo): Image { serviceId: service.serviceId!, serviceName: service.serviceName!, imageId: service.imageId!, + // Release id is optional in service, but it is always available + // on the target state releaseId: service.releaseId!, commit: service.commit!, }; diff --git a/src/compose/service-manager.ts b/src/compose/service-manager.ts index ecb05a315..e9d52e3a0 100644 --- a/src/compose/service-manager.ts +++ b/src/compose/service-manager.ts @@ -125,7 +125,6 @@ export async function getState() { 'appUuid', 'imageId', 'status', - 'releaseId', 'commit', 'createdAt', 'serviceName', @@ -159,7 +158,7 @@ export async function updateMetadata(service: Service, target: Service) { try { await docker.getContainer(svc.containerId).rename({ - name: `${service.serviceName}_${target.imageId}_${target.releaseId}_${target.commit}`, + name: `${service.serviceName}_${target.imageId}_${target.commit}`, }); } catch (e) { if (isNotFoundError(e)) { @@ -633,7 +632,7 @@ async function prepareForHandover(service: Service) { const container = docker.getContainer(svc.containerId); await container.update({ RestartPolicy: {} }); return await container.rename({ - name: `old_${service.serviceName}_${service.imageId}_${service.releaseId}_${service.commit}`, + name: `old_${service.serviceName}_${service.imageId}_${service.commit}`, }); } diff --git a/src/compose/service.ts b/src/compose/service.ts index 8e0a857c8..909666e4a 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -56,7 +56,12 @@ export class Service { public config: ServiceConfig; public serviceName: string; public commit: string; - public releaseId: number; + /** + * We make this available as it still needed by application manager + * but it is only available on the target state + * @deprecated + */ + public releaseId?: number; public serviceId: number; public imageName: string | null; public containerId: string | null; @@ -617,10 +622,10 @@ export class Service { 'Attempt to build Service class from container with malformed labels', ); } - const nameMatch = container.Name.match(/.*_(\d+)_(\d+)(?:_(.*?))?$/); + const nameMatch = container.Name.match(/.*_(\d+)(?:_(\d+))?(?:_(.*?))?$/); if (nameMatch == null) { throw new InternalInconsistencyError( - `Expected supervised container to have name '___', got: ${container.Name}`, + `Expected supervised container to have name '__', got: ${container.Name}`, ); } @@ -674,7 +679,7 @@ export class Service { this.config.networkMode = `container:${containerId}`; } return { - name: `${this.serviceName}_${this.imageId}_${this.releaseId}_${this.commit}`, + name: `${this.serviceName}_${this.imageId}_${this.commit}`, Tty: this.config.tty, Cmd: this.config.command, Volumes: volumes, diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index 1a83e52d8..d8b4f8972 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -8,6 +8,7 @@ import * as logger from '../logger'; import * as config from '../config'; import * as hostConfig from '../host-config'; import * as applicationManager from '../compose/application-manager'; +import * as imageManager from '../compose/images'; import { CompositionStepAction, generateStep, @@ -331,6 +332,15 @@ export const getSingleContainerApp = async (appId: number) => { ); } + // releaseId is not part of the documented outputs of this endpoint + // but it has been there for a while, we query the value from + // the image list if available. But we won't error if not found + const images = await imageManager.getState(); + service.releaseId = images.find( + (img) => + img.serviceName === service.serviceName && img.commit === service.commit, + )?.releaseId; + // Because we only have a single app, we can fetch the commit for that // app, and maintain backwards compatability const commit = await commitStore.getCommitForApp(appId); diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index a5316896e..d0c6b1943 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -408,7 +408,6 @@ describe('manages application lifecycle', () => { expect(body).to.have.property('appId', APP_ID); expect(body).to.have.property('containerId', containerId); expect(body).to.have.property('imageId', imageHash); - expect(body).to.have.property('releaseId', 1); // Should return the environment of the single service expect(body.env).to.have.property('BALENA_APP_ID', String(APP_ID)); expect(body.env).to.have.property('BALENA_SERVICE_NAME', serviceNames[0]); diff --git a/test/integration/state-engine.spec.ts b/test/integration/state-engine.spec.ts index 5fbb77b72..03952e730 100644 --- a/test/integration/state-engine.spec.ts +++ b/test/integration/state-engine.spec.ts @@ -134,8 +134,8 @@ describe('state engine', () => { expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_11_1_deadbeef', State: 'running' }, - { Name: '/two_12_1_deadbeef', State: 'running' }, + { Name: '/one_11_deadbeef', State: 'running' }, + { Name: '/two_12_deadbeef', State: 'running' }, ]); // Test that the service is running and accesssible via port 8080 @@ -181,7 +181,7 @@ describe('state engine', () => { const containers = await docker.listContainers(); expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), - ).to.have.deep.members([{ Name: '/one_21_2_deadca1f', State: 'running' }]); + ).to.have.deep.members([{ Name: '/one_21_deadca1f', State: 'running' }]); const containerIds = containers.map(({ Id }) => Id); await setTargetState({ @@ -237,8 +237,8 @@ describe('state engine', () => { expect( updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_21_2_deadca1f', State: 'running' }, - { Name: '/two_22_2_deadca1f', State: 'running' }, + { Name: '/one_21_deadca1f', State: 'running' }, + { Name: '/two_22_deadca1f', State: 'running' }, ]); // Container ids must have changed @@ -301,8 +301,8 @@ describe('state engine', () => { expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_11_1_deadbeef', State: 'running' }, - { Name: '/two_12_1_deadbeef', State: 'running' }, + { Name: '/one_11_deadbeef', State: 'running' }, + { Name: '/two_12_deadbeef', State: 'running' }, ]); const containerIds = containers.map(({ Id }) => Id); @@ -359,8 +359,8 @@ describe('state engine', () => { expect( updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_21_2_deadca1f', State: 'running' }, - { Name: '/two_22_2_deadca1f', State: 'running' }, + { Name: '/one_21_deadca1f', State: 'running' }, + { Name: '/two_22_deadca1f', State: 'running' }, ]); // Container ids must have changed From f37ee1b8901c6833cfe73540f8a54a9229862441 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 7 Mar 2023 18:15:41 -0300 Subject: [PATCH 2/5] Remove imageId from container name The image id is no longer necessary to report the current state since moving to v3 of the state endpoint and it is only kept for backwards compatibility for some supervisor API endpoings. Until now, `imageId` was part of the container name leading to longer names than desired. This change removes the value from the container name an modifies queries to look for the value in the image database. This also removes the imageId from the log stream which should result in reduced bandwidth usage. Change-type: minor --- src/compose/application-manager.ts | 45 ++++++++++----------- src/compose/images.ts | 2 + src/compose/service-manager.ts | 42 ++++++------------- src/compose/service.ts | 18 ++++++--- src/device-api/actions.ts | 4 +- src/device-api/v2.ts | 13 ++++-- src/logger.ts | 5 +-- test/integration/device-api/actions.spec.ts | 2 +- test/integration/state-engine.spec.ts | 18 ++++----- 9 files changed, 72 insertions(+), 77 deletions(-) diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index 8596484ff..74a3b5403 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -351,6 +351,7 @@ export async function getCurrentApps(): Promise { s.imageName = imageForService?.name ?? s.imageName; s.releaseId = imageForService?.releaseId ?? s.releaseId; + s.imageId = imageForService?.imageId ?? s.imageId; return s; }); @@ -697,8 +698,8 @@ function saveAndRemoveImages( (image) => !_.some(currentImages.concat(targetImages), (imageInUse) => { return _.isEqual( - _.omit(image, ['releaseId']), - _.omit(imageInUse, ['dockerImageId', 'id', 'releaseId']), + _.omit(image, ['releaseId', 'imageId']), + _.omit(imageInUse, ['dockerImageId', 'id', 'releaseId', 'imageId']), ); }), ); @@ -833,8 +834,16 @@ export async function getLegacyState() { // We iterate over the current running services and add them to the current state // of the app they belong to. for (const service of services) { - const { appId, imageId } = service; - if (!appId) { + const { appId } = service; + + // We get the image for the service so we can get service metadata not + // in the containers + const imageForService = images.find( + (img) => + img.serviceName === service.serviceName && + img.commit === service.commit, + ); + if (!appId || !imageForService) { continue; } if (apps[appId] == null) { @@ -845,12 +854,10 @@ export async function getLegacyState() { apps[appId].services = {}; } - // Get releaseId from the list of images if it can be found - service.releaseId = images.find( - (img) => - img.serviceName === service.serviceName && - img.commit === service.commit, - )?.releaseId; + const { releaseId: rId, imageId } = imageForService; + + // Replace the service releaseId with the one from the image table above + service.releaseId = rId; // We only send commit if all services have the same release, and it matches the target release if (releaseId == null) { @@ -858,11 +865,7 @@ export async function getLegacyState() { } else if (service.releaseId != null && releaseId !== service.releaseId) { releaseId = false; } - if (imageId == null) { - throw new InternalInconsistencyError( - `imageId not defined in ApplicationManager.getLegacyApplicationsState: ${service}`, - ); - } + if (apps[appId].services[imageId] == null) { apps[appId].services[imageId] = _.pick(service, ['status', 'releaseId']); creationTimesAndReleases[appId][imageId] = _.pick(service, [ @@ -881,20 +884,16 @@ export async function getLegacyState() { } for (const image of images) { - const { appId } = image; + const { appId, imageId } = image; if (apps[appId] == null) { apps[appId] = {}; } if (apps[appId].services == null) { apps[appId].services = {}; } - if (apps[appId].services[image.imageId] == null) { - apps[appId].services[image.imageId] = _.pick(image, [ - 'status', - 'releaseId', - ]); - apps[appId].services[image.imageId].download_progress = - image.downloadProgress; + if (apps[appId].services[imageId] == null) { + apps[appId].services[imageId] = _.pick(image, ['status', 'releaseId']); + apps[appId].services[imageId].download_progress = image.downloadProgress; } } diff --git a/src/compose/images.ts b/src/compose/images.ts index 8fcdc1ff0..f9fb22dee 100644 --- a/src/compose/images.ts +++ b/src/compose/images.ts @@ -179,6 +179,8 @@ export function imageFromService(service: ServiceInfo): Image { appUuid: service.appUuid!, serviceId: service.serviceId!, serviceName: service.serviceName!, + // Image id is optional in service, but it is always available + // on the target state imageId: service.imageId!, // Release id is optional in service, but it is always available // on the target state diff --git a/src/compose/service-manager.ts b/src/compose/service-manager.ts index e9d52e3a0..278c51fbb 100644 --- a/src/compose/service-manager.ts +++ b/src/compose/service-manager.ts @@ -123,7 +123,6 @@ export async function getState() { status[service.containerId] = _.pick(service, [ 'appId', 'appUuid', - 'imageId', 'status', 'commit', 'createdAt', @@ -158,7 +157,7 @@ export async function updateMetadata(service: Service, target: Service) { try { await docker.getContainer(svc.containerId).rename({ - name: `${service.serviceName}_${target.imageId}_${target.commit}`, + name: `${service.serviceName}_${target.commit}`, }); } catch (e) { if (isNotFoundError(e)) { @@ -351,14 +350,13 @@ export async function start(service: Service) { } const serviceId = service.serviceId; - const imageId = service.imageId; - if (serviceId == null || imageId == null) { + if (serviceId == null) { throw new InternalInconsistencyError( - `serviceId and imageId not defined for service: ${service.serviceName} in ServiceManager.start`, + `serviceId not defined for service: ${service.serviceName} in ServiceManager.start`, ); } - logger.attach(container.id, { serviceId, imageId }); + logger.attach(container.id, { serviceId }); if (!alreadyStarted) { logger.logSystemEvent(LogTypes.startServiceSuccess, { service }); @@ -414,15 +412,13 @@ export function listenToEvents() { }); const serviceId = service.serviceId; - const imageId = service.imageId; - if (serviceId == null || imageId == null) { + if (serviceId == null) { throw new InternalInconsistencyError( - `serviceId and imageId not defined for service: ${service.serviceName} in ServiceManager.listenToEvents`, + `serviceId not defined for service: ${service.serviceName} in ServiceManager.listenToEvents`, ); } logger.attach(data.id, { serviceId, - imageId, }); } else if (status === 'destroy') { await logMonitor.detach(data.id); @@ -466,10 +462,9 @@ export async function attachToRunning() { for (const service of services) { if (service.status === 'Running') { const serviceId = service.serviceId; - const imageId = service.imageId; - if (serviceId == null || imageId == null) { + if (serviceId == null) { throw new InternalInconsistencyError( - `serviceId and imageId not defined for service: ${service.serviceName} in ServiceManager.start`, + `serviceId not defined for service: ${service.serviceName} in ServiceManager.start`, ); } @@ -480,7 +475,6 @@ export async function attachToRunning() { } logger.attach(service.containerId, { serviceId, - imageId, }); } } @@ -521,15 +515,7 @@ function reportNewStatus( containerId, _.merge( { status }, - _.pick(service, [ - 'imageId', - 'appId', - 'appUuid', - 'serviceName', - 'releaseId', - 'createdAt', - 'commit', - ]), + _.pick(service, ['appUuid', 'serviceName', 'createdAt', 'commit']), ), ); } @@ -545,9 +531,7 @@ async function killContainer( // TODO: Remove the need for the wait flag logger.logSystemEvent(LogTypes.stopService, { service }); - if (service.imageId != null) { - reportNewStatus(containerId, service, 'Stopping'); - } + reportNewStatus(containerId, service, 'Stopping'); const containerObj = docker.getContainer(containerId); const killPromise = containerObj @@ -593,9 +577,7 @@ async function killContainer( }); }) .finally(() => { - if (service.imageId != null) { - reportChange(containerId); - } + reportChange(containerId); }); if (wait) { @@ -632,7 +614,7 @@ async function prepareForHandover(service: Service) { const container = docker.getContainer(svc.containerId); await container.update({ RestartPolicy: {} }); return await container.rename({ - name: `old_${service.serviceName}_${service.imageId}_${service.commit}`, + name: `old_${service.serviceName}_${service.commit}`, }); } diff --git a/src/compose/service.ts b/src/compose/service.ts index 909666e4a..95dff4b4f 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -52,7 +52,12 @@ export type ServiceStatus = export class Service { public appId: number; public appUuid?: string; - public imageId: number; + /** + * We make this available as it still needed by application manager to + * compare images, but it is only available in the target state. + * @deprecated + */ + public imageId?: number; public config: ServiceConfig; public serviceName: string; public commit: string; @@ -622,13 +627,16 @@ export class Service { 'Attempt to build Service class from container with malformed labels', ); } - const nameMatch = container.Name.match(/.*_(\d+)(?:_(\d+))?(?:_(.*?))?$/); + const nameMatch = container.Name.match( + /.*?(?:_(\d+))?(?:_(\d+))?(?:_([a-f0-9]*))?$/, + ); if (nameMatch == null) { throw new InternalInconsistencyError( - `Expected supervised container to have name '__', got: ${container.Name}`, + `Expected supervised container to have name '_', got: ${container.Name}`, ); } + // If we have not renamed the service yet we can still use the image id svc.imageId = parseInt(nameMatch[1], 10); svc.releaseId = parseInt(nameMatch[2], 10); svc.commit = nameMatch[3]; @@ -679,7 +687,7 @@ export class Service { this.config.networkMode = `container:${containerId}`; } return { - name: `${this.serviceName}_${this.imageId}_${this.commit}`, + name: `${this.serviceName}_${this.commit}`, Tty: this.config.tty, Cmd: this.config.command, Volumes: volumes, @@ -1139,7 +1147,7 @@ export class Service { log.warn( `Ignoring invalid compose volume definition ${ isString ? volume : JSON.stringify(volume) - }`, + } `, ); } } diff --git a/src/device-api/actions.ts b/src/device-api/actions.ts index d8b4f8972..5e4d3a253 100644 --- a/src/device-api/actions.ts +++ b/src/device-api/actions.ts @@ -274,9 +274,7 @@ export const executeServiceAction = async ({ ); } const targetService = targetApp.services.find( - (s) => - s.imageId === currentService.imageId || - s.serviceName === currentService.serviceName, + (s) => s.serviceName === currentService.serviceName, ); if (targetService == null) { throw new NotFoundError(messages.targetServiceNotFound); diff --git a/src/device-api/v2.ts b/src/device-api/v2.ts index cd71b7f47..960170cbc 100644 --- a/src/device-api/v2.ts +++ b/src/device-api/v2.ts @@ -424,25 +424,32 @@ router.get('/v2/containerId', async (req: AuthorizedRequest, res) => { router.get('/v2/state/status', async (req: AuthorizedRequest, res) => { const appIds: number[] = []; const pending = deviceState.isApplyInProgress(); + const allImages = await images.getState(); const containerStates = (await serviceManager.getAll()) .filter((service) => req.auth.isScoped({ apps: [service.appId] })) .map((svc) => { appIds.push(svc.appId); - return _.pick( + const s = _.pick( svc, 'status', 'serviceName', 'appId', - 'imageId', 'serviceId', + 'imageId', 'containerId', 'createdAt', ); + + s.imageId = allImages.find( + (img) => img.commit === svc.commit && img.serviceName === s.serviceName, + )?.imageId; + + return s; }); let downloadProgressTotal = 0; let downloads = 0; - const imagesStates = (await images.getState()) + const imagesStates = allImages .filter((img) => req.auth.isScoped({ apps: [img.appId] })) .map((img) => { appIds.push(img.appId); diff --git a/src/logger.ts b/src/logger.ts index 9919cc9bf..5fe7035da 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -137,10 +137,10 @@ export function lock(containerId: string): Bluebird.Disposer<() => void> { }); } -type ServiceInfo = { serviceId: number; imageId: number }; +type ServiceInfo = { serviceId: number }; export function attach( containerId: string, - { serviceId, imageId }: ServiceInfo, + { serviceId }: ServiceInfo, ): Bluebird { // First detect if we already have an attached log stream // for this container @@ -153,7 +153,6 @@ export function attach( containerId, (message: Parameters[0] & Partial) => { message.serviceId = serviceId; - message.imageId = imageId; log(message); }, ); diff --git a/test/integration/device-api/actions.spec.ts b/test/integration/device-api/actions.spec.ts index d0c6b1943..244d77aaf 100644 --- a/test/integration/device-api/actions.spec.ts +++ b/test/integration/device-api/actions.spec.ts @@ -128,7 +128,7 @@ describe('manages application lifecycle', () => { return { [appId]: { name: 'localapp', - commit: 'localcommit', + commit: '10ca12e1ea5e', releaseId: '1', services, volumes: { diff --git a/test/integration/state-engine.spec.ts b/test/integration/state-engine.spec.ts index 03952e730..d3c7758ad 100644 --- a/test/integration/state-engine.spec.ts +++ b/test/integration/state-engine.spec.ts @@ -134,8 +134,8 @@ describe('state engine', () => { expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_11_deadbeef', State: 'running' }, - { Name: '/two_12_deadbeef', State: 'running' }, + { Name: '/one_deadbeef', State: 'running' }, + { Name: '/two_deadbeef', State: 'running' }, ]); // Test that the service is running and accesssible via port 8080 @@ -181,7 +181,7 @@ describe('state engine', () => { const containers = await docker.listContainers(); expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), - ).to.have.deep.members([{ Name: '/one_21_deadca1f', State: 'running' }]); + ).to.have.deep.members([{ Name: '/one_deadca1f', State: 'running' }]); const containerIds = containers.map(({ Id }) => Id); await setTargetState({ @@ -237,8 +237,8 @@ describe('state engine', () => { expect( updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_21_deadca1f', State: 'running' }, - { Name: '/two_22_deadca1f', State: 'running' }, + { Name: '/one_deadca1f', State: 'running' }, + { Name: '/two_deadca1f', State: 'running' }, ]); // Container ids must have changed @@ -301,8 +301,8 @@ describe('state engine', () => { expect( containers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_11_deadbeef', State: 'running' }, - { Name: '/two_12_deadbeef', State: 'running' }, + { Name: '/one_deadbeef', State: 'running' }, + { Name: '/two_deadbeef', State: 'running' }, ]); const containerIds = containers.map(({ Id }) => Id); @@ -359,8 +359,8 @@ describe('state engine', () => { expect( updatedContainers.map(({ Names, State }) => ({ Name: Names[0], State })), ).to.have.deep.members([ - { Name: '/one_21_deadca1f', State: 'running' }, - { Name: '/two_22_deadca1f', State: 'running' }, + { Name: '/one_deadca1f', State: 'running' }, + { Name: '/two_deadca1f', State: 'running' }, ]); // Container ids must have changed From 5a84f227360852490db38d5b964c102b80fd99af Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 7 Mar 2023 19:32:10 -0300 Subject: [PATCH 3/5] Use the state-helper functions in app module tests --- test/unit/compose/app.spec.ts | 67 ++++++++--------------------------- 1 file changed, 15 insertions(+), 52 deletions(-) diff --git a/test/unit/compose/app.spec.ts b/test/unit/compose/app.spec.ts index a068d4bf5..60a20300d 100644 --- a/test/unit/compose/app.spec.ts +++ b/test/unit/compose/app.spec.ts @@ -7,9 +7,10 @@ import { import { Image } from '~/src/compose/images'; import Network from '~/src/compose/network'; import Service from '~/src/compose/service'; -import { ServiceComposeConfig } from '~/src/compose/types/service'; import Volume from '~/src/compose/volume'; +import { createService, createImage } from '~/test-lib/state-helper'; + const defaultContext = { keepVolumes: false, availableImages: [] as Image[], @@ -37,55 +38,6 @@ function createApp({ ); } -async function createService( - { - appId = 1, - appUuid = 'appuuid', - serviceName = 'test', - commit = 'test-commit', - ...conf - } = {} as Partial, - { state = {} as Partial, options = {} as any } = {}, -) { - const svc = await Service.fromComposeObject( - { - appId, - appUuid, - serviceName, - commit, - running: true, - ...conf, - }, - options, - ); - - // Add additonal configuration - for (const k of Object.keys(state)) { - (svc as any)[k] = (state as any)[k]; - } - return svc; -} - -function createImage( - { - appId = 1, - appUuid = 'appuuid', - name = 'test-image', - serviceName = 'test', - commit = 'test-commit', - ...extra - } = {} as Partial, -) { - return { - appId, - appUuid, - commit, - name, - serviceName, - ...extra, - } as Image; -} - const expectSteps = ( action: CompositionStepAction, steps: CompositionStep[], @@ -234,6 +186,7 @@ describe('compose/app', () => { const current = createApp({ services: [ await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }), ], @@ -242,6 +195,7 @@ describe('compose/app', () => { const target = createApp({ services: [ await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }), ], @@ -277,6 +231,7 @@ describe('compose/app', () => { it('should not output a kill step for a service which is already stopping when changing a volume', async () => { const service = await createService({ + serviceName: 'test', composition: { volumes: ['test-volume:/data'] }, }); service.status = 'Stopping'; @@ -516,6 +471,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -528,6 +484,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -625,6 +582,7 @@ describe('compose/app', () => { await createService({ appId: 1, appUuid: 'deadbeef', + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -634,7 +592,9 @@ describe('compose/app', () => { }); const target = createApp({ appUuid: 'deadbeef', - services: [await createService({ appUuid: 'deadbeef' })], + services: [ + await createService({ appUuid: 'deadbeef', serviceName: 'test' }), + ], networks: [], isTarget: true, }); @@ -655,6 +615,7 @@ describe('compose/app', () => { const current = createApp({ services: [ await createService({ + serviceName: 'test', composition: { networks: ['test-network'] }, }), ], @@ -663,6 +624,7 @@ describe('compose/app', () => { const target = createApp({ services: [ await createService({ + serviceName: 'test', composition: { networks: { 'test-network': {} } }, }), ], @@ -1450,11 +1412,12 @@ describe('compose/app', () => { }; const current = createApp({ - services: [await createService({ labels })], + services: [await createService({ serviceName: 'test', labels })], }); const target = createApp({ services: [ await createService({ + serviceName: 'test', labels, composition: { privileged: true, From aedfd31dd0ed420f80e5542f301eaa3f8a81b28f Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 7 Mar 2023 21:55:52 -0300 Subject: [PATCH 4/5] Remove release and image id from test services --- .../compose/application-manager.spec.ts | 215 +++++++++++------- test/lib/state-helper.ts | 9 +- 2 files changed, 139 insertions(+), 85 deletions(-) diff --git a/test/integration/compose/application-manager.spec.ts b/test/integration/compose/application-manager.spec.ts index 7683a0049..bc63a4d0f 100644 --- a/test/integration/compose/application-manager.spec.ts +++ b/test/integration/compose/application-manager.spec.ts @@ -398,23 +398,31 @@ describe('compose/application-manager', () => { const targetApps = createApps( { services: [ - await createService({ - image: 'main-image', - appId: 1, - appUuid: 'appuuid', - commit: 'new-release', - serviceName: 'main', - composition: { - depends_on: ['dep'], + await createService( + { + image: 'main-image', + appId: 1, + appUuid: 'appuuid', + commit: 'new-release', + serviceName: 'main', + composition: { + depends_on: ['dep'], + }, }, - }), - await createService({ - image: 'dep-image', - appId: 1, - appUuid: 'appuuid', - commit: 'new-release', - serviceName: 'dep', - }), + {}, + true, + ), + await createService( + { + image: 'dep-image', + appId: 1, + appUuid: 'appuuid', + commit: 'new-release', + serviceName: 'dep', + }, + {}, + true, + ), ], networks: [DEFAULT_NETWORK], }, @@ -487,19 +495,27 @@ describe('compose/application-manager', () => { const targetApps = createApps( { services: [ - await createService({ - image: 'main-image', - serviceName: 'main', - commit: 'new-release', - composition: { - depends_on: ['dep'], + await createService( + { + image: 'main-image', + serviceName: 'main', + commit: 'new-release', + composition: { + depends_on: ['dep'], + }, }, - }), - await createService({ - image: 'dep-image', - serviceName: 'dep', - commit: 'new-release', - }), + {}, + true, + ), + await createService( + { + image: 'dep-image', + serviceName: 'dep', + commit: 'new-release', + }, + {}, + true, + ), ], networks: [DEFAULT_NETWORK], }, @@ -549,19 +565,27 @@ describe('compose/application-manager', () => { const targetApps = createApps( { services: [ - await createService({ - image: 'main-image', - serviceName: 'main', - commit: 'new-release', - composition: { - depends_on: ['dep'], + await createService( + { + image: 'main-image', + serviceName: 'main', + commit: 'new-release', + composition: { + depends_on: ['dep'], + }, }, - }), - await createService({ - image: 'dep-image', - serviceName: 'dep', - commit: 'new-release', - }), + {}, + true, + ), + await createService( + { + image: 'dep-image', + serviceName: 'dep', + commit: 'new-release', + }, + {}, + true, + ), ], networks: [DEFAULT_NETWORK], }, @@ -615,7 +639,7 @@ describe('compose/application-manager', () => { it('infers to remove spurious containers', async () => { const targetApps = createApps( { - services: [await createService({ image: 'main-image' })], + services: [await createService({ image: 'main-image' }, {}, true)], networks: [DEFAULT_NETWORK], }, true, @@ -853,6 +877,7 @@ describe('compose/application-manager', () => { { image: 'main-image' }, // Target has a matching image already { options: { imageInfo: { Id: 'sha256:bbbb' } } }, + true, ), ], networks: [DEFAULT_NETWORK], @@ -1091,20 +1116,28 @@ describe('compose/application-manager', () => { 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', - }), + await createService( + { + running: true, + image: 'main-image-1', + appId: 1, + appUuid: 'app-one', + commit: 'commit-for-app-1', + }, + {}, + true, + ), + await createService( + { + running: true, + image: 'main-image-2', + appId: 2, + appUuid: 'app-two', + commit: 'commit-for-app-2', + }, + {}, + true, + ), ], networks: [ // Default networks for two apps @@ -1523,38 +1556,54 @@ describe('compose/application-manager', () => { targetApps = createApps( { services: [ - await createService({ - image: 'test-image', - serviceName: 'one', - running: true, - composition: { - restart: 'always', + await createService( + { + image: 'test-image', + serviceName: 'one', + running: true, + composition: { + restart: 'always', + }, }, - }), - await createService({ - image: 'test-image', - serviceName: 'two', - running: true, - composition: { - restart: 'unless-stopped', + {}, + true, + ), + await createService( + { + image: 'test-image', + serviceName: 'two', + running: true, + composition: { + restart: 'unless-stopped', + }, }, - }), - await createService({ - image: 'test-image', - serviceName: 'three', - running: true, - composition: { - restart: 'on-failure', + {}, + true, + ), + await createService( + { + image: 'test-image', + serviceName: 'three', + running: true, + composition: { + restart: 'on-failure', + }, }, - }), - await createService({ - image: 'test-image', - serviceName: 'four', - running: true, - composition: { - restart: 'no', + {}, + true, + ), + await createService( + { + image: 'test-image', + serviceName: 'four', + running: true, + composition: { + restart: 'no', + }, }, - }), + {}, + true, + ), ], networks: [DEFAULT_NETWORK], }, diff --git a/test/lib/state-helper.ts b/test/lib/state-helper.ts index 69fa62887..bff77b325 100644 --- a/test/lib/state-helper.ts +++ b/test/lib/state-helper.ts @@ -23,6 +23,7 @@ export async function createService( ...conf } = {} as Partial, { state = {} as Partial, options = {} as any } = {}, + isTarget = false, ) { const svc = await Service.fromComposeObject( { @@ -34,8 +35,12 @@ export async function createService( // are compared using _.isEqual so leaving this here to have image comparisons // match serviceId: 1, - imageId: 1, - releaseId: 1, + ...(isTarget + ? { + imageId: 1, + releaseId: 1, + } + : {}), ...conf, }, options, From f7ed27fb268ddc3168418d74877e1a3d7352eda5 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Wed, 15 Nov 2023 14:32:51 -0300 Subject: [PATCH 5/5] Add containerName to Service interface This allows to compare the current and target container names and trigger an `updateMetadata` step if they are different. This allows us to normalize the container name format with a new supervisor update. Change-type: minor --- src/compose/app.ts | 5 ++++- src/compose/service-manager.ts | 2 +- src/compose/service.ts | 10 +++++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index 43aa76718..c9e786d3b 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -658,7 +658,10 @@ export class App { private generateContainerStep(current: Service, target: Service) { // if the services release doesn't match, then rename the container... - if (current.commit !== target.commit) { + if ( + current.commit !== target.commit || + current.containerName !== target.containerName + ) { return generateStep('updateMetadata', { current, target }); } else if (target.config.running !== current.config.running) { if (target.config.running) { diff --git a/src/compose/service-manager.ts b/src/compose/service-manager.ts index 278c51fbb..cc38c2561 100644 --- a/src/compose/service-manager.ts +++ b/src/compose/service-manager.ts @@ -157,7 +157,7 @@ export async function updateMetadata(service: Service, target: Service) { try { await docker.getContainer(svc.containerId).rename({ - name: `${service.serviceName}_${target.commit}`, + name: target.containerName, }); } catch (e) { if (isNotFoundError(e)) { diff --git a/src/compose/service.ts b/src/compose/service.ts index 95dff4b4f..ad0750835 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -70,6 +70,7 @@ export class Service { public serviceId: number; public imageName: string | null; public containerId: string | null; + public containerName: string; public exitErrorMessage: string | null; public dependsOn: string[] | null; @@ -153,6 +154,9 @@ export class Service { service.commit = appConfig.commit; service.appUuid = appConfig.appUuid; + // The target container name + service.containerName = `${service.serviceName}_${service.commit}`; + // dependsOn is used by other parts of the step // calculation so we delete it from the composition service.dependsOn = appConfig.composition?.dependsOn || null; @@ -636,6 +640,9 @@ export class Service { ); } + // The current container name, minus the initial '/' + svc.containerName = container.Name.slice(1); + // If we have not renamed the service yet we can still use the image id svc.imageId = parseInt(nameMatch[1], 10); svc.releaseId = parseInt(nameMatch[2], 10); @@ -893,7 +900,8 @@ export class Service { ): boolean { return ( this.isEqualConfig(service, currentContainerIds) && - this.commit === service.commit + this.commit === service.commit && + this.containerName === service.containerName ); }