-
Notifications
You must be signed in to change notification settings - Fork 26
[DPE-4533] Pause Patroni in the TLS test #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #534 +/- ##
=======================================
Coverage 70.87% 70.87%
=======================================
Files 11 11
Lines 3021 3021
Branches 535 535
=======================================
Hits 2141 2141
Misses 764 764
Partials 116 116 ☔ View full report in Codecov by Sentry. |
255defd
to
e2b9025
Compare
2899902
to
a75bfa2
Compare
3a8f445
to
a576119
Compare
edccdc1
to
03863f4
Compare
03863f4
to
d5bfb7d
Compare
f903463
to
476c8a9
Compare
|
||
|
||
async def get_patroni_setting(ops_test: OpsTest, setting: str) -> Optional[int]: | ||
async def get_patroni_setting(ops_test: OpsTest, setting: str, tls: bool = False) -> Optional[int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need this, but kept for compatibility.
# Pause Patroni so it doesn't wipe the custom changes | ||
await change_patroni_setting(ops_test, "pause", True, use_random_unit=True, tls=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pause and upause Patroni, instead of trying to tweak the loop_wait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great solution!
# Pause Patroni so it doesn't wipe the custom changes | ||
await change_patroni_setting(ops_test, "pause", True, use_random_unit=True, tls=True) | ||
|
||
async with ops_test.fast_forward("24h"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable update_status so it doesn't run while we're tweaking the cluster.
for attempt in Retrying(stop=stop_after_attempt(10), wait=wait_fixed(5), reraise=True): | ||
with attempt: | ||
logger.info("Trying to grep for rewind logs.") | ||
await run_command_on_unit( | ||
ops_test, | ||
primary, | ||
"grep 'connection authorized: user=rewind database=postgres SSL enabled' /var/snap/charmed-postgresql/common/var/log/postgresql/postgresql-*.log", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retry for about a minute for the logs to flush.
await change_patroni_setting(ops_test, "pause", False, use_random_unit=True, tls=True) | ||
|
||
async with ops_test.fast_forward(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reenable Patroni and ffwd for the removal test.
TLS test for amd64 on Juju 3.4.4 succeeded 5 times in a row here |
Unrelated to the PR, but do we need the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# Pause Patroni so it doesn't wipe the custom changes | ||
await change_patroni_setting(ops_test, "pause", True, use_random_unit=True, tls=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great solution!
I think it can be removed nowadays because the issue related to it was solved a long time ago. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. 🤞
Try to reduce the TLS test flakyness.