From 0920c68898c3a0531070afc7b1f0e8b277db4024 Mon Sep 17 00:00:00 2001 From: Thomas Bonnin <233326+TBonnin@users.noreply.github.com> Date: Fri, 10 Jan 2025 14:25:00 -0500 Subject: [PATCH] fix(fleet): image override might contain the commitId tag the entire point of image override in fleet is to be able to create a node running a specific commit (for pre-deployment testing or verification purpose) This commit ensures that an image override with a commitId tag is properly handled --- .../supervisor/supervisor.integration.test.ts | 63 ++++++++++++++++--- packages/fleet/lib/supervisor/supervisor.ts | 30 ++++----- 2 files changed, 68 insertions(+), 25 deletions(-) diff --git a/packages/fleet/lib/supervisor/supervisor.integration.test.ts b/packages/fleet/lib/supervisor/supervisor.integration.test.ts index c190929c70..a72cf6bb17 100644 --- a/packages/fleet/lib/supervisor/supervisor.integration.test.ts +++ b/packages/fleet/lib/supervisor/supervisor.integration.test.ts @@ -107,22 +107,65 @@ describe('Supervisor', () => { const nodeAfter = (await nodes.get(dbClient.db, node.id)).unwrap(); expect(nodeAfter.state).toBe('OUTDATED'); }); - it('should mark nodes with config override as OUTDATED', async () => { + it('should mark nodes with resource override as OUTDATED', async () => { const node = await createNodeWithAttributes(dbClient.db, { state: 'RUNNING', deploymentId: activeDeployment.id }); - ( - await nodeConfigOverrides.create(dbClient.db, { - routingId: node.routingId, - image: 'new-image', - cpuMilli: 10000, - memoryMb: 1234, - storageMb: 567890 - }) - ).unwrap(); + await nodeConfigOverrides.create(dbClient.db, { + routingId: node.routingId, + image: node.image, + cpuMilli: 10000, + memoryMb: 1234, + storageMb: 567890 + }); + + await supervisor.tick(); + + const nodeAfter = (await nodes.get(dbClient.db, node.id)).unwrap(); + expect(nodeAfter.state).toBe('OUTDATED'); + + await supervisor.tick(); + + const newNode = (await nodes.search(dbClient.db, { states: ['PENDING'] })).unwrap().nodes.get(node.routingId)?.PENDING[0]; + expect(newNode).toMatchObject({ + state: 'PENDING', + routingId: node.routingId, + deploymentId: activeDeployment.id, + image: node.image, + cpuMilli: 10000, + memoryMb: 1234, + storageMb: 567890, + error: null + }); + }); + + it('should mark nodes with image override as OUTDATED', async () => { + const node = await createNodeWithAttributes(dbClient.db, { state: 'RUNNING', deploymentId: activeDeployment.id }); + const imageOverride = `${mockNodeProvider.defaultNodeConfig.image}:12345`; + await nodeConfigOverrides.create(dbClient.db, { + routingId: node.routingId, + image: imageOverride, + cpuMilli: node.cpuMilli, + memoryMb: node.memoryMb, + storageMb: node.storageMb + }); await supervisor.tick(); const nodeAfter = (await nodes.get(dbClient.db, node.id)).unwrap(); expect(nodeAfter.state).toBe('OUTDATED'); + + await supervisor.tick(); + + const newNode = (await nodes.search(dbClient.db, { states: ['PENDING'] })).unwrap().nodes.get(node.routingId)?.PENDING[0]; + expect(newNode).toMatchObject({ + state: 'PENDING', + routingId: node.routingId, + deploymentId: activeDeployment.id, + image: imageOverride, + cpuMilli: node.cpuMilli, + memoryMb: node.memoryMb, + storageMb: node.storageMb, + error: null + }); }); it('should create new nodes if only OUTDATED', async () => { diff --git a/packages/fleet/lib/supervisor/supervisor.ts b/packages/fleet/lib/supervisor/supervisor.ts index 8dd2fb69b8..ef35d1f39a 100644 --- a/packages/fleet/lib/supervisor/supervisor.ts +++ b/packages/fleet/lib/supervisor/supervisor.ts @@ -9,7 +9,7 @@ import { Err, Ok, retryWithBackoff } from '@nangohq/utils'; import type { Result } from '@nangohq/utils'; import { FleetError } from '../utils/errors.js'; import type { Node, NodeConfigOverride } from '../types.js'; -import type { Deployment, NodeConfig } from '@nangohq/types'; +import type { Deployment } from '@nangohq/types'; import { setTimeout } from 'node:timers/promises'; import type { NodeProvider } from '../node-providers/node_provider.js'; import { envs } from '../env.js'; @@ -182,8 +182,11 @@ export class Supervisor { if (!configOverride) { return false; } + // image override might have a commitId + // so we need to check if the image override with commitId is the same as the node's image + // or if the image override (without commitId) + current deployment commitId is the same as the node's image return !( - node.image === `${configOverride.image}:${deployment.commitId}` && + (configOverride.image === node.image || `${configOverride.image}:${deployment.commitId}` === node.image) && node.cpuMilli === configOverride.cpuMilli && node.memoryMb === configOverride.memoryMb && node.storageMb === configOverride.storageMb @@ -320,31 +323,28 @@ export class Supervisor { db: Knex, { routingId, - deployment, - nodeConfig + deployment }: { type: 'CREATE'; routingId: Node['routingId']; deployment: Deployment; - nodeConfig?: NodeConfig | undefined; } ): Promise> { let newNodeConfig = this.nodeProvider.defaultNodeConfig; - if (!nodeConfig) { - const nodeConfigOverride = await nodeConfigOverrides.search(db, { routingIds: [routingId] }); - if (nodeConfigOverride.isErr()) { - return Err(nodeConfigOverride.error); - } - const nodeConfigOverrideValue = nodeConfigOverride.value.get(routingId); - if (nodeConfigOverrideValue) { - newNodeConfig = nodeConfigOverrideValue; - } + + const nodeConfigOverride = await nodeConfigOverrides.search(db, { routingIds: [routingId] }); + if (nodeConfigOverride.isErr()) { + return Err(nodeConfigOverride.error); + } + const nodeConfigOverrideValue = nodeConfigOverride.value.get(routingId); + if (nodeConfigOverrideValue) { + newNodeConfig = nodeConfigOverrideValue; } return nodes.create(db, { routingId, deploymentId: deployment.id, - image: `${newNodeConfig.image}:${deployment.commitId}`, + image: newNodeConfig.image.includes(':') ? newNodeConfig.image : `${newNodeConfig.image}:${deployment.commitId}`, cpuMilli: newNodeConfig.cpuMilli, memoryMb: newNodeConfig.memoryMb, storageMb: newNodeConfig.storageMb