Project

Profile

Help

Story #3044

closed

Distribution create/update operations should be asynchronous

Added by amacdona@redhat.com about 5 years ago. Updated almost 3 years ago.

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

100%

Estimated time:
Platform Release:
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Sprint:
Sprint 50
Quarter:

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 tasksCLOSED - CURRENTRELEASEdkliban@redhat.com

Actions
Related to Pulp - Task #3051: Prevent Distribution base_path overlap in the data modelCLOSED - DUPLICATE

Actions
Blocked by Pulp - Story #4331: As a task writer, I can lock on an arbitrary stringCLOSED - CURRENTRELEASEjortel@redhat.com

Actions
Actions #1

Updated by amacdona@redhat.com about 5 years ago

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

Updated by amacdona@redhat.com about 5 years ago

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

Updated by amacdona@redhat.com about 4 years ago

  • Sprint Candidate changed from Yes to No
Actions #4

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

Actions #5

Updated by amacdona@redhat.com about 4 years ago

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

Updated by amacdona@redhat.com about 4 years ago

See 3051 for the rational behind this.

Actions #8

Updated by daviddavis almost 4 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.

Actions #9

Updated by jortel@redhat.com almost 4 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.

Actions #10

Updated by bmbouter almost 4 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?

Actions #11

Updated by jortel@redhat.com almost 4 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.

Actions #12

Updated by jortel@redhat.com almost 4 years ago

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

Updated by jortel@redhat.com almost 4 years ago

  • Description updated (diff)
Actions #14

Updated by daviddavis almost 4 years ago

  • Groomed changed from No to Yes
  • Sprint Candidate changed from No to Yes
Actions #15

Updated by bmbouter almost 4 years ago

  • Sprint set to Sprint 47
Actions #16

Updated by CodeHeeler almost 4 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to CodeHeeler
Actions #17

Updated by daviddavis almost 4 years ago

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

Updated by rchan almost 4 years ago

  • Sprint changed from Sprint 47 to Sprint 48
Actions #19

Updated by jortel@redhat.com almost 4 years ago

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

Updated by jortel@redhat.com almost 4 years ago

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

Actions #21

Updated by amacdona@redhat.com almost 4 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)

Actions #22

Updated by rchan almost 4 years ago

  • Sprint changed from Sprint 48 to Sprint 49
Actions #23

Updated by CodeHeeler almost 4 years ago

  • Status changed from ASSIGNED to POST

Added by CodeHeeler almost 4 years ago

Revision 794704f7

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 almost 4 years ago

Revision eec364af

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

Actions #24

Updated by rchan almost 4 years ago

  • Sprint changed from Sprint 49 to Sprint 50

Added by CodeHeeler over 3 years ago

Revision 159ceabf

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

Added by CodeHeeler over 3 years ago

Revision 8471025f

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

Actions #26

Updated by CodeHeeler over 3 years ago

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

Added by mdellweg over 3 years ago

Revision 5bd9de65

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

Actions #27

Updated by daviddavis over 3 years ago

  • Sprint/Milestone set to 3.0.0
Actions #28

Updated by bmbouter over 3 years ago

  • Tags deleted (Pulp 3, Pulp 3 RC Blocker)
Actions #29

Updated by bmbouter almost 3 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF