Skip to content

Commit

Permalink
[DPE-4224] Remove relation_{joined,changed}.emit() calls (#266)
Browse files Browse the repository at this point in the history
We should replace calls to `relation_{joined,changed}.emit()` by their
end-method's handler. Otherwise we will face situations like #265, where
a deferred event constantly gets called up, generates new events which
also get deferred and end in a snow ball of deferrals.

Closes #265

---------

Co-authored-by: Mehdi Bendriss <bendrissmehdi@gmail.com>
Co-authored-by: Carl Csaposs <carl.csaposs@canonical.com>
  • Loading branch information
3 people authored May 1, 2024
1 parent 2071f19 commit 5f2d6cd
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 25 deletions.
12 changes: 0 additions & 12 deletions lib/charms/opensearch/v0/helper_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
"""Utility functions for charms related operations."""
import re
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
Expand Down Expand Up @@ -118,12 +115,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))
14 changes: 11 additions & 3 deletions lib/charms/opensearch/v0/opensearch_base_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,10 @@ 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
self._is_peer_rel_changed_deferred = False

@property
@abc.abstractmethod
def _upgrade(self) -> typing.Optional[upgrade.Upgrade]:
Expand Down Expand Up @@ -429,8 +433,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._is_peer_rel_changed_deferred:
# 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()
# 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, []):
self.opensearch_provider.update_endpoints(relation)
Expand Down Expand Up @@ -593,9 +603,7 @@ 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)

previous_deployment_desc = self.opensearch_peer_cm.deployment_desc()
if self.unit.is_leader():
Expand Down
6 changes: 0 additions & 6 deletions lib/charms/opensearch/v0/opensearch_peer_clusters.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,6 @@ def run(self) -> None:
Scope.APP, "deployment-description", deployment_desc.to_dict()
)

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.apply_status_if_needed(deployment_desc)

# TODO: once peer clusters relation implemented, we should apply all directives
Expand Down
4 changes: 0 additions & 4 deletions lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit 5f2d6cd

Please sign in to comment.