[DPE-4772] Subordinate scale up #274
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #274 +/- ##
==========================================
+ Coverage 70.96% 71.13% +0.16%
==========================================
Files 7 7
Lines 1195 1202 +7
Branches 210 211 +1
==========================================
+ Hits 848 855 +7
Misses 269 269
Partials 78 78 ☔ View full report in Codecov by Sentry. |
src/relations/peers.py
Outdated
| if self.unit_databag.get("auth_file_set", "false") == "true" and self.unit_databag.get( | ||
| "backend_relation" | ||
| ): | ||
| self.charm.client_relation.set_ready() |
There was a problem hiding this comment.
At this point we should have a backend relation, rendered user list and config, so unit should be ready
| @@ -168,6 +179,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: | |||
| return | |||
|
|
|||
| self.charm.render_pgb_config(reload_pgbouncer=True) | |||
| self.set_ready() | |||
There was a problem hiding this comment.
Setting the leader ready immediately, so we don't have to wait in a single unit cluster.
| @@ -107,6 +111,9 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None: | |||
| Deferrals: | |||
| - If backend relation is not fully initialised | |||
| """ | |||
| rel_id = event.relation.id | |||
| self.database_provides.set_subordinated(rel_id) | |||
There was a problem hiding this comment.
Set the app level flag. There's other synchronisation for existing units, so it should be safe even if not immediately set.
| @@ -181,6 +181,7 @@ def _on_database_created(self, event: DatabaseCreatedEvent) -> None: | |||
| self.charm.render_pgb_config(reload_pgbouncer=True) | |||
| self.charm.render_prometheus_service() | |||
| self.charm.update_status() | |||
| self.charm.client_relation.set_ready() | |||
There was a problem hiding this comment.
We should have the peer data and secret here already, so no need to retrigger the peer changed hook.
| def set_ready(self) -> None: | ||
| """Marks the unit as ready for all database relations.""" | ||
| for relation in self.model.relations[self.relation_name]: | ||
| relation.data[self.charm.unit].update({"state": "ready"}) |
There was a problem hiding this comment.
Could we use a self.database_provides.update_relation_data(relation.id, {"state": "ready"}) helper from data_interfaces here?
I prefer to avoid direct modification of databag.
There was a problem hiding this comment.
update_relation_data() should be setting app level data, we need this in the unit databag. We'll need another helper for that.
There was a problem hiding this comment.
Possibly I can use _update_relation_data_without_secrets() but that's private.
There was a problem hiding this comment.
No, we can't use any of those. Cross-charm relations (incl. all related internal functions) are restricted to app data.
Only peer relations support interface functions to access unit data at the moment.
| if isinstance(key, Unit) and not key.name.startswith(self.charm.app.name): | ||
| remote_unit_data = event.relation.data[key] | ||
| elif isinstance(key, Application) and key.name != self.charm.app.name: | ||
| is_subordinate = event.relation.data[key].get("subordinated") == "true" |
There was a problem hiding this comment.
Huh, this feels kinda hacky... I get the rationale, but I think we should have a proper "frame" around this.
The DatabaseProviderEventHander -- up to this point was not supposed to directly dig in databags.
We are reading an manipulating relation data via the DatabaseProviderEventHander.relation_data... Which --as for now-- strictly mean app level data.
We can alter the existing workflows to be ready to handle unit level data as well.
However that should go alongside considerations similar to the existing ones across the module (not just direct access in various parts of the code).
I could go with this solution as a temporary one, and I'm glad to volunteer to introduce the missing concept of handling unit level data in the libs. At that point this code will be cleaned up.
There was a problem hiding this comment.
Let's keep it as is for the time being.
Implements a synchronisation mechanism so that scaled up units will only trigger
database_createdwhen pgbouncer is ready to accept connections.