Skip to content

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Aug 30, 2024

Conditionally add REST password to patroni config

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.81%. Comparing base (68b923e) to head (0aafbec).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
- Coverage   70.85%   70.81%   -0.04%     
==========================================
  Files          12       12              
  Lines        3043     3039       -4     
  Branches      538      537       -1     
==========================================
- Hits         2156     2152       -4     
  Misses        771      771              
  Partials      116      116              

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

Comment on lines +24 to +28
{%- if patroni_password %}
authentication:
username: patroni
password: {{ patroni_password }}
{%- endif %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the leader elected executes after the template is rendered we'll be stuck with None pass and will be unable to reload via the REST API

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this happen in wild? What is our answer there?

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 don't think so, but when we add rotation (DPE-5270) we should switch to SIGHUPing the service instead of reloading via REST.

Choose a reason for hiding this comment

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

Basic question: At which point during charm execution the patroni template is rendered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basic question: At which point during charm execution the patroni template is rendered?

I don't think there's a set order, especially during charm's bootstrap.

Choose a reason for hiding this comment

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

I might ask @marceloneppel once hes back, might be good to have this clear and documented somewhere (maybe on the diagrams!)

Copy link
Member

Choose a reason for hiding this comment

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

I believe that the issue might be happening at

self.charm.update_config()
, when a non-leader unit is the first one to be updated.

The changes from this PR LGTM.

Comment on lines -161 to -168
raft_encryption = (
int(
json.loads(self.peer_relation.data[self.charm.app].get("dependencies", "{}"))
.get("charm", {})
.get("version", 0)
)
< 3
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the 503 errors were API related not RAFT related.

@dragomirp dragomirp marked this pull request as ready for review August 30, 2024 13:12
@dragomirp dragomirp requested review from a team, taurus-forever, marceloneppel and lucasgameiroborges and removed request for a team August 30, 2024 13:12
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. The Patroni 503 is know reply when it is not-yet-started.
I am askin @marceloneppel to request upstream change it to 603 declined whole Patroni is starting... so we can recognize the real 503 internal errors and react or continue waiting...

Comment on lines +24 to +28
{%- if patroni_password %}
authentication:
username: patroni
password: {{ patroni_password }}
{%- endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this happen in wild? What is our answer there?

@dragomirp dragomirp merged commit 5d05a9e into main Aug 31, 2024
82 checks passed
@dragomirp dragomirp deleted the conditional-password branch August 31, 2024 12:31
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.

4 participants