From d472ada57b54903da0d7f17253c098f9a1f2341c Mon Sep 17 00:00:00 2001 From: Aaron van Meerten Date: Thu, 18 Apr 2024 11:48:18 -0500 Subject: [PATCH] pass current inventory instead of count to cloud-specific implementations --- src/cloud_instance_manager.ts | 5 +- src/cloud_manager.ts | 4 +- src/custom_instance_manager.ts | 3 +- src/instance_launcher.ts | 2 +- src/oracle_instance_manager.ts | 3 +- src/oracle_instance_pool_manager.ts | 56 ++++--- src/test/oracle_instance_pool_manager.ts | 183 ++++++++++++++++++----- 7 files changed, 190 insertions(+), 66 deletions(-) diff --git a/src/cloud_instance_manager.ts b/src/cloud_instance_manager.ts index 2e548f2..171097e 100644 --- a/src/cloud_instance_manager.ts +++ b/src/cloud_instance_manager.ts @@ -1,6 +1,7 @@ import { InstanceGroup } from './instance_group'; import { Context } from './context'; import { CloudRetryStrategy } from './cloud_manager'; +import { InstanceState } from './instance_tracker'; export interface CloudInstance { instanceId: string; @@ -12,7 +13,7 @@ export interface CloudInstanceManager { launchInstances( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, ): Promise>; @@ -33,7 +34,7 @@ export abstract class AbstractCloudInstanceManager implements CloudInstanceManag async launchInstances( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, ): Promise> { ctx.logger.info(`[CloudInstanceManager] Launching a batch of ${quantity} instances in group ${group.name}`); diff --git a/src/cloud_manager.ts b/src/cloud_manager.ts index eb5862c..926732c 100644 --- a/src/cloud_manager.ts +++ b/src/cloud_manager.ts @@ -78,7 +78,7 @@ export default class CloudManager { async scaleUp( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, isScaleDownProtected: boolean, ): Promise { @@ -91,7 +91,7 @@ export default class CloudManager { return 0; } - const scaleUpResult = await instanceManager.launchInstances(ctx, group, groupCurrentCount, quantity); + const scaleUpResult = await instanceManager.launchInstances(ctx, group, currentInventory, quantity); let scaleUpCount = 0; await Promise.all( diff --git a/src/custom_instance_manager.ts b/src/custom_instance_manager.ts index f10108c..71dbb2b 100644 --- a/src/custom_instance_manager.ts +++ b/src/custom_instance_manager.ts @@ -2,6 +2,7 @@ import { execFile } from 'child_process'; import { InstanceGroup } from './instance_group'; import { Context } from './context'; import { AbstractCloudInstanceManager } from './cloud_instance_manager'; +import { InstanceState } from './instance_tracker'; export interface CustomInstanceManagerOptions { isDryRun: boolean; @@ -27,7 +28,7 @@ export default class CustomInstanceManager extends AbstractCloudInstanceManager async launchInstances( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, ): Promise> { ctx.logger.info(`[custom] Launching a batch of ${quantity} instances in group ${group.name}`); diff --git a/src/instance_launcher.ts b/src/instance_launcher.ts index bd291b4..d894b8b 100644 --- a/src/instance_launcher.ts +++ b/src/instance_launcher.ts @@ -126,7 +126,7 @@ export default class InstanceLauncher { const scaleUpCount = await this.cloudManager.scaleUp( ctx, group, - count, + currentInventory, actualScaleUpQuantity, scaleDownProtected, ); diff --git a/src/oracle_instance_manager.ts b/src/oracle_instance_manager.ts index 619799c..5e98a3d 100644 --- a/src/oracle_instance_manager.ts +++ b/src/oracle_instance_manager.ts @@ -7,6 +7,7 @@ import { ResourceSearchClient } from 'oci-resourcesearch'; import * as resourceSearch from 'oci-resourcesearch'; import { CloudRetryStrategy } from './cloud_manager'; import { AbstractCloudInstanceManager, CloudInstanceManager, CloudInstance } from './cloud_instance_manager'; +import { InstanceState } from './instance_tracker'; interface FaultDomainMap { [key: string]: string[]; @@ -43,7 +44,7 @@ export default class OracleInstanceManager implements CloudInstanceManager { async launchInstances( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, ): Promise> { ctx.logger.info(`[oracle] Launching a batch of ${quantity} instances in group ${group.name}`); diff --git a/src/oracle_instance_pool_manager.ts b/src/oracle_instance_pool_manager.ts index 97dbed2..33638b6 100644 --- a/src/oracle_instance_pool_manager.ts +++ b/src/oracle_instance_pool_manager.ts @@ -5,6 +5,7 @@ import { Context } from './context'; import { CloudRetryStrategy } from './cloud_manager'; import { CloudInstanceManager, CloudInstance } from './cloud_instance_manager'; import { workrequests } from 'oci-sdk'; +import { InstanceState } from './instance_tracker'; const maxTimeInSeconds = 60 * 60; // The duration for waiter configuration before failing. Currently set to 1 hour. const maxDelayInSeconds = 30; // The max delay for the waiter configuration. Currently set to 30 seconds @@ -65,11 +66,13 @@ export default class OracleInstancePoolManager implements CloudInstanceManager { async launchInstances( ctx: Context, group: InstanceGroup, - groupCurrentCount: number, + currentInventory: InstanceState[], quantity: number, ): Promise> { ctx.logger.info(`[oraclepool] Launching a batch of ${quantity} instances in group ${group.name}`); + const result = []; + this.computeManagementClient.regionId = group.region; const poolDetails = await this.computeManagementClient.getInstancePool({ instancePoolId: group.instanceConfigurationId, @@ -86,25 +89,36 @@ export default class OracleInstancePoolManager implements CloudInstanceManager { return instance.id; }); - ctx.logger.debug(`[oraclepool] Instance pool ${group.name} instances`, { instances: poolInstances.items }); + const currentInstanceIds = currentInventory.map((instance) => { + return instance.instanceId; + }); - const newSize = quantity + groupCurrentCount; - if (groupCurrentCount == poolDetails.instancePool.size) { - ctx.logger.debug(`[oraclepool] Instance pool ${group.name} size matches current count`, { - current: groupCurrentCount, - size: poolDetails.instancePool.size, - newSize, + // mark any instances not previously seen as being launched now + result.push( + ...existingInstanceIds.filter((instanceId) => { + return !currentInstanceIds.includes(instanceId); + }), + ); + + ctx.logger.debug(`[oraclepool] Instance pool ${group.name} instances`, { instances: poolInstances.items }); + if (result.length > 0) { + ctx.logger.warn(`[oraclepool] Found instances in pool not in inventory, marking as launched now`, { + result, }); - } else { - ctx.logger.error(`[oraclepool] Instance pool ${group.name} size DOES NOT matches current count`, { - current: groupCurrentCount, - size: poolDetails.instancePool.size, + } + + // always use the group desired count for instance pools + const newSize = group.scalingOptions.desiredCount; + if (newSize == poolDetails.instancePool.size) { + // underlying pool size matches the desired count, so no need to update group + ctx.logger.info(`[oraclepool] Instance pool ${group.name} size matches desired count, no changes needed`, { newSize, }); + return result; } if (this.isDryRun) { - ctx.logger.info(`[oracle] Dry run enabled, instance pool size change skipped`, { newSize }); + ctx.logger.info(`[oraclepool] Dry run enabled, instance pool size change skipped`, { newSize }); } else { const updateResult = await this.computeManagementClient.updateInstancePool({ instancePoolId: group.instanceConfigurationId, @@ -142,13 +156,15 @@ export default class OracleInstancePoolManager implements CloudInstanceManager { instancePoolId: group.instanceConfigurationId, }); - const result = newPoolInstances.items - .map((instance) => { - return instance.id; - }) - .filter((instanceId) => { - return !existingInstanceIds.includes(instanceId); - }); + result.push( + ...newPoolInstances.items + .map((instance) => { + return instance.id; + }) + .filter((instanceId) => { + return !existingInstanceIds.includes(instanceId); + }), + ); ctx.logger.info(`[oraclepool] Finished launching all the instances in group ${group.name}`, { result }); diff --git a/src/test/oracle_instance_pool_manager.ts b/src/test/oracle_instance_pool_manager.ts index efdcd5b..ba9ac90 100644 --- a/src/test/oracle_instance_pool_manager.ts +++ b/src/test/oracle_instance_pool_manager.ts @@ -20,8 +20,35 @@ const group = { region: 'testregion', compartmentId: 'testcpt', instanceConfigurationId: 'testpoolid', + enableAutoScale: true, + enableLaunch: true, + scalingOptions: { + minDesired: 1, + maxDesired: 3, + desiredCount: 2, + scaleUpQuantity: 1, + scaleDownQuantity: 1, + scaleUpThreshold: 0.8, + scaleDownThreshold: 0.3, + scalePeriod: 60, + scaleUpPeriodsCount: 2, + scaleDownPeriodsCount: 2, + }, }; +const instancePool = { + id: group.instanceConfigurationId, + name: group.name, + compartmentId: group.compartmentId, + instanceConfigurationId: 'testid', + size: 2, +}; + +const instancePoolInstances = [{ id: 'testinstanceid-1' }, { id: 'testinstanceid-2' }]; +const currentInventoryInstances = instancePoolInstances.map((instance) => { + return { instanceId: instance.id }; +}); + describe('InstancePoolManager', () => { const manager = new OracleInstancePoolManager({ isDryRun: false, @@ -30,15 +57,9 @@ describe('InstancePoolManager', () => { }); const mockWaiters = { - forInstancePool: mock.fn((request, _) => { + forInstancePool: mock.fn(() => { return { - instancePool: { - id: request.instancePoolId, - name: group.name, - compartmentId: group.compartmentId, - instanceConfigurationId: 'testid', - size: 2, - }, + instancePool, }; }), forDetachInstancePoolInstance: mock.fn((request) => { @@ -53,27 +74,20 @@ describe('InstancePoolManager', () => { createWaiters: mock.fn(() => { return mockWaiters; }), - getInstancePool: mock.fn((request) => { + getInstancePool: mock.fn((_) => { return { - instancePool: { - id: request.instancePoolId, - name: group.name, - compartmentId: group.compartmentId, - instanceConfigurationId: 'testid', - size: 2, - }, + instancePool, }; }), listInstancePoolInstances: mock.fn(() => { - return { items: [{ id: 'testinstanceid-1' }, { id: 'testinstanceid-2' }] }; + return { items: instancePoolInstances }; }), updateInstancePool: mock.fn((request) => { - return { - id: request.instancePoolId, - name: group.name, - compartmentId: group.compartmentId, - instanceConfigurationId: 'testid', - size: request.size, + return { + instancePool: { + ...instancePool, + size: request.updateInstancePoolDetails.size, + }, }; }), }; @@ -98,6 +112,10 @@ describe('InstancePoolManager', () => { }; afterEach(() => { + mockComputeManagementClient.createWaiters.mock.resetCalls(); + mockComputeManagementClient.getInstancePool.mock.resetCalls(); + mockComputeManagementClient.listInstancePoolInstances.mock.resetCalls(); + mockComputeManagementClient.updateInstancePool.mock.resetCalls(); mock.restoreAll(); }); @@ -112,37 +130,124 @@ describe('InstancePoolManager', () => { retryableStatusCodes: [404, 429], }); log('TEST', 'ended getInstances test'); - assert.ok(instances); + assert.ok(instances, 'some instances should be returned'); log('TEST', 'found instances', instances); + assert.equal(instances.length, 2, 'two instances should be returned'); }); }); describe('launchInstances', () => { + console.log('Starting launchInstances test'); // This is a test for the launchInstances method test('will launch instances in a group', async () => { - console.log('Starting launchInstances test'); + console.log('Starting single launch test'); + const desiredCount = 3; mockWaiters.forInstancePool.mock.mockImplementationOnce((_) => { return { instancePool: { - id: group.instanceConfigurationId, - name: group.name, - compartmentId: group.compartmentId, - instanceConfigurationId: 'testid', - size: 3, + ...instancePool, + size: desiredCount, // this is the critical bit for showing that the instance pool has been updated }, }; }); - mockComputeManagementClient.listInstancePoolInstances.mock.mockImplementationOnce((_) => { - return { items: [{ id: 'testinstanceid-1' }, { id: 'testinstanceid-2' }, { id: 'new-instance-id' }] }; - }, 2); - const instances = await manager.launchInstances(context, group, 2, 1); - console.log(mockComputeManagementClient.updateInstancePool.mock.callCount()); - assert.equal(mockComputeManagementClient.updateInstancePool.mock.callCount(), 1); - assert.ok(instances); - assert.equal(instances[0], 'new-instance-id'); - assert.equal(instances.length, 1); + + // the second time listInstancePoolInstances is called, return a new instance with id 'new-instance-id' + mockComputeManagementClient.listInstancePoolInstances.mock.mockImplementationOnce( + (_) => { + return { + // list now includes new-instance-id + items: [...instancePoolInstances, { id: 'new-instance-id' }], + }; + }, + 1, // this is the critical count for mocking the second call instead of the first + ); + + // override group.scalingOptions.desiredCount to control size of instance pool + const lgroup = { ...group, scalingOptions: { ...group.scalingOptions, desiredCount: desiredCount } }; + const instances = await manager.launchInstances(context, lgroup, currentInventoryInstances, 1); + assert.equal( + mockComputeManagementClient.updateInstancePool.mock.callCount(), + 1, + 'updateInstancePool should be called', + ); + assert.ok(instances, 'some instances should be returned'); + assert.equal(instances[0], 'new-instance-id', 'new instance id should be returned'); + assert.equal(instances.length, 1, 'only one instance should be returned'); log('TEST', 'ended launchInstances test'); log('TEST', 'launched instances', instances); }); + test('will not launch instances in a group if desiredCount is already reached', async () => { + console.log('Starting skip launch test'); + const desiredCount = 3; + // return pool already in desired state + mockComputeManagementClient.getInstancePool.mock.mockImplementationOnce((_) => { + return { + instancePool: { + ...instancePool, + size: desiredCount, + }, + }; + }); + // when listInstancePoolInstances is called, return 3 instances including newest with id 'new-instance-id' + mockComputeManagementClient.listInstancePoolInstances.mock.mockImplementationOnce((_) => { + return { items: [...instancePoolInstances, { id: 'new-instance-id' }] }; + }); + + // override group.scalingOptions.desiredCount to control size of instance pool + const lgroup = { ...group, scalingOptions: { ...group.scalingOptions, desiredCount: desiredCount } }; + const instances = await manager.launchInstances( + context, + lgroup, + [...currentInventoryInstances, { instanceId: 'new-instance-id' }], + 1, + ); + assert.equal( + mockComputeManagementClient.updateInstancePool.mock.callCount(), + 0, + 'updateInstancePool should not be called', + ); + assert.equal(instances.length, 0, 'no instances should be returned'); + log('TEST', 'ended skip launch test'); + log('TEST', 'launched instances', instances); + }); + + test('will see previously launched instances in a group if missed the first time', async () => { + console.log('Starting find missing instances test'); + const desiredCount = 3; + // return pool already in desired state + mockComputeManagementClient.getInstancePool.mock.mockImplementationOnce((_) => { + return { + instancePool: { + ...instancePool, + size: desiredCount, + }, + }; + }); + // when listInstancePoolInstances is called, return 3 instances including newest with id 'new-instance-id' + mockComputeManagementClient.listInstancePoolInstances.mock.mockImplementationOnce((_) => { + return { items: [...instancePoolInstances, { id: 'new-instance-id' }] }; + }); + + // override group.scalingOptions.desiredCount to control size of instance pool + const lgroup = { ...group, scalingOptions: { ...group.scalingOptions, desiredCount: desiredCount } }; + + const instances = await manager.launchInstances( + context, + lgroup, + currentInventoryInstances, // we pass in currentInventoryInstances (with 2 entries) and expect to see the new instance as launched + 1, + ); + // still expect no pool updates + assert.equal( + mockComputeManagementClient.updateInstancePool.mock.callCount(), + 0, + 'updateInstancePool should not be called', + ); + assert.ok(instances, 'some instances should be returned'); + assert.equal(instances[0], 'new-instance-id', 'new instance id should be returned'); + assert.equal(instances.length, 1, 'only one instance should be returned'); + log('TEST', 'ended find missing instances test'); + log('TEST', 'launched instances', instances); + }); }); });