nixos/mastodon: Allow configuring sidekiq processes #202408
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
teutat3s
suggested changes
Nov 22, 2022
|
this definitely came in handy for me already - I needed to scale up sidekiq on a larger instance that had the local timeline behind by ~5-8 hours, and while increasing raw thread count would have helped, being able to have dedicated processes for local users and federation worked great. without this change, it's pretty hard to spin up additional sidekiq processes adhoc to handle specific queues; several job classes need imagemagick and ffmpeg so you have to make sure those are available etc. all stuff that's already handled in the systemd unit |
|
tests failed after the 2nd last push, sidekiq's getting called with malformed args. investigating |
teutat3s
approved these changes
Nov 23, 2022
|
came back green for me too, I had forgotten to use the new threads variable in the unit definition. thanks! |
|
is there anything else I need to do before this can be merged? this is my first time opening a pr against nixpkgs, so I'm not sure if there's a step I missed. |
|
Can you change ID in systemd logs? Example: diff --git a/nixos/modules/services/web-apps/mastodon.nix b/nixos/modules/services/web-apps/mastodon.nix
index 87079922a38..020a056a3a4 100644
--- a/nixos/modules/services/web-apps/mastodon.nix
+++ b/nixos/modules/services/web-apps/mastodon.nix
@@ -105,7 +105,7 @@ let
sidekiqUnits = lib.attrsets.mapAttrs' (name: processCfg:
lib.nameValuePair "mastodon-sidekiq-${name}" (let
jobClassArgs = lib.concatStringsSep " " (builtins.map (c: "-q ${c}") processCfg.jobClasses);
- jobClassLabel = lib.concatStringsSep " " ([""] ++ processCfg.jobClasses);
+ jobClassLabel = lib.concatStringsSep "-" ([""] ++ processCfg.jobClasses);
threads = toString (if processCfg.threads == null then cfg.sidekiqThreads else processCfg.threads);
in {
after = [ "network.target" "mastodon-init-dirs.service" ]
@@ -126,6 +126,7 @@ let
RestartSec = 20;
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-sidekiq${jobClassLabel}";
# System Call Filtering
SystemCallFilter = [ ("~" + lib.concatStringsSep " " systemCallsList) "@chown" "pipe" "pipe2" ];
} // cfgService;
@@ -618,6 +619,7 @@ in {
serviceConfig = {
Type = "oneshot";
WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-init-dirs";
# System Call Filtering
SystemCallFilter = [ ("~" + lib.concatStringsSep " " (systemCallsList ++ [ "@resources" ])) "@chown" "pipe" "pipe2" ];
} // cfgService;
@@ -662,6 +664,7 @@ in {
Type = "oneshot";
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-init-db";
# System Call Filtering
SystemCallFilter = [ ("~" + lib.concatStringsSep " " (systemCallsList ++ [ "@resources" ])) "@chown" "pipe" "pipe2" ];
} // cfgService;
@@ -690,6 +693,7 @@ in {
RestartSec = 20;
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-streaming";
# Runtime directory and mode
RuntimeDirectory = "mastodon-streaming";
RuntimeDirectoryMode = "0750";
@@ -717,6 +721,7 @@ in {
RestartSec = 20;
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-web";
# Runtime directory and mode
RuntimeDirectory = "mastodon-web";
RuntimeDirectoryMode = "0750";
@@ -732,6 +737,8 @@ in {
serviceConfig = {
Type = "oneshot";
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
+ WorkingDirectory = cfg.package;
+ SyslogIdentifier = "mastodon-media-auto-remove";
} // cfgService;
script = let
olderThanDays = toString cfg.mediaAutoRemove.olderThanDays;
Result: You have an extra space on this line: |
|
Can we add |
|
done. I also moved the working directory config to the common service config. |
|
@vivlim needed remove extra space: And add small fix: diff --git a/nixos/modules/services/web-apps/mastodon.nix b/nixos/modules/services/web-apps/mastodon.nix
index 48e64e1df6f..8659cbe2860 100644
--- a/nixos/modules/services/web-apps/mastodon.nix
+++ b/nixos/modules/services/web-apps/mastodon.nix
@@ -636,10 +636,10 @@ in {
Restart = "always";
RestartSec = 20;
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
+ SyslogIdentifier = "mastodon-streaming";
# Runtime directory and mode
RuntimeDirectory = "mastodon-streaming";
RuntimeDirectoryMode = "0750";
- SyslogIdentifier = "mastodon-streaming";
# System Call Filtering
SystemCallFilter = [ ("~" + lib.concatStringsSep " " (systemCallsList ++ [ "@memlock" "@resources" ])) "pipe" "pipe2" ];
} // cfgService;
@@ -663,10 +663,10 @@ in {
Restart = "always";
RestartSec = 20;
EnvironmentFile = "/var/lib/mastodon/.secrets_env";
+ SyslogIdentifier = "mastodon-web";
# Runtime directory and mode
RuntimeDirectory = "mastodon-web";
RuntimeDirectoryMode = "0750";
- SyslogIdentifier = "mastodon-web";
# System Call Filtering
SystemCallFilter = [ ("~" + lib.concatStringsSep " " systemCallsList) "@chown" "pipe" "pipe2" ];
} // cfgService; |
|
I thought I removed the trailing space in 85a32a465. Are you referring to the space between |
|
Here: |
vivlim
force-pushed
the
mastodon-configure-sidekiq-processes
branch
from
1989b9b to
8f7df65
Compare
|
i squashed the branch to a single commit and force-pushed. |
|
@vivlim with this configuration: Result: Is it possible to replace |
vivlim
force-pushed
the
mastodon-configure-sidekiq-processes
branch
from
8f7df65 to
145a80f
Compare
|
using the |
Sorry, I didn't fully check the PR. Instead of diff --git a/nixos/modules/services/web-apps/mastodon.nix b/nixos/modules/services/web-apps/mastodon.nix
index f8998a002af..61d413c2a51 100644
--- a/nixos/modules/services/web-apps/mastodon.nix
+++ b/nixos/modules/services/web-apps/mastodon.nix
@@ -105,7 +105,7 @@ let
sidekiqUnits = lib.attrsets.mapAttrs' (name: processCfg:
lib.nameValuePair "mastodon-sidekiq-${name}" (let
jobClassArgs = lib.concatStringsSep " " (builtins.map (c: "-q ${c}") processCfg.jobClasses);
- jobClassLabel = lib.concatStringsSep "," ([""] ++ processCfg.jobClasses);
+ jobClassLabel = lib.concatStringsSep "-" ([""] ++ processCfg.jobClasses);
threads = toString (if processCfg.threads == null then cfg.sidekiqThreads else processCfg.threads);
in {
after = [ "network.target" "mastodon-init-dirs.service" ] |
vivlim
force-pushed
the
mastodon-configure-sidekiq-processes
branch
from
145a80f to
3b3917f
Compare
Changed it back to using spaces, it's used in the |
Looked at it now, yes I agree with that. Now it does not affect format in systemd journal. |
13 tasks
|
@happy-river / @erictapen could one of you please take a look at this when you have a moment? |
|
Change |
|
Why? I want to stop churning on the label without having an explicit reason, we have already gone back and forth enough. |
erictapen
requested changes
Dec 5, 2022
There was a problem hiding this comment.
Thanks for your PR and the extensive explanations you provided with it.
I have some minor nits and questions but this looks like it is more or less ready to go. Also there seem to be some merge conflicts remaining.
@Izorkin The changes regarding SyslogIdentifier and mastodon.target didn't have anything to do with the scope of this PR. Please propose changes like this in separate PRs that just tackle one issue at a time, instead of sneaking them in in reviews. Makes the review process easier. For now I'd say the changes are uncontroversial enough to keep them in.
ofborg
bot
added
the
2.status: merge conflict
erictapen
reviewed
Dec 6, 2022
This change allows the number of sidekiq processes and which job classes they handle to be configured. An instance admin may choose to have separate sidekiq processes handling jobs related to local users (`default` job class) and jobs related to federation (`push`, `pull`, `ingress`), so that as the instance grows and takes on more federation traffic, the local users' experience is not as impacted. For more details, see https://docs.joinmastodon.org/admin/scaling/#sidekiq This pr also includes the following changes suggested in review: - adds syslog identifiers for mastodon services - moves working directory config to common cfgService - adds mastodon.target
vivlim
force-pushed
the
mastodon-configure-sidekiq-processes
branch
from
329b927 to
f5470fe
Compare
ofborg
bot
removed
the
2.status: merge conflict
| @@ -48,6 +48,8 @@ let | |||
| # User and group | |||
| User = cfg.user; | |||
| Group = cfg.group; | |||
| # Working directory | |||
There was a problem hiding this comment.
We don't need to repeat the actual code in a comment. Similar existing comments should be dropped, too.
|
Sorry for the delay on this, the earlier churning on labels and such in this PR really sapped my motivation to push this and it hasn’t come back yet. |
erictapen
approved these changes
Apr 6, 2023
|
I just added the assertion I wanted to have, ran the two integration tests and merged, as they were green. Thanks again for your work and the extensive explanations to it, @vivlim. I'm sorry that the review process diminished your motivation. By giving the discussion a second read, I feel like the request for I hope you haven't lost all interest in contributing to the Mastodon module on NixOS, would be glad to review another PR by you or maybe even see you as a maintainer! |
12 tasks
Janik-Haag
added
the
12. first-time contribution
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change allows the number of sidekiq processes and which job classes they handle to be configured.
An instance admin may choose to have separate sidekiq processes handling jobs related to local users (
defaultjob class) and jobs related to federation (push,pull,ingress), so that as the instance grows and takes on more federation traffic, the local users' experience is not as impacted.For more details, see https://docs.joinmastodon.org/admin/scaling/#sidekiq
Description of changes
I added a config setting to allow users to define any number of sidekiq processes that they want to run in separate systemd units.
As a side effect, the sidekiq unit started by the default config was renamed from
mastodon-sidekiqtomastodon-sidekiq-all. I updated the related test to match. Let me know if you have a preference for the base unit to not be renamed, and if you have any suggestions on idiomatic ways to to derive its name from an empty string key without appending a trailing-.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes