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

nixos/gitlab: ensure service started again after dependency restarts #245240

Conversation

osnyx
Copy link
Contributor

@osnyx osnyx commented Jul 24, 2023

Upstreaming of flyingcircusio/fc-nixos#707 which is running fine in production for several months now.

Description of changes

When a dependency, like postgresql.service or redis-gitlab.service, had been stopped and started at switch-to-configuration time, gitlab.service and its helper units had been stopped but not started again:
gitlab.service has a BindsTo=postgresql.service dependency. This means, that it gets stopped when its dependency service postgresql is stopped. But there is no dependency pulling it back to be started again.
multi-user.target only has a Wants relation to gitlab.target, but once gitlab.target has been successfully started once and is not stopped/ restarted again, it does not cause all its dependencies to stay activated the whole time.

This commit fixes this by upgrading the dependy relationship of gitlab.service towards gitlab.target from a "Wants" to a "Requires". It should be enough to do this for this single unit part of gitlab.target only, as all other units wantedBy gitlab.target are pulled in by gitlab.service as well or have bindsTo relations.

Let me know whether I shall add test coverage for the case of dependency stop and start. Constructing a case where a dependency is stopped and started can certainly be done in a NixOS VM test, but I'd like to know whether such an additional test case is desired before approaching thos and potentially blowing up the test runtime any further.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • not significant enough
  • Fits CONTRIBUTING.md.

When a dependency, like postgresql.service or redis-gitlab.service, had
been stopped and started at switch-to-configuration time, gitlab.service
and its helper units had been stopped but not started again.
`multi-user.target` only has a `Wants` relation to gitlab.target, but
once gitlab.target has been successfully started once and is not stopped/
restarted again, it does not cause all its dependencies to stay activated
the whole time.

This commit fixes this by upgrading the dependy relationship of
gitlab.service towards gitlab.target from a "Wants" to a "Requires". It
should be enough to do this for this single unit part of gitlab.target
only, as all other units wantedBy gitlab.target are pulled in by
gitlab.service as well or have bindsTo relations.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 24, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 24, 2023
Copy link
Member

@n0emis n0emis left a comment

Choose a reason for hiding this comment

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

this issue has been bugging me for a while, but I never got around to looking for the issue. thanks for fixing this 👍🏼

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jul 29, 2023
Copy link
Member

@yayayayaka yayayayaka left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Thank you for figuring it out ❤️

@yu-re-ka yu-re-ka merged commit ff9296f into NixOS:master Jul 30, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Successfully created backport PR for release-23.05:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants