-
Notifications
You must be signed in to change notification settings - Fork 180
feat: make platform db connection pooling configurable #2730
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
base: main
Are you sure you want to change the base?
Conversation
|
I think all the test files can be removed. |
| keycloak: | ||
| adminUsername: otomi-admin | ||
| theme: otomi | ||
| database: |
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.
It would make more sense for me if these connections values are also under the database objects? Then they would end up in the same file where all the other database settings are stored.
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.
They are not database settings, but application settings. When putting these in the same place, I would find it ambiguous how they relate to max_connections of the database. And applications can have different ways of connection pooling, that may also change independently from the database (note how Keycloak differs from Gitea and Harbor), while all PostgreSQL databases work the same way.
It only would make sense if we removed these and set them implicitly (e.g. set these numbers based on max_connections of the database) but I would prefer not to, as this seems like excessively opinionated template magic to me.
We could rename it however, to make it more clear. Maybe call it databaseConnection?
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.
Ah I saw it wrong. I thought these values where set on the database. But that is the postgresql settings.
Indeed having databaseMaxConnection etc would help. Seeing a database object gives me the idea that you can set database things in the application settings.
📌 Summary
This PR replaces hard-coded PostgreSQL parameters with platform values, which are identically set by default, but can be updated by the platform admin. Platform applications using PostgreSQL databases, i.e. Gitea, Keycloak, and Harbor also have the hard-coded connection pool settings replaced with variables. However, the defaults of maximum database connections on the app-side are reduced, for taking a deduction from the database
max_connectionsinto account.The default for
superuser_reserved_connectionsof 3. This has lead to Harbor not starting, when it was attempting to max out the connection pool.The max connections on the app-side is set to 28, considering the default of
superuser_reserved_connectionsplus one in case there are other maintenance connections. A backport for current releases, which does not involve schema changes, and just updates the hard-coded defaults, is published in #2731.Other changes to the schema are fixes of wrong locations or cleanups of unused values. In addition it showed that values introduced for each database's
backupSidecarResourceswere not applied in the template. This has been added here.🔍 Reviewer Notes
When upgrading to this feature branch, defaults should be added to the values repo. Changes are expected to require restarting the application pods, but not the databases as default settings are identical to current hard-coded parameters.
Note also when upgrading from
main, you need to setspecVersioninenv/settings/versions.yamlback to44for re-running schema migrations. Otherwise validations fail.🧹 Checklist