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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust staging environment configuration #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bdach
Copy link
Contributor

@bdach bdach commented Jan 10, 2025

No description provided.

@bdach bdach requested a review from ThePooN January 10, 2025 09:21
@bdach bdach self-assigned this Jan 10, 2025
builder.Services.AddTransient<BeatmapPackagePatcher>();
builder.Services.AddHttpClient();
builder.Services.AddTransient<ILegacyIO, LegacyIO>();
builder.Services.AddTransient<IMirrorService, NoOpMirrorService>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns off purge requests to beatmap mirrors - see implementation:

if (await performMirrorAction(mirror, "purge", new Dictionary<string, string> { ["s"] = beatmapSetId.ToString() }) != "1")

Wasn't sure whether it was fine to keep this from production config or not, so I turned it off. Whether that is a valid setup depends on how the beatmap mirror setup is going to work. If the local beatmap storage is going to use the mirror's cache directory as the storage path, then this is required to make that setup work; otherwise, it depends on the specifics of how the mirror is going to function.

Copy link
Member

@ThePooN ThePooN Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think it's better if applications have a single deployment config for consistency with everything else. More env vars can be used for further tuning (eg. turn off purging).

I'm not sure if a variable to turn off purging is required though, as that should never happen when using local storage (as that would always be a single shared volume), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if a variable to turn off purging is required though, as that should never happen when using local storage (as that would always be a single shared volume), right?

Not completely sure what "single shared volume" you're referring to there... Maybe I'll elaborate further on the logic here:

The purge operation on the beatmap mirror removes the .osz from the beatmapCache folder in the mirror folder structure (see https://github.com/peppy/osu-web-10/blob/00da7635a49a0edb7048a2ed924d726c5b8a7e56/www/web/d.php#L60-L68). Now if that cache folder is independent from whatever the primary/master beatmap storage is (which is S3 in production), then it's whatever, that call can be executed no problem, nothing needs to be disabled.

However, if for whatever reason (convenience, storage savings) the primary/master beatmap storage is symlinked/aliased/the same location as the beatmap cache, then the purge logic must be disabled, because every map would just get deleted from master after it's uploaded.

It's all a question of how you'd see the mirror functioning on staging I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no S3 in staging. How would the beatmap mirror retrieve the osz file if it's not in the same volume?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would the beatmap mirror retrieve the osz file if it's not in the same volume?

Yes well that is the question here kinda isn't it 😅

There are three options I can think of:

  1. Modify d.php on the staging mirror so that it just serves beatmaps from osu-server-beatmap-submission's local storage every time and delete all of the caching logic. In that case the purge calls will become no-ops on the mirror side, so they may as well stay.
  2. Modify d.php just for staging so that it can hit osu-server-beatmap-submission's local storage for the .osz rather than s3, and copy it from there instead of downloading it from s3. In that case the purge calls can stay, but there's a bit of a hitch in that there are now potentially 2 copies of the .osz on staging. Maybe fine, maybe not.
  3. Delete s3 logic from d.php completely and just have www/beatmapCache of the mirror be the same directory as osu-server-beatmap-submission's local storage via mounting / symlinking / whatever. The local beatmap storage has a file structure compatible with the beatmap cache's, so if you can point both osu-server-beatmap-submission's local storage and the beatmap mirror's cache at the same directory, things will just work without copying. But if you do that, then the purging logic has to be turned off for reasons described above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a think about this.

builder.Services.AddTransient<BeatmapPackagePatcher>();
builder.Services.AddHttpClient();
builder.Services.AddTransient<ILegacyIO, LegacyIO>();
builder.Services.AddTransient<IMirrorService, NoOpMirrorService>();
Copy link
Member

@ThePooN ThePooN Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think it's better if applications have a single deployment config for consistency with everything else. More env vars can be used for further tuning (eg. turn off purging).

I'm not sure if a variable to turn off purging is required though, as that should never happen when using local storage (as that would always be a single shared volume), right?


if (AppSettings.DatadogAgentHost == null)
{
throw new InvalidOperationException("DD_AGENT_HOST environment variable not set. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use ddog on staging 💸

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger, made optional in 8d398e1. Note that after that, if the envvar is not set, ddog will not be set up:

if (AppSettings.DatadogAgentHost != null)
{
DogStatsd.Configure(new StatsdConfig
{
StatsdServerName = AppSettings.DatadogAgentHost,
Prefix = "osu.server.beatmap-submission",
ConstantTags = new[]
{
$@"hostname:{Dns.GetHostName()}",
$@"startup:{DateTimeOffset.UtcNow.ToUnixTimeSeconds()}"
}
});
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants