-
Notifications
You must be signed in to change notification settings - Fork 20
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
[DPE-1782] Configuration support #281
Conversation
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 refactoring and well readable implementation.
One question inside, otherwise LGTM.
config.yaml
Outdated
type: boolean | ||
logging_log_min_duration_statement: | ||
description: | | ||
Sets the minimum running time above which statements will be logged |
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 need units for all relevant options.
or maybe even link to https://postgresqlco.nf/doc/en/param/log_min_duration_statement/14/ if Alex approve
Sets the minimum running time above which statements will be logged | |
Sets the minimum running time above which statements will be logged (ms) |
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.
Added defaults, allowed values and unit on fac0b95.
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.
Discussed with Marcelo, I would avoid references to the external resources we are not controlling.
Otherwise it will be hard to build consistent UX.
…buffers upper limit
…onnection_ssl config option
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.
Nice!
* Add missing parameters and add configuration support * Use fixed snap * Fix parameters validation * Add validations * Update library * Revert changes on charm.py * Add configuration logic and tests * PR feedback (including the feedback from canonical/postgresql-k8s-operator#281) * Fix TLS tests * Increase timeout
* Add missing parameters and add configuration support * Use fixed snap * Fix parameters validation * Add validations * Update library * Revert changes on charm.py * Add configuration logic and tests * PR feedback (including the feedback from canonical/postgresql-k8s-operator#281) * Fix TLS tests * Increase timeout
* Initial configuration support * Add pending restart check * Add missing parameters and make configuration support work * Fix parameters validation * Adjust config management * Fix configuration support and pending restart * Fix test * Add validations * Add shared_buffers min and max values * Add validation for instance_default_text_search_config * Add validation for request_time_zone * Add validation for request_date_style * Add validation for memory_max_prepared_transactions * Fix CPU core count retrieval * Remove check for primary endpoint being ready * Remove logs * Add comment * Uncomment test code * Remove default value * Remove unused constant * Minor changes in the config class * Check new settings on integration test * Fix validation of parameters relying on locales * Update LIBPATCH * Remove connection_ssl config option, fix CPU limits check and shared_buffers upper limit * Improve checks on test * Add config options defaults, allowed values and unit, also removing connection_ssl config option * One more improvement in the test * Fix tests * Update Juju 3 version * Skip test on Juju 3 * Skip Indico test
Issue
There is no way to provide custom configurations to the workload (PostgreSQL database).
Solution
Add to
patroni.yaml
the requested configurations with their default values.Expose the allowed configurations through charm config options (and add validators based on the allowed values).
max_connections
andmax_prepared_transactions
can be changed only through the Patroni API, so they are changed in the charm code differently from the other configurations.Also, added a call to
time.sleep(10)
to give some time to Patroni to reload the configurations before it's possible to know which ones need a database restart. Previously, sometimes a change inshared_buffers
wouldn't trigger a restart.