From 7530e9d288c23518fb12d7ca106111b9b91c3b7b Mon Sep 17 00:00:00 2001 From: Shayan Patel Date: Tue, 4 Jun 2024 12:28:06 +0000 Subject: [PATCH] Update outdated charm libs + fix failing unit tests --- .../data_platform_libs/v0/data_interfaces.py | 6 ++- lib/charms/operator_libs_linux/v2/snap.py | 7 +++- lib/charms/rolling_ops/v0/rollingops.py | 18 +++++++-- tests/unit/test_charm.py | 39 +++++++++++++++---- tests/unit/test_upgrade.py | 6 +-- 5 files changed, 58 insertions(+), 18 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index b331bdce83..59a97226a4 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -331,7 +331,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 36 +LIBPATCH = 37 PYDEPS = ["ops>=2.0.0"] @@ -658,6 +658,10 @@ def set_content(self, content: Dict[str, str]) -> None: if not self.meta: return + # DPE-4182: do not create new revision if the content stay the same + if content == self.get_content(): + return + if content: self._move_to_new_label_if_needed() self.meta.set_content(content) diff --git a/lib/charms/operator_libs_linux/v2/snap.py b/lib/charms/operator_libs_linux/v2/snap.py index 6d4dc385a6..9d09a78d36 100644 --- a/lib/charms/operator_libs_linux/v2/snap.py +++ b/lib/charms/operator_libs_linux/v2/snap.py @@ -83,7 +83,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 6 +LIBPATCH = 7 # Regex to locate 7-bit C1 ANSI sequences @@ -319,7 +319,10 @@ def get(self, key: Optional[str], *, typed: bool = False) -> Any: Default is to return a string. """ if typed: - config = json.loads(self._snap("get", ["-d", key])) + args = ["-d"] + if key: + args.append(key) + config = json.loads(self._snap("get", args)) if key: return config.get(key) return config diff --git a/lib/charms/rolling_ops/v0/rollingops.py b/lib/charms/rolling_ops/v0/rollingops.py index 5a7d4ce306..57aa9bf352 100644 --- a/lib/charms/rolling_ops/v0/rollingops.py +++ b/lib/charms/rolling_ops/v0/rollingops.py @@ -49,7 +49,7 @@ def _restart(self, event): To kick off the rolling restart, emit this library's AcquireLock event. The simplest way to do so would be with an action, though it might make sense to acquire the lock in -response to another event. +response to another event. ```python def _on_trigger_restart(self, event): @@ -88,7 +88,7 @@ def _on_trigger_restart(self, event): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 5 +LIBPATCH = 7 class LockNoRelationError(Exception): @@ -182,6 +182,7 @@ def _state(self) -> LockState: # Active acquire request. return LockState.ACQUIRE + logger.debug("Lock state: %s %s", unit_state, app_state) return app_state # Granted or unset/released @_state.setter @@ -202,21 +203,27 @@ def _state(self, state: LockState): if state is LockState.IDLE: self.relation.data[self.app].update({str(self.unit): state.value}) + logger.debug("state: %s", state.value) + def acquire(self): """Request that a lock be acquired.""" self._state = LockState.ACQUIRE + logger.debug("Lock acquired.") def release(self): """Request that a lock be released.""" self._state = LockState.RELEASE + logger.debug("Lock released.") def clear(self): """Unset a lock.""" self._state = LockState.IDLE + logger.debug("Lock cleared.") def grant(self): """Grant a lock to a unit.""" self._state = LockState.GRANTED + logger.debug("Lock granted.") def is_held(self): """This unit holds the lock.""" @@ -266,9 +273,11 @@ def __init__(self, handle, callback_override: Optional[str] = None): self.callback_override = callback_override or "" def snapshot(self): + """Snapshot of lock event.""" return {"callback_override": self.callback_override} def restore(self, snapshot): + """Restores lock event.""" self.callback_override = snapshot["callback_override"] @@ -288,7 +297,7 @@ def __init__(self, charm: CharmBase, relation: AnyStr, callback: Callable): charm: the charm we are attaching this to. relation: an identifier, by convention based on the name of the relation in the metadata.yaml, which identifies this instance of RollingOperatorsFactory, - distinct from other instances that may be hanlding other events. + distinct from other instances that may be handling other events. callback: a closure to run when we have a lock. (It must take a CharmBase object and EventBase object as args.) """ @@ -309,6 +318,7 @@ def __init__(self, charm: CharmBase, relation: AnyStr, callback: Callable): self.framework.observe(charm.on[self.name].acquire_lock, self._on_acquire_lock) self.framework.observe(charm.on[self.name].run_with_lock, self._on_run_with_lock) self.framework.observe(charm.on[self.name].process_locks, self._on_process_locks) + self.framework.observe(charm.on.leader_elected, self._on_process_locks) def _callback(self: CharmBase, event: EventBase) -> None: """Placeholder for the function that actually runs our event. @@ -381,7 +391,7 @@ def _on_acquire_lock(self: CharmBase, event: ActionEvent): """Request a lock.""" try: Lock(self).acquire() # Updates relation data - # emit relation changed event in the edge case where aquire does not + # emit relation changed event in the edge case where acquire does not relation = self.model.get_relation(self.name) # persist callback override for eventual run diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 48907eb4d0..114d8864c4 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -193,12 +193,16 @@ def test_primary_endpoint_no_peers(harness): @patch_network_get(private_address="1.1.1.1") def test_on_leader_elected(harness): - with patch( + with ( + patch( "charm.PostgresqlOperatorCharm._update_relation_endpoints", new_callable=PropertyMock - ) as _update_relation_endpoints, patch( - "charm.PostgresqlOperatorCharm.primary_endpoint", - new_callable=PropertyMock, - ) as _primary_endpoint, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config: + ) as _update_relation_endpoints, + patch( + "charm.PostgresqlOperatorCharm.primary_endpoint", + new_callable=PropertyMock, + ) as _primary_endpoint, patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), + ): # Assert that there is no password in the peer relation. assert harness.charm._peers.data[harness.charm.app].get("operator-password", None) is None @@ -570,6 +574,7 @@ def test_on_start(harness): "charm.PostgresqlOperatorCharm._is_storage_attached", side_effect=[False, True, True, True, True], ) as _is_storage_attached, + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): _get_postgresql_version.return_value = "14.0" @@ -690,6 +695,7 @@ def test_on_start_no_patroni_member(harness): patch( "charm.PostgresqlOperatorCharm._is_storage_attached", return_value=True ) as _is_storage_attached, + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): # Mock the passwords. patroni.return_value.member_started = False @@ -731,7 +737,10 @@ def test_on_start_after_blocked_state(harness): @patch_network_get(private_address="1.1.1.1") def test_on_get_password(harness): - with patch("charm.PostgresqlOperatorCharm.update_config"): + with ( + patch("charm.PostgresqlOperatorCharm.update_config"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), + ): rel_id = harness.model.get_relation(PEER).id # Create a mock event and set passwords in peer relation data. harness.set_leader(True) @@ -772,6 +781,7 @@ def test_on_set_password(harness): patch("charm.PostgresqlOperatorCharm.postgresql") as _postgresql, patch("charm.Patroni.are_all_members_ready") as _are_all_members_ready, patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): # Create a mock event. mock_event = MagicMock(params={}) @@ -1738,7 +1748,10 @@ def test_get_secret_from_databag(harness): This must be backwards-compatible so it runs on both juju2 and juju3. """ - with patch("charm.PostgresqlOperatorCharm._on_leader_elected"): + with ( + patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), + ): rel_id = harness.model.get_relation(PEER).id # App level changes require leader privileges harness.set_leader() @@ -1763,6 +1776,7 @@ def test_get_secret_from_databag(harness): def test_on_get_password_secrets(harness): with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): # Create a mock event and set passwords in peer relation data. harness.set_leader() @@ -1794,6 +1808,7 @@ def test_on_get_password_secrets(harness): def test_get_secret_secrets(harness, scope, field): with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): harness.set_leader() @@ -1808,7 +1823,10 @@ def test_set_secret_in_databag(harness, only_without_juju_secrets): This is juju2 specific. In juju3, set_secret writes to juju secrets. """ - with patch("charm.PostgresqlOperatorCharm._on_leader_elected"): + with ( + patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), + ): rel_id = harness.model.get_relation(PEER).id harness.set_leader() @@ -1841,6 +1859,7 @@ def test_set_secret_in_databag(harness, only_without_juju_secrets): def test_set_reset_new_secret(harness, scope, is_leader): with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): """NOTE: currently ops.testing seems to allow for non-leader to set secrets too!""" # App has to be leader, unit can be either @@ -1863,6 +1882,7 @@ def test_set_reset_new_secret(harness, scope, is_leader): def test_invalid_secret(harness, scope, is_leader): with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): # App has to be leader, unit can be either harness.set_leader(is_leader) @@ -1878,6 +1898,7 @@ def test_invalid_secret(harness, scope, is_leader): def test_delete_password(harness, _has_secrets, caplog): with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): """NOTE: currently ops.testing seems to allow for non-leader to remove secrets too!""" harness.set_leader(True) @@ -1929,6 +1950,7 @@ def test_migration_from_databag(harness, only_with_juju_secrets, scope, is_leade """ with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): rel_id = harness.model.get_relation(PEER).id # App has to be leader, unit can be either @@ -1957,6 +1979,7 @@ def test_migration_from_single_secret(harness, only_with_juju_secrets, scope, is """ with ( patch("charm.PostgresqlOperatorCharm._on_leader_elected"), + patch("charms.rolling_ops.v0.rollingops.RollingOpsManager._on_process_locks"), ): rel_id = harness.model.get_relation(PEER).id diff --git a/tests/unit/test_upgrade.py b/tests/unit/test_upgrade.py index b6489e29f2..396a23ac10 100644 --- a/tests/unit/test_upgrade.py +++ b/tests/unit/test_upgrade.py @@ -65,13 +65,13 @@ def test_on_upgrade_granted(harness): with ( patch("charm.Patroni.get_postgresql_version"), patch( - "charms.data_platform_libs.v0.upgrade.DataUpgrade.on_upgrade_changed" + "charm.PostgreSQLUpgrade.on_upgrade_changed" ) as _on_upgrade_changed, patch( - "charms.data_platform_libs.v0.upgrade.DataUpgrade.set_unit_failed" + "charm.PostgreSQLUpgrade.set_unit_failed" ) as _set_unit_failed, patch( - "charms.data_platform_libs.v0.upgrade.DataUpgrade.set_unit_completed" + "charm.PostgreSQLUpgrade.set_unit_completed" ) as _set_unit_completed, patch( "charm.Patroni.is_replication_healthy", new_callable=PropertyMock