Narrow distribution locks with safe base_path protection

[pablomh]

Problem

Async distribution CUD operations currently reserve the domain-wide
pdrn:<domain>:distributions resource too broadly. For ordinary updates
that leave base_path unchanged, this serializes unrelated
pulpcore.app.tasks.base.ageneral_update tasks behind a lock they do not
need.

This showed up as a bottleneck during capsule sync, where many
RefreshDistribution updates can run at once but end up waiting on the
same reservation.

What this changes

This patch narrows distribution task reservations while keeping the
base_path namespace protected in the safer way discussed in review:

• ordinary updates that leave base_path unchanged reserve only the
  distribution instance
• create and update operations that create or change a base_path also
  reserve a dedicated domain-scoped distribution.base_path resource
• delete does not take the new distribution.base_path lock
• the legacy domain:distributions reservation is still added in shared
  mode for mixed-version upgrade compatibility

The implementation also handles partial PATCH requests correctly by
falling back to instance.base_path when base_path is omitted from the
request body.

As part of the final cleanup, the shared_resources plumbing now lives in
the generic async mixins in base.py, while publication.py only defines
the distribution-specific lock policy.

Test coverage

Adds a functional test covering the reservation behavior for:

• create
• partial update with no base_path in the payload
• partial update with unchanged base_path
• partial update with changed base_path
• delete

Performance notes

Tested with Satellite 6.20 Stream,
~175 capsules, 15 Pulp workers.

For completed pulpcore.app.tasks.base.ageneral_update tasks, lock usage
shifted from all domain-wide reservations to a mix of domain and
instance-scoped reservations:

Lock type	Before patch	After patch
Domain-wide	110	26
Instance-only	0	42

Block wait times improved substantially:

Metric	Before (domain-locked)	After domain-locked	After instance-locked
P95	6.98s	75ms	28ms
P99	10.12s	105ms	29ms
Max	10.16s	121ms	29ms

These results are consistent with removing unnecessary serialization for
ordinary distribution updates while preserving explicit protection when the
base_path namespace can change.

[gerrod3]

Can you add a changelog for this issue? #3322

[pablomh]

Done :)

[gerrod3]

Not sure what is up with lint, maybe try rebasing with main. Also can you add fixes: #3322 to your commit message? Finally if you want this backported to older pulpcores mark the changelog as a bugfix.

[pablomh]

I'd like it to be backported to at least the release(s) that are part of Satellite 6.19.z.

[mdellweg]

I am a tiny bit concerned about Zero Downtime Upgrades here.
An old task (dispatched before this change) updating a distribution would only lock on the domain:distributions and a new one only on the single distribution.
Since distributions themselves are rather shallow, I cannot think about a situation where this collision leads to real world impacts. But my lack of imagination here is no guarantee for correctness.

[mdellweg]

Maybe add a statement that "base_path overlap validation" is the main concern why this function even exists.

I'm wondering if we can safely assume that deleting a distribution will never violate base_path overlaps?

[pablomh]

Yes, that is the main reason this logic exists, and I can make that clearer in the docstring.

I would still keep deletes on the broader lock. A delete does not create an overlap by itself, but it does release a base_path, so it is still part of the same domain-wide consistency concern as creates and moves.

[pablomh]

I am a tiny bit concerned about Zero Downtime Upgrades here. An old task (dispatched before this change) updating a distribution would only lock on the domain:distributions and a new one only on the single distribution. Since distributions themselves are rather shallow, I cannot think about a situation where this collision leads to real world impacts. But my lack of imagination here is no guarantee for correctness.

Thanks, I think this is the main caveat in the current approach.

From our look at the code, we see two ways to address the ZDU concern:

1. Add shared-resource support to the async mixins, so unchanged-base_path distribution updates can keep the narrower instance lock while still overlapping with older broad-lock tasks during rollout.
2. Keep the change local to distributions by overriding the update path there and adding the shared reservation only for that case.

Both approaches would keep the current concurrency improvement while restoring compatibility with older broad-lock tasks during mixed-version upgrades.

[dralley]

and a new one only on the single distribution.

But only if it doesn't actually change the base path, right? Tasks that do touch base_path would retain the domain:distributions lock. That seems OK.

[pablomh]

If I’m understanding correctly, the mixed-version caveat would only apply to updates whose effective base_path does not change.

Creates, deletes, and updates that do change base_path would still retain the broader domain:distributions lock, so the overlap-validation-sensitive cases would continue to serialize as before.

So the remaining ZDU concern would be narrower: it would apply only to ordinary unchanged-base_path distribution updates during a mixed-version rollout window.

If that matches your reading as well, would you prefer to keep the current scoped change, or would you want the mixed-version case addressed in this PR too?

[mdellweg]

I am certain that destroying a distribution can only release a base_path and so cannot even conflict with a prefixed-overlapped base_path that is created at the same time around. (Either the create fails, or the delete succeeded first. In any case the result is consistent wrt the overlap constraint.)

As for the ZDU concerns, I think if we aquire the existing domain-scoped lock shared, and introduce a new (uhhh, regrets...) domain-scoped-base-path lock to use alongside the specific entity lock, we'd be completely safe.
But still my contention is when the base_path is really the only thing that ever desparately needs lock protection, we could go with the current state. (I would then question whether we needed to add the entity-scoped lock at all.)

[pablomh]

Thank you both, this discussion has been very helpful.

My current understanding is that:

• the remaining mixed-version caveat would be limited to ordinary distribution updates whose effective base_path does not change
• creates, deletes, and updates that do change base_path would still retain the broader domain:distributions lock
• and, as noted above, the current rationale for keeping delete on the broader lock may be weaker than I had initially assumed

At this point, I can see a few possible directions for the PR:

1. Keep the current scoped change as it is
2. Further narrow the delete path as well
3. Rework this toward the more complete ZDU-safe shared-lock approach

I would appreciate your guidance on which direction you would prefer for this PR.

[mdellweg]

Ok, we had even more discussion about the bigger picture. And distributions in some plugins may not actually be as shallow as we wish. So let's play this safe:

• We should use a global lock to protect the base_path. Since we need a new name anyway, we can make the lock explicit about that. Thinking "pdrn:...:distribution.base_path".
• We should make the transition upgrade safe (actually this isn't even strictly without downtime, because waiting tasks can survive an app shutdown). So we should always add the old lock shared.
• As for the new base_path lock, only introducing a new path (create, update) needs protection.

Outlook:
I will then look into adding a database constraint trigger to protect the base_path afterwards. That would not be backportable anyway. And if we can turn distribution operations to be really atomic (on the db level) is yet to be determined.

[pablomh]

Thank you, this is very helpful.

My understanding is that the preferred direction would be:

• keep the instance-scoped lock for ordinary distribution updates
• introduce a new explicit global lock for the base_path invariant, something like pdrn:...:distribution.base_path
• always add the existing domain:distributions lock in shared mode for upgrade compatibility
• and only take the new base_path lock for operations that introduce or change a path, not for delete

Please let me know if that matches your understanding of the problem and the intended solution.

Also, if that direction looks right, would you like me to work on that rework in this PR, or would you prefer to handle that part yourselves?

[mdellweg]

Yes, please.

[pablomh]

Yes, to which option? :)

[mdellweg]

To make this Pr ready with the safe way. Ignore the "outlook" part.

[pablomh]

I pushed a rework of the PR along the safe direction discussed above.

My intent with this update was to:

• keep the instance-scoped lock for ordinary distribution updates
• introduce an explicit global distribution.base_path lock for operations that create or change a path
• always add the legacy domain:distributions lock in shared mode for upgrade compatibility
• avoid taking the new base_path lock for delete

I also updated the focused reservation test to cover the new lock shape.

Could you please take a look and let me know whether this matches your expectations for the safe version?

[pablomh]

I put together the alternative implementation on a separate comparison branch so we can sanity-check the shape before changing this PR.

Branch: pablomh:distribution-lock-narrowing-generic-hook
Compare to the current PR branch: pablomh/pulpcore@distribution-lock-narrowing-pr...distribution-lock-narrowing-generic-hook

From our look at the code, this keeps the same reservation semantics as the current version, but moves the shared_resources plumbing into the generic async mixins in base.py, so publication.py only needs to declare the distribution-specific lock policy.

If this direction matches better what you had in mind, I can rework the main PR branch to follow it.

[mdellweg]

That looks a lot cleaner, so yes. Can you merge that and I will review on top of it?

[pablomh]

I merged that approach into the main PR branch, so this PR now uses the generic shared_resources hook in base.py rather than the local async override in publication.py.

The behavior should stay the same as in the comparison branch; this just moves the dispatch plumbing back into the shared async mixins.

Happy to hear any follow-up feedback on top of this version.

[mdellweg]

I think we are there. Please squash all commits into one, now.
If you make sure "ruff check" and "ruff format" are happy before pushing that would be great.

[pablomh]

I squashed the branch into a single commit and updated the commit message to reflect the final direction. This PR should now be ready for review on top of the generic shared_resources hook approach.

[patchback]

Backport to 3.85: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply d57f727 on top of patchback/backports/3.85/d57f7275e4fb599f5488fe207fb1c8ae4472f4f5/pr-7896

Backporting merged PR #7896 into main

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/pulp/pulpcore.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.85/d57f7275e4fb599f5488fe207fb1c8ae4472f4f5/pr-7896 upstream/3.85

4. Now, cherry-pick PR Narrow distribution locks with safe base_path protection #7896 contents into that branch:

   $ git cherry-pick -x d57f7275e4fb599f5488fe207fb1c8ae4472f4f5

   If it'll yell at you with something like fatal: Commit d57f7275e4fb599f5488fe207fb1c8ae4472f4f5 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x d57f7275e4fb599f5488fe207fb1c8ae4472f4f5

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Narrow distribution locks with safe base_path protection #7896 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.85/d57f7275e4fb599f5488fe207fb1c8ae4472f4f5/pr-7896

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

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

[patchback]

Backport to 3.105: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.105/d57f7275e4fb599f5488fe207fb1c8ae4472f4f5/pr-7896

Backported as #7955

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

[patchback]

Backport to 3.115: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.115/d57f7275e4fb599f5488fe207fb1c8ae4472f4f5/pr-7896

Backported as #7956

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