Story #3044
closed
Distribution create/update operations should be asynchronous
100%
Description
To prevent race conditions related to base_path checking, Distribution create & update operations need to be done asynchronously under the protection of resource locking.
The plan:
- The locked resource will be a (string) URL for the distribution resource collection: " /api/v3/distributions/"
* The base_path checking needs to be moved from the serializer to the model. - Create a new AsyncCreateMixin following the same pattern as update & delete mixins.
- The DistributionViewSet will use the AsyncUpdateMixin, AsyncDeleteMixin, and AsyncCreateMixin
Related issues
Updated by amacdona@redhat.com over 6 years ago
- Related to Refactor #3038: DRY up asynchronous update and delete tasks added
Updated by amacdona@redhat.com over 6 years ago
- Tracker changed from Issue to Story
- % Done set to 0
- Sprint Candidate changed from No to Yes
- Tags Pulp 3 added
Updated by amacdona@redhat.com over 5 years ago
- Sprint Candidate changed from Yes to No
Updated by amacdona@redhat.com over 5 years ago
- Tags Pulp 3 RC Blocker added
If we still intend to do this, returning a 202 would be a very different api, we should do this before RC.
Updated by amacdona@redhat.com over 5 years ago
- Related to Task #3051: Prevent Distribution base_path overlap in the data model added
Updated by amacdona@redhat.com over 5 years ago
See 3051 for the rational behind this.
Updated by daviddavis over 5 years ago
The base_path checking has already been added to the serializer so we just need to protected CRUD operations with a distribution resource lock.
I think we still need to add (or move) validation to the model layer to prevent base_path overlaps. Consider the case where two requests get queued to update different distributors to have base paths foo and foo/bar. They would both pass serializer validation that happens before the tasks get queued. Then both base paths would get written to the database as the tasks get executed. If we move the validation from the serializer to the model layer (just before save) I think this would work.
Everything else looks good.
Updated by jortel@redhat.com over 5 years ago
daviddavis wrote:
If we move the validation from the serializer to the model layer (just before save) I think this would work.
Agreed.
Everything else looks good.
Updated by bmbouter over 5 years ago
Moving the checking to just before the save, +1 to that. +1 to your plan @jortel also.
Typically the locks are locking specific instances of a resource, e.g. repo 'foo' versus repo 'bar' being locked. In this case we need to serialize the write operations on any distribution, so I think we use the lock string 'distribution' versus locking on a specific distribution like /distribution/42/ Does this make sense?
Updated by jortel@redhat.com over 5 years ago
bmbouter wrote:
Moving the checking to just before the save, +1 to that. +1 to your plan @jortel also.
Typically the locks are locking specific instances of a resource, e.g. repo 'foo' versus repo 'bar' being locked. In this case we need to serialize the write operations on any distribution, so I think we use the lock string 'distribution' versus locking on a specific distribution like /distribution/42/ Does this make sense?
Yes. Exactly what I had in mind as well.
Updated by jortel@redhat.com over 5 years ago
- Subject changed from Distribution updates and deletes should be asynchronous to Distribution CRUD operations should be asynchronous
- Description updated (diff)
Updated by daviddavis over 5 years ago
- Groomed changed from No to Yes
- Sprint Candidate changed from No to Yes
Updated by CodeHeeler over 5 years ago
- Status changed from NEW to ASSIGNED
- Assignee set to CodeHeeler
Updated by daviddavis over 5 years ago
- Blocked by Story #4331: As a task writer, I can lock on an arbitrary string added
Updated by jortel@redhat.com about 5 years ago
- Subject changed from Distribution CRUD operations should be asynchronous to Distribution create/update operations should be asynchronous
- Description updated (diff)
Updated by jortel@redhat.com about 5 years ago
I updated the summary and description to omit READ and DELETE. We only need to protect the base-path validation.
Updated by amacdona@redhat.com about 5 years ago
- Description updated (diff)
Removed: move validation from serializer to model.
Since the task locks on all distributions, we are safe. Plus, we have the added benefit of checking at POST time and at write time, and we can respond immediately to the user that a base_path will not work.
If there is a desire/need to move validation to the model, we should open a new ticket (or reopen https://pulp.plan.io/issues/3051)
Updated by CodeHeeler about 5 years ago
- Status changed from ASSIGNED to POST
Added by CodeHeeler about 5 years ago
Added by CodeHeeler about 5 years ago
Revision eec364af | View on GitHub
Updates test to handle changes to Distribution CUD operations
Create, update, and delete for Distributions were made asynchronous in: pulp/pulpcore#6 in response to issue 3044: https://pulp.plan.io/issues/3044 Because of these changes, tests that create distributions previously expected this action to return a distribution but now receive a task so additional steps were needed to retrieve the distribution from the created_resources of the task
ref #3044 https://pulp.plan.io/issues/3044
Required PR: https://github.com/pulp/pulpcore/pull/6
Added by CodeHeeler about 5 years ago
Revision 159ceabf | View on GitHub
Updates test to handle changes to Distribution CUD operations
Create, update, and delete for Distributions were made asynchronous in: https://github.com/pulp/pulpcore/pull/6 in response to issue 3044: https://pulp.plan.io/issues/3044 Because of these changes, tests that create distributions previously expected this action to return a distribution but now receive a task so additional steps were needed to retrieve the distribution from the created_resources of the task
ref #3044 https://pulp.plan.io/issues/3044
Required PR: https://github.com/pulp/pulpcore/pull/6
Updated by amacdona@redhat.com about 5 years ago
Added by CodeHeeler about 5 years ago
Revision 8471025f | View on GitHub
Makes Distribution CUD operations async
Uses resource locking protection to create/update/destroy distributions asynchronously which prevents race conditions in base_path checking.
fixes #3044 https://pulp.plan.io/issues/3044
Required PR: https://github.com/pulp/pulp_file/pull/189
Updated by CodeHeeler about 5 years ago
- Status changed from POST to MODIFIED
- % Done changed from 0 to 100
Applied in changeset pulpcore|8471025fb8ae2241b9406ca0edf0aa6e024bf189.
Added by mdellweg about 5 years ago
Revision 5bd9de65 | View on GitHub
Updates test to handle changes to Distribution CUD operations
Create, update, and delete for Distributions were made asynchronous in: https://github.com/pulp/pulpcore/pull/6 in response to issue 3044: https://pulp.plan.io/issues/3044 Because of these changes, tests that create distributions previously expected this action to return a distribution but now receive a task so additional steps were needed to retrieve the distribution from the created_resources of the task
Updated by bmbouter over 4 years ago
- Status changed from MODIFIED to CLOSED - CURRENTRELEASE
.
Updates test to handle changes to Distribution CUD operations
Create, update, and delete for Distributions were made asynchronous in: pulp/pulpcore#6 in response to issue 3044: https://pulp.plan.io/issues/3044 Because of these changes, tests that create distributions previously expected this action to return a distribution but now receive a task so additional steps were needed to retrieve the distribution from the created_resources of the task
ref #3044 https://pulp.plan.io/issues/3044
Required PR: https://github.com/pulp/pulpcore/pull/6