Add package paragraph metadata checksum to uniqueness

[hstct]

it's a big commit because i needed to update test data 😔

fixes #1490

[quba42]

Rebased to latest main branch (which has a raised pulpcore ceiling giving tests a chance to run).

[daviddavis]

I have a few questions about this PR:

• What is the motivation for this change? There doesn't seem to be an attached issue or changelog entry that would help me to understand.
• What is the performance like for the migration? Have you benchmarked it at all?
• I assume this only impacts packages that are synced?

[quba42]

@daviddavis

* What is the motivation for this change? There doesn't seem to be an attached issue or changelog entry that would help me to understand.

I guess we were being lazy about re-creating an internal ticket on GitHub 😉. The background is that when syncing packages it is possible for the package paragraph in the repo metadata for the package in question to either contain fields or values that diverge from what the control file in the package itself contains. One example is Ubuntu metadata adding a "Phased-Update-Percentage:" field. Another example is the repo maintainer (e.g.: Ubuntu) setting a different "Priority:" value for the package than the package control file itself. The problem is that when such a value changes, the package checksum does not, and therefore Pulp does not update the relevant fields during re-sync. Another related problem, is that updating such a metadata field for one package in one repo, potentially also unexpectedly changes it for all existing repo versions containing that package (on the next re-publish). Adding a checksum for the metadata should fix this. A new package content would be created for changed metadata (pointing at the same package artifact).

* What is the performance like for the migration? Have you benchmarked it at all?

It is on our to do list. Calculating all those checksums... Also how the change affects sync performance.

* I assume this only impacts packages that are synced?

The underlying issue can only arise for packages that are synced. An uploaded package content can only be created from the values within the packages control file, which are necessarily part of the package checksum already. We can have a look if the implementation can be made to reflect this so that no checksums need to be calculated for uploaded packages. Not sure how hard this would be to implement vs. how much performance improvement it would bring. What I can say is that if you are only uploading to your Pulp instance, then you cannot be affected by the class of issues motivating this change.

[daviddavis]

@quba42 thanks for the response. That helps to clarify things.

It is on our to do list. Calculating all those checksums... Also how the change affects sync performance.

We have over 75,000 deb packages and serve millions of packages every day. The db is by far our biggest bottleneck so ideally we'd like to understand how the migration might impact us. Any information you can share from your performance testing would be greatly appreciated.

[hstct]

@quba42 thanks for the response. That helps to clarify things.

It is on our to do list. Calculating all those checksums... Also how the change affects sync performance.

We have over 75,000 deb packages and serve millions of packages every day. The db is by far our biggest bottleneck so ideally we'd like to understand how the migration might impact us. Any information you can share from your performance testing would be greatly appreciated.

I checked locally on oci-env with around 50k package rows by syncing (on demand) these packages before starting the migration. Which takes roughly around 14s on that environment tested it 3 times.

One thing that might be an issue is that this migration updates rows in batches, but everything still runs inside one large transaction. I checked the locks and they were kept from start until the end during the backfill.

I have rewritten the migration to use atomic = False and will commit each batch separately. That means during backfill it wll only hold RowExclusiveLock instead of AccessExclusiveLock. The migration can also be resumed in case something crashes and only processes rows where the metadata_sha256 is still null. However it takes roughly 1s longer for this dataset.

Both migration versions completed successfully though and produced valid hashes and no duplicate identities.

I'm sure we will test this on an katello test system with much more debian packages too, once we are ready.

But let me know how this sounds so far @daviddavis

[hstct]

added migration changes in an extra commit. not sure if we keep this yet

[daviddavis]

@hstct awesome, those changes sound great. Thanks for looking into that.

[quba42]

This is just a reminder, that we should add a bugfix changelog entry to this PR.