From ab80f198d85ac22c42e3a189a74246f10a9c7ec0 Mon Sep 17 00:00:00 2001 From: Christina Ying Wang Date: Wed, 14 Jun 2023 16:26:22 -0700 Subject: [PATCH] Add exitCode property to Service class Since we need to conditionally query the service's exit code during step inference, adding the exitCode property keeps the step inference function pure. See: https://github.com/balena-os/balena-supervisor/pull/2170#discussion_r1226805153 Signed-off-by: Christina Ying Wang --- src/compose/app.ts | 125 +++++++++++------------------ src/compose/application-manager.ts | 4 +- src/compose/service-manager.ts | 8 +- src/compose/service.ts | 2 + 4 files changed, 54 insertions(+), 85 deletions(-) diff --git a/src/compose/app.ts b/src/compose/app.ts index c45751f2..20015e42 100644 --- a/src/compose/app.ts +++ b/src/compose/app.ts @@ -13,7 +13,6 @@ import { CompositionStepAction, } from './composition-steps'; import { isOlderThan } from './utils'; -import { inspectByDockerContainerId } from './service-manager'; import * as targetStateCache from '../device-state/target-state-cache'; import { getNetworkGateway } from '../lib/docker-utils'; import * as constants from '../lib/constants'; @@ -102,10 +101,10 @@ export class App { } } - public async nextStepsForAppUpdate( + public nextStepsForAppUpdate( state: UpdateState, target: App, - ): Promise { + ): CompositionStep[] { // Check to see if we need to polyfill in some "new" data for legacy services this.migrateLegacy(target); @@ -131,12 +130,11 @@ export class App { } } - const { removePairs, installPairs, updatePairs } = - await this.compareServices( - this.services, - target.services, - state.containerIds, - ); + const { removePairs, installPairs, updatePairs } = this.compareServices( + this.services, + target.services, + state.containerIds, + ); for (const { current: svc } of removePairs) { // All removes get a kill action if they're not already stopping @@ -149,19 +147,18 @@ export class App { // For every service which needs to be updated, update via update strategy. const servicePairs = updatePairs.concat(installPairs); - const serviceSteps = await Promise.all( - servicePairs.map((pair) => - this.generateStepsForService(pair, { - ...state, - servicePairs, - targetApp: target, - networkPairs: networkChanges, - volumePairs: volumeChanges, - }), - ), - ); steps = steps.concat( - serviceSteps.filter((step) => step != null) as CompositionStep[], + servicePairs + .map((pair) => + this.generateStepsForService(pair, { + ...state, + servicePairs, + targetApp: target, + networkPairs: networkChanges, + volumePairs: volumeChanges, + }), + ) + .filter((step) => step != null) as CompositionStep[], ); // Generate volume steps @@ -300,15 +297,15 @@ export class App { return outputs; } - private async compareServices( + private compareServices( current: Service[], target: Service[], containerIds: Dictionary, - ): Promise<{ + ): { installPairs: Array>; removePairs: Array>; updatePairs: Array>; - }> { + } { const currentByServiceName = _.keyBy(current, 'serviceName'); const targetByServiceName = _.keyBy(target, 'serviceName'); @@ -367,7 +364,7 @@ export class App { * @param serviceCurrent * @param serviceTarget */ - const shouldBeStarted = async ( + const shouldBeStarted = ( serviceCurrent: Service, serviceTarget: Service, ) => { @@ -391,10 +388,7 @@ export class App { return ( (serviceCurrent.status === 'Installing' || serviceCurrent.status === 'Installed' || - (await this.requirementsMetForSpecialStart( - serviceCurrent, - serviceTarget, - ))) && + this.requirementsMetForSpecialStart(serviceCurrent, serviceTarget)) && isEqualExceptForRunningState(serviceCurrent, serviceTarget) ); }; @@ -430,24 +424,18 @@ export class App { /** * Filter all the services which should be updated due to run state change, or config mismatch. */ - const satisfiesRequirementsForUpdate = async (c: Service, t: Service) => - !isEqualExceptForRunningState(c, t) || - (await shouldBeStarted(c, t)) || - shouldBeStopped(c, t) || - shouldWaitForStop(c); - - const maybeUpdatePairs = maybeUpdate.map((serviceName) => ({ - current: currentByServiceName[serviceName], - target: targetByServiceName[serviceName], - })); - const maybeUpdatePairsFiltered = await Promise.all( - maybeUpdatePairs.map(({ current: c, target: t }) => - satisfiesRequirementsForUpdate(c, t), - ), - ); - const toBeUpdated = maybeUpdatePairs.filter( - (__, idx) => maybeUpdatePairsFiltered[idx], - ); + const toBeUpdated = maybeUpdate + .map((serviceName) => ({ + current: currentByServiceName[serviceName], + target: targetByServiceName[serviceName], + })) + .filter( + ({ current: c, target: t }) => + !isEqualExceptForRunningState(c, t) || + shouldBeStarted(c, t) || + shouldBeStopped(c, t) || + shouldWaitForStop(c), + ); return { installPairs: toBeInstalled, @@ -513,7 +501,7 @@ export class App { return steps; } - private async generateStepsForService( + private generateStepsForService( { current, target }: ChangingPair, context: { availableImages: Image[]; @@ -524,7 +512,7 @@ export class App { volumePairs: Array>; servicePairs: Array>; }, - ): Promise> { + ): Nullable { if (current?.status === 'Stopping') { // Theres a kill step happening already, emit a noop to ensure we stay alive while // this happens @@ -578,7 +566,7 @@ export class App { current.isEqualConfig(target, context.containerIds) ) { // we're only starting/stopping a service - return await this.generateContainerStep(current, target); + return this.generateContainerStep(current, target); } let strategy = @@ -656,11 +644,10 @@ export class App { // restart policy (this can happen in cases of Engine race conditions with // host resources that are slower to be created but that a service relies on), // we need to start the container after a delay. - private async requirementsMetForSpecialStart( + private requirementsMetForSpecialStart( current: Service, target: Service, - ): Promise { - // Shortcut the Engine inspect queries if status isn't exited + ): boolean { if ( current.status !== 'exited' || current.config.running !== false || @@ -668,36 +655,22 @@ export class App { ) { return false; } - let conditionsMetWithRestartPolicy = ['always', 'unless-stopped'].includes( - target.config.restart, - ); - - if (current.containerId != null && !conditionsMetWithRestartPolicy) { - const containerInspect = await inspectByDockerContainerId( - current.containerId, - ); - // If the container has previously been started but exited unsuccessfully, it needs to be started. - // If the container has a restart policy of 'no' and has never been started, it - // should also be started, however, in that case, the status of the container will - // be 'Installed' and the state funnel will already be trying to start it until success, - // so we don't need to add any additional handling. - if ( - target.config.restart === 'on-failure' && - containerInspect.State.ExitCode !== 0 - ) { - conditionsMetWithRestartPolicy = true; - } - } - return conditionsMetWithRestartPolicy; + const restartIsAlwaysOrUnlessStopped = [ + 'always', + 'unless-stopped', + ].includes(target.config.restart); + const restartIsOnFailureWithNonZeroExit = + target.config.restart === 'on-failure' && current.exitCode !== 0; + return restartIsAlwaysOrUnlessStopped || restartIsOnFailureWithNonZeroExit; } - private async generateContainerStep(current: Service, target: Service) { + private generateContainerStep(current: Service, target: Service) { // if the services release doesn't match, then rename the container... if (current.commit !== target.commit) { return generateStep('updateMetadata', { current, target }); } else if (target.config.running !== current.config.running) { if ( - (await this.requirementsMetForSpecialStart(current, target)) && + this.requirementsMetForSpecialStart(current, target) && !isOlderThan(current.createdAt, SECONDS_TO_WAIT_FOR_START) ) { return generateStep('noop', {}); diff --git a/src/compose/application-manager.ts b/src/compose/application-manager.ts index f352938e..f25ee913 100644 --- a/src/compose/application-manager.ts +++ b/src/compose/application-manager.ts @@ -207,7 +207,7 @@ export async function inferNextSteps( // do to move to the target state for (const id of targetAndCurrent) { steps = steps.concat( - await currentApps[id].nextStepsForAppUpdate( + currentApps[id].nextStepsForAppUpdate( { availableImages, containerIds: containerIdsByAppId[id], @@ -243,7 +243,7 @@ export async function inferNextSteps( false, ); steps = steps.concat( - await emptyCurrent.nextStepsForAppUpdate( + emptyCurrent.nextStepsForAppUpdate( { availableImages, containerIds: containerIdsByAppId[id] ?? {}, diff --git a/src/compose/service-manager.ts b/src/compose/service-manager.ts index 8e4e679e..90279e06 100644 --- a/src/compose/service-manager.ts +++ b/src/compose/service-manager.ts @@ -135,7 +135,7 @@ export async function getState() { export async function getByDockerContainerId( containerId: string, ): Promise { - const container = await inspectByDockerContainerId(containerId); + const container = await docker.getContainer(containerId).inspect(); if ( container.Config.Labels['io.balena.supervised'] == null && container.Config.Labels['io.resin.supervised'] == null @@ -145,12 +145,6 @@ export async function getByDockerContainerId( return Service.fromDockerContainer(container); } -export async function inspectByDockerContainerId( - containerId: string, -): Promise { - return await docker.getContainer(containerId).inspect(); -} - export async function updateMetadata(service: Service, target: Service) { const svc = await get(service); if (svc.containerId == null) { diff --git a/src/compose/service.ts b/src/compose/service.ts index 58b343e8..3906ccad 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -61,6 +61,7 @@ export class Service { public serviceId: number; public imageName: string | null; public containerId: string | null; + public exitCode: number | null; public dependsOn: string[] | null; @@ -503,6 +504,7 @@ export class Service { svc.createdAt = new Date(container.Created); svc.containerId = container.Id; + svc.exitCode = container.State.ExitCode; let hostname = container.Config.Hostname; if (hostname.length === 12 && _.startsWith(container.Id, hostname)) {