From 8c667ed51775b29191c9630ff8d3deed58e4ba32 Mon Sep 17 00:00:00 2001 From: Pedro Guimaraes Date: Fri, 26 Apr 2024 17:13:45 +0200 Subject: [PATCH 1/9] Remove relation_{joined,changed}.emit() calls --- lib/charms/opensearch/v0/helper_charm.py | 11 ----------- lib/charms/opensearch/v0/opensearch_base_charm.py | 14 ++++++++------ .../opensearch/v0/opensearch_peer_clusters.py | 6 ++---- .../v0/opensearch_relation_peer_cluster.py | 4 ---- 4 files changed, 10 insertions(+), 25 deletions(-) diff --git a/lib/charms/opensearch/v0/helper_charm.py b/lib/charms/opensearch/v0/helper_charm.py index fbf6312cf..97508ea52 100644 --- a/lib/charms/opensearch/v0/helper_charm.py +++ b/lib/charms/opensearch/v0/helper_charm.py @@ -6,8 +6,6 @@ import typing from datetime import datetime -from charms.data_platform_libs.v0.data_interfaces import Scope -from charms.opensearch.v0.constants_charm import PeerRelationName from charms.opensearch.v0.helper_enums import BaseStrEnum from ops import CharmBase from ops.model import ActiveStatus, StatusBase @@ -118,12 +116,3 @@ def relation_departure_reason(charm: CharmBase, relation_name: str) -> RelDepart return RelDepartureReason.SCALE_DOWN return RelDepartureReason.REL_BROKEN - - -def trigger_leader_peer_rel_changed(charm: CharmBase) -> None: - """Force trigger a peer rel changed event by leader.""" - if not charm.unit.is_leader(): - return - - charm.peers_data.put(Scope.APP, "triggered", datetime.now().timestamp()) - charm.on[PeerRelationName].relation_changed.emit(charm.model.get_relation(PeerRelationName)) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 7db8f32da..190c3d672 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -226,7 +226,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.on[PeerRelationName].relation_joined, self._on_peer_relation_joined ) self.framework.observe( - self.on[PeerRelationName].relation_changed, self._on_peer_relation_changed + self.on[PeerRelationName].relation_changed, self.peer_relation_changed ) self.framework.observe( self.on[PeerRelationName].relation_departed, self._on_peer_relation_departed @@ -422,7 +422,7 @@ def _on_peer_relation_joined(self, event: RelationJoinedEvent): else: event.defer() - def _on_peer_relation_changed(self, event: RelationChangedEvent): + def peer_relation_changed(self, event: RelationChangedEvent): """Handle peer relation changes.""" if ( self.unit.is_leader() @@ -593,14 +593,16 @@ def _on_config_changed(self, event: ConfigChangedEvent): # noqa C901 # since when an IP change happens, "_on_peer_relation_joined" won't be called, # we need to alert the leader that it must recompute the node roles for any unit whose # roles were changed while the current unit was cut-off from the rest of the network - self.on[PeerRelationName].relation_joined.emit( - self.model.get_relation(PeerRelationName) - ) + self._on_peer_relation_joined(event) + if event.deferred: + return previous_deployment_desc = self.opensearch_peer_cm.deployment_desc() if self.unit.is_leader(): # run peer cluster manager processing - self.opensearch_peer_cm.run() + self.opensearch_peer_cm.run(event) + if event.deferred: + return # handle cluster change to main-orchestrator (i.e: init_hold: true -> false) self._handle_change_to_main_orchestrator_if_needed(event, previous_deployment_desc) diff --git a/lib/charms/opensearch/v0/opensearch_peer_clusters.py b/lib/charms/opensearch/v0/opensearch_peer_clusters.py index 64741e5c3..dd69e7a51 100644 --- a/lib/charms/opensearch/v0/opensearch_peer_clusters.py +++ b/lib/charms/opensearch/v0/opensearch_peer_clusters.py @@ -64,7 +64,7 @@ def __init__(self, charm: "OpenSearchBaseCharm"): self._charm = charm self._opensearch = charm.opensearch - def run(self) -> None: + def run(self, event=None) -> None: """Init, or updates / recomputes current peer cluster related config if applies.""" user_config = self._user_config() if not (current_deployment_desc := self.deployment_desc()): @@ -90,9 +90,7 @@ def run(self) -> None: if deployment_desc.start == StartMode.WITH_GENERATED_ROLES: # role generation logic - self._charm.on[PeerRelationName].relation_changed.emit( - self._charm.model.get_relation(PeerRelationName) - ) + self._charm.peer_relation_changed(event) self.apply_status_if_needed(deployment_desc) diff --git a/lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py b/lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py index fbd5c269e..b52874efa 100644 --- a/lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py +++ b/lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py @@ -137,9 +137,6 @@ def _on_peer_cluster_relation_joined(self, event: RelationJoinedEvent): self.refresh_relation_data(event) - # TODO: is the below still needed - # self.charm.trigger_leader_peer_rel_changed() - def _on_peer_cluster_relation_changed(self, event: RelationChangedEvent): """Event received by all units in sub-cluster when a new sub-cluster joins the relation.""" if not self.charm.unit.is_leader(): @@ -415,7 +412,6 @@ def __init__(self, charm: "OpenSearchBaseCharm"): def _on_peer_cluster_relation_joined(self, event: RelationJoinedEvent): """Event received when a new main-failover cluster unit joins the fleet.""" - # self.charm.trigger_leader_peer_rel_changed() pass def _on_peer_cluster_relation_changed(self, event: RelationChangedEvent): From 45339f1cd248820ac870a58de2807bb62e1c7379 Mon Sep 17 00:00:00 2001 From: Pedro Guimaraes Date: Fri, 26 Apr 2024 18:38:18 +0200 Subject: [PATCH 2/9] Clean-up passing config-changed around --- lib/charms/opensearch/v0/opensearch_base_charm.py | 15 ++++++++++++--- .../opensearch/v0/opensearch_peer_clusters.py | 6 +----- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 190c3d672..47bbdf3d9 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -238,6 +238,11 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.framework.observe(self.on.set_password_action, self._on_set_password_action) self.framework.observe(self.on.get_password_action, self._on_get_password_action) + # There is an explosion of peer_relation_changed calls at the startup. + # As this corresponds to a local peer relation (not the peer-cluster), we can safely + # abandon repeated events. + self._local_peer_relation_changed_has_deferred = False + @property @abc.abstractmethod def _upgrade(self) -> typing.Optional[upgrade.Upgrade]: @@ -429,8 +434,14 @@ def peer_relation_changed(self, event: RelationChangedEvent): and self.opensearch.is_node_up() and self.health.apply() in [HealthColors.UNKNOWN, HealthColors.YELLOW_TEMP] ): + if self._local_peer_relation_changed_has_deferred: + # We had already tried this event before and deferred. Retry on the next + # hook call. + return # we defer because we want the temporary status to be updated event.defer() + # From now on, we will abandon the event if we need to defer it + self._local_peer_relation_changed_has_deferred = True for relation in self.model.relations.get(ClientRelationName, []): self.opensearch_provider.update_endpoints(relation) @@ -600,9 +611,7 @@ def _on_config_changed(self, event: ConfigChangedEvent): # noqa C901 previous_deployment_desc = self.opensearch_peer_cm.deployment_desc() if self.unit.is_leader(): # run peer cluster manager processing - self.opensearch_peer_cm.run(event) - if event.deferred: - return + self.opensearch_peer_cm.run() # handle cluster change to main-orchestrator (i.e: init_hold: true -> false) self._handle_change_to_main_orchestrator_if_needed(event, previous_deployment_desc) diff --git a/lib/charms/opensearch/v0/opensearch_peer_clusters.py b/lib/charms/opensearch/v0/opensearch_peer_clusters.py index dd69e7a51..97923426b 100644 --- a/lib/charms/opensearch/v0/opensearch_peer_clusters.py +++ b/lib/charms/opensearch/v0/opensearch_peer_clusters.py @@ -64,7 +64,7 @@ def __init__(self, charm: "OpenSearchBaseCharm"): self._charm = charm self._opensearch = charm.opensearch - def run(self, event=None) -> None: + def run(self) -> None: """Init, or updates / recomputes current peer cluster related config if applies.""" user_config = self._user_config() if not (current_deployment_desc := self.deployment_desc()): @@ -88,10 +88,6 @@ def run(self, event=None) -> None: Scope.APP, "deployment-description", deployment_desc.to_dict() ) - if deployment_desc.start == StartMode.WITH_GENERATED_ROLES: - # role generation logic - self._charm.peer_relation_changed(event) - self.apply_status_if_needed(deployment_desc) # TODO: once peer clusters relation implemented, we should apply all directives From 2a3295de0e71727303eee987976e7468a02983f6 Mon Sep 17 00:00:00 2001 From: Pedro Guimaraes Date: Tue, 30 Apr 2024 10:48:23 +0200 Subject: [PATCH 3/9] Forgotten lint fix --- lib/charms/opensearch/v0/helper_charm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/charms/opensearch/v0/helper_charm.py b/lib/charms/opensearch/v0/helper_charm.py index 97508ea52..fb288e623 100644 --- a/lib/charms/opensearch/v0/helper_charm.py +++ b/lib/charms/opensearch/v0/helper_charm.py @@ -4,7 +4,6 @@ """Utility functions for charms related operations.""" import re import typing -from datetime import datetime from charms.opensearch.v0.helper_enums import BaseStrEnum from ops import CharmBase From 38ff2dbe7492c001b40621feb91347cd5a358f03 Mon Sep 17 00:00:00 2001 From: phvalguima Date: Tue, 30 Apr 2024 20:36:09 +0200 Subject: [PATCH 4/9] Update opensearch_base_charm.py --- lib/charms/opensearch/v0/opensearch_base_charm.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 47bbdf3d9..e919ad3b1 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -226,7 +226,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.on[PeerRelationName].relation_joined, self._on_peer_relation_joined ) self.framework.observe( - self.on[PeerRelationName].relation_changed, self.peer_relation_changed + self.on[PeerRelationName].relation_changed, self._on_peer_relation_changed ) self.framework.observe( self.on[PeerRelationName].relation_departed, self._on_peer_relation_departed @@ -238,7 +238,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.framework.observe(self.on.set_password_action, self._on_set_password_action) self.framework.observe(self.on.get_password_action, self._on_get_password_action) - # There is an explosion of peer_relation_changed calls at the startup. + # There is an explosion of _on_peer_relation_changed calls at the startup. # As this corresponds to a local peer relation (not the peer-cluster), we can safely # abandon repeated events. self._local_peer_relation_changed_has_deferred = False @@ -427,7 +427,7 @@ def _on_peer_relation_joined(self, event: RelationJoinedEvent): else: event.defer() - def peer_relation_changed(self, event: RelationChangedEvent): + def _on_peer_relation_changed(self, event: RelationChangedEvent): """Handle peer relation changes.""" if ( self.unit.is_leader() @@ -605,8 +605,6 @@ def _on_config_changed(self, event: ConfigChangedEvent): # noqa C901 # we need to alert the leader that it must recompute the node roles for any unit whose # roles were changed while the current unit was cut-off from the rest of the network self._on_peer_relation_joined(event) - if event.deferred: - return previous_deployment_desc = self.opensearch_peer_cm.deployment_desc() if self.unit.is_leader(): From 950bdfa6c73b15b489eb6dd47f5d11a76672b603 Mon Sep 17 00:00:00 2001 From: phvalguima Date: Tue, 30 Apr 2024 20:36:48 +0200 Subject: [PATCH 5/9] Update opensearch_base_charm.py --- lib/charms/opensearch/v0/opensearch_base_charm.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index e919ad3b1..bbb0a3951 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -241,7 +241,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): # There is an explosion of _on_peer_relation_changed calls at the startup. # As this corresponds to a local peer relation (not the peer-cluster), we can safely # abandon repeated events. - self._local_peer_relation_changed_has_deferred = False + self._is_peer_rel_changed_deferred = False @property @abc.abstractmethod @@ -434,14 +434,14 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent): and self.opensearch.is_node_up() and self.health.apply() in [HealthColors.UNKNOWN, HealthColors.YELLOW_TEMP] ): - if self._local_peer_relation_changed_has_deferred: + if self._is_peer_rel_changed_deferred: # We had already tried this event before and deferred. Retry on the next # hook call. return # we defer because we want the temporary status to be updated event.defer() # From now on, we will abandon the event if we need to defer it - self._local_peer_relation_changed_has_deferred = True + self._is_peer_rel_changed_deferred = True for relation in self.model.relations.get(ClientRelationName, []): self.opensearch_provider.update_endpoints(relation) From bcf5561725b466016b79080e6bdcb3ca0aa873a4 Mon Sep 17 00:00:00 2001 From: phvalguima Date: Wed, 1 May 2024 16:51:40 +0200 Subject: [PATCH 6/9] Update lib/charms/opensearch/v0/opensearch_base_charm.py Co-authored-by: Carl Csaposs --- lib/charms/opensearch/v0/opensearch_base_charm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index bbb0a3951..ce2ed9eca 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -435,8 +435,8 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent): and self.health.apply() in [HealthColors.UNKNOWN, HealthColors.YELLOW_TEMP] ): if self._is_peer_rel_changed_deferred: - # We had already tried this event before and deferred. Retry on the next - # hook call. + # We already deferred this event during this Juju event. Retry on the next + # Juju event. return # we defer because we want the temporary status to be updated event.defer() From 35ca7ec9bf88a2b3595d61973ac3d936c9f7d1b5 Mon Sep 17 00:00:00 2001 From: phvalguima Date: Wed, 1 May 2024 16:52:19 +0200 Subject: [PATCH 7/9] Update lib/charms/opensearch/v0/opensearch_base_charm.py Co-authored-by: Carl Csaposs --- lib/charms/opensearch/v0/opensearch_base_charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index ce2ed9eca..8e678374f 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -440,7 +440,7 @@ def _on_peer_relation_changed(self, event: RelationChangedEvent): return # we defer because we want the temporary status to be updated event.defer() - # From now on, we will abandon the event if we need to defer it + # If the handler is called again within this Juju hook, we will abandon the event self._is_peer_rel_changed_deferred = True for relation in self.model.relations.get(ClientRelationName, []): From dc457cbb7afa43b5bad3e654770f89b7b642b548 Mon Sep 17 00:00:00 2001 From: phvalguima Date: Wed, 1 May 2024 16:52:37 +0200 Subject: [PATCH 8/9] Update lib/charms/opensearch/v0/opensearch_base_charm.py Co-authored-by: Carl Csaposs --- lib/charms/opensearch/v0/opensearch_base_charm.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 8e678374f..9390e207b 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -238,9 +238,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.framework.observe(self.on.set_password_action, self._on_set_password_action) self.framework.observe(self.on.get_password_action, self._on_get_password_action) - # There is an explosion of _on_peer_relation_changed calls at the startup. - # As this corresponds to a local peer relation (not the peer-cluster), we can safely - # abandon repeated events. + # Ensure that only one instance of the `_on_peer_relation_changed` handler exists in the deferred event queue self._is_peer_rel_changed_deferred = False @property From 0ed59f79c42e6057c55d550ed6f802834ede4b6d Mon Sep 17 00:00:00 2001 From: phvalguima Date: Wed, 1 May 2024 16:54:11 +0200 Subject: [PATCH 9/9] Update opensearch_base_charm.py --- lib/charms/opensearch/v0/opensearch_base_charm.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 9390e207b..56081f80d 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -238,7 +238,8 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): self.framework.observe(self.on.set_password_action, self._on_set_password_action) self.framework.observe(self.on.get_password_action, self._on_get_password_action) - # Ensure that only one instance of the `_on_peer_relation_changed` handler exists in the deferred event queue + # Ensure that only one instance of the `_on_peer_relation_changed` handler exists + # in the deferred event queue self._is_peer_rel_changed_deferred = False @property