canonical / opensearch-operator

OpenSearch operator
Apache License 2.0
10 stars 6 forks source link

Incorrect usage of `emit()` or incorrect comment #207

Closed carlcsaposs-canonical closed 1 week ago

carlcsaposs-canonical commented 6 months ago

This block of code does not behave as the comment describes https://github.com/canonical/opensearch-operator/blob/b0208dc2bcfce66234125f1ab39aed73c070bbae/lib/charms/opensearch/v0/opensearch_base_charm.py#L471-L476

relation-joined will only be emitted on that unit (where https://github.com/canonical/opensearch-operator/blob/b0208dc2bcfce66234125f1ab39aed73c070bbae/lib/charms/opensearch/v0/opensearch_base_charm.py#L466 evaluates to True)

And that unit will immediately return https://github.com/canonical/opensearch-operator/blob/b0208dc2bcfce66234125f1ab39aed73c070bbae/lib/charms/opensearch/v0/opensearch_base_charm.py#L294-L297

More info:

Note that the emission of custom events is handled immediately. In other words, custom events are not queued, but rather nested. For example:

1. Main hook handler (emits custom_event_1)
2.   Custom event 1 handler (emits custom_event_2)
3.     Custom event 2 handler
4.   Resume custom event 1 handler
5. Resume main hook handler

https://ops.readthedocs.io/en/latest/#ops.BoundEvent.emit

github-actions[bot] commented 6 months ago

https://warthogs.atlassian.net/browse/DPE-3828

carlcsaposs-canonical commented 6 months ago

Potential solution: instead of emitting custom event, write something to unit peer databag (and ensure peer databag is updated [i.e. don't write data that's already there]) to trigger a peer-relation-changed event on all other units

Mehdi-Bendriss commented 6 months ago

This looks like a bug indeed. Thanks!

In this case, I don't think we unfortunately can get away from emitting the event, in case the current unit is the elected leader unit - which will then not receive a peer-rel-changed event if the data is stored in its unit data bag..

I believe the fix is a mix of both approaches, something along the lines of:

if self.unit.is_leader():
    self.on[PeerRelationName].relation_joined.emit( 
        self.model.get_relation(PeerRelationName) 
    )
self.peers_data.put(
    Scope.UNIT, "updated_at", datetime.now().timestamp()
)
phvalguima commented 1 week ago

This has been fixed with #346: it generates a peer event when renovating the certificates, hence waking up all its peers.

@Mehdi-Bendriss now, the config-changed is the entry point to update the new IP in our config, we are not changing the unicasts_hosts.txt of each peer to the new IP thou.

phvalguima commented 1 week ago

Actually, a correction: config-changed does update the unitcast_hosts file:

config-changed > calls self.tls.request_new_unit_certificates() > triggers peer relation change with self.charm.peers_data.delete(Scope.UNIT, "tls_configured") > peer-relation-changed > calls self._add_cm_addresses_to_conf()