Issue #1662
closed
RepoUnitAssociationManager._units_from_criteria does not use unit_fields from the criteria
Description
Performing a unit copy to copy all the rpms a large repo (10,000 rpms) to another repo causes the pulp worker to consumer a large amount of memory. In my case it got up to 2.5 GB.
Steps to reproduce:
1) Create and sync a large repo, such as rhel 7 or rhel 6
2) Create a 2nd repo that is empty
3) unit copy all the rpms from one repo to the other.
I can provide the exact api call if needed, but was able to reproduce with this pulp-admin command:
pulp-admin -u admin -p password rpm repo copy rpm --from-repo-id="large-repo" --to-repo-id=empty-repo
On my box this causes the OOM killer to kick in and kill the worker, so i'm not entirely sure how much memory it will eventually consume.
Updated by rbarlow almost 9 years ago
- Status changed from NEW to ASSIGNED
- Assignee set to rbarlow
- Triaged changed from No to Yes
Justin has unreasonably refused to buy several more tens of GBs of RAM, so I guess I'll see if I can figure out what is going on.
Updated by rbarlow almost 9 years ago
I've determined that this line is where the OOM killer dies on my dev box:
Fixing it may or may not be easy, as it was using a set for de-duplication, which may be difficult to accomplish with a generator.
Updated by rbarlow almost 9 years ago
I've got an experimental fix that keeps the RHEL 7 copy under 190 MB on my dev box. It needs more testing, but I may be able to have a PR on Monday or Tuesday.
Updated by rbarlow almost 9 years ago
- Status changed from ASSIGNED to POST
I've submitted a WIP pull request with my experimental fix. I will perform more testing before I will mark it as ready for real review, but I would appreciate eyes on the code that I am considering for this fix:
Updated by mhrivnak almost 9 years ago
pulp-admin is still sending the list of "fields" in the criteria when it makes the associate call, so it's likely that the platform is ignoring that. Assuming that's the case, we should try to fix this in the platform.
$ pulp-admin -vv rpm repo copy rpm -f foo -t bar
2016-02-15 19:03:00,641 - DEBUG - sending POST request to /pulp/api/v2/repositories/bar/actions/associate/
2016-02-15 19:03:00,646 - INFO - POST request to /pulp/api/v2/repositories/bar/actions/associate/ with parameters {"source_repo_id": "foo", "override_config": {}, "criteria": {"fields": {"unit": ["name", "epoch", "version", "release", "arch", "checksum", "checksumtype"]}, "type_ids": ["rpm"], "filters": {"unit": {}}}}
Updated by bmbouter almost 9 years ago
If we fix it in platform, we could chain a fields() call onto the queryset which would be easy and achieve the goal (if I understand this correctly).
Added by rbarlow almost 9 years ago
Added by rbarlow almost 9 years ago
Revision 41f4d255 | View on GitHub
Use the unit_fields from criteria in unit_association.
pulp.server.managers.repo.unit_association.RepoUnitAssociationManager._units_from_criteria() was not passing the criteria's unit_fields list along to the next callee. This caused significant memory usage in certian Pulp operations. This commit corrects that oversight.
https://pulp.plan.io/issues/1662
fixes #1662
Updated by rbarlow almost 9 years ago
- Project changed from RPM Support to Pulp
- Subject changed from Unit copy on a large repo consumes large amounts of memory to RepoUnitAssociationManager._units_from_criteria does not use unit_fields from the criteria
When I started out on this, I had forgotten the hacky requirement for API callers to tell us a list of fields when they perform a copy. I still think that's hacky and is truly a bug in itself, but it turned out that that mongoengine conversion had stopped paying attention to that field inside pulp.server.managers.repo.unit_association.RepoUnitAssociationManager._units_from_criteria().
We definitely need to fix that code path as it is used by other things too, but I still think users shouldn't have to tell us a list of fields when copying units. We may be able to slap an if statement around the code in my other pull request to make it OK when users do this with RPMs, but it won't be a general fix. Switching to postgres will make fixing this the right way easy.
This PR addresses the issue in platform:
Updated by rbarlow almost 9 years ago
- Status changed from POST to MODIFIED
- % Done changed from 0 to 100
Applied in changeset pulp|41f4d255659f7c2d02ed8adf9a76569f7f5f1e84.
Updated by dkliban@redhat.com over 8 years ago
- Status changed from MODIFIED to 5
Updated by dkliban@redhat.com over 8 years ago
- Status changed from 5 to CLOSED - CURRENTRELEASE
.
Use the unit_fields from criteria in unit_association.
pulp.server.managers.repo.unit_association.RepoUnitAssociationManager._units_from_criteria() was not passing the criteria's unit_fields list along to the next callee. This caused significant memory usage in certian Pulp operations. This commit corrects that oversight.
https://pulp.plan.io/issues/1662
fixes #1662