Skip to content
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

Scaling #16

Merged
merged 169 commits into from Sep 5, 2022
Merged

Scaling #16

merged 169 commits into from Sep 5, 2022

Conversation

WRFitch
Copy link
Contributor

@WRFitch WRFitch commented Aug 24, 2022

Proposal

Jira issue: DPE-472
This PR allows PgBouncer to scale to multiple units.

Context

  • We're storing everything in the peer databag for now, including usernames and passwords for the db relations. These will be removed to Juju secrets once they exist.
  • The config is stored as an ini file in the databag, rather than a json file, because it's an existing string representation of the data and it's more readable (although there's a bug in jhack that doesn't display the headers)

Release Notes

  • added pgb_peers relation and corresponding integration test run
  • updated integration test runners to use juju version 2.9.29 to work around scale-down bug.
  • added juju get_secret interface

Testing

  • updated unit tests
  • updated the existing db integration test to work with scaled pgbouncer
    • db-admin test has not been updated because discourse behaves weirdly

Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

lgtm - left some comments here and there, nothing significant though

src/charm.py Outdated
"""Charmed PgBouncer connection pooler."""
"""Charmed PgBouncer connection pooler.

TODO the flow of this charm is getting a bit convoluted. The backend relation has to be ready
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not something to worry if you have a clear view on the conditions that are required at each step.
Making the conditions to run a hook (or defer when not met) as clear as possible will suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'd still like to try and simplify it but I'll document the requirements in the hook docstrings.


from constants import AUTH_FILE_PATH, INI_PATH, PG_USER, PGB
Copy link
Contributor

Choose a reason for hiding this comment

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

is PGB the app name?

Suggested change
from constants import AUTH_FILE_PATH, INI_PATH, PG_USER, PGB
from constants import AUTH_FILE_PATH, INI_PATH, PG_USER, PGB_APP_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the pgbouncer service name, so it's "pgbouncer", whereas the app name is "pgbouncer-k8s".

src/charm.py Show resolved Hide resolved
src/relations/backend_database.py Show resolved Hide resolved
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

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

LGTM! A couple comments

src/relations/backend_database.py Outdated Show resolved Hide resolved
src/relations/peers.py Outdated Show resolved Hide resolved
@WRFitch WRFitch merged commit c133ec0 into main Sep 5, 2022
@WRFitch WRFitch deleted the scaling branch September 5, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants