Merge pull request #1763 from tgstation/PortInUseDiagnosis [TGSDeploy]

Log warnings as to why ports cannot be allocated. Fix DME's with spaces

.github/workflows/ci-pipeline.yml

@@ -151,7 +151,7 @@ jobs:
 
         if hash DreamMaker 2>/dev/null
         then
-          DreamMaker tests/DMAPI/BasicOperation/basic_operation_test.dme 2>&1 | tee result.log
+          DreamMaker "tests/DMAPI/BasicOperation/basic operation_test.dme" 2>&1 | tee result.log
           retval=$?
           if ! grep '\- 0 errors, 0 warnings' result.log
           then
@@ -205,7 +205,7 @@ jobs:
     - name: Build DMAPI
       run: |
         cd tests/DMAPI/BasicOperation
-        $HOME/OpenDream/tgs_deploy/bin/compiler/DMCompiler --verbose --notices-enabled basic_operation_test.dme
+        $HOME/OpenDream/tgs_deploy/bin/compiler/DMCompiler --verbose --notices-enabled "basic operation_test.dme"
 
   pages-build:
     name: Build gh-pages

build/Version.props

@@ -3,7 +3,7 @@
   <!-- Integration tests will ensure they match across the board -->
   <Import Project="WebpanelVersion.props" />
   <PropertyGroup>
-    <TgsCoreVersion>6.1.1</TgsCoreVersion>
+    <TgsCoreVersion>6.1.2</TgsCoreVersion>
     <TgsConfigVersion>5.0.0</TgsConfigVersion>
     <TgsApiVersion>10.0.0</TgsApiVersion>
     <TgsCommonLibraryVersion>7.0.0</TgsCommonLibraryVersion>

src/Tgstation.Server.Host/Components/Deployment/DreamMaker.cs

@@ -610,7 +610,7 @@ await eventConsumer.HandleEvent(
 						throw new JobException(ErrorCode.DeploymentMissingDme);
 				}
 
-				logger.LogDebug("Selected {dmeName}.dme for compilation!", job.DmeName);
+				logger.LogDebug("Selected \"{dmeName}.dme\" for compilation!", job.DmeName);
 
 				progressReporter.StageName = "Modifying .dme";
 				await ModifyDme(job, cancellationToken);

src/Tgstation.Server.Host/Components/Engine/ByondInstallation.cs

@@ -118,7 +118,7 @@ public override string FormatServerArguments(
 
 			var arguments = String.Format(
 				CultureInfo.InvariantCulture,
-				"{0} -port {1} -ports 1-65535 {2}-close -verbose -{3} -{4}{5}{6}{7} -params \"{8}\"",
+				"\"{0}\" -port {1} -ports 1-65535 {2}-close -verbose -{3} -{4}{5}{6}{7} -params \"{8}\"",
 				dmbProvider.DmbName,
 				launchParameters.Port!.Value,
 				launchParameters.AllowWebClient!.Value

src/Tgstation.Server.Host/Core/Application.cs

@@ -405,7 +405,7 @@ void AddTypedContext<TContext>()
 			services.AddSingleton<ISwarmServiceController>(x => x.GetRequiredService<SwarmService>());
 
 			// configure component services
-			services.AddScoped<IPortAllocator, PortAllocator>();
+			services.AddSingleton<IPortAllocator, PortAllocator>();
 			services.AddSingleton<IInstanceFactory, InstanceFactory>();
 			services.AddSingleton<IGitRemoteFeaturesFactory, GitRemoteFeaturesFactory>();
 			services.AddSingleton<ILibGit2RepositoryFactory, LibGit2RepositoryFactory>();

src/Tgstation.Server.Host/Utils/PortAllocator.cs

@@ -17,7 +17,7 @@
 namespace Tgstation.Server.Host.Utils
 {
 	/// <inheritdoc />
-	sealed class PortAllocator : IPortAllocator
+	sealed class PortAllocator : IPortAllocator, IDisposable
 	{
 		/// <summary>
 		/// The <see cref="IServerPortProvider"/> for the <see cref="PortAllocator"/>.
@@ -27,7 +27,7 @@ sealed class PortAllocator : IPortAllocator
 		/// <summary>
 		/// The <see cref="IDatabaseContext"/> for the <see cref="PortAllocator"/>.
 		/// </summary>
-		readonly IDatabaseContext databaseContext;
+		readonly IDatabaseContextFactory databaseContextFactory;
 
 		/// <summary>
 		/// The <see cref="IPlatformIdentifier"/> for the <see cref="PortAllocator"/>.
@@ -44,45 +44,79 @@ sealed class PortAllocator : IPortAllocator
 		/// </summary>
 		readonly SwarmConfiguration swarmConfiguration;
 
+		/// <summary>
+		/// The <see cref="SemaphoreSlim"/> used to serialized port requisition requests.
+		/// </summary>
+		readonly SemaphoreSlim allocatorLock;
+
 		/// <summary>
 		/// Initializes a new instance of the <see cref="PortAllocator"/> class.
 		/// </summary>
 		/// <param name="serverPortProvider">The value of <see cref="serverPortProvider"/>.</param>
-		/// <param name="databaseContext">The value of <see cref="databaseContext"/>.</param>
+		/// <param name="databaseContextFactory">The value of <see cref="databaseContextFactory"/>.</param>
 		/// <param name="platformIdentifier">The value of <see cref="platformIdentifier"/>.</param>
 		/// <param name="swarmConfigurationOptions">The <see cref="IOptions{TOptions}"/> containing the value of <see cref="swarmConfiguration"/>.</param>
 		/// <param name="logger">The value of <see cref="logger"/>.</param>
 		public PortAllocator(
 			IServerPortProvider serverPortProvider,
-			IDatabaseContext databaseContext,
+			IDatabaseContextFactory databaseContextFactory,
 			IPlatformIdentifier platformIdentifier,
 			IOptions<SwarmConfiguration> swarmConfigurationOptions,
 			ILogger<PortAllocator> logger)
 		{
 			this.serverPortProvider = serverPortProvider ?? throw new ArgumentNullException(nameof(serverPortProvider));
-			this.databaseContext = databaseContext ?? throw new ArgumentNullException(nameof(databaseContext));
+			this.databaseContextFactory = databaseContextFactory ?? throw new ArgumentNullException(nameof(databaseContextFactory));
 			this.platformIdentifier = platformIdentifier ?? throw new ArgumentNullException(nameof(platformIdentifier));
 			swarmConfiguration = swarmConfigurationOptions?.Value ?? throw new ArgumentNullException(nameof(swarmConfigurationOptions));
 			this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
+
+			allocatorLock = new SemaphoreSlim(1);
 		}
 
+		/// <inheritdoc />
+		public void Dispose() => allocatorLock.Dispose();
+
 		/// <inheritdoc />
 		public async ValueTask<ushort?> GetAvailablePort(ushort basePort, bool checkOne, CancellationToken cancellationToken)
 		{
-			logger.LogTrace("Port allocation >= {basePort} requested...", basePort);
+			ushort? result = null;
+			using (await SemaphoreSlimContext.Lock(allocatorLock, cancellationToken))
+				await databaseContextFactory.UseContext(
+					async databaseContext => result = await GetAvailablePort(databaseContext, basePort, checkOne, cancellationToken));
+			return result;
+		}
 
+		/// <summary>
+		/// Gets a port not currently in use by TGS.
+		/// </summary>
+		/// <param name="databaseContext">The <see cref="IDatabaseContext"/> to use.</param>
+		/// <param name="basePort">The port to check first. Will not allocate a port lower than this.</param>
+		/// <param name="checkOne">If only <paramref name="basePort"/> should be checked and no others.</param>
+		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
+		/// <returns>A <see cref="ValueTask{TResult}"/> resulting in the first available port on success, <see langword="null"/> on failure.</returns>
+		async ValueTask<ushort?> GetAvailablePort(IDatabaseContext databaseContext, ushort basePort, bool checkOne, CancellationToken cancellationToken)
+		{
+			logger.LogTrace("Port allocation >= {basePort} requested...", basePort);
 			var ddPorts = await databaseContext
 				.DreamDaemonSettings
 				.AsQueryable()
 				.Where(x => x.Instance!.SwarmIdentifer == swarmConfiguration.Identifier)
-				.Select(x => x.Port)
+				.Select(x => new
+				{
+					Port = x.Port!.Value,
+					x.InstanceId,
+				})
 				.ToListAsync(cancellationToken);
 
 			var dmPorts = await databaseContext
 				.DreamMakerSettings
 				.AsQueryable()
 				.Where(x => x.Instance!.SwarmIdentifer == swarmConfiguration.Identifier)
-				.Select(x => x.ApiValidationPort)
+				.Select(x => new
+				{
+					ApiValidationPort = x.ApiValidationPort!.Value,
+					x.InstanceId,
+				})
 				.ToListAsync(cancellationToken);
 
 			var exceptions = new List<Exception>();
@@ -94,10 +128,35 @@ public PortAllocator(
 					if (checkOne && port != basePort)
 						break;
 
-					if (port == serverPortProvider.HttpApiPort
-						|| ddPorts.Contains(port)
-						|| dmPorts.Contains(port))
+					if (port == serverPortProvider.HttpApiPort)
+					{
+						logger.LogWarning("Cannot allocate port {port} as it is the TGS API port!", port);
+						continue;
+					}
+
+					var reservedGamePortData = ddPorts.Where(data => data.Port == port).ToList();
+					if (reservedGamePortData.Count > 0)
+					{
+						logger.LogWarning(
+							"Cannot allocate port {port} as it in use by the game server of instance(s): {instanceId}!",
+							port,
+							String.Join(
+								", ",
+								reservedGamePortData.Select(data => data.InstanceId)));
+						continue;
+					}
+
+					var reservedApiValidationPortData = dmPorts.Where(data => data.ApiValidationPort == port).ToList();
+					if (reservedApiValidationPortData.Count > 0)
+					{
+						logger.LogWarning(
+							"Cannot allocate port {port} as it in use by the API validation server of instance(s): {instanceId}!",
+							port,
+							String.Join(
+								", ",
+								reservedApiValidationPortData.Select(data => data.InstanceId)));
 						continue;
+					}
 
 					try
 					{

tests/Tgstation.Server.Tests/Live/Instance/WatchdogTest.cs

@@ -606,7 +606,7 @@ async Task TestDMApiFreeDeploy(CancellationToken cancellationToken)
 			var initialCompileJob = daemonStatus.ActiveCompileJob;
 			await CheckDMApiFail(daemonStatus.ActiveCompileJob, cancellationToken);
 
-			daemonStatus = await DeployTestDme("BasicOperation/basic_operation_test", DreamDaemonSecurity.Trusted, true, cancellationToken);
+			daemonStatus = await DeployTestDme("BasicOperation/basic operation_test", DreamDaemonSecurity.Trusted, true, cancellationToken);
 
 			Assert.AreEqual(WatchdogStatus.Online, daemonStatus.Status.Value);
 			Assert.AreEqual(false, daemonStatus.SoftRestart); // dme name change triggered, instant reboot
@@ -629,7 +629,7 @@ async Task RunBasicTest(CancellationToken cancellationToken)
 				AdditionalParameters = "test=bababooey"
 			}, cancellationToken);
 			Assert.AreEqual("test=bababooey", daemonStatus.AdditionalParameters);
-			daemonStatus = await DeployTestDme("BasicOperation/basic_operation_test", DreamDaemonSecurity.Trusted, true, cancellationToken);
+			daemonStatus = await DeployTestDme("BasicOperation/basic operation_test", DreamDaemonSecurity.Trusted, true, cancellationToken);
 
 			Assert.AreEqual(WatchdogStatus.Offline, daemonStatus.Status.Value);
 			Assert.IsNotNull(daemonStatus.ActiveCompileJob);

tgstation-server.sln

@@ -171,8 +171,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ApiFree", "ApiFree", "{7B8F
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "BasicOperation", "BasicOperation", "{F32B9514-AAD9-429D-841A-ED810FC2598C}"
 	ProjectSection(SolutionItems) = preProject
-		tests\DMAPI\BasicOperation\basic_operation_test.dme = tests\DMAPI\BasicOperation\basic_operation_test.dme
 		tests\DMAPI\BasicOperation\Config.dm = tests\DMAPI\BasicOperation\Config.dm
+		tests\DMAPI\BasicOperation\basic operation_test.dme = tests\DMAPI\BasicOperation\basic operation_test.dme
 		tests\DMAPI\BasicOperation\Test.dm = tests\DMAPI\BasicOperation\Test.dm
 	EndProjectSection
 EndProject