Improve performance of duplicate removal, and prevent content from contaminating typed repos #441

dralley · 2019-12-05T19:06:04Z

No description provided.

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

fao89

This is really great work!

fao89 · 2019-12-05T19:38:22Z

pulpcore/plugin/repo_version_utils.py

+                    field: getattr(content_unit, field) for field in type_obj.repo_key_fields
+                }
+                item_query = Q(**unit_q_dict) & ~Q(pk=content_unit.pk)
+                find_dup_qs |= item_query


I would do a batch queryset here, RHEL 7 has about 27000 packages, so this query would result in 27000 objects + 27000 unit_q_dicts

fao89 · 2019-12-05T19:47:43Z

pulpcore/plugin/repo_version_utils.py

+
+        if new_content_qs.count():
+            find_dup_qs = Q()
+            for content_unit in new_content_qs:


if you do:

Suggested change

for content_unit in new_content_qs:

repo_key_fields_with_pk = type_obj.repo_key_fields + ('pk',)

for content_unit in new_content_qs.values(*repo_key_fields_with_pk):

it would already bring the result in dicts, so you won't need to do a dict comprehension for each item

pk = str(content_unit.pop("pk")) item_query = Q(**content_unit) & ~Q(pk=pk)

fao89

🚀

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley · 2019-12-06T02:19:14Z

pulpcore/tests/unit/models/test_repo_version.py

@@ -1,47 +0,0 @@
-from unittest.mock import patch


All this is moved to pulp_file. It has to be, because "core.content" is no longer a valid type of content to be in a repository, and you're not supposed to be creating a generic "Repository" anymore either.

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

Update ProgressReport for batches of content instead of individually. ProgressReport updating took 30% of the runtime of syncs, now 1%. Required PR: pulp/pulpcore#441 [noissue]

re: #5701 https://pulp.plan.io/issues/5701

gmbnomis · 2019-12-07T20:15:35Z

pulpcore/plugin/repo_version_utils.py

+
+                for content_dict in batch:
+                    content_pk = content_dict.pop('pk')
+                    item_query = Q(**content_dict) & ~Q(pk=content_pk)


Do we need & ~Q(pk=content_pk) here? It looks like these pks are filtered out by .filter(pk__in=existing_content) below anyway?

Fair point, probably not.

gmbnomis · 2019-12-07T20:17:19Z

pulpcore/plugin/repo_version_utils.py

+    repository = repository_version.repository.cast()
+    content_types = {ctype.get_pulp_type(): ctype for ctype in repository.CONTENT_TYPES}
+
+    for pulp_type in content_types.keys():


Suggested change

for pulp_type in content_types.keys():

for pulp_type, type_obj in content_types.items():

and remove the next line.

closes: #5701 https://pulp.plan.io/issues/5701

It responsible for 13% of sync/resync runtime, and provides no value. The name of the content is external and would have no translation, and the name of the stage should not be translated, as it should correspond with the source code. [noissue]

Reduce code duplication for a common pattern [noissue]

dralley · 2019-12-08T20:12:24Z

Comments addressed

gmbnomis · 2019-12-08T20:21:50Z

pulpcore/plugin/repo_version_utils.py

+
+                duplicates_qs = type_obj.objects.filter(pk__in=existing_content)\
+                                                .filter(find_dup_qs)\
+                                                .only('pk')


This (.only('pk')) is really neat. I must remember to put it my toolbox.

gmbnomis · 2019-12-08T20:23:07Z

Comments addressed

@dralley this looks really great!

dralley · 2019-12-08T20:28:04Z

pulpcore/app/models/repository.py

                no_change = not self.added() and not self.removed()
                if no_change:
                    self.delete()
                else:
+                    content_types_seen = set(
+                        self.content.values_list('pulp_type', flat=True).distinct()


We might be able to get away with just checking the content used in the added content units. But I think I'll let that wait until I can start looking at the query generation. We'll definitely be doing more optimization work post-GA

Required PR: pulp/pulpcore#441 [noissue]

Update ProgressReport for batches of content instead of individually. ProgressReport updating took 30% of the runtime of syncs, now 1%. Required PR: pulp/pulpcore#441 [noissue]

bmbouter · 2019-12-09T18:02:07Z

pulpcore/plugin/stages/association_stages.py

@@ -79,7 +78,6 @@ def __init__(self, new_version, *args, **kwargs):
        with ProgressReport(message='Un-Associating Content', code='unassociating.content') as pb:
            async for queryset_to_unassociate in self.items():
                self.new_version.remove_content(queryset_to_unassociate)
-                pb.done = pb.done + queryset_to_unassociate.count()
-                pb.save()
+                pb.increase_by(queryset_to_unassociate.count())


Should we remove the auto-throttling from pulpcore's ProgressReport context manager? I'm concerned that we already have a mechanism (auto-throttle saving) that handles this, but we're side stepping that both here and in pulp_rpm.

We're not actually side stepping that here, the mechanism is inside of .save(), and .increase_by() calls .save(), so it's functionally equivalent.

In pulp_rpm, yes, I agree.

That's true. Still I'm getting the feeling that auto-throttling is an anti-feature. It's here to help plugin writers to not think about these things, but in practice plugin writers probably need to think about their data processing in batches. The throttling is here to serve plugin writers, but with plugins avoiding it I don't see who its serving. I'm +1 to remove it either in this PR or as a separate issue. It can be in 3.1 also it doesn't matter to me when. It's important to me that we don't have core offering a feature in its plugin API that isn't a good fit for plugin writers.

So can we remove this? and if so shall I file a separate story to?

Well, pulp_file is using it, pulp_cookbook uses it, pulp_container uses it in one place (but not another), and pulp_rpm is using it now (updated my PR). I'm not sure the statement that plugins are avoiding it is justified.

With that said, yeah, it does seem a bit hacky. I could go either way with removing it.

Well, pulp_file was using it, pulp_cookbook uses it, pulp_container uses it in one place (but not another), and pulp_rpm is using it now. I'm not sure the statement that plugins are avoiding it is justified.

It's accurate to say that plugins managing their own updates are not relying on pulpcore to provide that feature and avoiding its use.

With that said, yeah, it does seem a bit hacky. I could go either way with removing it.

I filed https://pulp.plan.io/issues/5855 and started a discussion on pulp-dev here for feedback: https://www.redhat.com/archives/pulp-dev/2019-December/msg00032.html

Update ProgressReport for batches of content instead of individually. ProgressReport updating took 30% of the runtime of syncs, now 1%. Required PR: pulp/pulpcore#441 [noissue]

bmbouter · 2019-12-09T20:19:57Z

I read through again and it looks good to me. Thank you @dralley and @gmbnomis

Use auto-throttling behavior of ProgressReport context manager. Required PR: pulp/pulpcore#441 [noissue]

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701 (cherry picked from commit 140d849)

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701 (cherry picked from commit 3caded8)

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 5, 2019

Add CONTENT_TYPES to repo definition

d476219

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 5, 2019

Add CONTENT_TYPES to repo definition

27d7156

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley force-pushed the performance branch from c6671f8 to 2a8f5d4 Compare December 5, 2019 19:11

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 5, 2019

Add CONTENT_TYPES to repo definition

1b7f832

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 5, 2019

Add CONTENT_TYPES to repo definition

6a43109

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley force-pushed the performance branch from 2a8f5d4 to ed369d6 Compare December 5, 2019 19:22

fao89 reviewed Dec 5, 2019

View reviewed changes

dralley force-pushed the performance branch 2 times, most recently from febc7ed to c965be0 Compare December 5, 2019 19:42

fao89 reviewed Dec 5, 2019

View reviewed changes

dralley force-pushed the performance branch 3 times, most recently from a7a9098 to 2bba500 Compare December 5, 2019 20:29

fao89 approved these changes Dec 5, 2019

View reviewed changes

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

e8a70d9

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley force-pushed the performance branch from 2bba500 to 5638573 Compare December 6, 2019 02:13

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

44ec341

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

e055cf2

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley commented Dec 6, 2019

View reviewed changes

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

3204b0a

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

fd21e06

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

418babc

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

fb96936

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

44f1b5c

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley mentioned this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition pulp/pulp_python#264

Merged

dralley added a commit to dralley/pulp_maven that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

6b2e200

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_maven that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

fc76024

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley mentioned this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition pulp/pulp_maven#27

Merged

dralley added a commit to dralley/pulp_maven that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

fb6d52c

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley added a commit to dralley/pulp_python that referenced this pull request Dec 6, 2019

Add CONTENT_TYPES to repo definition

47401bc

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701

dralley force-pushed the performance branch from ad12465 to c2bcdda Compare December 7, 2019 17:05

Add additional content type verification to RepositoryVersion

0604a7b

re: #5701 https://pulp.plan.io/issues/5701

gmbnomis reviewed Dec 7, 2019

View reviewed changes

dralley added 3 commits December 8, 2019 11:11

Improve performance of removing duplicates

1861257

closes: #5701 https://pulp.plan.io/issues/5701

Add increase_by() function to ProgressReport

17b01bf

Reduce code duplication for a common pattern [noissue]

dralley force-pushed the performance branch from c2bcdda to 17b01bf Compare December 8, 2019 19:46

gmbnomis reviewed Dec 8, 2019

View reviewed changes

dralley commented Dec 8, 2019

View reviewed changes

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 9, 2019

Increase sync speed by saving less progress reports

c9a12c4

Required PR: pulp/pulpcore#441 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 9, 2019

Increase sync speed by saving less progress reports

1df8e8c

Required PR: pulp/pulpcore#441 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 9, 2019

Increase sync speed by saving less progress reports

604e727

Required PR: pulp/pulpcore#441 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 9, 2019

Increase sync speed by saving less progress reports

91aac9d

Required PR: pulp/pulpcore#441 [noissue]

dralley added a commit to dralley/pulp_file that referenced this pull request Dec 9, 2019

Increase sync speed by saving less progress reports

4b92de7

Required PR: pulp/pulpcore#441 [noissue]

bmbouter reviewed Dec 9, 2019

View reviewed changes

dralley merged commit e00e864 into pulp:master Dec 9, 2019

dralley deleted the performance branch December 9, 2019 20:19

dralley added a commit to dralley/pulp_rpm that referenced this pull request Dec 9, 2019

Improve performance by cutting way down on ProgressReport updating

0072929

Use auto-throttling behavior of ProgressReport context manager. Required PR: pulp/pulpcore#441 [noissue]

goosemania pushed a commit to goosemania/pulp_rpm that referenced this pull request Dec 10, 2019

Add CONTENT_TYPES to repo definition

f4f190c

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701 (cherry picked from commit 140d849)

ipanova pushed a commit to ipanova/pulp_container that referenced this pull request Dec 11, 2019

Add CONTENT_TYPES to repo definition

cc2fb46

Required PR: pulp/pulpcore#441 re: #5701 https://pulp.plan.io/issues/5701 (cherry picked from commit 3caded8)

Improve performance of duplicate removal, and prevent content from contaminating typed repos #441

Improve performance of duplicate removal, and prevent content from contaminating typed repos #441

dralley commented Dec 5, 2019

fao89 left a comment

fao89 Dec 5, 2019

fao89 Dec 5, 2019

fao89 left a comment

dralley Dec 6, 2019

gmbnomis Dec 7, 2019 •

edited

dralley Dec 7, 2019

gmbnomis Dec 7, 2019

dralley commented Dec 8, 2019

gmbnomis Dec 8, 2019 •

edited

gmbnomis commented Dec 8, 2019

dralley Dec 8, 2019

bmbouter Dec 9, 2019

dralley Dec 9, 2019

bmbouter Dec 9, 2019

dralley Dec 9, 2019 •

edited

bmbouter Dec 9, 2019

bmbouter commented Dec 9, 2019

	for content_unit in new_content_qs:
	repo_key_fields_with_pk = type_obj.repo_key_fields + ('pk',)
	for content_unit in new_content_qs.values(*repo_key_fields_with_pk):

	for pulp_type in content_types.keys():
	for pulp_type, type_obj in content_types.items():

Improve performance of duplicate removal, and prevent content from contaminating typed repos #441

Improve performance of duplicate removal, and prevent content from contaminating typed repos #441

Conversation

dralley commented Dec 5, 2019

fao89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmbnomis Dec 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented Dec 8, 2019

gmbnomis Dec 8, 2019 • edited

Choose a reason for hiding this comment

gmbnomis commented Dec 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley Dec 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Dec 9, 2019

gmbnomis Dec 7, 2019 •

edited

gmbnomis Dec 8, 2019 •

edited

dralley Dec 9, 2019 •

edited