fix(dashmate): keep the node up when an image pull fails, and stop reporting success when it did not

packages/dashmate/configs/getConfigFileMigrationsFactory.js

@@ -331,6 +331,13 @@ export default function getConfigFileMigrationsFactory(homeDir, defaultConfigs)
                 if (fs.existsSync(oldFilePath)) {
                   fs.mkdirSync(path.dirname(newFilePath), { recursive: true });
                   fs.copyFileSync(oldFilePath, newFilePath);
+
+                  // A copy keeps the permissions of the source, and the private key
+                  // must not be readable by other users on the host
+                  if (filename === 'private.key') {
+                    fs.chmodSync(newFilePath, 0o600);
+                  }
+
                   fs.rmSync(oldFilePath, { recursive: true });
                 }
               }
@@ -712,6 +719,13 @@ export default function getConfigFileMigrationsFactory(homeDir, defaultConfigs)
                 if (fs.existsSync(oldFilePath)) {
                   fs.mkdirSync(path.dirname(newFilePath), { recursive: true });
                   fs.copyFileSync(oldFilePath, newFilePath);
+
+                  // A copy keeps the permissions of the source, and the private key
+                  // must not be readable by other users on the host
+                  if (filename === 'private.key') {
+                    fs.chmodSync(newFilePath, 0o600);
+                  }
+
                   fs.rmSync(oldFilePath, { recursive: true });
                 }
               }

packages/dashmate/src/commands/config/default.js

@@ -13,7 +13,6 @@ Shows default config name or sets another config as default
         name: 'config',
         required: false,
         description: 'config name',
-        default: null, // only allow input to be from a discrete set
       },
     ),
   };
@@ -32,7 +31,8 @@ Shows default config name or sets another config as default
     configFile,
     configFileRepository,
   ) {
-    if (configName === null) {
+    // The argument is omitted when only the current default config name is requested
+    if (configName === undefined) {
       // eslint-disable-next-line no-console
       console.log(configFile.getDefaultConfigName());
     } else {

packages/dashmate/src/commands/group/default.js

@@ -13,7 +13,6 @@ Shows default group name or sets another group as default
         name: 'group',
         required: false,
         description: 'group name',
-        default: null, // only allow input to be from a discrete set
       },
     ),
   };
@@ -32,7 +31,8 @@ Shows default group name or sets another group as default
     configFile,
     configFileRepository,
   ) {
-    if (groupName === null) {
+    // The argument is omitted when only the current default group name is requested
+    if (groupName === undefined) {
       // eslint-disable-next-line no-console
       console.log(configFile.getDefaultGroupName());
     } else {

packages/dashmate/src/commands/group/restart.js

@@ -1,6 +1,7 @@
 import { Listr } from 'listr2';
 import GroupBaseCommand from '../../oclif/command/GroupBaseCommand.js';
 import MuteOneLineError from '../../oclif/errors/MuteOneLineError.js';
+import isServiceBuildRequired from '../../util/isServiceBuildRequired.js';
 
 export default class GroupRestartCommand extends GroupBaseCommand {
   static description = 'Restart group nodes';
@@ -20,6 +21,7 @@ export default class GroupRestartCommand extends GroupBaseCommand {
    * @param {DockerCompose} dockerCompose
    * @param {stopNodeTask} stopNodeTask
    * @param {startGroupNodesTask} startGroupNodesTask
+   * @param {buildServicesTask} buildServicesTask
    * @param {Config[]} configGroup
    * @return {Promise<void>}
    */
@@ -32,15 +34,49 @@ export default class GroupRestartCommand extends GroupBaseCommand {
     dockerCompose,
     stopNodeTask,
     startGroupNodesTask,
+    buildServicesTask,
     configGroup,
   ) {
     const groupName = configGroup[0].get('group');
 
+    // The whole group shares one set of locally built images, so one config
+    // describes the build for all of them
+    const buildConfig = configGroup.find(isServiceBuildRequired);
+
     const tasks = new Listr(
       {
         title: `Restart ${groupName} nodes`,
         task: async () => (
           new Listr([
+            {
+              // An image built from local sources is in no registry, so the
+              // pull below cannot confirm it. Building before the group is
+              // stopped is what keeps a build failure from leaving it down
+              enabled: () => Boolean(buildConfig),
+              title: 'Build services',
+              task: (ctx) => {
+                // The group start builds the same images, and would otherwise
+                // repeat the whole build on every restart
+                ctx.skipBuildServices = true;
+
+                return buildServicesTask(buildConfig);
+              },
+            },
+            {
+              // Every node's images must be fetched before the first node is
+              // stopped, otherwise a failed pull leaves the group stopped
+              title: 'Pull missing images',
+              task: () => (
+                new Listr(configGroup.map((config) => ({
+                  task: (ctx, task) => dockerCompose.pullMissingImages(config, {
+                    onProgress: (message) => {
+                      // eslint-disable-next-line no-param-reassign
+                      task.output = message;
+                    },
+                  }),
+                })))
+              ),
+            },
             {
               title: 'Stop nodes',
               task: () => (

packages/dashmate/src/commands/update.js

@@ -118,7 +118,7 @@ export default class UpdateCommand extends ConfigBaseCommand {
 
       if (!result.ok) {
         // Nothing was fetched at all - not a per-image failure, which resolves
-        // as an error row and has always exited 0. Retained so it can be
+        // as an error row and is reported further down. Retained so it can be
         // raised once the certificate has had its say: returning quietly here
         // hands `update && start` a node whose images were never downloaded,
         // with no exit code for the caller to catch.
@@ -137,16 +137,29 @@ export default class UpdateCommand extends ConfigBaseCommand {
       const colors = {
         updated: chalk.yellow,
         'up to date': chalk.green,
+        'built locally': chalk.gray,
         error: chalk.red,
       };
 
       printArrayOfObjects(result.info.map(({
-        name, title, updated, image,
+        name, title, updated, image, error,
       }) => (format === OUTPUT_FORMATS.PLAIN
         ? { Service: title, Image: image, Updated: colors[updated](updated) }
         : {
-          name, title, updated, image,
+          name, title, updated, image, error,
         })), format);
+
+      const failedServices = result.info.filter(({ updated }) => updated === 'error');
+
+      if (failedServices.length > 0) {
+        const reasons = failedServices
+          .map(({ title, image, error }) => `  ${title} (${image}): ${error}`)
+          .join('\n');
+
+        // Reported on stderr so machine-readable output on stdout stays parseable
+        // eslint-disable-next-line no-console
+        console.error(`\nFailed to update ${failedServices.length} of ${result.info.length} images:\n\n${reasons}\n`);
+      }
     };
 
     const tasks = new Listr(
@@ -260,6 +273,9 @@ export default class UpdateCommand extends ConfigBaseCommand {
       throw new MuteOneLineError(unresolved);
     }
 
-    process.exitCode = 0;
+    // An image that failed to download is reported as a row in the table rather
+    // than thrown, so the exit code is the only thing that tells a caller apart
+    // an update that fetched everything from one that fetched some of it.
+    process.exitCode = this.pullResult?.failed > 0 ? 1 : 0;
   }
 }

packages/dashmate/src/docker/DockerCompose.js

@@ -61,19 +61,26 @@ export default class DockerCompose {
    */
   #getServiceList;
 
+  /**
+   * @type {dockerPull}
+   */
+  #dockerPull;
+
   /**
    * @param {Docker} docker
    * @param {StartedContainers} startedContainers
    * @param {HomeDir} homeDir
    * @param {generateEnvs} generateEnvs
    * @param {getServiceList} getServiceList
+   * @param {dockerPull} dockerPull
    */
-  constructor(docker, startedContainers, homeDir, generateEnvs, getServiceList) {
+  constructor(docker, startedContainers, homeDir, generateEnvs, getServiceList, dockerPull) {
     this.#docker = docker;
     this.#startedContainers = startedContainers;
     this.#homeDir = homeDir;
     this.#generateEnvs = generateEnvs;
     this.#getServiceList = getServiceList;
+    this.#dockerPull = dockerPull;
   }
 
   /**
@@ -498,6 +505,87 @@ export default class DockerCompose {
     }
   }
 
+  /**
+   * Pull images required by the config that are not present on the host
+   *
+   * Docker Compose pulls a missing image only when it creates the container,
+   * which during a restart happens after the node has already been stopped.
+   * A failed pull would then leave the node down, so images are fetched
+   * upfront and the caller can abort while the node is still running.
+   *
+   * @param {Config} config
+   * @param {Object} [options]
+   * @param {string[]} [options.profiles] - Filter by profiles
+   * @param {function} [options.onProgress] - Called with pull progress messages
+   * @return {Promise<string[]>} images that have been pulled
+   */
+  async pullMissingImages(config, { profiles = [], onProgress = undefined } = {}) {
+    await this.throwErrorIfNotInstalled();
+
+    let serviceList = this.#getServiceList(config);
+
+    if (profiles.length > 0) {
+      // Compose creates a service when one of its profiles is enabled, and
+      // always creates a service that declares no profiles at all
+      serviceList = serviceList.filter((service) => service.profiles.length === 0
+        || service.profiles.some((profile) => profiles.includes(profile)));
+    }
+
+    const images = serviceList
+      // Images built from sources on this host are not available in a registry
+      .filter((service) => !service.isBuiltLocally)
+      .map((service) => service.image);
+
+    const pulledImages = [];
+
+    for (const image of new Set(images)) {
+      if (await this.#isImagePresent(image)) {
+        continue;
+      }
+
+      try {
+        await this.#dockerPull(image, (message) => {
+          if (onProgress && message?.status) {
+            const progress = message.progress ? ` ${message.progress}` : '';
+
+            onProgress(`${image}: ${message.status}${progress}`);
+          }
+        });
+      } catch (e) {
+        throw new Error(`Failed to pull image ${image}: ${e.message}`);
+      }
+
+      // Docker can report a successful pull without producing the image,
+      // and the whole point of pulling here is to know the image is on the host
+      if (!await this.#isImagePresent(image)) {
+        throw new Error(`Failed to pull image ${image}: it is still not present on the host`);
+      }
+
+      pulledImages.push(image);
+    }
+
+    return pulledImages;
+  }
+
+  /**
+   * @private
+   * @param {string} image
+   * @return {Promise<boolean>}
+   */
+  async #isImagePresent(image) {
+    try {
+      await this.#docker.getImage(image).inspect();
+
+      return true;
+    } catch (e) {
+      if (e.statusCode === 404) {
+        return false;
+      }
+
+      throw new Error(`Failed to check image ${image}: ${e.message}`);
+    }
+  }
+
   /**
    * Logs
    *

packages/dashmate/src/docker/dockerPullFactory.js

@@ -1,3 +1,5 @@
+import findPullStreamError from './findPullStreamError.js';
+
 /**
  * @param {Docker} docker
  * @return {dockerPull}
@@ -6,9 +8,10 @@ export default function dockerPullFactory(docker) {
   /**
    * @typedef {dockerPull}
    * @param {string} image
+   * @param {function} [onProgress] - called with every pull stream message
    * @return {Promise<*>}
    */
-  function dockerPull(image) {
+  function dockerPull(image, onProgress = undefined) {
     return new Promise((resolve, reject) => {
       docker.pull(image, (err, stream) => {
         if (err) {
@@ -24,8 +27,18 @@ export default function dockerPullFactory(docker) {
             return;
           }
 
+          // followProgress collects stream messages without inspecting them,
+          // so a failed pull has to be recognized here
+          const streamError = findPullStreamError(output);
+
+          if (streamError) {
+            reject(new Error(streamError));
+
+            return;
+          }
+
           resolve(output);
-        });
+        }, onProgress);
       });
     });
   }

packages/dashmate/src/docker/findPullStreamError.js

@@ -0,0 +1,24 @@
+import sanitizeRemoteText from '../util/sanitizeRemoteText.js';
+
+/**
+ * Find a failure reported inside a Docker pull progress stream
+ *
+ * Docker answers a pull request with 200 and then reports registry and disk
+ * failures as a message in the progress stream, so a completed stream doesn't
+ * mean the image was pulled.
+ *
+ * The registry chooses the text of that message and it ends up on the
+ * operator's terminal, so it is made safe to print before it is passed on.
+ *
+ * @param {Object[]} output - messages collected from the pull stream
+ * @return {string|undefined} failure reason
+ */
+export default function findPullStreamError(output) {
+  const failure = output.find((message) => message?.error);
+
+  if (!failure) {
+    return undefined;
+  }
+
+  return sanitizeRemoteText(failure.errorDetail?.message ?? failure.error);
+}

packages/dashmate/src/docker/getServiceListFactory.js

@@ -40,14 +40,20 @@ export default function getServiceListFactory(generateEnvs, getConfigProfiles) {
       // map to array of services and populate with data
       .map((composeFileServiceEntry) => {
         const [serviceName,
-          { image: serviceImage, labels, profiles: serviceProfiles }] = composeFileServiceEntry;
+          {
+            image: serviceImage, labels, profiles: serviceProfiles, build: serviceBuild,
+          }] = composeFileServiceEntry;
 
         const title = labels?.['org.dashmate.service.title'];
 
         if (!title) {
           throw new Error(`Label for dashmate service ${serviceName} is not defined`);
         }
 
+        // A service with a build section is built from sources on this host,
+        // so its image exists only locally and can't be pulled from a registry
+        const isBuiltLocally = Boolean(serviceBuild);
+
         // Use hardcoded version for dashmate helper
         // Or parse image env variable name and extract version from the env
         const serviceImageEnv = serviceImage.match(/([A-Z_]+)/);
@@ -61,6 +67,7 @@ export default function getServiceListFactory(generateEnvs, getConfigProfiles) {
           name: serviceName,
           title,
           image,
+          isBuiltLocally,
           profiles: serviceProfiles ?? [],
         });
       });