Skip to content

Commit

Permalink
Remove imageId from container name
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@pipex
pipex committed Mar 7, 2023
1 parent 8410f8d commit e285017
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 55 deletions.
48 changes: 25 additions & 23 deletions src/compose/application-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -735,8 +735,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']),
);
}),
);
Expand Down Expand Up @@ -871,8 +871,20 @@ 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(
// Ideally we should compare services by commit and service name, but there
// is a chance that the device has upgraded the supervisor but the services
// have not been recreated yet (because of locks or something else), in that
// case the commit won't be available and appId is the really reliable way to
// compare services to images
(img) =>
img.serviceName === service.serviceName && img.appId === service.appId,
);
if (!appId || !imageForService) {
continue;
}
if (apps[appId] == null) {
Expand All @@ -883,24 +895,18 @@ 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) {
releaseId = service.releaseId;
} 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, [
Expand All @@ -919,20 +925,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;
}
}

Expand Down
32 changes: 11 additions & 21 deletions src/compose/service-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ export async function getState() {
status[service.containerId] = _.pick(service, [
'appId',
'appUuid',
'imageId',
'status',
'commit',
'createdAt',
Expand Down Expand Up @@ -153,7 +152,7 @@ export async function updateMetadata(service: Service, target: Service) {
}

await docker.getContainer(svc.containerId).rename({
name: `${service.serviceName}_${target.imageId}_${target.commit}`,
name: `${service.serviceName}_${target.commit}`,
});
}

Expand Down Expand Up @@ -336,14 +335,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 });
Expand Down Expand Up @@ -399,15 +397,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);
Expand Down Expand Up @@ -448,10 +444,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`,
);
}

Expand All @@ -462,7 +457,6 @@ export async function attachToRunning() {
}
logger.attach(service.containerId, {
serviceId,
imageId,
});
}
}
Expand Down Expand Up @@ -528,9 +522,7 @@ function killContainer(

return Bluebird.try(() => {
logger.logSystemEvent(LogTypes.stopService, { service });
if (service.imageId != null) {
reportNewStatus(containerId, service, 'Stopping');
}
reportNewStatus(containerId, service, 'Stopping');

const containerObj = docker.getContainer(containerId);
const killPromise = Bluebird.resolve(containerObj.stop())
Expand Down Expand Up @@ -577,9 +569,7 @@ function killContainer(
});
})
.finally(() => {
if (service.imageId != null) {
reportChange(containerId);
}
reportChange(containerId);
});

if (wait) {
Expand Down Expand Up @@ -618,7 +608,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}`,
});
}

Expand Down
18 changes: 12 additions & 6 deletions src/compose/service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,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;
Expand Down Expand Up @@ -653,14 +658,15 @@ 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 '<serviceName>_<imageId>_<commit>', got: ${container.Name}`,
`Expected supervised container to have name '<serviceName>_<commit>', got: ${container.Name}`,
);
}

svc.imageId = parseInt(nameMatch[1], 10);
svc.commit = nameMatch[3];
svc.containerId = container.Id;
svc.dockerImageId = container.Config.Image;
Expand Down Expand Up @@ -709,7 +715,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,
Expand Down Expand Up @@ -1180,7 +1186,7 @@ export class Service {
log.warn(
`Ignoring invalid compose volume definition ${
isString ? volume : JSON.stringify(volume)
}`,
} `,
);
}
}
Expand Down
25 changes: 21 additions & 4 deletions src/device-api/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,10 @@ export const executeServiceAction = async ({
isLegacy?: boolean;
}): Promise<void> => {
// Get current and target apps
const [currentApp, targetApp] = await Promise.all([
const [currentApp, targetApp, serviceImages] = await Promise.all([
getCurrentApp(appId, isLegacy ? BadRequestError : NotFoundError),
getApp(appId),
imageManager.getState(),
]);
const isSingleContainer = currentApp.services.length === 1;
if (!isSingleContainer && !serviceName && !imageId) {
Expand All @@ -367,7 +368,20 @@ export const executeServiceAction = async ({
const currentService = isSingleContainer
? currentApp.services[0]
: currentApp.services.find(
(s) => s.imageId === imageId || s.serviceName === serviceName,
(s) =>
s.serviceName === serviceName ||
// We find an image for the service with the correct image id
serviceImages.some(
// Ideally we should compare services by commit and service name, but there
// is a chance that the device has upgraded the supervisor but the services
// have not been recreated yet (because of locks or something else), in that
// case the commit won't be available and appId is the really reliable way to
// compare services to images
(img) =>
img.serviceName === s.serviceName &&
img.appId === s.appId &&
img.imageId === imageId,
),
);
if (currentService == null) {
// Legacy (v1) throws 400 while v2 throws 404, and we have to keep the interface consistent.
Expand All @@ -377,8 +391,8 @@ export const executeServiceAction = async ({
}
const targetService = targetApp.services.find(
(s) =>
s.imageId === currentService.imageId ||
s.serviceName === currentService.serviceName,
s.serviceName === currentService.serviceName &&
s.appId === currentService.appId,
);
if (targetService == null) {
throw new NotFoundError(messages.targetServiceNotFound);
Expand Down Expand Up @@ -496,6 +510,9 @@ export const getLegacyDeviceState = async () => {
_.toPairs(state.local?.apps)[0]?.[1]?.services,
)[0]?.[1];

// Default to idle when no release has been installed yet
// or during local mode
stateToSend.status = 'Idle';
if (service != null) {
stateToSend.status = service.status;
if (stateToSend.status === 'Running') {
Expand Down
2 changes: 1 addition & 1 deletion src/logger.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export function lock(containerId: string): Bluebird.Disposer<() => void> {

export function attach(
containerId: string,
serviceInfo: { serviceId: number; imageId: number },
serviceInfo: { serviceId: number },
): Bluebird<void> {
// First detect if we already have an attached log stream
// for this container
Expand Down

0 comments on commit e285017

Please sign in to comment.