Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

oom_score_adj support. Test merge URL deduction #1794

Merged
merged 4 commits into from
Mar 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ docker run \
--network="host" \ # Not recommended, eases networking setup if your sql server is on the same machine
--name="tgs" \ # Name for the container
--cap-add=sys_nice \ # Recommended, allows TGS to lower the niceness of child processes if it sees fit
--cap-add=sys_resource \ # Recommended, allows TGS to not be killed by the OOM killer before its child processes
--init \ #Highly recommended, reaps potential zombie processes
-p 5000:5000 \ # Port bridge for accessing TGS, you can change this if you need
-p 0.0.0.0:<public game port>:<public game port> \ # Port bridge for accessing DreamDaemon
Expand Down
2 changes: 1 addition & 1 deletion src/Tgstation.Server.Api/Tgstation.Server.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
<!-- Usage: HTTP constants reference -->
<PackageReference Include="Microsoft.AspNetCore.Http.Extensions" Version="2.2.0" />
<!-- Usage: Decoding the 'nbf' property of JWTs -->
<PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="7.3.1" />
<PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="7.4.0" />
<!-- Usage: Primary JSON library -->
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />
<!-- Usage: Data model annotating -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -855,11 +855,12 @@ async ValueTask<bool> RunDreamMaker(IEngineExecutableLock engineLock, Models.Com
var environment = await engineLock.LoadEnv(logger, true, cancellationToken);
var arguments = engineLock.FormatCompilerArguments($"{job.DmeName}.{DmeExtension}");

await using var dm = processExecutor.LaunchProcess(
await using var dm = await processExecutor.LaunchProcess(
engineLock.CompilerExePath,
ioManager.ResolvePath(
job.DirectoryName!.Value.ToString()),
arguments,
cancellationToken,
environment,
readStandardHandles: true,
noShellExecute: true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,11 @@ public override async ValueTask Install(EngineVersion version, string installPat
async shortenedPath =>
{
var shortenedDeployPath = IOManager.ConcatPath(shortenedPath, DeployDir);
await using var buildProcess = ProcessExecutor.LaunchProcess(
await using var buildProcess = await ProcessExecutor.LaunchProcess(
dotnetPath,
shortenedPath,
$"run -c Release --project OpenDreamPackageTool -- --tgs -o {shortenedDeployPath}",
cancellationToken,
null,
null,
!GeneralConfiguration.OpenDreamSuppressInstallOutput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,10 +283,11 @@ async ValueTask InstallDirectX(string path, CancellationToken cancellationToken)
try
{
// noShellExecute because we aren't doing runas shennanigans
await using var directXInstaller = processExecutor.LaunchProcess(
await using var directXInstaller = await processExecutor.LaunchProcess(
IOManager.ConcatPath(rbdx, "DXSETUP.exe"),
rbdx,
"/silent",
cancellationToken,
noShellExecute: true);

int exitCode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public GitHubRemoteFeatures(IGitHubServiceFactory gitHubServiceFactory, ILogger<
Comment = parameters.Comment,
Number = parameters.Number,
TargetCommitSha = revisionToUse,
Url = pr?.HtmlUrl ?? errorMessage,
Url = pr?.HtmlUrl ?? $"https://github.com/{RemoteRepositoryOwner}/{RemoteRepositoryName}/pull/{parameters.Number}",
};

return testMerge;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public GitLabRemoteFeatures(ILogger<GitLabRemoteFeatures> logger, Uri remoteUrl)
Comment = parameters.Comment,
Number = parameters.Number,
TargetCommitSha = parameters.TargetCommitSha,
Url = ex.Message,
Url = $"https://gitlab.com/{RemoteRepositoryOwner}/{RemoteRepositoryName}/-/merge_requests/{parameters.Number}",
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,11 @@ await using (process)
? logFilePath
: null);

var process = processExecutor.LaunchProcess(
var process = await processExecutor.LaunchProcess(
engineLock.ServerExePath,
dmbProvider.Directory,
arguments,
cancellationToken,
environment,
logFilePath,
engineLock.HasStandardOutput,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ async ValueTask ExecuteEventScripts(IEnumerable<string?> parameters, bool deploy
foreach (var scriptFile in scriptFiles)
{
logger.LogTrace("Running event script {scriptFile}...", scriptFile);
await using (var script = processExecutor.LaunchProcess(
await using (var script = await processExecutor.LaunchProcess(
ioManager.ConcatPath(resolvedScriptsDir, scriptFile),
resolvedScriptsDir,
String.Join(
Expand All @@ -778,6 +778,7 @@ async ValueTask ExecuteEventScripts(IEnumerable<string?> parameters, bool deploy

return $"\"{arg}\"";
})),
cancellationToken,
readStandardHandles: true,
noShellExecute: true))
using (cancellationToken.Register(() => script.Terminate()))
Expand Down
1 change: 1 addition & 0 deletions src/Tgstation.Server.Host/Core/Application.cs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ void AddTypedContext<TContext>()
services.AddSingleton<IPostWriteHandler, PosixPostWriteHandler>();

services.AddSingleton<IProcessFeatures, PosixProcessFeatures>();
services.AddHostedService<PosixProcessFeatures>();

// PosixProcessFeatures also needs a IProcessExecutor for gcore
services.AddSingleton(x => new Lazy<IProcessExecutor>(() => x.GetRequiredService<IProcessExecutor>(), true));
Expand Down
22 changes: 14 additions & 8 deletions src/Tgstation.Server.Host/IO/DefaultIOManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,14 +221,7 @@ public string GetDirectoryName(string path) => Path.GetDirectoryName(path ?? thr
/// <inheritdoc />
public async ValueTask<byte[]> ReadAllBytes(string path, CancellationToken cancellationToken)
{
path = ResolvePath(path);
await using var file = new FileStream(
path,
FileMode.Open,
FileAccess.Read,
FileShare.ReadWrite | FileShare.Delete,
DefaultBufferSize,
FileOptions.Asynchronous | FileOptions.SequentialScan);
await using var file = CreateAsyncSequentialReadStream(path);
byte[] buf;
buf = new byte[file.Length];
await file.ReadAsync(buf, cancellationToken);
Expand Down Expand Up @@ -261,6 +254,19 @@ public FileStream CreateAsyncSequentialWriteStream(string path)
FileOptions.Asynchronous | FileOptions.SequentialScan);
}

/// <inheritdoc />
public FileStream CreateAsyncSequentialReadStream(string path)
{
path = ResolvePath(path);
return new FileStream(
path,
FileMode.Open,
FileAccess.Read,
FileShare.ReadWrite | FileShare.Delete,
DefaultBufferSize,
FileOptions.Asynchronous | FileOptions.SequentialScan);
}

/// <inheritdoc />
public Task<IReadOnlyList<string>> GetDirectories(string path, CancellationToken cancellationToken) => Task.Factory.StartNew(
() =>
Expand Down
8 changes: 8 additions & 0 deletions src/Tgstation.Server.Host/IO/IIOManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ public interface IIOManager
/// <param name="path">The path of the file to read.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask"/> that results in the contents of a file at <paramref name="path"/>.</returns>
/// <remarks>This function will fail to read files from the /proc filesystem on Linux.</remarks>
ValueTask<byte[]> ReadAllBytes(string path, CancellationToken cancellationToken);

/// <summary>
Expand All @@ -110,6 +111,13 @@ public interface IIOManager
/// <returns>The open <see cref="FileStream"/>.</returns>
FileStream CreateAsyncSequentialWriteStream(string path);

/// <summary>
/// Creates an asynchronous <see cref="FileStream"/> for sequential reading.
/// </summary>
/// <param name="path">The path of the file to write, will be truncated.</param>
/// <returns>The open <see cref="FileStream"/>.</returns>
FileStream CreateAsyncSequentialReadStream(string path);

/// <summary>
/// Writes some <paramref name="contents"/> to a file at <paramref name="path"/> overwriting previous content.
/// </summary>
Expand Down
8 changes: 6 additions & 2 deletions src/Tgstation.Server.Host/System/IProcessExecutor.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
锘縰sing System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace Tgstation.Server.Host.System
{
Expand All @@ -13,15 +15,17 @@ interface IProcessExecutor
/// <param name="fileName">The full path to the executable file.</param>
/// <param name="workingDirectory">The working directory for the <see cref="IProcess"/>.</param>
/// <param name="arguments">The arguments for the <see cref="IProcess"/>.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <param name="environment">A <see cref="IReadOnlyDictionary{TKey, TValue}"/> of environment variables to set.</param>
/// <param name="fileRedirect">File to write process output and error streams to. Requires <paramref name="readStandardHandles"/> to be <see langword="true"/>.</param>
/// <param name="readStandardHandles">If the process output and error streams should be read.</param>
/// <param name="noShellExecute">If shell execute should not be used. Must be set if <paramref name="readStandardHandles"/> is set.</param>
/// <returns>The new <see cref="IProcess"/>.</returns>
IProcess LaunchProcess(
/// <returns>A <see cref="ValueTask{TResult}"/> resulting in the new <see cref="IProcess"/>.</returns>
ValueTask<IProcess> LaunchProcess(
string fileName,
string workingDirectory,
string arguments,
CancellationToken cancellationToken,
IReadOnlyDictionary<string, string>? environment = null,
string? fileRedirect = null,
bool readStandardHandles = false,
Expand Down
14 changes: 11 additions & 3 deletions src/Tgstation.Server.Host/System/IProcessFeatures.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,31 @@ interface IProcessFeatures
/// <summary>
/// Suspend a given <paramref name="process"/>.
/// </summary>
/// <param name="process">The <see cref="Process"/> to suspend.</param>
/// <param name="process">The <see cref="global::System.Diagnostics.Process"/> to suspend.</param>
void SuspendProcess(global::System.Diagnostics.Process process);

/// <summary>
/// Resume a given suspended <see cref="Process"/>.
/// Resume a given suspended <see cref="global::System.Diagnostics.Process"/>.
/// </summary>
/// <param name="process">The <see cref="Process"/> to suspended.</param>
void ResumeProcess(global::System.Diagnostics.Process process);

/// <summary>
/// Create a dump file for a given <paramref name="process"/>.
/// </summary>
/// <param name="process">The <see cref="Process"/> to dump.</param>
/// <param name="process">The <see cref="global::System.Diagnostics.Process"/> to dump.</param>
/// <param name="outputFile">The full path to the output file.</param>
/// <param name="minidump">If a minidump should be taken as opposed to a full dump.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
ValueTask CreateDump(global::System.Diagnostics.Process process, string outputFile, bool minidump, CancellationToken cancellationToken);

/// <summary>
/// Run events on starting a process.
/// </summary>
/// <param name="process">The <see cref="global::System.Diagnostics.Process"/> that was started.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask{TResult}"/> resulting in the <paramref name="process"/> ID.</returns>
ValueTask<int> HandleProcessStart(global::System.Diagnostics.Process process, CancellationToken cancellationToken);
}
}
104 changes: 102 additions & 2 deletions src/Tgstation.Server.Host/System/PosixProcessFeatures.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
锘縰sing System;
using System.Globalization;
using System.IO;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Mono.Unix;
using Mono.Unix.Native;
Expand All @@ -13,8 +17,18 @@
namespace Tgstation.Server.Host.System
{
/// <inheritdoc />
sealed class PosixProcessFeatures : IProcessFeatures
sealed class PosixProcessFeatures : IProcessFeatures, IHostedService
{
/// <summary>
/// Difference from <see cref="baselineOomAdjust"/> to set our own oom_score_adj to. 1 higher host watchdog.
/// </summary>
const short SelfOomAdjust = 1;

/// <summary>
/// Difference from <see cref="baselineOomAdjust"/> to set the oom_score_adj of child processes to. 1 higher than ourselves.
/// </summary>
const short ChildProcessOomAdjust = SelfOomAdjust + 1;

/// <summary>
/// <see cref="Lazy{T}"/> loaded <see cref="IProcessExecutor"/>.
/// </summary>
Expand All @@ -30,6 +44,11 @@ sealed class PosixProcessFeatures : IProcessFeatures
/// </summary>
readonly ILogger<PosixProcessFeatures> logger;

/// <summary>
/// The original value of oom_score_adj as read from the /proc/ filesystem. Inherited from parent process.
/// </summary>
short baselineOomAdjust;

/// <summary>
/// Initializes a new instance of the <see cref="PosixProcessFeatures"/> class.
/// </summary>
Expand Down Expand Up @@ -88,10 +107,11 @@ public async ValueTask CreateDump(global::System.Diagnostics.Process process, st

string? output;
int exitCode;
await using (var gcoreProc = lazyLoadedProcessExecutor.Value.LaunchProcess(
await using (var gcoreProc = await lazyLoadedProcessExecutor.Value.LaunchProcess(
GCorePath,
Environment.CurrentDirectory,
$"{(!minidump ? "-a " : String.Empty)}-o {outputFile} {process.Id}",
cancellationToken,
readStandardHandles: true,
noShellExecute: true))
{
Expand All @@ -112,5 +132,85 @@ public async ValueTask CreateDump(global::System.Diagnostics.Process process, st
var generatedGCoreFile = $"{outputFile}.{pid}";
await ioManager.MoveFile(generatedGCoreFile, outputFile, cancellationToken);
}

/// <inheritdoc />
public async ValueTask<int> HandleProcessStart(global::System.Diagnostics.Process process, CancellationToken cancellationToken)
{
ArgumentNullException.ThrowIfNull(process);
var pid = process.Id;
try
{
// make sure all processes we spawn are killed _before_ us
await AdjustOutOfMemoryScore(pid, ChildProcessOomAdjust, cancellationToken);
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
logger.LogWarning(ex, "Failed to adjust OOM killer score for pid {pid}!", pid);
}

return pid;
}

/// <inheritdoc />
public async Task StartAsync(CancellationToken cancellationToken)
{
// let this all throw
string originalString;
{
// can't use ReadAllBytes here, /proc files have 0 length so the buffer is initialized to empty
// https://stackoverflow.com/questions/12237712/how-can-i-show-the-size-of-files-in-proc-it-should-not-be-size-zero
await using var fileStream = ioManager.CreateAsyncSequentialReadStream(
"/proc/self/oom_score_adj");
using var reader = new StreamReader(fileStream, Encoding.UTF8, leaveOpen: true);
originalString = await reader.ReadToEndAsync(cancellationToken);
}

var trimmedString = originalString.Trim();

logger.LogTrace("Original oom_score_adj is \"{original}\"", trimmedString);

var originalOomAdjust = Int16.Parse(trimmedString, CultureInfo.InvariantCulture);
baselineOomAdjust = Math.Clamp(originalOomAdjust, (short)-1000, (short)1000);

if (originalOomAdjust != baselineOomAdjust)
logger.LogWarning("oom_score_adj is at it's limit of 1000 (Clamped from {original}). TGS cannot guarantee the kill order of its parent/child processes!", originalOomAdjust);
else
logger.LogWarning("oom_score_adj is at it's limit of 1000. TGS cannot guarantee the kill order of its parent/child processes!");

try
{
// we do not want to be killed before the host watchdog
await AdjustOutOfMemoryScore(null, SelfOomAdjust, cancellationToken);
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
logger.LogWarning(ex, "Could not increase oom_score_adj!");
}
}

/// <inheritdoc />
public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask;

/// <summary>
/// Set oom_score_adj for a given <paramref name="pid"/>.
/// </summary>
/// <param name="pid">The <see cref="global::System.Diagnostics.Process.Id"/> or <see langword="null"/> to self adjust.</param>
/// <param name="adjustment">The value being written to the adjustment file.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
ValueTask AdjustOutOfMemoryScore(int? pid, short adjustment, CancellationToken cancellationToken)
{
var adjustedValue = Math.Clamp(baselineOomAdjust + adjustment, -1000, 1000);

var pidStr = pid.HasValue
? pid.Value.ToString(CultureInfo.InvariantCulture)
: "self";
logger.LogTrace(
"Setting oom_score_adj of {pid} to {adjustment}...", pidStr, adjustedValue);
return ioManager.WriteAllBytes(
$"/proc/{pidStr}/oom_score_adj",
Encoding.UTF8.GetBytes(adjustedValue.ToString(CultureInfo.InvariantCulture)),
cancellationToken);
}
}
}