Skip to content

WPB-28377: migrate gundeck presence from redis to PostGreSQL - #5493

Merged
blackheaven merged 7 commits into
developfrom
gdifolco/WPB-28377-cannon-postgresql
Sep 3, 2026
Merged

WPB-28377: migrate gundeck presence from redis to PostGreSQL#5493
blackheaven merged 7 commits into
developfrom
gdifolco/WPB-28377-cannon-postgresql

Conversation

@blackheaven

Copy link
Copy Markdown
Contributor

https://wearezeta.atlassian.net/browse/WPB-28377

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@blackheaven
blackheaven requested review from a team as code owners August 28, 2026 11:17
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 28, 2026
@blackheaven
blackheaven force-pushed the gdifolco/WPB-28377-cannon-postgresql branch 3 times, most recently from 6fa7155 to de7d9d6 Compare August 28, 2026 13:54
Comment on lines +16 to +23


postgresql:
host: postgres
port: "5432"
user: wire-server
dbname: backendA
password: posty-the-gres

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not right, the federation-v0 code still needs redis.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restored the redis config and services for the federation docker-compose parties (v0 and v1, and also v2 which had the same problem — all three run released wire-server images whose gundeck still requires the redis block and a redis server), plus the redis values block for the fed-v0 helm party. Byte-identical to what develop had.

Comment on lines +16 to +23


postgresql:
host: postgres
port: "5432"
user: wire-server
dbname: backendA
password: posty-the-gres

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same as federation-v0, this still needs redis.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same fix: redis service + config restored (byte-identical to develop). v2 restored as well since it has the identical problem.

Comment on lines +208 to +213
postgresql:
host: postgresql
port: "5432"
user: wire-server
dbname: backendA
password: posty-the-gres

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as docker setup, this would still require redis.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Restored: redis-v2 service in federation-v2.yaml and the original redis block (including its TLS settings) in federation-v2/gundeck.yaml, verbatim from develop. Also restored deploy/dockerephemeral/docker/redis-ca.pem which the tlsCa path references.

conn_id text NOT NULL,
resource text NOT NULL,
client_id text,
created_at bigint NOT NULL,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not use timstamptz?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to timestamptz: the migration now declares created_at timestamptz, and Gundeck.Presence.Data passes/read UTCTime with :: timestamptz statement annotations (wire format stays epoch-millis; conversion helpers msToUtc/utcToMs are exact — milliseconds nest inside timestamptz microseconds). Edited in place since it's unreleased.

deleteStale =
[resultlessStatement|
DELETE FROM presence
WHERE created_at < ($1 :: int8)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should create an index for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added: CREATE INDEX presence_created_at_idx ON presence (created_at) in the same migration (naming follows the conversation_codes_expires_at_idx precedent); it backs the weekly cleanup delete.

Comment on lines +88 to +90
statement
(toUUID (userId p), connIdText (connId p), fromIntegral (ms (createdAt p)))
deleteOne

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sending n statements is not good. Can we do this in one query for all the presences?
Makes me wonder if we should have a "presence id", so we can precisely delete the presences for which cannon says they're not actually present. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done: deleteAll is now a single statement — DELETE FROM presence p USING unnest($1 :: uuid[], $2 :: text[], $3 :: timestamptz[]) AS d (...) WHERE ... AND p.created_at <= d.created_at — so it's one round trip while keeping the per-row compare-and-delete guard (a newer re-registration with the same conn id must not be deleted by a stale disconnect; empty input still short-circuits).

On the presence id: I'd rather not, at least not in this PR. Cannon already knows exactly which (user_id, conn_id) pairs dropped, and that pair is the row's primary key — so a surrogate id wouldn't add precision cannon doesn't already have. The reconnect race (same conn id re-registered between gundeck's presence list and cannon's negative-ack) is guarded by the created_at compare-and-delete, the same semantics the redis implementation had. A presence id would also change Wire.API.Presence (wire format + golden schema + cannon interop) for no behavioral gain. Happy to follow up separately if you still want it.

@blackheaven
blackheaven force-pushed the gdifolco/WPB-28377-cannon-postgresql branch from c4cb60e to 586fac9 Compare September 1, 2026 09:17
- Restore redis config/services for the federation docker-compose parties
  (v0/v1/v2) and the fed-v0 helm values: those parties run released
  wire-server images whose gundeck still requires a redis server.
- presence.created_at: bigint epoch millis -> timestamptz; boundary
  conversion in Gundeck.Presence.Data via msToUtc/utcToMs (exact, no
  second truncation). Migration edited in place (unreleased).
- Add presence_created_at_idx to back the weekly cleanup delete.
- Replace the per-presence delete loop with a single
  DELETE ... USING unnest(...) statement, keeping the per-row
  created_at <= compare-and-delete guard (reconnect-race safety).
- docker-compose.yaml: drop the now-empty top-level volumes key left
  by the redis-cluster removal (broke compose parsing).
- Regenerate postgres-schema.sql (timestamptz column + new index).
@blackheaven
blackheaven force-pushed the gdifolco/WPB-28377-cannon-postgresql branch from 586fac9 to 2247a19 Compare September 3, 2026 00:10

@akshaymankar akshaymankar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, nits in comments.

Comment on lines +14 to +15
- Presence data does not carry over: the presence cache starts empty on
upgrade and refills as clients reconnect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- Presence data does not carry over: the presence cache starts empty on
upgrade and refills as clients reconnect.
- Restart all cannons after deployment is successful. Presence data does not carry over, this will make sure all client reconnect after the presence data is being written to Postgresql

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in fa9fc3e — applied the suggestion, with the double space collapsed and "Postgresql" spelled as on line 1 of the file.

following the same format as brig's postgresql settings.
- Presence data does not carry over: the presence cache starts empty on
upgrade and refills as clients reconnect.
- The `redis-ephemeral` and `reaper` helm charts have been removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- The `redis-ephemeral` and `reaper` helm charts have been removed.
- Stop deploying `redis-ephemeral` and `reaper`, these have been removed.

This way it goes with the "Operators must:" heading.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in fa9fc3e — applied the suggestion.

Comment on lines +104 to +106
nowMs <- posixTime
let cutoff = msToUtc (fromIntegral (ms nowMs - 7 * 24 * 60 * 60 * 1000))
runPool $ statement cutoff deleteStale

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could just leave all time calculations to postgres, using the now() function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in fa9fc3edeleteStale is now parameterless and computes the cutoff as created_at < now() - interval '7 days', which is exactly the query shape already named in the index comment of 20260828093750-gundeck-presence.sql. cleanup no longer computes any time application-side.

@blackheaven
blackheaven merged commit 9a39b9c into develop Sep 3, 2026
10 checks passed
@blackheaven
blackheaven deleted the gdifolco/WPB-28377-cannon-postgresql branch September 3, 2026 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants