From d771da71b9bbf1b05b8c39671eeeb6f6c6431cac Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 23 Apr 2023 02:34:11 -0400 Subject: [PATCH 1/3] Add `General:DeploymentDirectoryCopyTasksPerCore` config option --- .../Components/InstanceFactory.cs | 14 ++- .../Components/Repository/Repository.cs | 14 ++- .../Repository/RepositoryManager.cs | 12 +- .../Components/StaticFiles/Configuration.cs | 19 +++- .../Configuration/GeneralConfiguration.cs | 25 +++++ .../IO/DefaultIOManager.cs | 16 ++- src/Tgstation.Server.Host/IO/IIOManager.cs | 10 +- src/Tgstation.Server.Host/appsettings.yml | 1 + .../IO/TestIOManager.cs | 106 ++++++++++++++++++ .../Live/Instance/ConfigurationTest.cs | 6 +- .../Tgstation.Server.Tests/TestRepository.cs | 2 + 11 files changed, 207 insertions(+), 18 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/InstanceFactory.cs b/src/Tgstation.Server.Host/Components/InstanceFactory.cs index c9b891f087..6f68acec65 100644 --- a/src/Tgstation.Server.Host/Components/InstanceFactory.cs +++ b/src/Tgstation.Server.Host/Components/InstanceFactory.cs @@ -139,6 +139,11 @@ namespace Tgstation.Server.Host.Components /// readonly IRemoteDeploymentManagerFactory remoteDeploymentManagerFactory; + /// + /// The for the . + /// + readonly GeneralConfiguration generalConfiguration; + /// /// The for the . /// @@ -177,6 +182,7 @@ namespace Tgstation.Server.Host.Components /// The value of . /// The value of . /// The value of . + /// The containing the value of . /// The containing the value of . public InstanceFactory( IIOManager ioManager, @@ -201,6 +207,7 @@ namespace Tgstation.Server.Host.Components IFileTransferTicketProvider fileTransferService, IGitRemoteFeaturesFactory gitRemoteFeaturesFactory, IRemoteDeploymentManagerFactory remoteDeploymentManagerFactory, + IOptions generalConfigurationOptions, IOptions sessionConfigurationOptions) { this.ioManager = ioManager ?? throw new ArgumentNullException(nameof(ioManager)); @@ -225,6 +232,7 @@ namespace Tgstation.Server.Host.Components this.fileTransferService = fileTransferService ?? throw new ArgumentNullException(nameof(fileTransferService)); this.gitRemoteFeaturesFactory = gitRemoteFeaturesFactory ?? throw new ArgumentNullException(nameof(gitRemoteFeaturesFactory)); this.remoteDeploymentManagerFactory = remoteDeploymentManagerFactory ?? throw new ArgumentNullException(nameof(remoteDeploymentManagerFactory)); + generalConfiguration = generalConfigurationOptions?.Value ?? throw new ArgumentNullException(nameof(generalConfigurationOptions)); sessionConfiguration = sessionConfigurationOptions?.Value ?? throw new ArgumentNullException(nameof(sessionConfigurationOptions)); } #pragma warning restore CA1502 @@ -266,7 +274,8 @@ namespace Tgstation.Server.Host.Components postWriteHandler, platformIdentifier, fileTransferService, - loggerFactory.CreateLogger()); + loggerFactory.CreateLogger(), + generalConfiguration); var eventConsumer = new EventConsumer(configuration); var repoManager = new RepositoryManager( repositoryFactory, @@ -276,7 +285,8 @@ namespace Tgstation.Server.Host.Components postWriteHandler, gitRemoteFeaturesFactory, loggerFactory.CreateLogger(), - loggerFactory.CreateLogger()); + loggerFactory.CreateLogger(), + generalConfiguration); try { var byond = new ByondManager(byondIOManager, byondInstaller, eventConsumer, loggerFactory.CreateLogger()); diff --git a/src/Tgstation.Server.Host/Components/Repository/Repository.cs b/src/Tgstation.Server.Host/Components/Repository/Repository.cs index 8bdbadc48e..62e41c67c2 100644 --- a/src/Tgstation.Server.Host/Components/Repository/Repository.cs +++ b/src/Tgstation.Server.Host/Components/Repository/Repository.cs @@ -12,6 +12,7 @@ using Microsoft.Extensions.Logging; using Tgstation.Server.Api.Models; using Tgstation.Server.Api.Models.Internal; using Tgstation.Server.Host.Components.Events; +using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; @@ -106,6 +107,11 @@ namespace Tgstation.Server.Host.Components.Repository /// readonly ILogger logger; + /// + /// The for the . + /// + readonly GeneralConfiguration generalConfiguration; + /// /// to be taken when is called. /// @@ -127,6 +133,7 @@ namespace Tgstation.Server.Host.Components.Repository /// The value of . /// The to provide the value of . /// The value of . + /// The value of . /// The value if . public Repository( LibGit2Sharp.IRepository libGitRepo, @@ -137,6 +144,7 @@ namespace Tgstation.Server.Host.Components.Repository IPostWriteHandler postWriteHandler, IGitRemoteFeaturesFactory gitRemoteFeaturesFactory, ILogger logger, + GeneralConfiguration generalConfiguration, Action onDispose) { this.libGitRepo = libGitRepo ?? throw new ArgumentNullException(nameof(libGitRepo)); @@ -149,6 +157,7 @@ namespace Tgstation.Server.Host.Components.Repository throw new ArgumentNullException(nameof(gitRemoteFeaturesFactory)); this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); + this.generalConfiguration = generalConfiguration ?? throw new ArgumentNullException(nameof(generalConfiguration)); this.onDispose = onDispose ?? throw new ArgumentNullException(nameof(onDispose)); gitRemoteFeatures = gitRemoteFeaturesFactory.CreateGitRemoteFeatures(this); @@ -524,8 +533,6 @@ namespace Tgstation.Server.Host.Components.Repository throw new ArgumentNullException(nameof(path)); logger.LogTrace("Copying to {0}...", path); await ioMananger.CopyDirectory( - ioMananger.ResolvePath(), - path, new List { ".git" }, (src, dest) => { @@ -534,6 +541,9 @@ namespace Tgstation.Server.Host.Components.Repository return Task.CompletedTask; }, + ioMananger.ResolvePath(), + path, + generalConfiguration.GetCopyDirectoryTaskThrottle(), cancellationToken); } diff --git a/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs b/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs index ba78fb23c5..237053a5d0 100644 --- a/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs +++ b/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Logging; using Tgstation.Server.Api.Models; using Tgstation.Server.Host.Components.Events; +using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.Core; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; @@ -62,6 +63,11 @@ namespace Tgstation.Server.Host.Components.Repository /// readonly ILogger logger; + /// + /// The for the . + /// + readonly GeneralConfiguration generalConfiguration; + /// /// Used for controlling single access to the . /// @@ -78,6 +84,7 @@ namespace Tgstation.Server.Host.Components.Repository /// The value of . /// The value of . /// The value of . + /// The value of . public RepositoryManager( ILibGit2RepositoryFactory repositoryFactory, ILibGit2Commands commands, @@ -86,7 +93,8 @@ namespace Tgstation.Server.Host.Components.Repository IPostWriteHandler postWriteHandler, IGitRemoteFeaturesFactory gitRemoteFeaturesFactory, ILogger repositoryLogger, - ILogger logger) + ILogger logger, + GeneralConfiguration generalConfiguration) { this.repositoryFactory = repositoryFactory ?? throw new ArgumentNullException(nameof(repositoryFactory)); this.commands = commands ?? throw new ArgumentNullException(nameof(commands)); @@ -96,6 +104,7 @@ namespace Tgstation.Server.Host.Components.Repository this.gitRemoteFeaturesFactory = gitRemoteFeaturesFactory ?? throw new ArgumentNullException(nameof(gitRemoteFeaturesFactory)); this.repositoryLogger = repositoryLogger ?? throw new ArgumentNullException(nameof(repositoryLogger)); this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); + this.generalConfiguration = generalConfiguration ?? throw new ArgumentNullException(nameof(generalConfiguration)); semaphore = new SemaphoreSlim(1); } @@ -217,6 +226,7 @@ namespace Tgstation.Server.Host.Components.Repository postWriteHandler, gitRemoteFeaturesFactory, repositoryLogger, + generalConfiguration, () => { logger.LogTrace("Releasing semaphore due to Repository disposal..."); diff --git a/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs b/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs index 9ed5959137..2f8002e354 100644 --- a/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs +++ b/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs @@ -13,6 +13,7 @@ using Microsoft.Extensions.Logging; using Tgstation.Server.Api.Models; using Tgstation.Server.Api.Models.Response; using Tgstation.Server.Host.Components.Events; +using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.Core; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; @@ -111,6 +112,11 @@ namespace Tgstation.Server.Host.Components.StaticFiles /// readonly ILogger logger; + /// + /// The for . + /// + readonly GeneralConfiguration generalConfiguration; + /// /// The for . Also used as a . /// @@ -137,6 +143,7 @@ namespace Tgstation.Server.Host.Components.StaticFiles /// The value of . /// The value of . /// The value of . + /// The value of . public Configuration( IIOManager ioManager, ISynchronousIOManager synchronousIOManager, @@ -145,7 +152,8 @@ namespace Tgstation.Server.Host.Components.StaticFiles IPostWriteHandler postWriteHandler, IPlatformIdentifier platformIdentifier, IFileTransferTicketProvider fileTransferService, - ILogger logger) + ILogger logger, + GeneralConfiguration generalConfiguration) { this.ioManager = ioManager ?? throw new ArgumentNullException(nameof(ioManager)); this.synchronousIOManager = synchronousIOManager ?? throw new ArgumentNullException(nameof(synchronousIOManager)); @@ -155,6 +163,7 @@ namespace Tgstation.Server.Host.Components.StaticFiles this.platformIdentifier = platformIdentifier ?? throw new ArgumentNullException(nameof(platformIdentifier)); this.fileTransferService = fileTransferService ?? throw new ArgumentNullException(nameof(fileTransferService)); this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); + this.generalConfiguration = generalConfiguration ?? throw new ArgumentNullException(nameof(generalConfiguration)); semaphore = new SemaphoreSlim(1); disposeCts = new CancellationTokenSource(); @@ -180,7 +189,13 @@ namespace Tgstation.Server.Host.Components.StaticFiles var dmeExistsTask = ioManager.FileExists(ioManager.ConcatPath(CodeModificationsSubdirectory, dmeFile), cancellationToken); var headFileExistsTask = ioManager.FileExists(ioManager.ConcatPath(CodeModificationsSubdirectory, CodeModificationsHeadFile), cancellationToken); var tailFileExistsTask = ioManager.FileExists(ioManager.ConcatPath(CodeModificationsSubdirectory, CodeModificationsTailFile), cancellationToken); - var copyTask = ioManager.CopyDirectory(CodeModificationsSubdirectory, destination, null, null, cancellationToken); + var copyTask = ioManager.CopyDirectory( + null, + null, + CodeModificationsSubdirectory, + destination, + generalConfiguration.GetCopyDirectoryTaskThrottle(), + cancellationToken); await Task.WhenAll(dmeExistsTask, headFileExistsTask, tailFileExistsTask, copyTask); diff --git a/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs b/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs index 7be48930ed..0bfab0901b 100644 --- a/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs +++ b/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs @@ -116,6 +116,11 @@ namespace Tgstation.Server.Host.Configuration /// public bool SkipAddingByondFirewallException { get; set; } + /// + /// A limit on the amount of tasks used for asynchronous I/O when copying directories during the deployment process as a multiplier to the machine's . Too few can significantly increase deployment times, too many can make TGS unresponsive and slowdown other I/O operations on the machine. + /// + public uint? DeploymentDirectoryCopyTasksPerCore { get; set; } + /// /// Initializes a new instance of the class. /// @@ -147,6 +152,26 @@ namespace Tgstation.Server.Host.Configuration CurrentConfigVersion); else logger.LogWarning("Your `ConfigVersion` is out-of-date. Please follow migration instructions from the TGS release notes."); + + if (DeploymentDirectoryCopyTasksPerCore == 0) + throw new InvalidOperationException( + $"{nameof(DeploymentDirectoryCopyTasksPerCore)} must be at least 1!"); + else if (GetCopyDirectoryTaskThrottle() < 1) + throw new InvalidOperationException( + $"{nameof(DeploymentDirectoryCopyTasksPerCore)} is too large for the CPU core count of {Environment.ProcessorCount} and overflows a 32-bit signed integer. Please lower the value!"); + } + + /// + /// Gets the total number of tasks that may run simultaneously during an asynchronous directory copy operation. + /// + /// The total number of tasks that may run simultaneously during an asynchronous directory copy operation. + public int? GetCopyDirectoryTaskThrottle() + { + if (!DeploymentDirectoryCopyTasksPerCore.HasValue) + return null; + + var taskThrottle = (uint)Environment.ProcessorCount * DeploymentDirectoryCopyTasksPerCore.Value; + return (int)taskThrottle; } } } diff --git a/src/Tgstation.Server.Host/IO/DefaultIOManager.cs b/src/Tgstation.Server.Host/IO/DefaultIOManager.cs index 6edc397308..75f0c66f1d 100644 --- a/src/Tgstation.Server.Host/IO/DefaultIOManager.cs +++ b/src/Tgstation.Server.Host/IO/DefaultIOManager.cs @@ -62,10 +62,11 @@ namespace Tgstation.Server.Host.IO /// public async Task CopyDirectory( - string src, - string dest, IEnumerable ignore, Func postCopyCallback, + string src, + string dest, + int? taskThrottle, CancellationToken cancellationToken) { if (src == null) @@ -73,10 +74,13 @@ namespace Tgstation.Server.Host.IO if (dest == null) throw new ArgumentNullException(nameof(src)); + if (taskThrottle.HasValue && taskThrottle < 1) + throw new ArgumentOutOfRangeException(nameof(taskThrottle), taskThrottle, "taskThrottle must be at least 1!"); + src = ResolvePath(src); dest = ResolvePath(dest); - using var semaphore = new SemaphoreSlim(100 * Environment.ProcessorCount); + using var semaphore = taskThrottle.HasValue ? new SemaphoreSlim(taskThrottle.Value) : null; await Task.WhenAll(CopyDirectoryImpl(src, dest, ignore, postCopyCallback, semaphore, cancellationToken)); } @@ -327,7 +331,7 @@ namespace Tgstation.Server.Host.IO /// The destination directory path. /// Files and folders to ignore at the root level. /// The optional callback called for each source/dest file pair post copy. - /// used to limit degree of parallelism. + /// Optional used to limit degree of parallelism. /// The for the operation. /// A of s representing the running operations. The first returned is always the necessary call to . IEnumerable CopyDirectoryImpl( @@ -377,7 +381,9 @@ namespace Tgstation.Server.Host.IO async Task CopyThisFile() { await subdirCreationTask; - using var lockContext = await SemaphoreSlimContext.Lock(semaphore, cancellationToken); + using var lockContext = semaphore != null + ? await SemaphoreSlimContext.Lock(semaphore, cancellationToken) + : null; await CopyFile(sourceFile, destFile, cancellationToken); if (postCopyCallback != null) await postCopyCallback(sourceFile, destFile); diff --git a/src/Tgstation.Server.Host/IO/IIOManager.cs b/src/Tgstation.Server.Host/IO/IIOManager.cs index f9275e1216..56a5343573 100644 --- a/src/Tgstation.Server.Host/IO/IIOManager.cs +++ b/src/Tgstation.Server.Host/IO/IIOManager.cs @@ -48,17 +48,19 @@ namespace Tgstation.Server.Host.IO /// /// Copies a directory from to . /// - /// The source directory path. - /// The destination directory path. /// Files and folders to ignore at the root level. /// The optional callback called for each source/dest file pair post copy. + /// The source directory path. + /// The destination directory path. + /// The optional maximum number of simultaneous tasks allowed to execute. /// The for the operation. /// A representing the running operation. Task CopyDirectory( - string src, - string dest, IEnumerable ignore, Func postCopyCallback, + string src, + string dest, + int? taskThrottle, CancellationToken cancellationToken); /// diff --git a/src/Tgstation.Server.Host/appsettings.yml b/src/Tgstation.Server.Host/appsettings.yml index a94f7e0d47..38fe9f57bd 100644 --- a/src/Tgstation.Server.Host/appsettings.yml +++ b/src/Tgstation.Server.Host/appsettings.yml @@ -12,6 +12,7 @@ General: ValidInstancePaths: HostApiDocumentation: false SkipAddingByondFirewallException: false + DeploymentDirectoryCopyTasksPerCore: 100 Session: HighPriorityLiveDreamDaemon: false LowPriorityDeploymentProcesses: true diff --git a/tests/Tgstation.Server.Host.Tests/IO/TestIOManager.cs b/tests/Tgstation.Server.Host.Tests/IO/TestIOManager.cs index d4a486b584..f92ffaa04d 100644 --- a/tests/Tgstation.Server.Host.Tests/IO/TestIOManager.cs +++ b/tests/Tgstation.Server.Host.Tests/IO/TestIOManager.cs @@ -1,5 +1,8 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; +using Remora.Discord.API.Objects; + +using System; using System.IO; using System.Threading.Tasks; @@ -71,5 +74,108 @@ namespace Tgstation.Server.Host.IO.Tests throw; } } + + [TestMethod] + public async Task TestCopyDirectoryThrows() + { + int? throttle = null; + var tempPath1 = Guid.NewGuid().ToString(); + var tempPath2 = Guid.NewGuid().ToString(); + + await Assert.ThrowsExceptionAsync(() => ioManager.CopyDirectory( + null, + null, + null, + tempPath2, + throttle, + default)); + await Assert.ThrowsExceptionAsync(() => ioManager.CopyDirectory( + null, + null, + tempPath1, + null, + throttle, + default)); + await Assert.ThrowsExceptionAsync(() => ioManager.CopyDirectory( + null, + null, + null, + null, + throttle, + default)); + await Assert.ThrowsExceptionAsync(() => ioManager.CopyDirectory( + null, + null, + tempPath1, + tempPath2, + -1, + default)); + } + + [TestMethod] + public async Task TestCopyDirectoryOneTask() + { + await TestCopyDirectory(1); + } + + [TestMethod] + public async Task TestCopyDirectoryMaxTasks() + { + await TestCopyDirectory(Int32.MaxValue); + } + + [TestMethod] + public async Task TestCopyDirectoryUnlimitedTasks() + { + await TestCopyDirectory(null); + } + + async Task TestCopyDirectory(int? throttle) + { + var tempPath = Path.GetTempFileName(); + File.Delete(tempPath); + Directory.CreateDirectory(tempPath); + try + { + var tempPath2 = Path.GetTempFileName(); + File.Delete(tempPath2); + + await File.WriteAllTextAsync(Path.Combine(tempPath, "file.txt"), "asdf"); + var subDir = Path.Combine(tempPath, "subdir"); + Directory.CreateDirectory(subDir); + await File.WriteAllTextAsync(Path.Combine(subDir, "file2.txt"), "fdsa"); + + try + { + await ioManager.CopyDirectory( + null, + null, + tempPath, + tempPath2, + throttle, + default); + + Assert.IsTrue(Directory.Exists(tempPath2)); + var newFilePath = Path.Combine(tempPath2, "file.txt"); + Assert.IsTrue(File.Exists(newFilePath)); + var newFileText = await File.ReadAllTextAsync(newFilePath); + Assert.AreEqual("asdf", newFileText); + var newDirPath = Path.Combine(tempPath2, "subdir"); + Assert.IsTrue(Directory.Exists(newDirPath)); + var newFile2Path = Path.Combine(newDirPath, "file2.txt"); + Assert.IsTrue(File.Exists(newFile2Path)); + var newFile2Text = await File.ReadAllTextAsync(newFile2Path); + Assert.AreEqual("fdsa", newFile2Text); + } + finally + { + Directory.Delete(tempPath2, true); + } + } + finally + { + Directory.Delete(tempPath, true); + } + } } } diff --git a/tests/Tgstation.Server.Tests/Live/Instance/ConfigurationTest.cs b/tests/Tgstation.Server.Tests/Live/Instance/ConfigurationTest.cs index cb98a62cba..46a0772c05 100644 --- a/tests/Tgstation.Server.Tests/Live/Instance/ConfigurationTest.cs +++ b/tests/Tgstation.Server.Tests/Live/Instance/ConfigurationTest.cs @@ -106,15 +106,17 @@ namespace Tgstation.Server.Tests.Live.Instance var ioManager = new DefaultIOManager(); return Task.WhenAll( ioManager.CopyDirectory( + Enumerable.Empty(), + null, "../../../../DMAPI", ioManager.ConcatPath(instance.Path, "Repository", "tests", "DMAPI"), - Enumerable.Empty(), null, cancellationToken), ioManager.CopyDirectory( + Enumerable.Empty(), + null, "../../../../../src/DMAPI", ioManager.ConcatPath(instance.Path, "Repository", "src", "DMAPI"), - Enumerable.Empty(), null, cancellationToken) ); diff --git a/tests/Tgstation.Server.Tests/TestRepository.cs b/tests/Tgstation.Server.Tests/TestRepository.cs index 7366c43e44..9a8a42c92a 100644 --- a/tests/Tgstation.Server.Tests/TestRepository.cs +++ b/tests/Tgstation.Server.Tests/TestRepository.cs @@ -7,6 +7,7 @@ using Moq; using Tgstation.Server.Host.Components.Events; using Tgstation.Server.Host.Components.Repository; +using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; using Tgstation.Server.Tests.Live; @@ -31,6 +32,7 @@ namespace Tgstation.Server.Tests Mock.Of(), Mock.Of(), Mock.Of>(), + new GeneralConfiguration(), () => { }); const string StartSha = "af4da8beb9f9b374b04a3cc4d65acca662e8cc1a"; From 06043de5027f57278cd3e328320bbfe121a83525 Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 23 Apr 2023 02:40:26 -0400 Subject: [PATCH 2/3] Fix a ton of Repository linter errors --- .../Components/Repository/Repository.cs | 53 +++++++++---------- .../Repository/RepositoryManager.cs | 2 +- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Repository/Repository.cs b/src/Tgstation.Server.Host/Components/Repository/Repository.cs index 62e41c67c2..886896105e 100644 --- a/src/Tgstation.Server.Host/Components/Repository/Repository.cs +++ b/src/Tgstation.Server.Host/Components/Repository/Repository.cs @@ -65,7 +65,7 @@ namespace Tgstation.Server.Host.Components.Repository public string Reference => libGitRepo.Head.FriendlyName; /// - public Uri Origin => new Uri(libGitRepo.Network.Remotes.First().Url); + public Uri Origin => new (libGitRepo.Network.Remotes.First().Url); /// /// The for the . @@ -201,9 +201,9 @@ namespace Tgstation.Server.Host.Components.Repository throw new ArgumentNullException(nameof(progressReporter)); logger.LogDebug( - "Begin AddTestMerge: #{0} at {1} ({2}) by <{3} ({4})>", + "Begin AddTestMerge: #{prNumber} at {targetSha} ({comment}) by <{committerName} ({committerEmail})>", testMergeParameters.Number, - testMergeParameters.TargetCommitSha?.Substring(0, 7), + testMergeParameters.TargetCommitSha?[..7], testMergeParameters.Comment, committerName, committerEmail); @@ -242,7 +242,7 @@ namespace Tgstation.Server.Host.Components.Repository { try { - logger.LogTrace("Fetching refspec {0}...", refSpec); + logger.LogTrace("Fetching refspec {refSpec}...", refSpec); var remote = libGitRepo.Network.Remotes.First(); commands.Fetch( @@ -276,15 +276,13 @@ namespace Tgstation.Server.Host.Components.Repository cancellationToken.ThrowIfCancellationRequested(); var objectName = testMergeParameters.TargetCommitSha ?? localBranchName; - var gitObject = libGitRepo.Lookup(objectName); - if (gitObject == null) - throw new JobException($"Could not find object to merge: {objectName}"); + var gitObject = libGitRepo.Lookup(objectName) ?? throw new JobException($"Could not find object to merge: {objectName}"); testMergeParameters.TargetCommitSha = gitObject.Sha; cancellationToken.ThrowIfCancellationRequested(); - logger.LogTrace("Merging {0} into {1}...", testMergeParameters.TargetCommitSha[..7], Reference); + logger.LogTrace("Merging {targetCommitSha} into {currentReference}...", testMergeParameters.TargetCommitSha[..7], Reference); result = libGitRepo.Merge(testMergeParameters.TargetCommitSha, sig, new MergeOptions { @@ -312,7 +310,7 @@ namespace Tgstation.Server.Host.Components.Repository conflictedPaths.Add(file.FilePath); var revertTo = originalCommit.CanonicalName ?? originalCommit.Tip.Sha; - logger.LogDebug("Merge conflict, aborting and reverting to {0}", revertTo); + logger.LogDebug("Merge conflict, aborting and reverting to {revertTarget}", revertTo); progressReporter.ReportProgress(0); RawCheckout(revertTo, progressReporter.CreateSection("Hard Reset to {revertTo}", 1.0), cancellationToken); cancellationToken.ThrowIfCancellationRequested(); @@ -349,7 +347,7 @@ namespace Tgstation.Server.Host.Components.Repository if (result.Status != MergeStatus.UpToDate) { - logger.LogTrace("Committing merge: \"{0}\"...", commitMessage); + logger.LogTrace("Committing merge: \"{commitMessage}\"...", commitMessage); await Task.Factory.StartNew( () => libGitRepo.Commit(commitMessage, sig, sig, new CommitOptions { @@ -400,7 +398,7 @@ namespace Tgstation.Server.Host.Components.Repository throw new ArgumentNullException(nameof(committish)); if (progressReporter == null) throw new ArgumentNullException(nameof(progressReporter)); - logger.LogDebug("Checkout object: {0}...", committish); + logger.LogDebug("Checkout object: {committish}...", committish); await eventConsumer.HandleEvent(EventType.RepoCheckout, new List { committish }, cancellationToken); await Task.Factory.StartNew( () => @@ -506,7 +504,7 @@ namespace Tgstation.Server.Host.Components.Repository if (progressReporter == null) throw new ArgumentNullException(nameof(progressReporter)); - logger.LogDebug("Reset to sha: {0}", sha.Substring(0, 7)); + logger.LogDebug("Reset to sha: {sha}", sha[..7]); libGitRepo.RemoveUntrackedFiles(); cancellationToken.ThrowIfCancellationRequested(); @@ -531,7 +529,7 @@ namespace Tgstation.Server.Host.Components.Repository { if (path == null) throw new ArgumentNullException(nameof(path)); - logger.LogTrace("Copying to {0}...", path); + logger.LogTrace("Copying to {path}...", path); await ioMananger.CopyDirectory( new List { ".git" }, (src, dest) => @@ -590,7 +588,7 @@ namespace Tgstation.Server.Host.Components.Repository trackedBranch = libGitRepo.Head.TrackedBranch; logger.LogDebug( - "Merge origin/{0}: <{1} ({2})>", + "Merge origin/{trackedBranch}: <{committerName} ({committerEmail})>", trackedBranch.FriendlyName, committerName, committerEmail); @@ -607,7 +605,7 @@ namespace Tgstation.Server.Host.Components.Repository if (result.Status == MergeStatus.Conflicts) { - logger.LogDebug("Merge conflict, aborting and reverting to {0}", oldHead.FriendlyName); + logger.LogDebug("Merge conflict, aborting and reverting to {oldHeadFriendlyName}", oldHead.FriendlyName); progressReporter.ReportProgress(0); libGitRepo.Reset(ResetMode.Hard, oldTip, new CheckoutOptions { @@ -664,7 +662,7 @@ namespace Tgstation.Server.Host.Components.Repository var startHead = Head; - logger.LogTrace("Configuring <{0} ({1})> as author/committer", committerName, committerEmail); + logger.LogTrace("Configuring <{committerName} ({committerEmail})> as author/committer", committerName, committerEmail); await Task.Factory.StartNew( () => { @@ -721,7 +719,7 @@ namespace Tgstation.Server.Host.Components.Repository var sameHead = Head == startHead; if (sameHead || !Tracking) { - logger.LogTrace("Aborted synchronize due to {0}!", sameHead ? "lack of changes" : "not being on tracked reference"); + logger.LogTrace("Aborted synchronize due to {abortReason}!", sameHead ? "lack of changes" : "not being on tracked reference"); return false; } @@ -795,7 +793,7 @@ namespace Tgstation.Server.Host.Components.Repository var targetCommit = libGitRepo.Lookup(sha); if (targetCommit == null) { - logger.LogTrace("Commit {0} not found in repository", sha); + logger.LogTrace("Commit {sha} not found in repository", sha); return false; } @@ -846,10 +844,7 @@ namespace Tgstation.Server.Host.Components.Repository if (sha == null) throw new ArgumentNullException(nameof(sha)); - var commit = libGitRepo.Lookup(sha); - if (commit == null) - throw new JobException($"Commit {sha} does not exist in the repository!"); - + var commit = libGitRepo.Lookup(sha) ?? throw new JobException($"Commit {sha} does not exist in the repository!"); return commit.Committer.When; }, cancellationToken, @@ -864,7 +859,7 @@ namespace Tgstation.Server.Host.Components.Repository /// The for the operation. void RawCheckout(string committish, JobProgressReporter progressReporter, CancellationToken cancellationToken) { - logger.LogTrace("Checkout: {0}", committish); + logger.LogTrace("Checkout: {committish}", committish); var stage = $"Checkout {committish}"; progressReporter = progressReporter.CreateSection(stage, 1.0); @@ -897,7 +892,7 @@ namespace Tgstation.Server.Host.Components.Repository if (remoteBranch == default) throw; - logger.LogDebug("Creating local branch for {0}...", remoteBranch.FriendlyName); + logger.LogDebug("Creating local branch for {remoteBranchFriendlyName}...", remoteBranch.FriendlyName); var branch = libGitRepo.CreateBranch(committish, remoteBranch.Tip); libGitRepo.Branches.Update(branch, branchUpdate => branchUpdate.TrackedBranch = remoteBranch.CanonicalName); @@ -1005,7 +1000,7 @@ namespace Tgstation.Server.Host.Components.Repository return; } - logger.LogTrace("Updating submodules with{0} credentials...", username == null ? "out" : String.Empty); + logger.LogTrace("Updating submodules with{orWithout} credentials...", username == null ? "out" : String.Empty); var factor = 1.0 / submoduleCount / 2; foreach (var submodule in libGitRepo.Submodules) @@ -1023,7 +1018,7 @@ namespace Tgstation.Server.Host.Components.Repository progressReporter.CreateSection($"Checkout submodule {submodule.Name}", factor)), }; - logger.LogDebug("Updating submodule {0}...", submodule.Name); + logger.LogDebug("Updating submodule {submoduleName}...", submodule.Name); Task RawSubModuleUpdate() => Task.Factory.StartNew( () => libGitRepo.Submodules.Update(submodule.Name, submoduleUpdateOptions), cancellationToken, @@ -1039,14 +1034,14 @@ namespace Tgstation.Server.Host.Components.Repository // kill off the modules/ folder in .git and try again progressReporter.ReportProgress(null); credentialsProvider.CheckBadCredentialsException(ex); - logger.LogWarning(ex, "Initial update of submodule {0} failed. Deleting submodule directories and re-attempting...", submodule.Name); + logger.LogWarning(ex, "Initial update of submodule {submoduleName} failed. Deleting submodule directories and re-attempting...", submodule.Name); await Task.WhenAll( ioMananger.DeleteDirectory($".git/modules/{submodule.Path}", cancellationToken), ioMananger.DeleteDirectory(submodule.Path, cancellationToken)) ; - logger.LogTrace("Second update attempt for submodule {0}...", submodule.Name); + logger.LogTrace("Second update attempt for submodule {submoduleName}...", submodule.Name); try { await RawSubModuleUpdate(); @@ -1058,7 +1053,7 @@ namespace Tgstation.Server.Host.Components.Repository catch (LibGit2SharpException ex2) { credentialsProvider.CheckBadCredentialsException(ex2); - logger.LogTrace(ex2, "Retried update of submodule {0} failed!", submodule.Name); + logger.LogTrace(ex2, "Retried update of submodule {submoduleName} failed!", submodule.Name); throw new AggregateException(ex, ex2); } } diff --git a/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs b/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs index 237053a5d0..ccf009173c 100644 --- a/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs +++ b/src/Tgstation.Server.Host/Components/Repository/RepositoryManager.cs @@ -130,7 +130,7 @@ namespace Tgstation.Server.Host.Components.Repository if (progressReporter == null) throw new ArgumentNullException(nameof(progressReporter)); - logger.LogInformation("Begin clone {0} (Branch: {1})", url, initialBranch); + logger.LogInformation("Begin clone {url} (Branch: {initialBranch})", url, initialBranch); lock (semaphore) { if (CloneInProgress) From ac2a9f11d179988c68b167316d13ae4aac20fb4d Mon Sep 17 00:00:00 2001 From: Dominion Date: Sun, 23 Apr 2023 03:35:30 -0400 Subject: [PATCH 3/3] Fix CA1024 --- .../Components/Repository/Repository.cs | 1 + .../Components/StaticFiles/Configuration.cs | 1 + .../Configuration/GeneralConfiguration.cs | 16 ++-------- .../GeneralConfigurationExtensions.cs | 30 +++++++++++++++++++ .../TestGeneralConfigurationExtensions.cs | 16 ++++++++++ 5 files changed, 50 insertions(+), 14 deletions(-) create mode 100644 src/Tgstation.Server.Host/Extensions/GeneralConfigurationExtensions.cs create mode 100644 tests/Tgstation.Server.Host.Tests/Extensions/TestGeneralConfigurationExtensions.cs diff --git a/src/Tgstation.Server.Host/Components/Repository/Repository.cs b/src/Tgstation.Server.Host/Components/Repository/Repository.cs index 886896105e..bcfa0c24a0 100644 --- a/src/Tgstation.Server.Host/Components/Repository/Repository.cs +++ b/src/Tgstation.Server.Host/Components/Repository/Repository.cs @@ -13,6 +13,7 @@ using Tgstation.Server.Api.Models; using Tgstation.Server.Api.Models.Internal; using Tgstation.Server.Host.Components.Events; using Tgstation.Server.Host.Configuration; +using Tgstation.Server.Host.Extensions; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; diff --git a/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs b/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs index 2f8002e354..74f771da31 100644 --- a/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs +++ b/src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs @@ -15,6 +15,7 @@ using Tgstation.Server.Api.Models.Response; using Tgstation.Server.Host.Components.Events; using Tgstation.Server.Host.Configuration; using Tgstation.Server.Host.Core; +using Tgstation.Server.Host.Extensions; using Tgstation.Server.Host.IO; using Tgstation.Server.Host.Jobs; using Tgstation.Server.Host.Security; diff --git a/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs b/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs index 0bfab0901b..4b17fffa15 100644 --- a/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs +++ b/src/Tgstation.Server.Host/Configuration/GeneralConfiguration.cs @@ -5,6 +5,7 @@ using Newtonsoft.Json; using Newtonsoft.Json.Converters; using Tgstation.Server.Api.Models.Internal; +using Tgstation.Server.Host.Extensions; using Tgstation.Server.Host.Properties; using Tgstation.Server.Host.Setup; @@ -156,22 +157,9 @@ namespace Tgstation.Server.Host.Configuration if (DeploymentDirectoryCopyTasksPerCore == 0) throw new InvalidOperationException( $"{nameof(DeploymentDirectoryCopyTasksPerCore)} must be at least 1!"); - else if (GetCopyDirectoryTaskThrottle() < 1) + else if (this.GetCopyDirectoryTaskThrottle() < 1) throw new InvalidOperationException( $"{nameof(DeploymentDirectoryCopyTasksPerCore)} is too large for the CPU core count of {Environment.ProcessorCount} and overflows a 32-bit signed integer. Please lower the value!"); } - - /// - /// Gets the total number of tasks that may run simultaneously during an asynchronous directory copy operation. - /// - /// The total number of tasks that may run simultaneously during an asynchronous directory copy operation. - public int? GetCopyDirectoryTaskThrottle() - { - if (!DeploymentDirectoryCopyTasksPerCore.HasValue) - return null; - - var taskThrottle = (uint)Environment.ProcessorCount * DeploymentDirectoryCopyTasksPerCore.Value; - return (int)taskThrottle; - } } } diff --git a/src/Tgstation.Server.Host/Extensions/GeneralConfigurationExtensions.cs b/src/Tgstation.Server.Host/Extensions/GeneralConfigurationExtensions.cs new file mode 100644 index 0000000000..9a8418fde4 --- /dev/null +++ b/src/Tgstation.Server.Host/Extensions/GeneralConfigurationExtensions.cs @@ -0,0 +1,30 @@ +using System; + +using Tgstation.Server.Host.Configuration; + +namespace Tgstation.Server.Host.Extensions +{ + /// + /// Extension methods for the . + /// + static class GeneralConfigurationExtensions + { + /// + /// Gets the total number of tasks that may run simultaneously during an asynchronous directory copy operation. + /// + /// The to read the from. + /// The total number of tasks that may run simultaneously during an asynchronous directory copy operation. + public static int? GetCopyDirectoryTaskThrottle(this GeneralConfiguration generalConfiguration) + { + if (generalConfiguration == null) + throw new ArgumentNullException(nameof(generalConfiguration)); + + var tasksPerCore = generalConfiguration.DeploymentDirectoryCopyTasksPerCore; + if (!tasksPerCore.HasValue) + return null; + + var taskThrottle = (uint)Environment.ProcessorCount * tasksPerCore.Value; + return (int)taskThrottle; + } + } +} diff --git a/tests/Tgstation.Server.Host.Tests/Extensions/TestGeneralConfigurationExtensions.cs b/tests/Tgstation.Server.Host.Tests/Extensions/TestGeneralConfigurationExtensions.cs new file mode 100644 index 0000000000..1f7b726a6e --- /dev/null +++ b/tests/Tgstation.Server.Host.Tests/Extensions/TestGeneralConfigurationExtensions.cs @@ -0,0 +1,16 @@ +using System; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Tgstation.Server.Host.Extensions.Tests +{ + [TestClass] + public sealed class TestGeneralConfigurationExtensions + { + [TestMethod] + public void TestThrowsOnNullParameter() + { + Assert.ThrowsException(() => GeneralConfigurationExtensions.GetCopyDirectoryTaskThrottle(null)); + } + } +}