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.

Sign up for GitHub

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

[DPE-3562] Don't block on missing Postgresql version #626

Merged
merged 5 commits into from
Aug 19, 2024
Merged

[DPE-3562] Don't block on missing Postgresql version #626

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dc3c1e
Don't block on missing Postgresql version
dragomirp Aug 13, 2024
d711468
Merge branch 'main' into dpe-3562-blocking-legacy-relation
dragomirp Aug 16, 2024
74dbdc4
Defer on failure to create db resources
dragomirp Aug 16, 2024
97c6b46
Fix lint
dragomirp Aug 16, 2024
e05981f
Bump libs
dragomirp Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/charms/postgresql_k8s/v0/postgresql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 33
LIBPATCH = 34

INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles"

Expand Down Expand Up @@ -425,14 +425,20 @@ def get_postgresql_timezones(self) -> Set[str]:
timezones = cursor.fetchall()
return {timezone[0] for timezone in timezones}

def get_postgresql_version(self) -> str:
def get_postgresql_version(self, current_host=True) -> str:
"""Returns the PostgreSQL version.

Returns:
PostgreSQL version number.
"""
if current_host:
host = self.current_host
else:
host = None
try:
with self._connect_to_database() as connection, connection.cursor() as cursor:
with self._connect_to_database(
database_host=host
) as connection, connection.cursor() as cursor:
cursor.execute("SELECT version();")
# Split to get only the version number.
return cursor.fetchone()[0].split(" ")[1]
Expand Down
48 changes: 27 additions & 21 deletions src/relations/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,9 @@ def set_up_relation(self, relation: Relation) -> bool:
self.charm.unit.status = BlockedStatus(ROLES_BLOCKING_MESSAGE)
return False

database = relation.data.get(relation.app, {}).get("database")
if not database:
if not (database := relation.data.get(relation.app, {}).get("database")):
for unit in relation.units:
unit_database = relation.data.get(unit, {}).get("database")
if unit_database:
database = unit_database
if database := relation.data.get(unit, {}).get("database"):
break

if not database:
Expand Down Expand Up @@ -207,31 +204,40 @@ def set_up_relation(self, relation: Relation) -> bool:
)
)

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
)

Comment on lines +213 to +221
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a time for Pebble Notices and stop relying update-status?
Up 2 Marcelo here.

Copy link
Member

Choose a reason for hiding this comment

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

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.

# Set the data in both application and unit data bag.
# It's needed to run this logic on every relation changed event
# setting the data again in the databag, otherwise the application charm that
# is connecting to this database will receive a "database gone" event from the
# old PostgreSQL library (ops-lib-pgsql) and the connection between the
# application and this charm will not work.
for databag in [application_relation_databag, unit_relation_databag]:
updates = {
"allowed-subnets": self._get_allowed_subnets(relation),
"allowed-units": self._get_allowed_units(relation),
"host": self.charm.endpoint,
"master": primary,
"port": DATABASE_PORT,
"standbys": standbys,
"version": self.charm.postgresql.get_postgresql_version(),
"user": user,
"password": password,
"database": database,
"extensions": ",".join(required_extensions),
}
databag.update(updates)
updates = {
"allowed-subnets": self._get_allowed_subnets(relation),
"allowed-units": self._get_allowed_units(relation),
"host": self.charm.endpoint,
"master": primary,
"port": DATABASE_PORT,
"standbys": standbys,
"user": user,
"password": password,
"database": database,
"extensions": ",".join(required_extensions),
}
if postgresql_version:
updates["version"] = postgresql_version
application_relation_databag.update(updates)
unit_relation_databag.update(updates)
Comment on lines +242 to +243
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that that mccabe doesn't complain.

except (
PostgreSQLCreateDatabaseError,
PostgreSQLCreateUserError,
PostgreSQLGetPostgreSQLVersionError,
):
self.charm.unit.status = BlockedStatus(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should most probably defer here. Should I change this as well?

Copy link
Member

Choose a reason for hiding this comment

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

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).

f"Failed to initialize {self.relation_name} relation"
Expand Down
26 changes: 10 additions & 16 deletions tests/unit/test_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ def test_set_up_relation(harness):
patch("relations.db.DbProvides._update_unit_status") as _update_unit_status,
patch("relations.db.new_password", return_value="test-password") as _new_password,
patch("relations.db.DbProvides._get_extensions") as _get_extensions,
patch("relations.db.logger") as _logger,
):
rel_id = harness.model.get_relation(RELATION_NAME).id
# Define some mocks' side effects.
Expand All @@ -210,16 +211,7 @@ def test_set_up_relation(harness):
postgresql_mock.create_database = PropertyMock(
side_effect=[None, None, PostgreSQLCreateDatabaseError, None]
)
postgresql_mock.get_postgresql_version = PropertyMock(
side_effect=[
POSTGRESQL_VERSION,
POSTGRESQL_VERSION,
POSTGRESQL_VERSION,
POSTGRESQL_VERSION,
POSTGRESQL_VERSION,
PostgreSQLGetPostgreSQLVersionError,
]
)
postgresql_mock.get_postgresql_version = PropertyMock(return_value=POSTGRESQL_VERSION)

# Assert no operation is done when at least one of the requested extensions
# is disabled.
Expand All @@ -244,7 +236,7 @@ def test_set_up_relation(harness):
postgresql_mock.create_database.assert_called_once_with(
DATABASE, user, plugins=[], client_relations=[relation]
)
assert postgresql_mock.get_postgresql_version.call_count == 2
assert postgresql_mock.get_postgresql_version.call_count == 1
_update_unit_status.assert_called_once()
expected_data = {
"allowed-units": "application/0",
Expand Down Expand Up @@ -289,7 +281,7 @@ def test_set_up_relation(harness):
postgresql_mock.create_database.assert_called_once_with(
DATABASE, user, plugins=[], client_relations=[relation]
)
assert postgresql_mock.get_postgresql_version.call_count == 2
assert postgresql_mock.get_postgresql_version.call_count == 1
_update_unit_status.assert_called_once()
assert harness.get_relation_data(rel_id, harness.charm.app.name) == expected_data
assert harness.get_relation_data(rel_id, harness.charm.unit.name) == expected_data
Expand Down Expand Up @@ -343,11 +335,13 @@ def test_set_up_relation(harness):
assert harness.get_relation_data(rel_id, harness.charm.app.name) == {}
assert harness.get_relation_data(rel_id, harness.charm.unit.name) == {}

# BlockedStatus due to a PostgreSQLGetPostgreSQLVersionError.
# version is not updated due to a PostgreSQLGetPostgreSQLVersionError.
postgresql_mock.get_postgresql_version.side_effect = PostgreSQLGetPostgreSQLVersionError
harness.charm.unit.status = ActiveStatus()
assert not harness.charm.legacy_db_relation.set_up_relation(relation)
_update_unit_status.assert_not_called()
assert isinstance(harness.model.unit.status, BlockedStatus)
assert harness.charm.legacy_db_relation.set_up_relation(relation)
_logger.exception.assert_called_once_with(
"Failed to retrieve the PostgreSQL version to initialise/update db relation"
)


def test_update_unit_status(harness):
Expand Down
Loading