Skip to content

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Nov 19, 2024

Currently, the charm sets all replica units as readonly endpoints to clients, no matter if the unit is functioning or not.

The PR adds filtering for the read only endpoints based on Patroni's cluster endpoint.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.49%. Comparing base (f50d373) to head (0efacb1).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
+ Coverage   71.40%   71.49%   +0.08%     
==========================================
  Files          13       13              
  Lines        3189     3199      +10     
  Branches      475      476       +1     
==========================================
+ Hits         2277     2287      +10     
  Misses        797      797              
  Partials      115      115              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@dragomirp dragomirp force-pushed the dpe-5714-degraded-replicas branch from b69b4de to c2188c0 Compare November 19, 2024 15:48
Comment on lines +530 to +535
response = requests.get(
f"{self._patroni_url}/cluster",
verify=self.verify,
auth=self._patroni_auth,
timeout=PATRONI_TIMEOUT,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if the majority of units goes down, the cluster endpoint will stop being updated. Best to address this after the RAFT reinit PRs land.

replicas_endpoint = list(self.charm.members_ips - {self.charm.primary_endpoint})
replicas_endpoint.sort()
cluster_state = self.charm._patroni.are_replicas_up()
if cluster_state:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting the primary endpoint just above, so we should be able to get the cluster endpoint as well, but if we can't, no filtering will be done.

FIRST_DATABASE_RELATION_NAME,
"read-only-endpoints",
)
assert data is None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two units in the cluster, so there will be no readonly endpoints if the down unit is filtered out.

@dragomirp dragomirp marked this pull request as ready for review November 19, 2024 20:14
@dragomirp dragomirp requested review from a team, taurus-forever, marceloneppel and lucasgameiroborges and removed request for a team November 19, 2024 20:14
Copy link
Member

@marceloneppel marceloneppel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but two points:

  • Q: will we trigger DPlib events, so client app will react (restart) every time we add RO unit?
  • BTW, did we agree on consensus with @delgod and @7annaba3l about adding RW as RO endpoint if the system has 1 unit only?

@dragomirp
Copy link
Contributor Author

  • Q: will we trigger DPlib events, so client app will react (restart) every time we add RO unit?

DPL should fire a separate event (read_only_endpoints_changed) for read only changes so it would be up to the client charm to decide.

  • BTW, did we agree on consensus with @delgod and @7annaba3l about adding RW as RO endpoint if the system has 1 unit only?

I don't think so, we can add it in a follow up PR.

@dragomirp dragomirp merged commit 3eb537d into main Nov 22, 2024
97 checks passed
@dragomirp dragomirp deleted the dpe-5714-degraded-replicas branch November 22, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants