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/matrix-synapse: drop old DB check assertion, actually require DB to be up #259980

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Oct 9, 2023

Description of changes

Closes #236062

The PR #236062 was submitted because of the following problem: a synapse instance was running in a NixOS container attached to the host network and a postgresql instance on the host as database. In this setup, synapse connected to its DB via 127.0.0.1, but the DB wasn't locally set up and thus not configured in NixOS (i.e.
config.services.postgresql.enable was false). This caused the assertion removed in this patch to fail.

Over three years ago this assertion was introduced when this module stopped doing autoconfiguration of postgresql entirely[1] because a breaking change in synapse couldn't be managed via an auto-upgrade on our side. To make sure people don't deploy their DB away by accident, this assertion was introduced.

Nowadays this doesn't serve any value anymore because people with existing instances should've upgraded by now (otherwise it's their job to carefully read the release notes when missing upgrades for several years) and people deploying fresh instances are instructed by the docs to also configure postgresql[2].

Instead, it only causes issues in corner cases like #236062, so after some discussion in that PR I think it's time to remove the assertion altogether.

Also, there's no Requires= for postgresql.service in the systemd units which means that it's not strictly guaranteed that the DB is up when synapse starts up. This is fixed now by adding requires. To avoid being bitten by above mentioned cases again, this only happens if config.services.postgresql.enable is true.

If somebody uses a non-local postgresql, but has also deployed a local postgresql instance on the synapse server (rather unlikely IMHO), it's their job to opt out of this behavior with mkForce (this is precisely one of the use-cases mkForce and friends were built for IMHO).

[1] #80447
[2] https://nixos.org/manual/nixos/stable/#module-services-matrix-synapse

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)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

…B to be up

Closes NixOS#236062

The PR NixOS#236062 was submitted because of the following problem: a synapse
instance was running in a NixOS container attached to the host network
and a postgresql instance on the host as database. In this setup,
synapse connected to its DB via 127.0.0.1, but the DB wasn't locally set
up and thus not configured in NixOS (i.e.
`config.services.postgresql.enable` was `false`). This caused the
assertion removed in this patch to fail.

Over three years ago this assertion was introduced when this module
stopped doing autoconfiguration of postgresql entirely[1] because a
breaking change in synapse couldn't be managed via an auto-upgrade on
our side. To make sure people don't deploy their DB away by accident,
this assertion was introduced.

Nowadays this doesn't serve any value anymore because people with
existing instances should've upgraded by now (otherwise it's their job
to carefully read the release notes when missing upgrades for
several years) and people deploying fresh instances are instructed by
the docs to also configure postgresql[2].

Instead, it only causes issues in corner cases like NixOS#236062, so after
some discussion in that PR I think it's time to remove the assertion
altogether.

Also, there's no `Requires=` for `postgresql.service` in the systemd
units which means that it's not strictly guaranteed that the DB is up
when synapse starts up. This is fixed now by adding `requires`. To avoid
being bitten by above mentioned cases again, this only happens if
`config.services.postgresql.enable` is `true`.

If somebody uses a non-local postgresql, but has also deployed a local
postgresql instance on the synapse server (rather unlikely IMHO), it's
their job to opt out of this behavior with `mkForce` (this is precisely one
of the use-cases `mkForce` and friends were built for IMHO).

[1] NixOS#80447
[2] https://nixos.org/manual/nixos/stable/#module-services-matrix-synapse
@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 Oct 9, 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 Oct 9, 2023
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 9, 2023
@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 9, 2023
@Ma27 Ma27 merged commit 3d3b99a into NixOS:master Oct 10, 2023
23 checks passed
@Ma27 Ma27 deleted the synapse-db-assertion branch October 10, 2023 09:41
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.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants