From ce4b7f4dd618ac2b6a4f6a5552885ef1dffc5d3b Mon Sep 17 00:00:00 2001 From: Lokesh Gopu Date: Mon, 12 May 2025 16:54:43 -0400 Subject: [PATCH] Prefer _migrated config on startup (#3853) --- src/Runner.Common/ConfigurationStore.cs | 9 ++ src/Runner.Common/Constants.cs | 4 + src/Runner.Listener/BrokerMessageListener.cs | 45 ++++++- .../Configuration/ConfigurationManager.cs | 17 +++ src/Runner.Listener/Runner.cs | 120 ++++++++++++++++-- .../L0/Listener/BrokerMessageListenerL0.cs | 43 +++++++ 6 files changed, 225 insertions(+), 13 deletions(-) diff --git a/src/Runner.Common/ConfigurationStore.cs b/src/Runner.Common/ConfigurationStore.cs index 576360c6f..8d47f96c0 100644 --- a/src/Runner.Common/ConfigurationStore.cs +++ b/src/Runner.Common/ConfigurationStore.cs @@ -116,6 +116,7 @@ namespace GitHub.Runner.Common bool IsConfigured(); bool IsServiceConfigured(); bool HasCredentials(); + bool IsMigratedConfigured(); CredentialData GetCredentials(); CredentialData GetMigratedCredentials(); RunnerSettings GetSettings(); @@ -198,6 +199,14 @@ namespace GitHub.Runner.Common return serviceConfigured; } + public bool IsMigratedConfigured() + { + Trace.Info("IsMigratedConfigured()"); + bool configured = new FileInfo(_migratedConfigFilePath).Exists; + Trace.Info("IsMigratedConfigured: {0}", configured); + return configured; + } + public CredentialData GetCredentials() { if (_creds == null) diff --git a/src/Runner.Common/Constants.cs b/src/Runner.Common/Constants.cs index 24166646c..515031424 100644 --- a/src/Runner.Common/Constants.cs +++ b/src/Runner.Common/Constants.cs @@ -155,6 +155,10 @@ namespace GitHub.Runner.Common public const int RunnerUpdating = 3; public const int RunOnceRunnerUpdating = 4; public const int SessionConflict = 5; + // Temporary error code to indicate that the runner configuration has been refreshed + // and the runner should be restarted. This is a temporary code and will be removed in the future after + // the runner is migrated to runner admin. + public const int RunnerConfigurationRefreshed = 6; } public static class Features diff --git a/src/Runner.Listener/BrokerMessageListener.cs b/src/Runner.Listener/BrokerMessageListener.cs index 447439863..a25670c98 100644 --- a/src/Runner.Listener/BrokerMessageListener.cs +++ b/src/Runner.Listener/BrokerMessageListener.cs @@ -38,6 +38,19 @@ namespace GitHub.Runner.Listener private readonly TimeSpan _clockSkewRetryLimit = TimeSpan.FromMinutes(30); private bool _needRefreshCredsV2 = false; private bool _handlerInitialized = false; + private bool _isMigratedSettings = false; + private const int _maxMigratedSettingsRetries = 3; + private int _migratedSettingsRetryCount = 0; + + public BrokerMessageListener() + { + } + + public BrokerMessageListener(RunnerSettings settings, bool isMigratedSettings = false) + { + _settings = settings; + _isMigratedSettings = isMigratedSettings; + } public override void Initialize(IHostContext hostContext) { @@ -53,9 +66,22 @@ namespace GitHub.Runner.Listener { Trace.Entering(); - // Settings - var configManager = HostContext.GetService(); - _settings = configManager.LoadSettings(); + // Load settings if not provided through constructor + if (_settings == null) + { + var configManager = HostContext.GetService(); + _settings = configManager.LoadSettings(); + Trace.Info("Settings loaded from config manager"); + } + else + { + Trace.Info("Using provided settings"); + if (_isMigratedSettings) + { + Trace.Info("Using migrated settings from .runner_migrated"); + } + } + var serverUrlV2 = _settings.ServerUrlV2; var serverUrl = _settings.ServerUrl; Trace.Info(_settings); @@ -141,6 +167,19 @@ namespace GitHub.Runner.Listener Trace.Error("Catch exception during create session."); Trace.Error(ex); + // If using migrated settings, limit the number of retries before returning failure + if (_isMigratedSettings) + { + _migratedSettingsRetryCount++; + Trace.Warning($"Migrated settings retry {_migratedSettingsRetryCount} of {_maxMigratedSettingsRetries}"); + + if (_migratedSettingsRetryCount >= _maxMigratedSettingsRetries) + { + Trace.Warning("Reached maximum retry attempts for migrated settings. Returning failure to try default settings."); + return CreateSessionResult.Failure; + } + } + if (!HostContext.AllowAuthMigration && ex is VssOAuthTokenRequestException vssOAuthEx && _credsV2.Federated is VssOAuthCredential vssOAuthCred) diff --git a/src/Runner.Listener/Configuration/ConfigurationManager.cs b/src/Runner.Listener/Configuration/ConfigurationManager.cs index a5f6c6deb..15a576317 100644 --- a/src/Runner.Listener/Configuration/ConfigurationManager.cs +++ b/src/Runner.Listener/Configuration/ConfigurationManager.cs @@ -25,6 +25,7 @@ namespace GitHub.Runner.Listener.Configuration Task UnconfigureAsync(CommandSettings command); void DeleteLocalRunnerConfig(); RunnerSettings LoadSettings(); + RunnerSettings LoadMigratedSettings(); } public sealed class ConfigurationManager : RunnerService, IConfigurationManager @@ -66,6 +67,22 @@ namespace GitHub.Runner.Listener.Configuration return settings; } + public RunnerSettings LoadMigratedSettings() + { + Trace.Info(nameof(LoadMigratedSettings)); + + // Check if migrated settings file exists + if (!_store.IsMigratedConfigured()) + { + throw new NonRetryableException("No migrated configuration found."); + } + + RunnerSettings settings = _store.GetMigratedSettings(); + Trace.Info("Migrated Settings Loaded"); + + return settings; + } + public async Task ConfigureAsync(CommandSettings command) { _term.WriteLine(); diff --git a/src/Runner.Listener/Runner.cs b/src/Runner.Listener/Runner.cs index 0935afca4..569d5505c 100644 --- a/src/Runner.Listener/Runner.cs +++ b/src/Runner.Listener/Runner.cs @@ -325,7 +325,7 @@ namespace GitHub.Runner.Listener } // Run the runner interactively or as service - return await RunAsync(settings, command.RunOnce || settings.Ephemeral); + return await ExecuteRunnerAsync(settings, command.RunOnce || settings.Ephemeral); } else { @@ -387,12 +387,12 @@ namespace GitHub.Runner.Listener } } - private IMessageListener GetMessageListener(RunnerSettings settings) + private IMessageListener GetMessageListener(RunnerSettings settings, bool isMigratedSettings = false) { if (settings.UseV2Flow) { Trace.Info($"Using BrokerMessageListener"); - var brokerListener = new BrokerMessageListener(); + var brokerListener = new BrokerMessageListener(settings, isMigratedSettings); brokerListener.Initialize(HostContext); return brokerListener; } @@ -406,15 +406,65 @@ namespace GitHub.Runner.Listener try { Trace.Info(nameof(RunAsync)); - _listener = GetMessageListener(settings); - CreateSessionResult createSessionResult = await _listener.CreateSessionAsync(HostContext.RunnerShutdownToken); - if (createSessionResult == CreateSessionResult.SessionConflict) + + // First try using migrated settings if available + var configManager = HostContext.GetService(); + RunnerSettings migratedSettings = null; + + try { - return Constants.Runner.ReturnCode.SessionConflict; + migratedSettings = configManager.LoadMigratedSettings(); + Trace.Info("Loaded migrated settings from .runner_migrated file"); + Trace.Info(migratedSettings); } - else if (createSessionResult == CreateSessionResult.Failure) + catch (Exception ex) { - return Constants.Runner.ReturnCode.TerminatedError; + // If migrated settings file doesn't exist or can't be loaded, we'll use the provided settings + Trace.Info($"Failed to load migrated settings: {ex.Message}"); + } + + bool usedMigratedSettings = false; + + if (migratedSettings != null) + { + // Try to create session with migrated settings first + Trace.Info("Attempting to create session using migrated settings"); + _listener = GetMessageListener(migratedSettings, isMigratedSettings: true); + + try + { + CreateSessionResult createSessionResult = await _listener.CreateSessionAsync(HostContext.RunnerShutdownToken); + if (createSessionResult == CreateSessionResult.Success) + { + Trace.Info("Successfully created session with migrated settings"); + settings = migratedSettings; // Use migrated settings for the rest of the process + usedMigratedSettings = true; + } + else + { + Trace.Warning($"Failed to create session with migrated settings: {createSessionResult}"); + } + } + catch (Exception ex) + { + Trace.Error($"Exception when creating session with migrated settings: {ex}"); + } + } + + // If migrated settings weren't used or session creation failed, use original settings + if (!usedMigratedSettings) + { + Trace.Info("Falling back to original .runner settings"); + _listener = GetMessageListener(settings); + CreateSessionResult createSessionResult = await _listener.CreateSessionAsync(HostContext.RunnerShutdownToken); + if (createSessionResult == CreateSessionResult.SessionConflict) + { + return Constants.Runner.ReturnCode.SessionConflict; + } + else if (createSessionResult == CreateSessionResult.Failure) + { + return Constants.Runner.ReturnCode.TerminatedError; + } } HostContext.WritePerfCounter("SessionCreated"); @@ -428,6 +478,8 @@ namespace GitHub.Runner.Listener // Should we try to cleanup ephemeral runners bool runOnceJobCompleted = false; bool skipSessionDeletion = false; + bool restartSession = false; // Flag to indicate session restart + bool restartSessionPending = false; try { var notification = HostContext.GetService(); @@ -443,6 +495,15 @@ namespace GitHub.Runner.Listener while (!HostContext.RunnerShutdownToken.IsCancellationRequested) { + // Check if we need to restart the session and can do so (job dispatcher not busy) + if (restartSessionPending && !jobDispatcher.Busy) + { + Trace.Info("Pending session restart detected and job dispatcher is not busy. Restarting session now."); + messageQueueLoopTokenSource.Cancel(); + restartSession = true; + break; + } + TaskAgentMessage message = null; bool skipMessageDeletion = false; try @@ -679,6 +740,17 @@ namespace GitHub.Runner.Listener configType: runnerRefreshConfigMessage.ConfigType, serviceType: runnerRefreshConfigMessage.ServiceType, configRefreshUrl: runnerRefreshConfigMessage.ConfigRefreshUrl); + + // Set flag to schedule session restart if ConfigType is "runner" + if (string.Equals(runnerRefreshConfigMessage.ConfigType, "runner", StringComparison.OrdinalIgnoreCase)) + { + Trace.Info("Runner configuration was updated. Session restart has been scheduled"); + restartSessionPending = true; + } + else + { + Trace.Info($"No session restart needed for config type: {runnerRefreshConfigMessage.ConfigType}"); + } } else { @@ -733,10 +805,16 @@ namespace GitHub.Runner.Listener if (settings.Ephemeral && runOnceJobCompleted) { - var configManager = HostContext.GetService(); configManager.DeleteLocalRunnerConfig(); } } + + // After cleanup, check if we need to restart the session + if (restartSession) + { + Trace.Info("Restarting runner session after config update..."); + return Constants.Runner.ReturnCode.RunnerConfigurationRefreshed; + } } catch (TaskAgentAccessTokenExpiredException) { @@ -750,6 +828,28 @@ namespace GitHub.Runner.Listener return Constants.Runner.ReturnCode.Success; } + private async Task ExecuteRunnerAsync(RunnerSettings settings, bool runOnce) + { + int returnCode = Constants.Runner.ReturnCode.Success; + bool restart = false; + do + { + restart = false; + returnCode = await RunAsync(settings, runOnce); + + if (returnCode == Constants.Runner.ReturnCode.RunnerConfigurationRefreshed) + { + Trace.Info("Runner configuration was refreshed, restarting session..."); + // Reload settings in case they changed + var configManager = HostContext.GetService(); + settings = configManager.LoadSettings(); + restart = true; + } + } while (restart); + + return returnCode; + } + private void HandleAuthMigrationChanged(object sender, AuthMigrationEventArgs e) { Trace.Verbose("Handle AuthMigrationChanged in Runner"); diff --git a/src/Test/L0/Listener/BrokerMessageListenerL0.cs b/src/Test/L0/Listener/BrokerMessageListenerL0.cs index f1d2d65d4..c42d134dd 100644 --- a/src/Test/L0/Listener/BrokerMessageListenerL0.cs +++ b/src/Test/L0/Listener/BrokerMessageListenerL0.cs @@ -363,6 +363,49 @@ namespace GitHub.Runner.Common.Tests.Listener } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Runner")] + public async Task CreatesSessionWithProvidedSettings() + { + using (TestHostContext tc = CreateTestContext()) + using (var tokenSource = new CancellationTokenSource()) + { + Tracing trace = tc.GetTrace(); + + // Arrange. + var expectedSession = new TaskAgentSession(); + _brokerServer + .Setup(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token)) + .Returns(Task.FromResult(expectedSession)); + + _credMgr.Setup(x => x.LoadCredentials(true)).Returns(new VssCredentials()); + + // Make sure the config is never called when settings are provided + _config.Setup(x => x.LoadSettings()).Throws(new InvalidOperationException("Should not be called")); + + // Act. + // Use the constructor that accepts settings + BrokerMessageListener listener = new(_settings); + listener.Initialize(tc); + + CreateSessionResult result = await listener.CreateSessionAsync(tokenSource.Token); + trace.Info("result: {0}", result); + + // Assert. + Assert.Equal(CreateSessionResult.Success, result); + _brokerServer + .Verify(x => x.CreateSessionAsync( + It.Is(y => y != null), + tokenSource.Token), Times.Once()); + + // Verify LoadSettings was never called + _config.Verify(x => x.LoadSettings(), Times.Never()); + } + } + private TestHostContext CreateTestContext([CallerMemberName] String testName = "") { TestHostContext tc = new(this, testName);