From 0b34e01dcf2e00687e6feee6ff85efa4497f8e0a Mon Sep 17 00:00:00 2001 From: Tommaso Scarlatti Date: Mon, 5 Jun 2023 17:22:33 +0200 Subject: [PATCH] Disable Cost Monitoring in unsupported regions (#240) * Add a way to handle unsupported regions for features in featureFlagProvider * Update tests and references to reflect changes in featureFlagProvider --- .../__tests__/FeatureFlagsProvider.test.ts | 33 ++++++++++++++----- .../src/feature-flags/featureFlagsProvider.ts | 26 ++++++++++++++- frontend/src/feature-flags/useFeatureFlag.ts | 6 ++-- .../EnableCostMonitoringButton.test.tsx | 9 +++++ .../useCostMonitoringStatus.test.tsx | 25 ++++++++++++++ .../src/old-pages/Configure/Queues/Queues.tsx | 5 ++- .../__tests__/mapComputeResources.test.ts | 19 ++++++----- .../Configure/Queues/queues.mapper.ts | 3 +- frontend/src/old-pages/Configure/util.tsx | 7 +++- 9 files changed, 110 insertions(+), 23 deletions(-) diff --git a/frontend/src/feature-flags/__tests__/FeatureFlagsProvider.test.ts b/frontend/src/feature-flags/__tests__/FeatureFlagsProvider.test.ts index f0cecd87..d5be518a 100644 --- a/frontend/src/feature-flags/__tests__/FeatureFlagsProvider.test.ts +++ b/frontend/src/feature-flags/__tests__/FeatureFlagsProvider.test.ts @@ -13,24 +13,25 @@ import {AvailableFeature} from '../types' describe('given a feature flags provider and a list of rules', () => { const subject = featureFlagsProvider + let region: string | undefined = 'us-west-1' describe('when the features list is retrieved', () => { it('should return the list', async () => { - const features = await subject('0.0.0') + const features = await subject('0.0.0', region) expect(features).toEqual([]) }) }) describe('when the version is between 3.1.0 and 3.2.0', () => { it('should return the list of available features', async () => { - const features = await subject('3.1.5') + const features = await subject('3.1.5', region) expect(features).toEqual(['multiuser_cluster']) }) }) describe('when the version is between 3.2.0 and 3.3.0', () => { it('should return the list of available features', async () => { - const features = await subject('3.2.5') + const features = await subject('3.2.5', region) expect(features).toEqual([ 'multiuser_cluster', 'fsx_ontap', @@ -46,7 +47,7 @@ describe('given a feature flags provider and a list of rules', () => { describe('when the version is between 3.3.0 and 3.4.0', () => { it('should return the list of available features', async () => { - const features = await subject('3.3.2') + const features = await subject('3.3.2', region) expect(features).toEqual([ 'multiuser_cluster', 'fsx_ontap', @@ -68,7 +69,7 @@ describe('given a feature flags provider and a list of rules', () => { describe('when the version is between 3.4.0 and 3.6.0', () => { it('should return the list of available features', async () => { - const features = await subject('3.4.1') + const features = await subject('3.4.1', region) expect(features).toEqual([ 'multiuser_cluster', 'fsx_ontap', @@ -92,7 +93,7 @@ describe('given a feature flags provider and a list of rules', () => { describe('when the version is above and 3.6.0', () => { it('should return the list of available features', async () => { - const features = await subject('3.6.0') + const features = await subject('3.6.0', region) expect(features).toEqual([ 'multiuser_cluster', 'fsx_ontap', @@ -122,7 +123,7 @@ describe('given a feature flags provider and a list of rules', () => { window.sessionStorage.setItem('additionalFeatures', '["cost_monitoring"]') }) it('should be included in the list of features', async () => { - const features = await subject('3.1.5') + const features = await subject('3.1.5', region) expect(features).toEqual([ 'multiuser_cluster', 'cost_monitoring', @@ -135,8 +136,24 @@ describe('given a feature flags provider and a list of rules', () => { window.sessionStorage.clear() }) it('should not be included in the list of features', async () => { - const features = await subject('3.1.5') + const features = await subject('3.1.5', region) expect(features).toEqual(['multiuser_cluster']) }) }) + + describe('when a feature is not supported in a region', () => { + it('should return the list of available features without the unsupported feature', async () => { + region = 'us-gov-west-1' + const features = await subject('3.6.0', region) + expect(features).not.toContain(['cost_monitoring']) + }) + }) + + describe('when a feature is not supported in a region and the region is undefined', () => { + it('should return an empty list', async () => { + region = undefined + const features = await subject('3.6.0', region) + expect(features).not.toContain(['cost_monitoring']) + }) + }) }) diff --git a/frontend/src/feature-flags/featureFlagsProvider.ts b/frontend/src/feature-flags/featureFlagsProvider.ts index e26a77d5..0507b086 100644 --- a/frontend/src/feature-flags/featureFlagsProvider.ts +++ b/frontend/src/feature-flags/featureFlagsProvider.ts @@ -34,6 +34,26 @@ const versionToFeaturesMap: Record = { '3.6.0': ['rhel8', 'new_resources_limits'], } +const featureToUnsupportedRegionsMap: Partial< + Record +> = { + cost_monitoring: ['us-gov-west-1'], +} + +function isSupportedInRegion( + feature: AvailableFeature, + region?: string, +): boolean { + if (feature in featureToUnsupportedRegionsMap) { + if (!region) { + return false + } else { + return !featureToUnsupportedRegionsMap[feature]!.includes(region) + } + } + return true +} + function composeFlagsListByVersion(currentVersion: string): AvailableFeature[] { let features: Set = new Set([]) @@ -46,7 +66,10 @@ function composeFlagsListByVersion(currentVersion: string): AvailableFeature[] { return Array.from(features) } -export function featureFlagsProvider(version: string): AvailableFeature[] { +export function featureFlagsProvider( + version: string, + region?: string, +): AvailableFeature[] { const features: AvailableFeature[] = [] const additionalFeatures = window.sessionStorage.getItem('additionalFeatures') const additionalFeaturesParsed = additionalFeatures @@ -56,4 +79,5 @@ export function featureFlagsProvider(version: string): AvailableFeature[] { return features .concat(composeFlagsListByVersion(version)) .concat(additionalFeaturesParsed) + .filter(feature => isSupportedInRegion(feature, region)) } diff --git a/frontend/src/feature-flags/useFeatureFlag.ts b/frontend/src/feature-flags/useFeatureFlag.ts index 959178f6..ba270c70 100644 --- a/frontend/src/feature-flags/useFeatureFlag.ts +++ b/frontend/src/feature-flags/useFeatureFlag.ts @@ -14,14 +14,16 @@ import {AvailableFeature} from './types' export function useFeatureFlag(feature: AvailableFeature): boolean { const version = useState(['app', 'version', 'full']) - return isFeatureEnabled(version, feature) + const region = useState(['aws', 'region']) + return isFeatureEnabled(version, region, feature) } export function isFeatureEnabled( version: string, + region: string, feature: AvailableFeature, ): boolean { - const features = new Set(featureFlagsProvider(version)) + const features = new Set(featureFlagsProvider(version, region)) return features.has(feature) } diff --git a/frontend/src/old-pages/Clusters/Costs/__tests__/EnableCostMonitoringButton.test.tsx b/frontend/src/old-pages/Clusters/Costs/__tests__/EnableCostMonitoringButton.test.tsx index 9986bcc3..2d248640 100644 --- a/frontend/src/old-pages/Clusters/Costs/__tests__/EnableCostMonitoringButton.test.tsx +++ b/frontend/src/old-pages/Clusters/Costs/__tests__/EnableCostMonitoringButton.test.tsx @@ -61,6 +61,9 @@ describe('given a component to activate cost monitoring for the account', () => app: { version: {full: '3.2.0'}, }, + aws: { + region: 'us-west-1', + }, }) screen = render( @@ -91,6 +94,9 @@ describe('given a component to activate cost monitoring for the account', () => app: { version: {full: '3.2.0'}, }, + aws: { + region: 'us-west-1', + }, }) mockGetCostMonitoringStatus.mockResolvedValue(true) @@ -117,6 +123,9 @@ describe('given a component to activate cost monitoring for the account', () => app: { version: {full: '3.1.5'}, }, + aws: { + region: 'us-west-1', + }, }) screen = render( diff --git a/frontend/src/old-pages/Clusters/Costs/__tests__/useCostMonitoringStatus.test.tsx b/frontend/src/old-pages/Clusters/Costs/__tests__/useCostMonitoringStatus.test.tsx index 34c4dc5e..1d6f461b 100644 --- a/frontend/src/old-pages/Clusters/Costs/__tests__/useCostMonitoringStatus.test.tsx +++ b/frontend/src/old-pages/Clusters/Costs/__tests__/useCostMonitoringStatus.test.tsx @@ -41,6 +41,7 @@ jest.mock('../../../../model', () => { describe('given a hook to get the cost monitoring status', () => { beforeEach(() => { jest.clearAllMocks() + mockQueryClient.resetQueries() }) describe('when PC version is at least 3.2.0', () => { @@ -49,6 +50,9 @@ describe('given a hook to get the cost monitoring status', () => { app: { version: {full: '3.2.0'}, }, + aws: { + region: 'us-west-1', + }, }) }) @@ -65,6 +69,27 @@ describe('given a hook to get the cost monitoring status', () => { app: { version: {full: '3.1.5'}, }, + aws: { + region: 'us-west-1', + }, + }) + }) + it('should not request the cost monitoring status', async () => { + renderHook(() => useCostMonitoringStatus(), {wrapper}) + + expect(mockGetCostMonitoringStatus).toHaveBeenCalledTimes(0) + }) + }) + + describe('when PC version is at least 3.2.0, and the region is us-gov-west-1', () => { + beforeEach(() => { + mockStore.getState.mockReturnValue({ + app: { + version: {full: '3.2.0'}, + }, + aws: { + region: 'us-gov-west-1', + }, }) }) it('should not request the cost monitoring status', async () => { diff --git a/frontend/src/old-pages/Configure/Queues/Queues.tsx b/frontend/src/old-pages/Configure/Queues/Queues.tsx index 2a96e39d..311067a8 100644 --- a/frontend/src/old-pages/Configure/Queues/Queues.tsx +++ b/frontend/src/old-pages/Configure/Queues/Queues.tsx @@ -68,6 +68,7 @@ import InfoLink from '../../../components/InfoLink' // Constants const queuesPath = ['app', 'wizard', 'config', 'Scheduling', 'SlurmQueues'] const queuesErrorsPath = ['app', 'wizard', 'errors', 'queues'] +const defaultRegion = getState(['aws', 'region']) export function useClusterResourcesLimits(): ClusterResourcesLimits { const newResourcesLimits = useFeatureFlag('new_resources_limits') @@ -203,7 +204,7 @@ function queueValidate(queueIndex: any) { } const version = getState(['app', 'version', 'full']) - const isMultiAZActive = isFeatureEnabled(version, 'multi_az') + const isMultiAZActive = isFeatureEnabled(version, defaultRegion, 'multi_az') if (!queueSubnet) { let message: string if (isMultiAZActive) { @@ -219,6 +220,7 @@ function queueValidate(queueIndex: any) { const isMultiInstanceTypesActive = isFeatureEnabled( version, + defaultRegion, 'queues_multiple_instance_types', ) const {validateComputeResources} = !isMultiInstanceTypesActive @@ -246,6 +248,7 @@ function queueValidate(queueIndex: any) { const isMemoryBasedSchedulingActive = isFeatureEnabled( version, + defaultRegion, 'memory_based_scheduling', ) if (isMemoryBasedSchedulingActive) { diff --git a/frontend/src/old-pages/Configure/Queues/__tests__/mapComputeResources.test.ts b/frontend/src/old-pages/Configure/Queues/__tests__/mapComputeResources.test.ts index 8c3e8778..2aacc8ca 100644 --- a/frontend/src/old-pages/Configure/Queues/__tests__/mapComputeResources.test.ts +++ b/frontend/src/old-pages/Configure/Queues/__tests__/mapComputeResources.test.ts @@ -17,6 +17,7 @@ import { describe('given a mapper to import the ComputeResources section of the Scheduling config', () => { let mockVersion: string + let mockRegion: string describe('when the application supports multiple instance types', () => { beforeEach(() => { @@ -49,9 +50,9 @@ describe('given a mapper to import the ComputeResources section of the Schedulin ], }, ] - expect(mapComputeResources(mockVersion, mockComputeResources)).toEqual( - expected, - ) + expect( + mapComputeResources(mockVersion, mockRegion, mockComputeResources), + ).toEqual(expected) }) }) describe('when the configuration was created with the flexible instance types format', () => { @@ -72,9 +73,9 @@ describe('given a mapper to import the ComputeResources section of the Schedulin ] }) it('should leave the configuration as is', () => { - expect(mapComputeResources(mockVersion, mockComputeResources)).toEqual( - mockComputeResources, - ) + expect( + mapComputeResources(mockVersion, mockRegion, mockComputeResources), + ).toEqual(mockComputeResources) }) }) }) @@ -95,9 +96,9 @@ describe('given a mapper to import the ComputeResources section of the Schedulin }) it('should leave the configuration as is', () => { - expect(mapComputeResources(mockVersion, mockComputeResources)).toEqual( - mockComputeResources, - ) + expect( + mapComputeResources(mockVersion, mockRegion, mockComputeResources), + ).toEqual(mockComputeResources) }) }) }) diff --git a/frontend/src/old-pages/Configure/Queues/queues.mapper.ts b/frontend/src/old-pages/Configure/Queues/queues.mapper.ts index 298dee91..1ffe0b1c 100644 --- a/frontend/src/old-pages/Configure/Queues/queues.mapper.ts +++ b/frontend/src/old-pages/Configure/Queues/queues.mapper.ts @@ -35,11 +35,12 @@ function mapComputeResource( export function mapComputeResources( version: string, + region: string, computeResourcesConfig: | SingleInstanceComputeResource[] | MultiInstanceComputeResource[], ) { - if (!isFeatureEnabled(version, 'queues_multiple_instance_types')) { + if (!isFeatureEnabled(version, region, 'queues_multiple_instance_types')) { return computeResourcesConfig } diff --git a/frontend/src/old-pages/Configure/util.tsx b/frontend/src/old-pages/Configure/util.tsx index 8997440f..1b542444 100644 --- a/frontend/src/old-pages/Configure/util.tsx +++ b/frontend/src/old-pages/Configure/util.tsx @@ -26,6 +26,7 @@ function loadTemplateLazy(config: any, callback?: () => void) { const keypairNames = new Set(keypairs.map((kp: any) => kp.KeyName)) const keypairPath = ['HeadNode', 'Ssh', 'KeyName'] const version = getState(['app', 'version', 'full']) + const defaultRegion = getState(['aws', 'region']) if (getIn(config, ['Image', 'CustomAmi'])) setState(['app', 'wizard', 'customAMI', 'enabled'], true) @@ -67,7 +68,11 @@ function loadTemplateLazy(config: any, callback?: () => void) { i, 'ComputeResources', ]) - computeResources = mapComputeResources(version, computeResources) + computeResources = mapComputeResources( + version, + defaultRegion, + computeResources, + ) config = setIn( config, ['Scheduling', 'SlurmQueues', i, 'ComputeResources'],