Narrow distribution task locks for unchanged base_path

[pablomh]

Problem

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

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

What this changes

This patch narrows distribution task reservations when the effective
base_path does not change:

• create: keep the domain-wide distributions lock
• delete: keep the domain-wide distributions lock
• update with changed base_path: keep the broader lock
• update with unchanged base_path: reserve only the distribution instance

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

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 the broader lock 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.