Skip to content

fix: cgroups auto-tune before driver configuration #3940

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tilgovi
Copy link

@tilgovi tilgovi commented Feb 11, 2025

Move the cgroups auto-tuning into the driver factory so that its effect is automatically applied to the database connection pool defaults.

BREAKING CHANGE: Automatic GOMAXPROCS tuning based on cgroups influences the default database connection limits. Set connection limits explicitly based on the number of CPUs in the deployment to keep previous behavior.

Related issue(s)

There is no issue filed because the behavior is not clearly documented one way or another. The database documentation states that the default connection limit is twice the number of CPUs, but it is undocumented that GOMAXPROCS changes this default.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Move the cgroups auto-tuning into the driver factory so that its effect
is automatically applied to the database connection pool defaults.

BREAKING CHANGE: Automatic GOMAXPROCS tuning based on cgroups influences
the default database connection limits. Set connection limits explicitly
based on the number of CPUs in the deployment to keep previous behavior.
@tilgovi tilgovi requested review from aeneasr and a team as code owners February 11, 2025 01:22
@CLAassistant
Copy link

CLAassistant commented Feb 11, 2025

CLA assistant check
All committers have signed the CLA.

@tilgovi
Copy link
Author

tilgovi commented Feb 11, 2025

I can add tests and update documentation if it's determined that this would be a good change. I hesitate only because this will also influence the janitor and migration commands, and I don't know if it's appropriate to do that. If not, some greater refactoring would be needed due to the way the driver factory is responsible for constructing the configuration options.

@tilgovi
Copy link
Author

tilgovi commented Feb 11, 2025

I can add tests and update documentation if it's determined that this would be a good change. I hesitate only because this will also influence the janitor and migration commands, and I don't know if it's appropriate to do that. If not, some greater refactoring would be needed due to the way the driver factory is responsible for constructing the configuration options.

I guess I could just note the impact on the janitor and migrate commands as part of the breaking changes. Still, I wasn't sure if it was very deliberate to pull the actual execution of the auto-tuning up to the entry points and keep it out of the driver factory.

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2025

I think the change is fine - to be honest it probably won’t have a large effect since nothing runs concurrently except the HTTP server. What‘s the underlying problem the change is addressing?

@tilgovi
Copy link
Author

tilgovi commented Feb 12, 2025

I think the change is fine - to be honest it probably won’t have a large effect since nothing runs concurrently except the HTTP server. What‘s the underlying problem the change is addressing?

I saw that the database configures maximum connections based on GOMAXPROCS (if less than runtime.NumCPUs()) and expected that the cgroups auto-tune feature would therefore configure maximum database connections based on cgroups quotas.

It's no problem to set ?max_conns on the DSN and such, but the auto-tuning based on cgroups to align with the auto-tuned GOMAXPROCS is maybe nice? Mostly I opened the ticket in case this was the intended behavior and it was just a bug that it didn't work the way I expected.

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.

3 participants