[DPE-3562] Don't block on missing Postgresql version #626
Conversation
| application_relation_databag.update(updates) | ||
| unit_relation_databag.update(updates) |
There was a problem hiding this comment.
So that that mccabe doesn't complain.
| except ( | ||
| PostgreSQLCreateDatabaseError, | ||
| PostgreSQLCreateUserError, | ||
| PostgreSQLGetPostgreSQLVersionError, | ||
| ): | ||
| self.charm.unit.status = BlockedStatus( |
There was a problem hiding this comment.
We should most probably defer here. Should I change this as well?
There was a problem hiding this comment.
I think a defer would be good here. We just need to ensure that we can change the status to Active later when the database/user creation succeeds (which is what we are already missing today).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 70.71% 70.66% -0.05%
==========================================
Files 11 11
Lines 2926 2932 +6
Branches 511 512 +1
==========================================
+ Hits 2069 2072 +3
- Misses 748 750 +2
- Partials 109 110 +1 ☔ View full report in Codecov by Sentry. |
| postgresql_version = None | ||
| try: | ||
| postgresql_version = self.charm.postgresql.get_postgresql_version() | ||
| except PostgreSQLGetPostgreSQLVersionError: | ||
| logger.exception( | ||
| "Failed to retrieve the PostgreSQL version to initialise/update %s relation" | ||
| % self.relation_name | ||
| ) | ||
|
|
There was a problem hiding this comment.
K8S charm will not update the endpoints as aggressively as VM. This will be reran only on config change. Should I try add something to update status?
There was a problem hiding this comment.
Is it a time for Pebble Notices and stop relying update-status?
Up 2 Marcelo here.
There was a problem hiding this comment.
If it's possible to avoid calls to sleep or similar things on the notice event received by the charm (if we need to notify again from inside that event handler because the database version couldn't still be retrieved), I'm fine with using Pebble notices for that. Otherwise, I'd keep it on update status for now.
| postgresql_version = None | ||
| try: | ||
| postgresql_version = self.charm.postgresql.get_postgresql_version() | ||
| except PostgreSQLGetPostgreSQLVersionError: | ||
| logger.exception( | ||
| "Failed to retrieve the PostgreSQL version to initialise/update %s relation" | ||
| % self.relation_name | ||
| ) | ||
|
|
There was a problem hiding this comment.
Is it a time for Pebble Notices and stop relying update-status?
Up 2 Marcelo here.
| except ( | ||
| PostgreSQLCreateDatabaseError, | ||
| PostgreSQLCreateUserError, | ||
| PostgreSQLGetPostgreSQLVersionError, | ||
| ): | ||
| self.charm.unit.status = BlockedStatus( |
There was a problem hiding this comment.
I think a defer would be good here. We just need to ensure that we can change the status to Active later when the database/user creation succeeds (which is what we are already missing today).
| if ( | ||
| not self.set_up_relation(event.relation) | ||
| and self.charm.unit.status.message | ||
| == f"Failed to initialize {self.relation_name} relation" | ||
| ): | ||
| event.defer() | ||
| return |
There was a problem hiding this comment.
Charm should be unblocked by _update_unit_status if set_up_relation succeeds.
Port of canonical/postgresql-operator#578
Don't block the relation if Postgresql can't get the database version.