Project

Profile

Help

Story #3044

Distribution create/update operations should be asynchronous

Added by amacdona@redhat.com about 2 years ago. Updated 6 months ago.

Status:
MODIFIED
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Platform Release:
Blocks Release:
Backwards Incompatible:
No
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 50

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

Related to Pulp - Refactor #3038: DRY up asynchronous update and delete tasks MODIFIED Actions
Related to Pulp - Task #3051: Prevent Distribution base_path overlap in the data model CLOSED - DUPLICATE Actions
Blocked by Pulp - Story #4331: As a task writer, I can lock on an arbitrary string MODIFIED Actions

Associated revisions

Revision 794704f7 View on GitHub
Added by CodeHeeler 8 months ago

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

Revision eec364af View on GitHub
Added by CodeHeeler 8 months ago

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

Revision 159ceabf View on GitHub
Added by CodeHeeler 7 months ago

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

Revision 8471025f View on GitHub
Added by CodeHeeler 7 months ago

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

Revision 5bd9de65 View on GitHub
Added by mdellweg 7 months ago

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

History

#1 Updated by amacdona@redhat.com about 2 years ago

  • Related to Refactor #3038: DRY up asynchronous update and delete tasks added

#2 Updated by amacdona@redhat.com about 2 years ago

  • Tracker changed from Issue to Story
  • % Done set to 0
  • Sprint Candidate changed from No to Yes
  • Tags Pulp 3 added

#3 Updated by amacdona@redhat.com about 1 year ago

  • Sprint Candidate changed from Yes to No

#4 Updated by amacdona@redhat.com 11 months 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.

#5 Updated by amacdona@redhat.com 11 months ago

  • Related to Task #3051: Prevent Distribution base_path overlap in the data model added

#6 Updated by amacdona@redhat.com 11 months ago

See 3051 for the rational behind this.

#8 Updated by daviddavis 10 months 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.

#9 Updated by jortel@redhat.com 10 months 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.

#10 Updated by bmbouter 10 months 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?

#11 Updated by jortel@redhat.com 10 months 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.

#12 Updated by jortel@redhat.com 10 months ago

  • Subject changed from Distribution updates and deletes should be asynchronous to Distribution CRUD operations should be asynchronous
  • Description updated (diff)

#13 Updated by jortel@redhat.com 10 months ago

  • Description updated (diff)

#14 Updated by daviddavis 10 months ago

  • Groomed changed from No to Yes
  • Sprint Candidate changed from No to Yes

#15 Updated by bmbouter 10 months ago

  • Sprint set to Sprint 47

#16 Updated by CodeHeeler 10 months ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to CodeHeeler

#17 Updated by daviddavis 9 months ago

  • Blocked by Story #4331: As a task writer, I can lock on an arbitrary string added

#18 Updated by rchan 9 months ago

  • Sprint changed from Sprint 47 to Sprint 48

#19 Updated by jortel@redhat.com 8 months ago

  • Subject changed from Distribution CRUD operations should be asynchronous to Distribution create/update operations should be asynchronous
  • Description updated (diff)

#20 Updated by jortel@redhat.com 8 months ago

I updated the summary and description to omit READ and DELETE. We only need to protect the base-path validation.

#21 Updated by amacdona@redhat.com 8 months 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)

#22 Updated by rchan 8 months ago

  • Sprint changed from Sprint 48 to Sprint 49

#23 Updated by CodeHeeler 8 months ago

  • Status changed from ASSIGNED to POST

#24 Updated by rchan 7 months ago

  • Sprint changed from Sprint 49 to Sprint 50

#26 Updated by CodeHeeler 7 months ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#27 Updated by daviddavis 6 months ago

  • Sprint/Milestone set to 3.0

#28 Updated by bmbouter 6 months ago

  • Tags deleted (Pulp 3, Pulp 3 RC Blocker)

Please register to edit this issue

Also available in: Atom PDF