Fix package repo uniqueness #922

quba42 · 2023-10-26T16:16:12Z

No description provided.

quba42 · 2023-10-31T13:53:20Z

I have now tested that this code works for at least some of the cases that prompted this change.
I will now start working on systematic test coverage. In particular we need to cover the following cases:

Sync and/or upload of a package set that contains forbidden duplicates => Must raise ValueError
Sync and/or upload of a package set that contains allowed duplicates => Must complete and must not discard any duplicates
Sync and/or upload of a package that collides with a already present package => previously added colliding package must be removed from the repo version (along with any PRC that reference it).
Sync and/or upload of a package that is a allowed duplicate of a already present package => both packages must remain.

quba42 · 2023-10-31T13:56:10Z

pulp_deb/app/models/repository.py

+
+                for content_dict in batch:
+                    sha256 = content_dict.pop("sha256")
+                    item_query = models.Q(**content_dict) & ~models.Q(sha256=sha256)


This is most likely the critical line for performance. This change must not cause a significant performance degradation!

pulp_deb/app/models/repository.py

daviddavis · 2023-11-01T12:09:27Z

I'm grateful that you tested out the use case where a package replaces an existing existing package and removes the PackageReleaseComponent. Thank you for that.

It sounds like the issue is that you have two packages with the same NVA but different paths. Is it not possible to solve this problem by adding relative_path to repo_key_fields?

quba42 · 2023-11-02T09:56:10Z

It sounds like the issue is that you have two packages with the same NVA but different paths. Is it not possible to solve this problem by adding relative_path to repo_key_fields?

The problem is, that a duplicate package (same package, version, architecture fields, but different relative_path) are only allowed, if they have an identical sha256, that is, they really are the same package, stored in two different pool folder locations (for "reasons"). If they have differing sha256, they are not allowed to exist in the same repo twice. The spec is pretty clear on that. So the key difference between what I built here, and the pulpcore's remove_duplicates is the following line:

item_query = models.Q(**content_dict) & ~models.Q(sha256=sha256)  # For pulp_deb vs.:
item_query = Q(**content_dict) # In remove_duplicates from pulpcore

The same issue exists for pulpcore's validate_duplicate_content (used for checking the incoming content for duplicates). Some time ago I dealt with that part of the problem by not using validate_duplicate_content for pulp_deb, because it was preventing synchronization of clearly legal repositories. With hindsight, it would have been better if I had used that occasion to fully understand the problem (rather than trial and error fixing the symptoms). Not using validate_duplicate_content at all solved one problem but caused/masked others. This PR essentially adds a deb specific version of validate_duplicate_content back in. So in that way this PR makes duplicate checking more strict.

quba42 · 2023-11-02T11:36:02Z

@daviddavis I have been wondering if there is a case to be made, to be stricter for uploads than for sync. For example: It is legal for the pool folder to contain an identical copy of some package in two places, but I would not exactly call it good practice. For the sync we must be able to sync such legal repositories. However, we could choose to protect our upload users from shooting themselves in the foot, by making this forbidden for uploads. Of course, we also support mixing uploaded and synced content in a single Pulp repository so such discrimination could quickly get complicated. For now I am leaning towards not doing anything like this, but I would be interested to hear your thoughts.

daviddavis · 2023-11-03T13:49:16Z

@quba42 in terms of our use case, we don't allow users to set the relative_path when they upload. So it's not a problem for us I don't think. But I think being stricter for uploads makes sense. And I think postponing that work until it becomes an issue also makes sense.

pulp_deb/app/models/repository.py

closes pulp#921 As a result of the behaviour fixes, we can also drop the now superfluous duplicate distribution checking. We are using validate_duplicate_content to catch incoming duplicates, and we are removing old duplicates, so it is not possible to run into this error. As a result it is not worth the performance cost to check for it.

[noissue]

patchback · 2023-11-16T20:15:43Z

Backport to 3.0: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.0/7ede5bed2dfd9caa5c3722ed837821eda5052106/pr-922

Backported as #947

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2023-11-16T20:15:43Z

Backport to 3.1: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.1/7ede5bed2dfd9caa5c3722ed837821eda5052106/pr-922

Backported as #948

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Fix package repo uniqueness (cherry picked from commit 7ede5be)

…d9caa5c3722ed837821eda5052106/pr-922 [PR #922/7ede5bed backport][3.0] Fix package repo uniqueness

…d9caa5c3722ed837821eda5052106/pr-922 [PR #922/7ede5bed backport][3.1] Fix package repo uniqueness

quba42 force-pushed the fix_package_repo_uniqueness branch from 20a64c7 to 45098d9 Compare October 30, 2023 15:38

quba42 linked an issue Oct 30, 2023 that may be closed by this pull request

pulp_deb duplicate package handling is erroneous #921

Closed

quba42 force-pushed the fix_package_repo_uniqueness branch from 45098d9 to 44ca24f Compare October 31, 2023 13:39

quba42 commented Oct 31, 2023

View reviewed changes

pulp_deb/app/models/repository.py Show resolved Hide resolved

quba42 force-pushed the fix_package_repo_uniqueness branch from 44ca24f to 387a181 Compare October 31, 2023 14:43

quba42 mentioned this pull request Oct 31, 2023

Expose batch_qs() to the plugin API pulp/pulpcore#4607

Closed

daviddavis reviewed Nov 3, 2023

View reviewed changes

pulp_deb/app/models/repository.py Show resolved Hide resolved

quba42 added .bugfix CHANGES/<issue_number>.bugfix Tests labels Nov 13, 2023

quba42 force-pushed the fix_package_repo_uniqueness branch from 387a181 to 2e2d11f Compare November 13, 2023 09:31

quba42 force-pushed the fix_package_repo_uniqueness branch from 2e2d11f to 00c3656 Compare November 15, 2023 14:02

quba42 marked this pull request as ready for review November 15, 2023 14:03

Add duplicate package tests

6c97a75

[noissue]

quba42 force-pushed the fix_package_repo_uniqueness branch from 00c3656 to 6c97a75 Compare November 16, 2023 16:28

hstct approved these changes Nov 16, 2023

View reviewed changes

quba42 merged commit 7ede5be into pulp:main Nov 16, 2023
8 checks passed

quba42 added backport-3.0 labels Nov 16, 2023

patchback bot pushed a commit that referenced this pull request Nov 16, 2023

Merge pull request #922 from ATIX-AG/fix_package_repo_uniqueness

d4a338b

Fix package repo uniqueness (cherry picked from commit 7ede5be)

patchback bot pushed a commit that referenced this pull request Nov 16, 2023

Merge pull request #922 from ATIX-AG/fix_package_repo_uniqueness

ef1f458

Fix package repo uniqueness (cherry picked from commit 7ede5be)

patchback bot mentioned this pull request Nov 16, 2023

[PR #922/7ede5bed backport][3.0] Fix package repo uniqueness #947

Merged

patchback bot mentioned this pull request Nov 16, 2023

[PR #922/7ede5bed backport][3.1] Fix package repo uniqueness #948

Merged

quba42 deleted the fix_package_repo_uniqueness branch November 16, 2023 20:16

hstct added a commit that referenced this pull request Nov 17, 2023

Merge pull request #947 from pulp/patchback/backports/3.0/7ede5bed2df…

9f7ada9

…d9caa5c3722ed837821eda5052106/pr-922 [PR #922/7ede5bed backport][3.0] Fix package repo uniqueness

hstct added a commit that referenced this pull request Nov 17, 2023

Merge pull request #948 from pulp/patchback/backports/3.1/7ede5bed2df…

e4ef53a

…d9caa5c3722ed837821eda5052106/pr-922 [PR #922/7ede5bed backport][3.1] Fix package repo uniqueness

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix package repo uniqueness #922

Fix package repo uniqueness #922

quba42 commented Oct 26, 2023

quba42 commented Oct 31, 2023 •

edited

Loading

quba42 Oct 31, 2023

daviddavis commented Nov 1, 2023 •

edited

Loading

quba42 commented Nov 2, 2023 •

edited

Loading

quba42 commented Nov 2, 2023

daviddavis commented Nov 3, 2023

patchback bot commented Nov 16, 2023 •

edited

Loading

patchback bot commented Nov 16, 2023 •

edited

Loading

Fix package repo uniqueness #922

Fix package repo uniqueness #922

Conversation

quba42 commented Oct 26, 2023

quba42 commented Oct 31, 2023 • edited Loading

quba42 Oct 31, 2023

Choose a reason for hiding this comment

daviddavis commented Nov 1, 2023 • edited Loading

quba42 commented Nov 2, 2023 • edited Loading

quba42 commented Nov 2, 2023

daviddavis commented Nov 3, 2023

patchback bot commented Nov 16, 2023 • edited Loading

Backport to 3.0: 💚 backport PR created

patchback bot commented Nov 16, 2023 • edited Loading

Backport to 3.1: 💚 backport PR created

quba42 commented Oct 31, 2023 •

edited

Loading

daviddavis commented Nov 1, 2023 •

edited

Loading

quba42 commented Nov 2, 2023 •

edited

Loading

patchback bot commented Nov 16, 2023 •

edited

Loading

patchback bot commented Nov 16, 2023 •

edited

Loading