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

Fixes #31815 - Allow setting number of workers for content app #170

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Feb 5, 2021

No description provided.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

In other places we have set an upper bound. This is important on large systems with 10s of CPUs. I think we should do the same here.

@ehelms
Copy link
Member Author

ehelms commented Feb 5, 2021

In other places we have set an upper bound. This is important on large systems with 10s of CPUs. I think we should do the same here.

Which should be?

@ekohl
Copy link
Member

ekohl commented Feb 5, 2021

Now I had hoped you had an answer for that. We do see that $worker_count is limited to 8 CPUs so if we follow that logic, perhaps it should limit content as well on 8 CPUs (which will result in 17 workers).

@ehelms
Copy link
Member Author

ehelms commented Feb 5, 2021

Sounds reasonable to go with how we set this in other places, and then see how the tuning plays out.

@ehelms ehelms requested a review from ekohl February 5, 2021 13:38
manifests/init.pp Outdated Show resolved Hide resolved
@wbclark
Copy link
Collaborator

wbclark commented Feb 5, 2021

Is there any documentation on the difference between pulpcore workers and pulpcore content app workers? I'm not finding it readily

I did find however some more notes about hardware requirements vs worker counts at https://docs.pulpproject.org/pulpcore/components.html#hardware-requirements

We currently consider CPUs but not memory, which looks like it can also be a significant bottleneck. In particular for the RPM content plugin a single worker can consume several GBs of memory while syncing. In the future we should at least update the docs here to include this information.

@ehelms
Copy link
Member Author

ehelms commented Feb 5, 2021

pulpcore workers are the background workers, content app workers are really gunicorn workers for the webserver piece of the content app.

@ekohl
Copy link
Member

ekohl commented Feb 5, 2021

I wouldn't be surprised if at some point we also need to be able to tune the API workers. If you have more jobs and end up polling more, a single worker may not be able to handle it anymore. It would be good to add a parameter for that as well, even if we hardcode it to a single worker now.

@ehelms
Copy link
Member Author

ehelms commented Feb 5, 2021

Related to what sparked this change I have a pulp-list thread started -- https://listman.redhat.com/archives/pulp-list/2021-February/msg00007.html

@ekohl
Copy link
Member

ekohl commented Feb 5, 2021

Regarding the mentioned issue: we can't use the event worker on EL7's httpd. In theforeman/foreman-installer@0ed45f7 we reverted this. There's a bugzilla somewhere that's closed as WONTFIX so we either need to move to the httpd SCL or drop EL7 for that.

@ehelms
Copy link
Member Author

ehelms commented Feb 8, 2021

Added an entry for the API service to adjust worker counts as well.

@@ -178,6 +186,8 @@
Integer[0] $worker_count = min(8, $facts['processors']['count']),
Boolean $service_enable = true,
Boolean $service_ensure = true,
Integer[0] $content_service_worker_count = (2*min(8, $facts['processors']['count']) + 1),
Integer[0] $api_service_worker_count = 1,
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: this matches the default according to https://docs.gunicorn.org/en/latest/settings.html#workers

@ehelms ehelms merged commit cb40e28 into theforeman:master Feb 8, 2021
@ehelms ehelms added the Enhancement New feature or request label Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants