nixos.gitlab: loosen the coupling of gitlab services to postgresql and redis #286532
Conversation
This file contains 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
|
This currently makes the gitlab tests fail, I could use some help figuring out how to resolve this. gitaly fails to restart due to a missing |
ofborg
bot
added
10.rebuild-darwin: 0
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
talyz
reviewed
Feb 6, 2024
talyz
reviewed
Feb 6, 2024
talyz
reviewed
Feb 6, 2024
talyz
requested changes
Feb 6, 2024
There was a problem hiding this comment.
Sounds reasonable. Sorry about the amount of separate reviews. I have apparently forgotten how to do this properly on GitHub.. Lastly, though:
The tests fail because gitlab-config.service isn't properly stopped before it's started again. The issue seems to be that stopping a target, in this case gitlab.target, is not a blocking operation. It probably had time to stop when more had to be done before postgresql.service could be stopped, and therefore this wasn't apparent. Adding gitlab-config.service to the list of units to stop before removing the database in the gitlab test solves this issue. Here's a patch:
--- a/nixos/tests/gitlab.nix
+++ b/nixos/tests/gitlab.nix
@@ -419,7 +419,7 @@ in {
gitlab.systemctl("start gitlab-backup.service")
gitlab.wait_for_unit("gitlab-backup.service")
gitlab.wait_for_file("${nodes.gitlab.services.gitlab.statePath}/backup/dump_gitlab_backup.tar")
- gitlab.systemctl("stop postgresql.service gitlab.target")
+ gitlab.systemctl("stop postgresql.service gitlab-config.service gitlab.target")
gitlab.succeed(
"find ${nodes.gitlab.services.gitlab.statePath} -mindepth 1 -maxdepth 1 -not -name backup -execdir rm -r {} +"
)
osnyx
force-pushed
the
PL-131811-gitlab-loose-coupling-upstream
branch
from
45af7df to
85423d5
Compare
|
@talyz Thanks for catching the bug in the test. The binding of gitlab-config to gitlab-db-config apparently was aimed in the right direction, as I did that to ensure gitlab-config is stopped when gitlab is stopped after backup and thus ready to be started again and initialise files at the next start. |
|
Regarding release notes, I consider this to just be a change in the under-the-hood mechanics and not worth a mention there. |
12 tasks
…s/ redis to avoid restarts and races Gitlab stays running at redis and postgresql restarts as if these components were on a different host anyways. Handling reconnetctions is part of the application logic. Co-authored-by: Kim Lindberger <[email protected]> for formatting fixes and test failure debugging.
osnyx
force-pushed
the
PL-131811-gitlab-loose-coupling-upstream
branch
from
85423d5 to
13ba002
Compare
|
Tests are now passing for me as well. |
talyz
approved these changes
Feb 7, 2024
|
Successfully created backport PR for |
1 task
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.
Description of changes
This reduces the strong degree of coupling between Gitlab and its supporting 3rd party services redis and postgresql.
It's a potential solution for #286530.
Background:
When Gitlab and its database postgresql are running on the same system, its systemd services are strongly coupled via
binds-torelations to ensure start/stop/restart of the individual services propagates to the other services.We already had some slight issues with this in the past and fixed the reliability of coupled restarts in #245240. But since then, we still have seen some additional hard-to-reproduce race conditions that show the approach of strongly-coupled service restarts is brittle here. In one case when postgresql needed to restart, Gitlab was shut down while trying to start and it stayed inactive.
As a consequence, I propose to drop the approach of strong coupling between these services altogether and let the services postgresql or redis restart without forcing a restart of gitlab.service. This just causes the running service to temporarily loose its database connections, but regain them after a successful restart of the db services. Given the long startup times of gitlab.service, this actually even reduces the downtime caused by a database restart in many cases.
Reasoning/ why this decoupling is fine:
By design, both redis and postgresql can be hosted on another host than the gitlab service itself. The NixOS module actually supports this for postgresql, but not for redis (as a dependency of sidekiq). Still, Gitlab and Sidekiq are designed to cope with temporary unavailabilities of redis and postgresql and do recover from such situations automatically.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.