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

Performance fix for collection version list. #1430

Merged
merged 1 commit into from May 4, 2023

Conversation

jctanner
Copy link
Contributor

@jctanner jctanner commented Apr 25, 2023

https://issues.redhat.com/browse/AAH-2229

On a system with community.general's versions all synced, continuous queries against the list endpoint will consume all available memory until the worker is OOMKilled.

Thanks to @rochacbruno for the reproducer:

$ cat REPRO.sh 
#!/bin/bash

watch -n 1 \
curl -u admin:admin http://localhost:5001/api/automation-hub/v3/plugin/ansible/content/community/collections/index/community/general/versions/\?limit\=200\&offset\=0

@jctanner jctanner marked this pull request as draft April 25, 2023 19:03
@jctanner
Copy link
Contributor Author

jctanner commented Apr 26, 2023

Test failure is due to a caching lookup bug in ansible-core==2.13.9. We'll be waiting on a fix and a release for that before we can move this forward.

ansible/ansible#80648

@jctanner jctanner marked this pull request as ready for review April 30, 2023 23:41
@jctanner jctanner requested a review from gerrod3 April 30, 2023 23:41
functest_requirements.txt Outdated Show resolved Hide resolved
@jctanner jctanner requested a review from mdellweg May 2, 2023 15:14
@@ -858,14 +858,40 @@ def get_list_serializer(self, *args, **kwargs):
kwargs.setdefault("context", self.get_serializer_context)
return self.list_serializer_class(*args, **kwargs)

def get_list_queryset(self):
Copy link
Member

Choose a reason for hiding this comment

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

Is this also beneficial for the detail view? We could just overwrite get_queryset at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not actually sure, but I can see if it breaks any tests.

functest_requirements.txt Outdated Show resolved Hide resolved
@jctanner
Copy link
Contributor Author

jctanner commented May 3, 2023

Collection deletion tests are causing tracebacks when get_queryset is overridden ...

oci_env-pulp-1   | pulp [bf8cde0156fa4c4ea86b5c288bb0f390]: 127.0.0.1 - admin [03/May/2023:15:35:01 +0000] "DELETE /pulp_ansible/galaxy/6452de55-d612-432f-a374-a8c0946c32bf/api/v3/collections/jpmipskp/nbzbxzxi/versions/1.0.1/ HTTP/1.0" 302 0 "-" "OpenAPI-Generator/0.17.0/python"
oci_env-pulp-1   | pulp [64213f3b622a4a53a3142eb4da6b6796]: django.request:ERROR: Internal Server Error: /pulp_ansible/galaxy/6452de55-d612-432f-a374-a8c0946c32bf/api/v3/plugin/ansible/content/6452de55-d612-432f-a374-a8c0946c32bf/collections/index/jpmipskp/nbzbxzxi/versions/1.0.1/
oci_env-pulp-1   | Traceback (most recent call last):
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
oci_env-pulp-1   |     response = get_response(request)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/core/handlers/base.py", line 181, in _get_response
oci_env-pulp-1   |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
oci_env-pulp-1   |     return view_func(*args, **kwargs)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/rest_framework/viewsets.py", line 125, in view
oci_env-pulp-1   |     return self.dispatch(request, *args, **kwargs)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/rest_framework/views.py", line 509, in dispatch
oci_env-pulp-1   |     response = self.handle_exception(exc)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/rest_framework/views.py", line 469, in handle_exception
oci_env-pulp-1   |     self.raise_uncaught_exception(exc)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
oci_env-pulp-1   |     raise exc
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/rest_framework/views.py", line 506, in dispatch
oci_env-pulp-1   |     response = handler(request, *args, **kwargs)
oci_env-pulp-1   |   File "/src/pulp_ansible/pulp_ansible/app/galaxy/v3/views.py", line 948, in destroy
oci_env-pulp-1   |     exclusive_resources=collection_version.repositories.all(),
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 536, in __get__
oci_env-pulp-1   |     return self.related_manager_cls(instance)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 846, in __init__
oci_env-pulp-1   |     self.core_filters[core_filter_key] = getattr(instance, rh_field.attname)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/db/models/query_utils.py", line 142, in __get__
oci_env-pulp-1   |     val = self._check_parent_chain(instance)
oci_env-pulp-1   |   File "/usr/local/lib/python3.8/site-packages/django/db/models/query_utils.py", line 158, in _check_parent_chain
oci_env-pulp-1   |     return getattr(instance, link_field.attname)
oci_env-pulp-1   | AttributeError: 'NoneType' object has no attribute 'attname'

@jctanner jctanner requested a review from mdellweg May 3, 2023 16:41
# prevent OOMKILL ...
queryset = queryset.only("pk", "content_ptr_id")

queryset = self.filter_queryset(queryset)
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear to me why filter_queryset may already OOM, because it should really only apply filters to the queryset, not evaluate it. And I don't see anything obvious in either inherited filter_queryset nor the CollectionVersionFilter class. Does the paginated version OOM too? Then I would suspect this fix will just by us some time until these two fields are able to fill the memory again. Approving anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add some of these thought to the comment to spark further investigation.

fixes: pulp#1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
@mdellweg mdellweg merged commit a52dc98 into pulp:main May 4, 2023
14 checks passed
jctanner added a commit to jctanner/pulp_ansible that referenced this pull request May 4, 2023
)

fixes: pulp#1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
jctanner added a commit to jctanner/pulp_ansible that referenced this pull request May 4, 2023
)

fixes: pulp#1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
(cherry picked from commit a52dc98)
jctanner added a commit to jctanner/pulp_ansible that referenced this pull request May 5, 2023
)

fixes: pulp#1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
(cherry picked from commit a52dc98)
jctanner added a commit to jctanner/pulp_ansible that referenced this pull request May 9, 2023
)

fixes: pulp#1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
(cherry picked from commit a52dc98)
gerrod3 pushed a commit that referenced this pull request May 9, 2023
…#1444)

fixes: #1433

Signed-off-by: James Tanner <tanner.jc@gmail.com>
(cherry picked from commit a52dc98)
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

3 participants