[DPE - 6153] - Propagate the security index intitialized through the peercluster relation #517
Conversation
There was a problem hiding this comment.
Thank you @skourta for investigating and bringing up this solution for the issue. I think it is a valid approach and helps to set the security_index_initialized more consistently on the peer-cluster applications.
There are only some minor things that need to be adjusted from my point of view. Good Job!
| if data.security_index_initialised: | ||
| self.charm.peers_data.put(Scope.APP, "security_index_initialised", True) |
There was a problem hiding this comment.
This seems to be happening twice: here and in _set_security_conf. Is this wanted?
I noticed when deploying that the PeerClusterRequirers are updating their opensearch-peers app relation data with security_index_initialised: "True", but the PeerClusterProvider does not. I believe we should add this to the _on_peer_cluster_relation_changed method on the Provider side:
...
if self._get_security_index_initialised():
self.charm.peers_data.put(Scope.APP, "security_index_initialised", True)
There was a problem hiding this comment.
Correct me if I am wrong. All providers other than the main orchestrator are also requirers of the main orchestrator, correct? That is why I didn't add it for the provider side. The main orchestrator will pick it up and propagate it to the others. The only place the flag might be missing would be the peer databag of the main orchestrator if it was not the one that initialized it. Should we add it there as well for consistency?
There was a problem hiding this comment.
Yes that is correct.
The only place the flag might be missing would be the peer databag of the main orchestrator if it was not the one that initialized it.
This is what I meant with my comment, sorry for not being exact enough.
There was a problem hiding this comment.
Understood. I will add it.
| # check all other clusters if they have initialised the security index | ||
| all_relation_ids = [ | ||
| rel.id for rel in self.charm.model.relations[self.relation_name] if len(rel.units) > 0 | ||
| ] | ||
|
|
||
| for rel_id in all_relation_ids: | ||
| if self.get_from_rel("security_index_initialised", rel_id=rel_id, remote_app=True): | ||
| return True |
There was a problem hiding this comment.
I think this is correctly done.
| logger.debug( | ||
| f"is_leader: {self.unit.is_leader()}, security_index_initialised: {self.peers_data.get(Scope.APP, 'security_index_initialised')}, roles: {self.opensearch_peer_cm.deployment_desc().config.roles}, start: {self.opensearch_peer_cm.deployment_desc().start}" | ||
| ) |
There was a problem hiding this comment.
Please make this a multi-line-statement for better readability.
| @@ -403,6 +403,7 @@ def cleanup(): | |||
| # when "data" node joins -> start cluster-manager via _on_peer_cluster_relation_changed | |||
| # cluster-manager notifies "data" node via refresh of peer cluster relation data | |||
| # "data" node starts and initializes security index | |||
| logger.info(f"Deployment description at on start: {deployment_desc}") | |||
There was a problem hiding this comment.
The log level here should be debug.
Fixes #516
Summary
lib/charms/opensearch/v0/models.py: Added a new attributesecurity_index_initialisedto thePeerClusterRelDataclass to track the initialization status of the security index.lib/charms/opensearch/v0/opensearch_base_charm.py: Added a new methodput_or_update_security_index_initializedto set the security index initialization flag and notify the main orchestrator. This method is called in_post_start_initto replace the direct setting of the flag. [1] [2]lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py: Added a method_get_security_index_initialisedto check the initialization status across clusters and a methodset_security_index_initialisedto update the status in the unit data bag. These methods are used to ensure consistent handling of the security index initialization. [1] [2]lib/charms/opensearch/v0/opensearch_relation_peer_cluster.py: Updated methods_rel_data,_on_peer_cluster_relation_changed, and_set_security_confto incorporate the newsecurity_index_initialisedattribute and ensure the status is correctly propagated and logged. [1] [2] [3]