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

Pulp tunings #2565

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Pulp tunings #2565

wants to merge 5 commits into from

Conversation

Imaanpreet
Copy link
Contributor

Please cherry-pick my commits into:

  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (planned Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4/6.5 on EL8 only)
  • Foreman 3.2/Katello 4.4 on EL7 & EL8
  • Foreman 3.1/Katello 4.3 on EL7 & EL8 (Satellite 6.11 EL7/8; orcharhino 6.3 on EL7/8)
  • We do not accept PRs for Foreman older than 3.1.

@github-actions
Copy link

github-actions bot commented Oct 30, 2023

guides/common/modules/con_pulp-tuning.adoc Outdated Show resolved Hide resolved

.Use the below procedure to tune the pulpcore API workers:

. Update the pulpcore api_service_worker_count on your {ProjectServer} or {SmartProxyServer} in the custom-hiera.yaml file.:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
. Update the pulpcore api_service_worker_count on your {ProjectServer} or {SmartProxyServer} in the custom-hiera.yaml file.:
. Update the pulpcore api_service_worker_count on your {ProjectServer} or {SmartProxyServer} in the `custom-hiera.yaml` file.:

I am unsure if we should recommend users editing this file vs. using foreman-installer.

Copy link
Member

Choose a reason for hiding this comment

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

I've always taken a stance that custom-hiera.yaml is unsupported. Anything that's written in docs, so we can't refer to it.

If there's always a recommendation to lower the number of workers, that should be filed as a bug. Telling users "we have poor defaults, always change this" is a poor experience.

My suggestion is to drop this chapter altogether.

Copy link
Member

Choose a reason for hiding this comment

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

And if it is common to change it, an RFE to add a parameter as was done with the worker count is also a valid solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @ekohl: I totally agree on custom-hiera.yaml is unsupported part. Do you mean to dropping off this section or proc_configuring-pulpcore-workers as well?

Copy link
Member

Choose a reason for hiding this comment

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

I'm questioning the usefulness of proc_configuring-pulpcore-workers. When I read it, it comes down to "here's a parameter you can change, but we noticed it doesn't make a difference". So as a reader of that, is it adding any value or just confusing me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, let's drop this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @ekohl , I have removed this chapter proc_configuring-pulpcore-workers from this PR.

Regarding proc_configuring-pulpcore-api-workers.adoc section, can we suggest updating pulpcore api_service_worker_count with another method, for example - via foreman-installer?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding proc_configuring-pulpcore-api-workers.adoc section, can we suggest updating pulpcore api_service_worker_count with another method, for example - via foreman-installer?

Currently this is the code that sets it:
https://github.com/theforeman/puppet-pulpcore/blob/297a48acafcbdb9871689b6e267598c7414692c2/manifests/init.pp#L246-L247

https://projects.theforeman.org/issues/36957 was opened by @maximiliankolb to expose a parameter for this and I just opened theforeman/puppet-foreman_proxy_content#467.

I still think that most users shouldn't touch this and if our default tuning is poor, then it should be fixed.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Oct 31, 2023
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 31, 2023
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Nov 1, 2023
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Nov 1, 2023
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

If the approch to change a value to "custom-hiera.yaml" and running foreman-installer is unspoorted, I question the whole PR. Also regarding line 14: "outcome looks similar", which means there's not even a "real reason" to change any value.

What do you think @ekohl & @Imaanpreet ? Is there a RFE to include "pulpcore API worker count" to foreman-installer?

@ekohl
Copy link
Member

ekohl commented Nov 17, 2023

@Imaanpreet please don't manually mark comments as resolved without resolving them. Usually when you modify a line GitHub will automatically hide the comment as outdated.

@maximiliankolb I'm not aware of such an RFE. Such an RFE would be easy to implemen5, if there's a need for it. Currently on my phone and it's a bit hard to search through this PR now

@maximiliankolb
Copy link
Contributor

maximiliankolb commented Nov 29, 2023

I have opened a RFE: https://projects.theforeman.org/issues/36957 and converted this PR to draft so that we do not triage it again.

Does this work for you? @Imaanpreet


[width="100%",cols="50%,50%",options="header",]
|===
|{Project} VM with 8 CPUs, 32 GiB RAM, 3 pulpcore-api_workers |{Project} VM with 8 CPUs, 32 GiB RAM, 10 pulpcore-api_workers
Copy link
Member

Choose a reason for hiding this comment

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

The default tuning for this is min(4, $cpu_count). So the user would have 4 workers. There is no way they get 10 unless they know what they're doing. That makes me question this advice.

Comment on lines +4 to +5
The pulpcore-api workers respond to incoming API requests.
Fewer API workers consume less memory and results in better performance.
Copy link
Member

Choose a reason for hiding this comment

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

This is questionable advice. Not enough workers means you can't serve the requests in parallel. Taking your advice to the extreme means I simply have 1 worker. But with enough clients, you can saturate that and performance is lower than it could be.

The API service is used by Foreman (via Katello), but also by container content. If your tests didn't test any container workloads then you're only testing a fraction of what it needs to serve.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Imaanpreet Is this something you've observed? Do you have any numbers on this?

[id="Configuring_Pulpcore_API-Workers_{context}"]
= Configuring Pulpcore API Workers

The pulpcore-api workers respond to incoming API requests.
Copy link
Member

Choose a reason for hiding this comment

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

And some content types, such as containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants