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

feat(controller): permit setting GUNICORN_WORKERS #137

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

kingdonb
Copy link
Member

permit setting GUNICORN_WORKERS

This is the chart-only change. #75 had another separate change for CONN_MAX_AGE, but I could not get controller to build tonight... this variable is already mentioned in controller v2.21.2, though!

This chart-only change makes GUNICORN_WORKERS settable from in the controller chart.

#75 will close and we can reopen with whatever changes are needed for CONN_MAX_AGE, if that's still something we want to configure. (I think we weren't convinced this other setting is really needed, whenever that conversation last left off...)

Kingdon Barrett added 2 commits November 22, 2020 21:40
and CONN_MAX_AGE from global values.yaml
The controller values.yaml should mention all of the settings that can
affect the controller in values.yaml too.
@kingdonb
Copy link
Member Author

I tested this chart change, and it works. Setting gunicorn_workers to 5 and exec'ing into controller, I can see 6 python processes running (5 workers). Leaving the chart value unset, I can see 18 processes running for python (17 workers, my 4 core machine * 4 + 1, according to the comment in values.yaml, this is exactly as expected)

5 is probably a sensible default value for my clusters, but it would not be a sensible default for multitenant Workflow providers. 🤷‍♂️ I think this is the right change, it's easy to accidentally make your nodes too big, and saturate database with too many connections just based on this calculation.

YMMV. Serious operators should probably examine this value and determine if it is sensible for themselves. Now, they can at least be able to override it when the value is wrong.

@kingdonb
Copy link
Member Author

This got tested on a Rancher RKE quickstart cluster at 1.17.14 tonight, and it performed as expected.

@dmcnaught
Copy link

Moved this into two of our clusters for testing

@Cryptophobia Cryptophobia self-requested a review November 23, 2020 21:59
Copy link
Member

@Cryptophobia Cryptophobia left a comment

Choose a reason for hiding this comment

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

lgtm!

@Cryptophobia Cryptophobia merged commit 9aa180d into teamhephy:master Nov 23, 2020
@kingdonb kingdonb mentioned this pull request Nov 27, 2020
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