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

Increase the default worker count to 4 #296

Closed
wants to merge 1 commit into from
Closed

Conversation

dralley
Copy link
Contributor

@dralley dralley commented May 15, 2020

2 workers is quite a low default, 4 is probably more reasonable for most users.

[noissue]

@pulpbot
Copy link
Member

pulpbot commented May 15, 2020

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

Copy link
Member

@fao89 fao89 left a comment

Choose a reason for hiding this comment

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

LGTM, but I feel we need a changelog entry for this change

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Looks good, and definitely beneficial for our users, but I am concerned about the memory usage on CI.

We run against 3 distro containers (ansible molecule) at a time in a job, so that's 3 instances of each other process, plus now 12 instances of pulp-worker. The 8GB VM is likely swapping.

@dralley
Copy link
Contributor Author

dralley commented May 19, 2020

@mikedep333 Do you have any ideas about how we could minimize or avoid those issues?

@bmbouter
Copy link
Member

Personally I think shipping the smaller default is better. Users need to set playbook options often so users can change the pulp_workers var pretty easily I think.

@dralley
Copy link
Contributor Author

dralley commented May 19, 2020

In that case, I can close this issue. However, it would be really nice if the user could just specify the number of workers to create, rather than copying and pasting and changing the numbers. Would there be a way to do that?

@dralley dralley closed this May 19, 2020
@bmbouter
Copy link
Member

@dralley I agree it needs to be that easy. I think the user's playbook can specify: pulp_workers: N from these docs here: https://pulp-installer.readthedocs.io/en/latest/roles/pulp_workers/

@dralley
Copy link
Contributor Author

dralley commented May 19, 2020

@bmbouter If you look at this PR, which uses the pulp_workers variable, I don't think it is actually that easy at the moment. Unless I'm misunderstanding, you have to provide a little configuration for each worker, and that is how the number of workers is determined.

@bmbouter
Copy link
Member

I think we need to make it that easy, and that is the installer improvement to make. @dralley what do you think?

@dralley
Copy link
Contributor Author

dralley commented May 19, 2020

I definitely agree, I just wanted to be clear that I don't think we're already there :) I might have misinterpreted #296 (comment) if that wasn't what you were saying.

@bmbouter
Copy link
Member

I was wrong. I thought we were already there from the docstring that says N workers. I missed the part saying it needs a complex data structure. I filed this https://pulp.plan.io/issues/6774 and put it on the installer agenda for tomorrow.

@dralley
Copy link
Contributor Author

dralley commented May 19, 2020

Awesome, thank you!

@bmbouter
Copy link
Member

Thank you for bringing it up!

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.

None yet

5 participants