From c3d803423333208d9f11515b18873bbdb7cd2287 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Thu, 20 Apr 2023 09:59:12 -0700 Subject: [PATCH 01/11] Get allowedVmSizes at run time --- src/TesApi.Web/BatchScheduler.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/TesApi.Web/BatchScheduler.cs b/src/TesApi.Web/BatchScheduler.cs index f13c37dcb..b8ab4d2bb 100644 --- a/src/TesApi.Web/BatchScheduler.cs +++ b/src/TesApi.Web/BatchScheduler.cs @@ -63,10 +63,10 @@ public partial class BatchScheduler : IBatchScheduler private readonly string dockerInDockerImageName; private readonly string blobxferImageName; private readonly string cromwellDrsLocalizerImageName; + private readonly IConfiguration configuration; private readonly ILogger logger; private readonly IAzureProxy azureProxy; private readonly IStorageAccessProvider storageAccessProvider; - private readonly IEnumerable allowedVmSizes; private readonly IBatchQuotaVerifier quotaVerifier; private readonly IBatchSkuInformationProvider skuInformationProvider; private readonly List tesTaskStateTransitions; @@ -90,6 +90,9 @@ public partial class BatchScheduler : IBatchScheduler private HashSet onlyLogBatchTaskStateOnce = new(); + private static string GetStringValue(IConfiguration configuration, string key, string defaultValue = "") => string.IsNullOrWhiteSpace(configuration[key]) ? defaultValue : configuration[key]; + private IEnumerable allowedVmSizes => GetStringValue(this.configuration, "AllowedVmSizes", null)?.Split(',', StringSplitOptions.RemoveEmptyEntries).ToList(); + /// /// Orchestrates s on Azure Batch /// @@ -140,8 +143,8 @@ public BatchScheduler( this.quotaVerifier = quotaVerifier; this.skuInformationProvider = skuInformationProvider; this.containerRegistryProvider = containerRegistryProvider; + this.configuration = configuration; - this.allowedVmSizes = GetStringValue(configuration, "AllowedVmSizes", null)?.Split(',', StringSplitOptions.RemoveEmptyEntries).ToList(); this.usePreemptibleVmsOnly = batchSchedulingOptions.Value.UsePreemptibleVmsOnly; this.batchNodesSubnetId = batchNodesOptions.Value.SubnetId; this.dockerInDockerImageName = batchImageNameOptions.Value.Docker; @@ -188,8 +191,6 @@ public BatchScheduler( logger.LogInformation($"usePreemptibleVmsOnly: {usePreemptibleVmsOnly}"); - static string GetStringValue(IConfiguration configuration, string key, string defaultValue = "") => string.IsNullOrWhiteSpace(configuration[key]) ? defaultValue : configuration[key]; - static bool tesTaskIsQueuedInitializingOrRunning(TesTask tesTask) => tesTask.State == TesState.QUEUEDEnum || tesTask.State == TesState.INITIALIZINGEnum || tesTask.State == TesState.RUNNINGEnum; static bool tesTaskIsInitializingOrRunning(TesTask tesTask) => tesTask.State == TesState.INITIALIZINGEnum || tesTask.State == TesState.RUNNINGEnum; static bool tesTaskIsQueuedOrInitializing(TesTask tesTask) => tesTask.State == TesState.QUEUEDEnum || tesTask.State == TesState.INITIALIZINGEnum; From 7d68c9a43ab6a3d2efc3137223a87c4d3bc58a73 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Fri, 28 Apr 2023 16:21:30 -0700 Subject: [PATCH 02/11] Hold allowed vms in background service instead of config --- src/TesApi.Tests/ConfigurationUtilsTests.cs | 4 +- src/TesApi.Web/AllowedVmSizesService.cs | 73 +++++++++++++++++++++ src/TesApi.Web/BatchScheduler.cs | 9 ++- src/TesApi.Web/ConfigurationUtils.cs | 9 +-- src/TesApi.Web/DoOnceAtStartUpService.cs | 52 --------------- src/TesApi.Web/Startup.cs | 2 +- 6 files changed, 85 insertions(+), 64 deletions(-) create mode 100644 src/TesApi.Web/AllowedVmSizesService.cs delete mode 100644 src/TesApi.Web/DoOnceAtStartUpService.cs diff --git a/src/TesApi.Tests/ConfigurationUtilsTests.cs b/src/TesApi.Tests/ConfigurationUtilsTests.cs index fed161189..e41f86f51 100644 --- a/src/TesApi.Tests/ConfigurationUtilsTests.cs +++ b/src/TesApi.Tests/ConfigurationUtilsTests.cs @@ -76,9 +76,9 @@ public async Task UnsupportedVmSizeInAllowedVmSizesFileIsIgnoredAndTaggedWithWar batchQuotaProvider: GetMockQuotaProvider()); var configurationUtils = serviceProvider.GetT(); - await configurationUtils.ProcessAllowedVmSizesConfigurationFileAsync(); + var result = await configurationUtils.ProcessAllowedVmSizesConfigurationFileAsync(); - Assert.AreEqual("VmSize1,VmSize2,VmFamily3", serviceProvider.Configuration["AllowedVmSizes"]); + Assert.AreEqual("VmSize1,VmSize2,VmFamily3", string.Join(",", result)); var expectedAllowedVmSizesFileContent = "VmSize1\n" + diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs new file mode 100644 index 000000000..bb33ca971 --- /dev/null +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace TesApi.Web +{ + /// + /// Hosted service that executes one-time set up tasks at start up. + /// + public class AllowedVmSizesService : BackgroundService + { + private readonly ILogger logger; + private readonly ConfigurationUtils configUtils; + private List allowedVmSizes; + private Task startTask; + + /// + /// Hosted service that executes one-time set-up tasks at start up. + /// + /// + /// + public AllowedVmSizesService(ConfigurationUtils configUtils, ILogger logger) + { + ArgumentNullException.ThrowIfNull(configUtils); + ArgumentNullException.ThrowIfNull(logger); + + this.configUtils = configUtils; + this.logger = logger; + } + + /// + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + startTask = Task.Run(async () => + { + using (logger.BeginScope("Executing Start Up tasks")) + { + try + { + logger.LogInformation("Executing Configuration Utils Setup"); + allowedVmSizes = await configUtils.ProcessAllowedVmSizesConfigurationFileAsync(); + } + catch (Exception e) + { + logger.LogError(e, "Failed to execute start up tasks"); + throw; + } + } + }); + await startTask; + } + + /// + /// Awaits start up and then return allowed vm sizes. + /// + /// + public async Task> GetAllowedVmSizes() + { + if (allowedVmSizes == null) + { + await startTask; + } + + return allowedVmSizes; + } + } +} diff --git a/src/TesApi.Web/BatchScheduler.cs b/src/TesApi.Web/BatchScheduler.cs index b8ab4d2bb..4cf69dd51 100644 --- a/src/TesApi.Web/BatchScheduler.cs +++ b/src/TesApi.Web/BatchScheduler.cs @@ -87,11 +87,10 @@ public partial class BatchScheduler : IBatchScheduler private readonly IBatchPoolFactory _batchPoolFactory; private readonly string[] taskRunScriptContent; private readonly string[] taskCleanupScriptContent; + private readonly AllowedVmSizesService allowedVmSizesService; private HashSet onlyLogBatchTaskStateOnce = new(); - private static string GetStringValue(IConfiguration configuration, string key, string defaultValue = "") => string.IsNullOrWhiteSpace(configuration[key]) ? defaultValue : configuration[key]; - private IEnumerable allowedVmSizes => GetStringValue(this.configuration, "AllowedVmSizes", null)?.Split(',', StringSplitOptions.RemoveEmptyEntries).ToList(); /// /// Orchestrates s on Azure Batch @@ -111,6 +110,7 @@ public partial class BatchScheduler : IBatchScheduler /// Sku information provider /// Container registry information /// Batch pool factory + /// Service to get allowed vm sizes. public BatchScheduler( ILogger logger, IOptions batchGen1Options, @@ -126,7 +126,8 @@ public BatchScheduler( IBatchQuotaVerifier quotaVerifier, IBatchSkuInformationProvider skuInformationProvider, ContainerRegistryProvider containerRegistryProvider, - IBatchPoolFactory poolFactory) + IBatchPoolFactory poolFactory, + AllowedVmSizesService allowedVmSizesService) { ArgumentNullException.ThrowIfNull(logger); ArgumentNullException.ThrowIfNull(configuration); @@ -161,6 +162,7 @@ public BatchScheduler( this.marthaSecretName = marthaOptions.Value.SecretName; this.globalStartTaskPath = StandardizeStartTaskPath(batchNodesOptions.Value.GlobalStartTask, this.defaultStorageAccountName); this.globalManagedIdentity = batchNodesOptions.Value.GlobalManagedIdentity; + this.allowedVmSizesService = allowedVmSizesService; if (!this.enableBatchAutopool) { @@ -1618,6 +1620,7 @@ private static string RemoveQueryStringsFromLocalFilePaths(string originalString /// The virtual machine info public async Task GetVmSizeAsync(TesTask tesTask, bool forcePreemptibleVmsOnly = false) { + var allowedVmSizes = await allowedVmSizesService.GetAllowedVmSizes(); bool allowedVmSizesFilter(VirtualMachineInformation vm) => allowedVmSizes is null || !allowedVmSizes.Any() || allowedVmSizes.Contains(vm.VmSize, StringComparer.OrdinalIgnoreCase) || allowedVmSizes.Contains(vm.VmFamily, StringComparer.OrdinalIgnoreCase); var tesResources = tesTask.Resources; diff --git a/src/TesApi.Web/ConfigurationUtils.cs b/src/TesApi.Web/ConfigurationUtils.cs index 38e02c21d..0498bda1a 100644 --- a/src/TesApi.Web/ConfigurationUtils.cs +++ b/src/TesApi.Web/ConfigurationUtils.cs @@ -72,7 +72,7 @@ public ConfigurationUtils( /// entries in the allowed-vm-sizes file with a warning. Sets the AllowedVmSizes configuration key. /// /// - public async Task ProcessAllowedVmSizesConfigurationFileAsync() + public async Task> ProcessAllowedVmSizesConfigurationFileAsync() { var supportedVmSizesFilePath = $"/{defaultStorageAccountName}/configuration/supported-vm-sizes"; var allowedVmSizesFilePath = $"/{defaultStorageAccountName}/configuration/allowed-vm-sizes"; @@ -95,7 +95,7 @@ public async Task ProcessAllowedVmSizesConfigurationFileAsync() if (allowedVmSizesFileContent is null) { logger.LogWarning($"Unable to read from {allowedVmSizesFilePath}. All supported VM sizes will be eligible for Azure Batch task scheduling."); - return; + return new List(); } // Read the allowed-vm-sizes configuration file and remove any previous warnings (those start with "<" following the VM size or family name) @@ -141,10 +141,7 @@ public async Task ProcessAllowedVmSizesConfigurationFileAsync() } } - if (allowedAndSupportedVmSizes.Any()) - { - this.configuration["AllowedVmSizes"] = string.Join(',', allowedAndSupportedVmSizes); - } + return allowedAndSupportedVmSizes; } /// diff --git a/src/TesApi.Web/DoOnceAtStartUpService.cs b/src/TesApi.Web/DoOnceAtStartUpService.cs deleted file mode 100644 index 8d02bf1d7..000000000 --- a/src/TesApi.Web/DoOnceAtStartUpService.cs +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Hosting; -using Microsoft.Extensions.Logging; - -namespace TesApi.Web -{ - /// - /// Hosted service that executes one-time set up tasks at start up. - /// - public class DoOnceAtStartUpService : BackgroundService - { - private readonly ILogger logger; - private readonly ConfigurationUtils configUtils; - - /// - /// Hosted service that executes one-time set-up tasks at start up. - /// - /// - /// - public DoOnceAtStartUpService(ConfigurationUtils configUtils, ILogger logger) - { - ArgumentNullException.ThrowIfNull(configUtils); - ArgumentNullException.ThrowIfNull(logger); - - this.configUtils = configUtils; - this.logger = logger; - } - - /// - protected override async Task ExecuteAsync(CancellationToken stoppingToken) - { - using (logger.BeginScope("Executing Start Up tasks")) - { - try - { - logger.LogInformation("Executing Configuration Utils Setup"); - await configUtils.ProcessAllowedVmSizesConfigurationFileAsync(); - } - catch (Exception e) - { - logger.LogError(e, "Failed to execute start up tasks"); - throw; - } - } - } - } -} diff --git a/src/TesApi.Web/Startup.cs b/src/TesApi.Web/Startup.cs index 51c02a0d4..b027ad61c 100644 --- a/src/TesApi.Web/Startup.cs +++ b/src/TesApi.Web/Startup.cs @@ -132,7 +132,7 @@ public void ConfigureServices(IServiceCollection services) }) // Order is important for hosted services - .AddHostedService() + .AddHostedService() .AddHostedService() .AddHostedService() .AddHostedService() From 5cdb324f3744be344bc13708ee16c652ac552ef3 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Fri, 28 Apr 2023 16:26:54 -0700 Subject: [PATCH 03/11] Remove configuration from batchscheduler --- src/TesApi.Web/BatchScheduler.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/TesApi.Web/BatchScheduler.cs b/src/TesApi.Web/BatchScheduler.cs index 4cf69dd51..d2f680f97 100644 --- a/src/TesApi.Web/BatchScheduler.cs +++ b/src/TesApi.Web/BatchScheduler.cs @@ -63,7 +63,6 @@ public partial class BatchScheduler : IBatchScheduler private readonly string dockerInDockerImageName; private readonly string blobxferImageName; private readonly string cromwellDrsLocalizerImageName; - private readonly IConfiguration configuration; private readonly ILogger logger; private readonly IAzureProxy azureProxy; private readonly IStorageAccessProvider storageAccessProvider; @@ -120,7 +119,6 @@ public BatchScheduler( IOptions batchImageNameOptions, IOptions batchNodesOptions, IOptions batchSchedulingOptions, - IConfiguration configuration, IAzureProxy azureProxy, IStorageAccessProvider storageAccessProvider, IBatchQuotaVerifier quotaVerifier, @@ -130,7 +128,6 @@ public BatchScheduler( AllowedVmSizesService allowedVmSizesService) { ArgumentNullException.ThrowIfNull(logger); - ArgumentNullException.ThrowIfNull(configuration); ArgumentNullException.ThrowIfNull(azureProxy); ArgumentNullException.ThrowIfNull(storageAccessProvider); ArgumentNullException.ThrowIfNull(quotaVerifier); @@ -144,7 +141,6 @@ public BatchScheduler( this.quotaVerifier = quotaVerifier; this.skuInformationProvider = skuInformationProvider; this.containerRegistryProvider = containerRegistryProvider; - this.configuration = configuration; this.usePreemptibleVmsOnly = batchSchedulingOptions.Value.UsePreemptibleVmsOnly; this.batchNodesSubnetId = batchNodesOptions.Value.SubnetId; From 91c3d6f8f0cf3368c69406e67346030a22170436 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Fri, 28 Apr 2023 16:27:33 -0700 Subject: [PATCH 04/11] whitespace --- src/TesApi.Web/BatchScheduler.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/TesApi.Web/BatchScheduler.cs b/src/TesApi.Web/BatchScheduler.cs index d2f680f97..4c84b15af 100644 --- a/src/TesApi.Web/BatchScheduler.cs +++ b/src/TesApi.Web/BatchScheduler.cs @@ -90,7 +90,6 @@ public partial class BatchScheduler : IBatchScheduler private HashSet onlyLogBatchTaskStateOnce = new(); - /// /// Orchestrates s on Azure Batch /// From 639859b2b8486458fba4f7af0f06f413d4ab9152 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 11:31:21 -0700 Subject: [PATCH 05/11] Fix DI and run allowedvmsizes task once daily --- src/TesApi.Web/AllowedVmSizesService.cs | 29 ++++++++++++++++++++----- src/TesApi.Web/Startup.cs | 3 ++- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs index bb33ca971..4fd9cb8da 100644 --- a/src/TesApi.Web/AllowedVmSizesService.cs +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -18,7 +18,7 @@ public class AllowedVmSizesService : BackgroundService private readonly ILogger logger; private readonly ConfigurationUtils configUtils; private List allowedVmSizes; - private Task startTask; + private Task firstTask; /// /// Hosted service that executes one-time set-up tasks at start up. @@ -37,7 +37,7 @@ public AllowedVmSizesService(ConfigurationUtils configUtils, ILogger protected override async Task ExecuteAsync(CancellationToken stoppingToken) { - startTask = Task.Run(async () => + var getAllowedVms = async () => { using (logger.BeginScope("Executing Start Up tasks")) { @@ -52,8 +52,23 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) throw; } } - }); - await startTask; + }; + firstTask = Task.Run(getAllowedVms); + await firstTask; + + using PeriodicTimer timer = new(TimeSpan.FromHours(24)); + + try + { + while (await timer.WaitForNextTickAsync(stoppingToken)) + { + await Task.Run(getAllowedVms); + } + } + catch (OperationCanceledException) + { + logger.LogInformation("AllowedVmSizes Service is stopping."); + } } /// @@ -64,7 +79,11 @@ public async Task> GetAllowedVmSizes() { if (allowedVmSizes == null) { - await startTask; + while (firstTask is null) + { + await Task.Delay(TimeSpan.FromSeconds(10)); + } + await firstTask; } return allowedVmSizes; diff --git a/src/TesApi.Web/Startup.cs b/src/TesApi.Web/Startup.cs index b027ad61c..ecb41ad54 100644 --- a/src/TesApi.Web/Startup.cs +++ b/src/TesApi.Web/Startup.cs @@ -104,6 +104,7 @@ public void ConfigureServices(IServiceCollection services) .AddSingleton(CreateBatchQuotaProviderFromConfiguration) .AddSingleton() .AddSingleton() + .AddSingleton() .AddSingleton(s => new DefaultAzureCredential()) .AddSwaggerGen(c => @@ -132,7 +133,7 @@ public void ConfigureServices(IServiceCollection services) }) // Order is important for hosted services - .AddHostedService() + .AddHostedService(sp => (AllowedVmSizesService)sp.GetRequiredService(typeof(AllowedVmSizesService))) .AddHostedService() .AddHostedService() .AddHostedService() From 8a48a3f2536831321ebfcf3dd53859a723fcd86f Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 14:14:52 -0700 Subject: [PATCH 06/11] Fix tests --- src/TesApi.Tests/BatchSchedulerTests.cs | 9 ++++++++- .../TestServices/TestServiceProvider.cs | 11 ++++++++++ src/TesApi.Web/AllowedVmSizesService.cs | 2 +- src/TesApi.Web/BatchScheduler.cs | 4 ++-- src/TesApi.Web/IAllowedVmSizesService.cs | 20 +++++++++++++++++++ src/TesApi.Web/Startup.cs | 1 + 6 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 src/TesApi.Web/IAllowedVmSizesService.cs diff --git a/src/TesApi.Tests/BatchSchedulerTests.cs b/src/TesApi.Tests/BatchSchedulerTests.cs index 560fd2026..8ec4ae456 100644 --- a/src/TesApi.Tests/BatchSchedulerTests.cs +++ b/src/TesApi.Tests/BatchSchedulerTests.cs @@ -1389,6 +1389,12 @@ public async Task PoolIsCreatedWithoutPublicIpWhenSubnetAndDisableBatchNodesPubl return (jobId, cloudTask, poolInformation, batchPoolsModel); } + private static Action> GetMockAllowedVms() + => new(proxy => + proxy.Setup(p => p.GetAllowedVmSizes()) + .ReturnsAsync(new List())); + + private static Action> GetMockSkuInfoProvider(AzureProxyReturnValues azureProxyReturnValues) => new(proxy => proxy.Setup(p => p.GetVmSizesAndPricesAsync(It.IsAny())) @@ -1578,7 +1584,8 @@ private static TestServices.TestServiceProvider GetServiceProvi configuration: GetMockConfig(false)(), azureProxy: GetMockAzureProxy(azureProxyReturn), batchQuotaProvider: GetMockQuotaProvider(azureProxyReturn), - batchSkuInformationProvider: GetMockSkuInfoProvider(azureProxyReturn)); + batchSkuInformationProvider: GetMockSkuInfoProvider(azureProxyReturn), + allowedVmSizesServiceSetup: GetMockAllowedVms()); } private static async Task AddPool(BatchScheduler batchScheduler) diff --git a/src/TesApi.Tests/TestServices/TestServiceProvider.cs b/src/TesApi.Tests/TestServices/TestServiceProvider.cs index c13f216ca..3494c9602 100644 --- a/src/TesApi.Tests/TestServices/TestServiceProvider.cs +++ b/src/TesApi.Tests/TestServices/TestServiceProvider.cs @@ -42,10 +42,13 @@ internal TestServiceProvider( Action> batchQuotaProvider = default, (Func>> expression, Action> action) armBatchQuotaProvider = default, //added so config utils gets the arm implementation, to be removed once config utils is refactored. Action> containerRegistryProviderSetup = default, + Action> allowedVmSizesServiceSetup = default, Action additionalActions = default) { Configuration = GetConfiguration(configuration); provider = new ServiceCollection() + .AddSingleton() + .AddSingleton(_ => GetAllowedVmSizesServiceProviderProvider(allowedVmSizesServiceSetup).Object) .AddSingleton(_ => GetContainerRegisterProvider(containerRegistryProviderSetup).Object) .AddSingleton(Configuration) .AddSingleton(BindHelper(BatchAccountOptions.SectionName)) @@ -100,6 +103,7 @@ internal TestServiceProvider( internal Mock> TesTaskRepository { get; private set; } internal Mock StorageAccessProvider { get; private set; } internal Mock ContainerRegistryProvider { get; private set; } + internal Mock AllowedVmSizesServiceProvider { get; private set; } internal T GetT() => GetT(Array.Empty(), Array.Empty()); @@ -162,6 +166,13 @@ private Mock GetAzureProxy(Action> action) return AzureProxy = proxy; } + private Mock GetAllowedVmSizesServiceProviderProvider(Action> action) + { + var proxy = new Mock(); + action?.Invoke(proxy); + return AllowedVmSizesServiceProvider = proxy; + } + private Mock GetContainerRegisterProvider(Action> action) { var proxy = new Mock(); diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs index 4fd9cb8da..c7d130139 100644 --- a/src/TesApi.Web/AllowedVmSizesService.cs +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -13,7 +13,7 @@ namespace TesApi.Web /// /// Hosted service that executes one-time set up tasks at start up. /// - public class AllowedVmSizesService : BackgroundService + public class AllowedVmSizesService : BackgroundService, IAllowedVmSizesService { private readonly ILogger logger; private readonly ConfigurationUtils configUtils; diff --git a/src/TesApi.Web/BatchScheduler.cs b/src/TesApi.Web/BatchScheduler.cs index 4c84b15af..96303ece5 100644 --- a/src/TesApi.Web/BatchScheduler.cs +++ b/src/TesApi.Web/BatchScheduler.cs @@ -86,7 +86,7 @@ public partial class BatchScheduler : IBatchScheduler private readonly IBatchPoolFactory _batchPoolFactory; private readonly string[] taskRunScriptContent; private readonly string[] taskCleanupScriptContent; - private readonly AllowedVmSizesService allowedVmSizesService; + private readonly IAllowedVmSizesService allowedVmSizesService; private HashSet onlyLogBatchTaskStateOnce = new(); @@ -124,7 +124,7 @@ public BatchScheduler( IBatchSkuInformationProvider skuInformationProvider, ContainerRegistryProvider containerRegistryProvider, IBatchPoolFactory poolFactory, - AllowedVmSizesService allowedVmSizesService) + IAllowedVmSizesService allowedVmSizesService) { ArgumentNullException.ThrowIfNull(logger); ArgumentNullException.ThrowIfNull(azureProxy); diff --git a/src/TesApi.Web/IAllowedVmSizesService.cs b/src/TesApi.Web/IAllowedVmSizesService.cs new file mode 100644 index 000000000..920799e47 --- /dev/null +++ b/src/TesApi.Web/IAllowedVmSizesService.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Generic; +using System.Threading.Tasks; + +namespace TesApi.Web +{ + /// + /// Interface to get allowed vm sizes for TES. + /// + public interface IAllowedVmSizesService + { + /// + /// Gets allowed vm sizes. + /// + /// A list of allowed vm sizes. + Task> GetAllowedVmSizes(); + } +} diff --git a/src/TesApi.Web/Startup.cs b/src/TesApi.Web/Startup.cs index ecb41ad54..3a21250bb 100644 --- a/src/TesApi.Web/Startup.cs +++ b/src/TesApi.Web/Startup.cs @@ -105,6 +105,7 @@ public void ConfigureServices(IServiceCollection services) .AddSingleton() .AddSingleton() .AddSingleton() + .AddSingleton(sp => (AllowedVmSizesService)sp.GetRequiredService(typeof(AllowedVmSizesService))) .AddSingleton(s => new DefaultAzureCredential()) .AddSwaggerGen(c => From fdc0db055d9ff904aba83a56e24f0302af06a060 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 15:08:44 -0700 Subject: [PATCH 07/11] Fix tests --- src/TesApi.Tests/BatchSchedulerTests.cs | 34 +++++++++++++++++-------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/TesApi.Tests/BatchSchedulerTests.cs b/src/TesApi.Tests/BatchSchedulerTests.cs index 8ec4ae456..b234f1549 100644 --- a/src/TesApi.Tests/BatchSchedulerTests.cs +++ b/src/TesApi.Tests/BatchSchedulerTests.cs @@ -247,12 +247,14 @@ public async Task TestIfVmSizeIsAvailable(string vmSize, bool preemptible) task.Resources.Preemptible = preemptible; task.Resources.BackendParameters = new() { { "vm_size", vmSize } }; + var config = GetMockConfig(false)(); using var serviceProvider = GetServiceProvider( - GetMockConfig(false)(), + config, GetMockAzureProxy(AzureProxyReturnValues.Defaults), GetMockQuotaProvider(AzureProxyReturnValues.Defaults), GetMockSkuInfoProvider(AzureProxyReturnValues.Defaults), - GetContainerRegistryInfoProvider(AzureProxyReturnValues.Defaults)); + GetContainerRegistryInfoProvider(AzureProxyReturnValues.Defaults), + GetMockAllowedVms(config)); var batchScheduler = serviceProvider.GetT(); var size = await ((BatchScheduler)batchScheduler).GetVmSizeAsync(task); @@ -620,12 +622,14 @@ void Validator(TesTask _1, IEnumerable<(LogLevel logLevel, Exception exception)> public async Task BatchJobContainsExpectedBatchPoolInformation() { var tesTask = GetTesTask(); + var config = GetMockConfig(false)(); using var serviceProvider = GetServiceProvider( - GetMockConfig(false)(), + config, GetMockAzureProxy(AzureProxyReturnValues.Defaults), GetMockQuotaProvider(AzureProxyReturnValues.Defaults), GetMockSkuInfoProvider(AzureProxyReturnValues.Defaults), - GetContainerRegistryInfoProvider(AzureProxyReturnValues.Defaults)); + GetContainerRegistryInfoProvider(AzureProxyReturnValues.Defaults), + GetMockAllowedVms(config)); var batchScheduler = serviceProvider.GetT(); await batchScheduler.ProcessTesTaskAsync(tesTask); @@ -1372,6 +1376,7 @@ public async Task PoolIsCreatedWithoutPublicIpWhenSubnetAndDisableBatchNodesPubl GetMockQuotaProvider(azureProxyReturnValues), GetMockSkuInfoProvider(azureProxyReturnValues), GetContainerRegistryInfoProvider(azureProxyReturnValues), + GetMockAllowedVms(configuration), additionalActions: additionalActions); var batchScheduler = serviceProvider.GetT(); @@ -1389,10 +1394,18 @@ public async Task PoolIsCreatedWithoutPublicIpWhenSubnetAndDisableBatchNodesPubl return (jobId, cloudTask, poolInformation, batchPoolsModel); } - private static Action> GetMockAllowedVms() + private static Action> GetMockAllowedVms(IEnumerable<(string Key, string Value)> configuration) => new(proxy => + { + var allowedVmsConfig = configuration.FirstOrDefault(x => x.Key == "AllowedVmSizes").Value; + var allowedVms = new List(); + if (!string.IsNullOrWhiteSpace(allowedVmsConfig)) + { + allowedVms = allowedVmsConfig.Split(",").ToList(); + } proxy.Setup(p => p.GetAllowedVmSizes()) - .ReturnsAsync(new List())); + .ReturnsAsync(allowedVms); + }); private static Action> GetMockSkuInfoProvider(AzureProxyReturnValues azureProxyReturnValues) @@ -1437,8 +1450,8 @@ private static Action> GetMockQuotaProvider(AzureProxy new(batchQuotas.ActiveJobAndJobScheduleQuota, batchQuotas.PoolQuota, batchQuotas.DedicatedCoreQuota, batchQuotas.LowPriorityCoreQuota))); }); - private static TestServices.TestServiceProvider GetServiceProvider(IEnumerable<(string Key, string Value)> configuration, Action> azureProxy, Action> quotaProvider, Action> skuInfoProvider, Action> containerRegistryProviderSetup, Action additionalActions = default) - => new(wrapAzureProxy: true, configuration: configuration, azureProxy: azureProxy, batchQuotaProvider: quotaProvider, batchSkuInformationProvider: skuInfoProvider, accountResourceInformation: GetNewBatchResourceInfo(), containerRegistryProviderSetup: containerRegistryProviderSetup, additionalActions: additionalActions); + private static TestServices.TestServiceProvider GetServiceProvider(IEnumerable<(string Key, string Value)> configuration, Action> azureProxy, Action> quotaProvider, Action> skuInfoProvider, Action> containerRegistryProviderSetup, Action> allowedVmSizesServiceSetup, Action additionalActions = default) + => new(wrapAzureProxy: true, configuration: configuration, azureProxy: azureProxy, batchQuotaProvider: quotaProvider, batchSkuInformationProvider: skuInfoProvider, accountResourceInformation: GetNewBatchResourceInfo(), containerRegistryProviderSetup: containerRegistryProviderSetup, allowedVmSizesServiceSetup: allowedVmSizesServiceSetup, additionalActions: additionalActions); private static async Task GetNewTesTaskStateAsync(TesTask tesTask, AzureProxyReturnValues azureProxyReturnValues) { @@ -1578,14 +1591,15 @@ private static IEnumerable GetFilesToDownload(Mock private static TestServices.TestServiceProvider GetServiceProvider(AzureProxyReturnValues azureProxyReturn = default) { azureProxyReturn ??= AzureProxyReturnValues.Defaults; + var config = GetMockConfig(false)(); return new( wrapAzureProxy: true, accountResourceInformation: new("defaultbatchaccount", "defaultresourcegroup", "defaultsubscription", "defaultregion"), - configuration: GetMockConfig(false)(), + configuration: config, azureProxy: GetMockAzureProxy(azureProxyReturn), batchQuotaProvider: GetMockQuotaProvider(azureProxyReturn), batchSkuInformationProvider: GetMockSkuInfoProvider(azureProxyReturn), - allowedVmSizesServiceSetup: GetMockAllowedVms()); + allowedVmSizesServiceSetup: GetMockAllowedVms(config)); } private static async Task AddPool(BatchScheduler batchScheduler) From 57e5e5ccd79fd08e62926d968ae13badca7f3e24 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 15:28:45 -0700 Subject: [PATCH 08/11] Code review --- src/TesApi.Web/AllowedVmSizesService.cs | 40 +++++++++++++------------ src/TesApi.Web/Startup.cs | 5 ++-- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs index c7d130139..7dca53d47 100644 --- a/src/TesApi.Web/AllowedVmSizesService.cs +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -15,6 +15,7 @@ namespace TesApi.Web /// public class AllowedVmSizesService : BackgroundService, IAllowedVmSizesService { + private readonly TimeSpan refreshInterval = TimeSpan.FromHours(24); private readonly ILogger logger; private readonly ConfigurationUtils configUtils; private List allowedVmSizes; @@ -34,35 +35,36 @@ public AllowedVmSizesService(ConfigurationUtils configUtils, ILogger - protected override async Task ExecuteAsync(CancellationToken stoppingToken) + private async Task GetAllowedVmSizesImpl() { - var getAllowedVms = async () => + using (logger.BeginScope("Executing Start Up tasks")) { - using (logger.BeginScope("Executing Start Up tasks")) + try { - try - { - logger.LogInformation("Executing Configuration Utils Setup"); - allowedVmSizes = await configUtils.ProcessAllowedVmSizesConfigurationFileAsync(); - } - catch (Exception e) - { - logger.LogError(e, "Failed to execute start up tasks"); - throw; - } + logger.LogInformation("Executing Configuration Utils Setup"); + allowedVmSizes = await configUtils.ProcessAllowedVmSizesConfigurationFileAsync(); } - }; - firstTask = Task.Run(getAllowedVms); + catch (Exception e) + { + logger.LogError(e, "Failed to execute start up tasks"); + throw; + } + } + } + + /// + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + firstTask = GetAllowedVmSizesImpl(); await firstTask; - using PeriodicTimer timer = new(TimeSpan.FromHours(24)); + using PeriodicTimer timer = new(refreshInterval); try { while (await timer.WaitForNextTickAsync(stoppingToken)) { - await Task.Run(getAllowedVms); + await GetAllowedVmSizesImpl(); } } catch (OperationCanceledException) @@ -81,7 +83,7 @@ public async Task> GetAllowedVmSizes() { while (firstTask is null) { - await Task.Delay(TimeSpan.FromSeconds(10)); + await Task.Delay(TimeSpan.FromSeconds(1)); } await firstTask; } diff --git a/src/TesApi.Web/Startup.cs b/src/TesApi.Web/Startup.cs index 3a21250bb..4fc732a40 100644 --- a/src/TesApi.Web/Startup.cs +++ b/src/TesApi.Web/Startup.cs @@ -104,8 +104,7 @@ public void ConfigureServices(IServiceCollection services) .AddSingleton(CreateBatchQuotaProviderFromConfiguration) .AddSingleton() .AddSingleton() - .AddSingleton() - .AddSingleton(sp => (AllowedVmSizesService)sp.GetRequiredService(typeof(AllowedVmSizesService))) + .AddSingleton() .AddSingleton(s => new DefaultAzureCredential()) .AddSwaggerGen(c => @@ -134,7 +133,7 @@ public void ConfigureServices(IServiceCollection services) }) // Order is important for hosted services - .AddHostedService(sp => (AllowedVmSizesService)sp.GetRequiredService(typeof(AllowedVmSizesService))) + .AddHostedService(sp => (AllowedVmSizesService)sp.GetRequiredService(typeof(IAllowedVmSizesService))) .AddHostedService() .AddHostedService() .AddHostedService() From e50e2686be1117cb3e99f8b7b88ac6e5471f1ebc Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 15:39:55 -0700 Subject: [PATCH 09/11] Update logging --- src/TesApi.Web/AllowedVmSizesService.cs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs index 7dca53d47..8cb693d2f 100644 --- a/src/TesApi.Web/AllowedVmSizesService.cs +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -37,18 +37,15 @@ public AllowedVmSizesService(ConfigurationUtils configUtils, ILogger Date: Mon, 1 May 2023 15:43:05 -0700 Subject: [PATCH 10/11] Fix whitespace? --- src/TesApi.Tests/BatchSchedulerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TesApi.Tests/BatchSchedulerTests.cs b/src/TesApi.Tests/BatchSchedulerTests.cs index b234f1549..0ba80b85b 100644 --- a/src/TesApi.Tests/BatchSchedulerTests.cs +++ b/src/TesApi.Tests/BatchSchedulerTests.cs @@ -1406,7 +1406,7 @@ private static Action> GetMockAllowedVms(IEnumerabl proxy.Setup(p => p.GetAllowedVmSizes()) .ReturnsAsync(allowedVms); }); - + private static Action> GetMockSkuInfoProvider(AzureProxyReturnValues azureProxyReturnValues) => new(proxy => From 4e7e1ccbf08df42a4ec41fb8785d352159a3da08 Mon Sep 17 00:00:00 2001 From: Jonathon Saunders Date: Mon, 1 May 2023 17:21:04 -0700 Subject: [PATCH 11/11] Update comments --- src/TesApi.Web/AllowedVmSizesService.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/TesApi.Web/AllowedVmSizesService.cs b/src/TesApi.Web/AllowedVmSizesService.cs index 8cb693d2f..28cc2b792 100644 --- a/src/TesApi.Web/AllowedVmSizesService.cs +++ b/src/TesApi.Web/AllowedVmSizesService.cs @@ -11,7 +11,7 @@ namespace TesApi.Web { /// - /// Hosted service that executes one-time set up tasks at start up. + /// Service that periodically fetches the allowed vms list from storage, and updates the supported vms list. /// public class AllowedVmSizesService : BackgroundService, IAllowedVmSizesService { @@ -22,7 +22,7 @@ public class AllowedVmSizesService : BackgroundService, IAllowedVmSizesService private Task firstTask; /// - /// Hosted service that executes one-time set-up tasks at start up. + /// Service that periodically fetches the allowed vms list from storage, and updates the supported vms list. /// /// /// @@ -73,7 +73,7 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) /// /// Awaits start up and then return allowed vm sizes. /// - /// + /// List of allowed vms. public async Task> GetAllowedVmSizes() { if (allowedVmSizes == null)