[DPE-4595] Immediate connection on database created #257
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #257 +/- ##
==========================================
+ Coverage 70.48% 70.79% +0.31%
==========================================
Files 7 7
Lines 1162 1178 +16
Branches 205 209 +4
==========================================
+ Hits 819 834 +15
- Misses 266 268 +2
+ Partials 77 76 -1 ☔ View full report in Codecov by Sentry. |
src/relations/pgbouncer_provider.py
Outdated
| self.charm.render_pgb_config(reload_pgbouncer=True) | ||
|
|
There was a problem hiding this comment.
This is enough to fix the immediate issue, but it won't work if we have multiple client units.
There was a problem hiding this comment.
... but it won't work if we have multiple client units.
It is subordinate... 1:1 relation here. Nope?
There was a problem hiding this comment.
PGB will be 1:1 for the client units, but it's not a given that we'll have a single client unit. Say we have a client with two units set up like so: c0→pgb0 and c1→pgb1, when establishing the relation pgb1 will create the user and db, update the db mapping, reload pgb and trigger a database_created event for the client. While pgb1 may be good to go, at the time database_created triggers, pgb0 may not yet be fully set up, it may not have synced up the dbs from peer data, it may still be setuping the backend relation. If c0 tries to use pgb0 while it's still setting up, the connection will error out.
There was a problem hiding this comment.
Is this fix related to #245?
There was a problem hiding this comment.
Is this fix related to #245?
Yes
src/relations/pgbouncer_provider.py
Outdated
| for key in self.charm.peers.relation.data.keys(): | ||
| if key != self.charm.app: | ||
| if self.charm.peers.relation.data[key].get("auth_file_set", "false") != "true": | ||
| logger.info("Backend credentials not yet set on all units") | ||
| event.defer() | ||
| return | ||
|
|
There was a problem hiding this comment.
Make sure all the units have the userlist rendered.
src/relations/pgbouncer_provider.py
Outdated
| def _on_relation_changed(self, event: RelationChangedEvent) -> None: | ||
| """Injects the database name for follower units.""" | ||
| if not self.charm.unit.is_leader(): | ||
| if "database" in event.relation.data[event.app]: | ||
| database = event.relation.data[event.app]["database"] | ||
| dbs = self.charm.generate_relation_databases() | ||
| if database not in [db["name"] for db in dbs]: | ||
| self.charm.render_pgb_config(reload_pgbouncer=True, inject_db=database) | ||
|
|
There was a problem hiding this comment.
Bypassing the lib to inject the requested database in follower pgbouncers. This us needed because _on_database_requested will only fire on the leader and followers will need to wait for peer data hooks to fire to catch up. Alternatively, I can try to checksum the dblists in peer data and defer _on_database_requested until all units are in sync. Thoughts?
There was a problem hiding this comment.
IMO, it's better to wait for all units to be in sync because this event can still be in the follower unit queue (not executed yet) while the leader has already set the connection info in the client relation data.
There was a problem hiding this comment.
Another important thing for the current implementation of this PR, in my opinion, is to ensure that other calls to render_pgb_config in the follower units are safe and don't remove the injected database after it was added at this point. I mean the situation where this call injects the database, another queued event calls render_pgb_config, which removes the injected database, and later on, the follower unit receives the peer-relation-changed event (because the leader took some time to set the peer relation data).
There was a problem hiding this comment.
I'll give it a go to sync configs.
There was a problem hiding this comment.
Reworked to hash db list and defer until all units are in sync.
|
This seems to cause instability on juju 2 scale up, but I want early feedback since the changes are aggressive. |
src/relations/pgbouncer_provider.py
Outdated
| self.charm.render_pgb_config(reload_pgbouncer=True) | ||
|
|
There was a problem hiding this comment.
... but it won't work if we have multiple client units.
It is subordinate... 1:1 relation here. Nope?
* Sync configs between units * Remove readonly db hash
Issue
Pgbouncer is not set up to receive connections immediately when publishing credentials
Solution
Sync up configuration and reload PGB when establishing the interface
Related to #245