Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES/7831.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Added an optional ``base_version`` filter to the content list endpoints. When combined with
``repository_version_added`` or ``repository_version_removed``, it returns the net set of content
added or removed between two arbitrary repository versions instead of only the single-step
difference against the filtered version's immediate predecessor.
31 changes: 22 additions & 9 deletions pulpcore/app/models/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -1000,13 +1000,26 @@ def get_content(self, content_qs=None):
content_ids = self.content_ids
if len(content_ids) >= 65535:
# Workaround for PostgreSQL's limit on the number of parameters in a query
content_ids = (
RepositoryVersion.objects.filter(pk=self.pk)
.annotate(cids=Func(F("content_ids"), function="unnest"))
.values_list("cids", flat=True)
)
content_ids = self.content_ids_subquery()
return content_qs.filter(pk__in=content_ids)

def content_ids_subquery(self):
"""
Return this version's ``content_ids`` as a database-side ``unnest`` subquery.

Using a subquery keeps the content unit UUIDs inside PostgreSQL instead of loading the
whole array into Python and passing each UUID as a bound query parameter. This avoids the
per-query parameter limit and the memory/serialization cost for large repository versions.

Returns:
django.db.models.QuerySet: A values queryset yielding the content unit UUIDs.
"""
return (
RepositoryVersion.objects.filter(pk=self.pk)
.annotate(cids=Func(F("content_ids"), function="unnest"))
.values_list("cids", flat=True)
)

@property
def content(self):
"""
Expand Down Expand Up @@ -1119,8 +1132,8 @@ def added(self, base_version=None):
if not base_version:
return Content.objects.filter(version_memberships__version_added=self)

return Content.objects.filter(pk__in=self.content_ids).exclude(
pk__in=base_version.content_ids
return Content.objects.filter(pk__in=self.content_ids_subquery()).exclude(
pk__in=base_version.content_ids_subquery()
Comment on lines +1135 to +1136

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sounds like the real improvement here.
Can we even mark content_ids as deferred so that the humongous list never makes it to python in the first place? (Maybe that's already the case...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also does this change already improve the q filter scenario?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll try to sum what we talked about at our pulpcore meeting but generally we agree on investigating these two items here. For the latter, we think that the q filter could/should be using subqueries. If it's not, it's worth understanding why it's not. Fixing that should alleviate the performance problem.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure if this will correctly answer the question of whether the subquery I used here improves q performance, but:

  • The filter we pass through q originally does eventually reach the ContentRepositoryVersionFilter logic and calls get_content(). However, there's a condition there where a subquery strategy is only used if a version has more than 65535 units. Below that, it's still inlining the content id list.
  • q also has a different query shape compared to the implementation here that directly targets the repository version table. The NOT repository_version=y part becomes Content.objects.all() EXCEPT (content in Y), which means it's still running through the entire table
  • The base_version implementation here works around the expression filter strategy that is q (which I presume has other uses) and goes straight to filter(pk__in=X).exclude(pk__in=Y). The subquerying optimizes this filter path to be sure, but I'm hesitant in changing things related to q because I don't fully understand its other uses.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • However, there's a condition there where a subquery strategy is only used if a version has more than 65535 units.

I always wondered why we should have this in the first place. I cannot think of the subquery on an small or empty table being an issue at all.

)

def removed(self, base_version=None):
Expand All @@ -1134,8 +1147,8 @@ def removed(self, base_version=None):
if not base_version:
return Content.objects.filter(version_memberships__version_removed=self)

return Content.objects.filter(pk__in=base_version.content_ids).exclude(
pk__in=self.content_ids
return Content.objects.filter(pk__in=base_version.content_ids_subquery()).exclude(
pk__in=self.content_ids_subquery()
)

def contains(self, content):
Expand Down
6 changes: 6 additions & 0 deletions pulpcore/app/viewsets/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
ContentAddedRepositoryVersionFilter,
ContentRemovedRepositoryVersionFilter,
ContentRepositoryVersionFilter,
RepositoryVersionBaseFilter,
)


Expand Down Expand Up @@ -125,6 +126,10 @@ class ContentFilter(BaseFilterSet):
Return Content which was added in this repository version.
repository_version_removed:
Return Content which was removed from this repository version.
base_version:
When combined with repository_version_added / repository_version_removed, compute the
net difference relative to this base repository version instead of the filtered
version's immediate predecessor. Has no effect on its own.
orphaned_for:
Return Content which has been orphaned for a given number of minutes;
-1 uses ORPHAN_PROTECTION_TIME value.
Expand All @@ -135,6 +140,7 @@ class ContentFilter(BaseFilterSet):
repository_version = ContentRepositoryVersionFilter()
repository_version_added = ContentAddedRepositoryVersionFilter()
repository_version_removed = ContentRemovedRepositoryVersionFilter()
base_version = RepositoryVersionBaseFilter()
orphaned_for = OrphanedFilter(
help_text="Minutes Content has been orphaned for. -1 uses ORPHAN_PROTECTION_TIME."
)
Expand Down
51 changes: 49 additions & 2 deletions pulpcore/app/viewsets/custom_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,53 @@ def filter(self, qs, value):
"""
raise NotImplementedError()

def get_base_version(self, field_name="base_version"):
"""
Resolve the companion ``base_version`` filter, if the request supplied one.

The added/removed filters use this to diff against an *arbitrary* base repository
version instead of the filtered version's immediate predecessor.

Args:
field_name (string): The name of the companion base-version filter on the filterset.

Returns:
pulpcore.app.models.RepositoryVersion or None: The resolved base version, or None
when no base version was supplied.
"""
if self.parent is None:
return None
base_value = self.parent.form.cleaned_data.get(field_name)
if not base_value:
return None
return self.get_repository_version(base_value)


class RepositoryVersionBaseFilter(RepoVersionHrefPrnFilter):
"""
Companion filter that designates the base repository version for
``repository_version_added`` / ``repository_version_removed`` diffs.

On its own this filter does not alter the queryset. It is consumed by the added/removed
filters to compute the net difference between two arbitrary repository versions, rather than
the single-step difference against the filtered version's immediate predecessor.
"""

def __init__(self, *args, **kwargs):
kwargs.setdefault(
"help_text",
_(
"Repository Version referenced by HREF/PRN to use as the base for "
"repository_version_added / repository_version_removed. When set, added/removed "
"content is computed relative to this version instead of the immediate predecessor."
),
)
super().__init__(*args, **kwargs)

def filter(self, qs, value):
# No-op on its own; the value is consumed by the added/removed filters.
return qs


class RepositoryVersionFilter(RepoVersionHrefPrnFilter):
"""
Expand Down Expand Up @@ -251,7 +298,7 @@ def filter(self, qs, value):
return qs

repo_version = self.get_repository_version(value)
return qs.filter(pk__in=repo_version.added())
return qs.filter(pk__in=repo_version.added(base_version=self.get_base_version()))


class ContentRemovedRepositoryVersionFilter(RepoVersionHrefPrnFilter):
Expand All @@ -273,7 +320,7 @@ def filter(self, qs, value):
return qs

repo_version = self.get_repository_version(value)
return qs.filter(pk__in=repo_version.removed())
return qs.filter(pk__in=repo_version.removed(base_version=self.get_base_version()))


class CharInFilter(BaseInFilter, CharFilter):
Expand Down
76 changes: 76 additions & 0 deletions pulpcore/tests/functional/api/using_plugin/test_repo_versions.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,82 @@ def test_add_remove_content(
assert latest_version.content_summary.removed == {}


@pytest.fixture
def file_repo_three_versions(
file_bindings,
file_repository_factory,
file_9_contents,
monitor_task,
):
"""Build a repo with three versions whose diffs span more than one step.

Using the content units "A" through "E", the versions end up as::

v1: add A, B, C -> present {A, B, C}
v2: add D, remove A -> present {B, C, D}
v3: add E, remove B -> present {C, D, E}

So a diff between the non-adjacent versions v1 and v3 differs from the single-step diff at v3.
"""
repo = file_repository_factory()
contents = file_9_contents

def modify(add=(), remove=()):
body = {
"add_content_units": [contents[name].pulp_href for name in add],
"remove_content_units": [contents[name].pulp_href for name in remove],
}
monitor_task(file_bindings.RepositoriesFileApi.modify(repo.pulp_href, body).task)
return file_bindings.RepositoriesFileApi.read(repo.pulp_href).latest_version_href

v1 = modify(add=["A", "B", "C"])
v2 = modify(add=["D"], remove=["A"])
v3 = modify(add=["E"], remove=["B"])
return repo, v1, v2, v3


def _relative_paths(list_response):
return {content.relative_path for content in list_response.results}


@pytest.mark.parallel
def test_repository_version_added_base_version(file_bindings, file_repo_three_versions):
"""``base_version`` turns ``repository_version_added`` into a net diff between two versions."""
_, v1, v2, v3 = file_repo_three_versions

# Without base_version: single-step diff against the immediate predecessor (v2 -> v3).
single_step = file_bindings.ContentFilesApi.list(repository_version_added=v3)
assert _relative_paths(single_step) == {"E"}

# With base_version=v1: net content in v3 but not in v1 => {C, D, E} - {A, B, C}.
net = file_bindings.ContentFilesApi.list(repository_version_added=v3, base_version=v1)
assert _relative_paths(net) == {"D", "E"}


@pytest.mark.parallel
def test_repository_version_removed_base_version(file_bindings, file_repo_three_versions):
"""``base_version`` turns ``repository_version_removed`` into a net diff across versions."""
_, v1, v2, v3 = file_repo_three_versions

# Without base_version: single-step diff against the immediate predecessor (v2 -> v3).
single_step = file_bindings.ContentFilesApi.list(repository_version_removed=v3)
assert _relative_paths(single_step) == {"B"}

# With base_version=v1: net content in v1 but not in v3 => {A, B, C} - {C, D, E}.
net = file_bindings.ContentFilesApi.list(repository_version_removed=v3, base_version=v1)
assert _relative_paths(net) == {"A", "B"}


@pytest.mark.parallel
def test_base_version_ignored_without_added_or_removed(file_bindings, file_repo_three_versions):
"""``base_version`` has no effect unless paired with added/removed filters."""
_, v1, v2, v3 = file_repo_three_versions

present = file_bindings.ContentFilesApi.list(repository_version=v3)
present_with_base = file_bindings.ContentFilesApi.list(repository_version=v3, base_version=v1)
assert _relative_paths(present_with_base) == _relative_paths(present) == {"C", "D", "E"}


@pytest.mark.parallel
def test_add_remove_repo_version(
file_bindings,
Expand Down
Loading