From 9c0ea11d1c0dbd0fbf4d582543fe63ea40cdef1a Mon Sep 17 00:00:00 2001
From: Aaron van Meerten <aaron.van.meerten@8x8.com>
Date: Tue, 21 May 2024 14:16:24 -0500
Subject: [PATCH] feat(validator): active instances validation properly
 filtered for shutdown instances (#156)

adds unit tests for validator groupHasActiveInstances method
---
 src/app.ts            |   2 +-
 src/test/validator.ts | 105 ++++++++++++++++++++++++++++++++++++++++++
 src/validator.ts      |  21 ++++++++-
 3 files changed, 126 insertions(+), 2 deletions(-)
 create mode 100644 src/test/validator.ts

diff --git a/src/app.ts b/src/app.ts
index 7e606d1..939b487 100644
--- a/src/app.ts
+++ b/src/app.ts
@@ -287,7 +287,7 @@ const h = new Handlers({
     scalingManager,
 });
 
-const validator = new Validator({ instanceTracker, instanceGroupManager });
+const validator = new Validator({ instanceTracker, instanceGroupManager, metricsLoop });
 const loggedPaths = ['/sidecar*', '/groups*'];
 app.use(loggedPaths, stats.middleware);
 app.use('/', context.injectContext);
diff --git a/src/test/validator.ts b/src/test/validator.ts
new file mode 100644
index 0000000..eb8a16f
--- /dev/null
+++ b/src/test/validator.ts
@@ -0,0 +1,105 @@
+/* eslint-disable @typescript-eslint/ban-ts-comment */
+// @ts-nocheck
+
+import assert from 'node:assert';
+import test, { afterEach, describe, mock } from 'node:test';
+
+import Validator from '../validator';
+
+describe('Validator', () => {
+    let context = {
+        logger: {
+            info: mock.fn(),
+            debug: mock.fn(),
+            error: mock.fn(),
+            warn: mock.fn(),
+        },
+    };
+
+    const instanceTracker = {
+        trimCurrent: mock.fn(),
+    };
+
+    const instanceGroupManager = {
+        getInstanceGroup: mock.fn(),
+    };
+
+    const metricsLoop = {
+        getCloudInstances: mock.fn(),
+    };
+
+    const groupName = 'group';
+
+    const validator = new Validator({ instanceTracker, instanceGroupManager, metricsLoop });
+
+    afterEach(() => {
+        context = {
+            logger: {
+                info: mock.fn(),
+                debug: mock.fn(),
+                error: mock.fn(),
+                warn: mock.fn(),
+            },
+        };
+    });
+
+    // these tests are for the groupHasActiveInstances method
+    describe('validator', () => {
+        test('should return false for a group with no instances', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => []);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => []);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, false);
+        });
+
+        test('should return true for a group with an instance', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => [{ instanceId: '1' }]);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => []);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, true);
+        });
+
+        test('should return false for a group with an instance, shutdown completed', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => [
+                { instanceId: '1', shutdownComplete: true },
+            ]);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => []);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, false);
+        });
+
+        test('should return true for a group with one active and one shutdown instance', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => [
+                { instanceId: '1' },
+                { instanceId: '2', shutdownComplete: new Date().toISOString() },
+            ]);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => []);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, true);
+        });
+
+        test('should return true for a group with cloud status running', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => [{ instanceId: '1' }]);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => [
+                { instanceId: '1', cloudStatus: 'running' },
+            ]);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, true);
+        });
+
+        test('should return false for a group with cloud status shutdown', async () => {
+            instanceTracker.trimCurrent.mock.mockImplementationOnce(() => [{ instanceId: '1' }]);
+            metricsLoop.getCloudInstances.mock.mockImplementationOnce(() => [
+                { instanceId: '1', cloudStatus: 'shutdown' },
+            ]);
+
+            const result = await validator.groupHasActiveInstances(context, groupName);
+            assert.strictEqual(result, false);
+        });
+    });
+});
diff --git a/src/validator.ts b/src/validator.ts
index c43b607..ca60500 100644
--- a/src/validator.ts
+++ b/src/validator.ts
@@ -3,9 +3,11 @@ import { Context } from './context';
 import { Request } from 'express';
 import InstanceGroupManager, { InstanceGroup } from './instance_group';
 import { InstanceGroupDesiredValuesRequest } from './handlers';
+import MetricsLoop from './metrics_loop';
 
 export interface ValidatorOptions {
     instanceTracker: InstanceTracker;
+    metricsLoop: MetricsLoop;
     instanceGroupManager: InstanceGroupManager;
     scaleStatus?: string;
     cloudStatus?: string;
@@ -16,17 +18,34 @@ export interface ValidatorOptions {
 export default class Validator {
     private instanceTracker: InstanceTracker;
     private instanceGroupManager: InstanceGroupManager;
+    private metricsLoop: MetricsLoop;
 
     constructor(options: ValidatorOptions) {
         this.instanceTracker = options.instanceTracker;
         this.instanceGroupManager = options.instanceGroupManager;
+        this.metricsLoop = options.metricsLoop;
 
         this.groupHasActiveInstances = this.groupHasActiveInstances.bind(this);
     }
 
     async groupHasActiveInstances(context: Context, name: string): Promise<boolean> {
         const instanceStates = await this.instanceTracker.trimCurrent(context, name, false);
-        return instanceStates.length > 0;
+        const cloudInstances = await this.metricsLoop.getCloudInstances(name);
+        const shutdownInstances = cloudInstances
+            .filter((cv, _) => {
+                return cv.cloudStatus.toLowerCase() == 'shutdown' || cv.cloudStatus.toLowerCase() == 'terminated';
+            })
+            .map((cv, _) => cv.instanceId);
+
+        return (
+            instanceStates.filter((v, _) => {
+                // skip any that have completed shutdown
+                if (v.shutdownComplete) return false;
+
+                // only include instances that are not listed as SHUTDOWN or TERMINATED
+                return !shutdownInstances.includes(v.instanceId);
+            }).length > 0
+        );
     }
 
     groupHasValidDesiredValues(minDesired: number, maxDesired: number, desiredCount: number): boolean {