From 021fa1ccb81acf965ee10c2e00763e1adaec571c Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 7 May 2024 16:09:44 -0400 Subject: [PATCH] Uninstall containers using force remove When uninstalling a container for a restart from the API on when moving between releases, this prefers to force remove the container rather than stop then remove. This has the same effect in the end, but force remove is an atomic call, meaning if the supervisor is killed between the stop and remove, the engine will proceed with the operation and the supervisor will know to recover from the target state after the restart. Change-type: patch Relates-to: #2257 --- src/compose/composition-steps.ts | 8 +--- src/compose/service-manager.ts | 71 ++++++++++++++++++++------------ src/lib/log-types.ts | 4 ++ 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/src/compose/composition-steps.ts b/src/compose/composition-steps.ts index ded3b9663..111c0fae1 100644 --- a/src/compose/composition-steps.ts +++ b/src/compose/composition-steps.ts @@ -14,9 +14,6 @@ import type { DeviceLegacyReport } from '../types/state'; interface CompositionStepArgs { stop: { current: Service; - options?: { - wait?: boolean; - }; }; kill: { current: Service; @@ -119,10 +116,7 @@ export function getExecutors(app: { callbacks: CompositionCallbacks }) { stop: async (step) => { // Should always be preceded by a takeLock step, // so the call is executed assuming that the lock is taken. - await serviceManager.kill(step.current, { - removeContainer: false, - wait: step.options?.wait || false, - }); + await serviceManager.stop(step.current); }, kill: async (step) => { // Should always be preceded by a takeLock step, diff --git a/src/compose/service-manager.ts b/src/compose/service-manager.ts index cb782ecfd..6e1806dff 100644 --- a/src/compose/service-manager.ts +++ b/src/compose/service-manager.ts @@ -1,6 +1,5 @@ import type Dockerode from 'dockerode'; import { EventEmitter } from 'events'; -import { isLeft } from 'fp-ts/lib/Either'; import JSONStream from 'JSONStream'; import _ from 'lodash'; import { promises as fs } from 'fs'; @@ -10,7 +9,6 @@ import * as config from '../config'; import { docker } from '../lib/docker-utils'; import * as logger from '../logger'; -import { PermissiveNumber } from '../config/types'; import * as constants from '../lib/constants'; import type { StatusCodeError } from '../lib/errors'; import { @@ -38,7 +36,6 @@ type ServiceManagerEventEmitter = StrictEventEmitter< const events: ServiceManagerEventEmitter = new EventEmitter(); interface KillOpts { - removeContainer?: boolean; wait?: boolean; } @@ -201,6 +198,15 @@ export async function killAllLegacy(): Promise { } } +export function stop(service: Service) { + if (service.containerId == null) { + throw new InternalInconsistencyError( + `Attempt to stop container without containerId! Service :${service}`, + ); + } + return stopContainer(service.containerId, service); +} + export function kill(service: Service, opts: KillOpts = {}) { if (service.containerId == null) { throw new InternalInconsistencyError( @@ -536,10 +542,43 @@ function reportNewStatus( ); } +async function stopContainer( + containerId: string, + service: Partial = {}, +) { + logger.logSystemEvent(LogTypes.stopService, { service }); + if (service.imageId != null) { + reportNewStatus(containerId, service, 'Stopping'); + } + + try { + await docker.getContainer(containerId).stop(); + delete containerHasDied[containerId]; + logger.logSystemEvent(LogTypes.stopServiceSuccess, { service }); + } catch (e) { + if (isStatusError(e) && e.statusCode === 304) { + delete containerHasDied[containerId]; + logger.logSystemEvent(LogTypes.stopServiceSuccess, { service }); + } else if (isStatusError(e) && e.statusCode === 404) { + logger.logSystemEvent(LogTypes.stopService404Error, { service }); + delete containerHasDied[containerId]; + } else { + logger.logSystemEvent(LogTypes.stopServiceError, { + service, + error: e, + }); + } + } finally { + if (service.imageId != null) { + reportChange(containerId); + } + } +} + async function killContainer( containerId: string, service: Partial = {}, - { removeContainer = true, wait = false }: KillOpts = {}, + { wait = false }: KillOpts = {}, ): Promise { // To maintain compatibility of the `wait` flag, this function is not // async, but it feels like whether or not the promise should be waited on @@ -553,29 +592,9 @@ async function killContainer( const containerObj = docker.getContainer(containerId); const killPromise = containerObj - .stop() - .then(() => { - if (removeContainer) { - return containerObj.remove({ v: true }); - } - }) + .remove({ v: true, force: true }) .catch((e) => { - // Get the statusCode from the original cause and make sure it's - // definitely an int for comparison reasons - const maybeStatusCode = PermissiveNumber.decode(e.statusCode); - if (isLeft(maybeStatusCode)) { - throw new Error(`Could not parse status code from docker error: ${e}`); - } - const statusCode = maybeStatusCode.right; - - // 304 means the container was already stopped, so we can just remove it - if (statusCode === 304) { - logger.logSystemEvent(LogTypes.stopServiceNoop, { service }); - // Why do we attempt to remove the container again? - if (removeContainer) { - return containerObj.remove({ v: true }); - } - } else if (statusCode === 404) { + if (isStatusError(e) && e.statusCode === 304) { // 404 means the container doesn't exist, precisely what we want! logger.logSystemEvent(LogTypes.stopRemoveServiceNoop, { service, diff --git a/src/lib/log-types.ts b/src/lib/log-types.ts index 8ecf0a9c5..78814a1f2 100644 --- a/src/lib/log-types.ts +++ b/src/lib/log-types.ts @@ -19,6 +19,10 @@ export const stopRemoveServiceNoop: LogType = { eventName: 'Service already stopped and container removed', humanName: 'Service is already stopped and the container removed', }; +export const stopService404Error: LogType = { + eventName: 'Service stop error', + humanName: 'Service container not found', +}; export const stopServiceError: LogType = { eventName: 'Service stop error', humanName: 'Failed to kill service',