Fix race condition in handling of reserved resources #1228

dralley · 2021-04-02T00:50:46Z

closes: #8352
https://pulp.plan.io/issues/8352

pulpbot · 2021-04-02T17:32:25Z

Attached issue: https://pulp.plan.io/issues/8352

daviddavis · 2021-04-05T20:47:21Z

The code looks good to me. I am going to try to test out/debug the code some tomorrow with some sleep statements so I can understand it a bit better.

dralley · 2021-04-05T20:49:50Z

pulpcore/tasking/tasks.py

-            # no worker is ready so we need to wait
-            time.sleep(0.25)
-            continue
+        with transaction.atomic():


I'm about 80% certain we still need the select_for_update() here also to make it "complete" (these queries don't touch the actual worker so there's nothing for the lock to block otherwise)... however, apparently it doesn't get along with the distinct() on line 46 (they are apparently incompatible constructs?). Any ideas? What is that distinct() actually doing and do we need it?

The "simple" option might be to just ignore locking during acquire_worker and just drop a worker = Worker.objects.select_for_update().get(pk=worker.pk) on line 127 below so that just the resource reservation is covered by the lock. I don't like the double-query but it would work. And it might be "good enough" if we want a fast patch with minimal risk.

Would still like to do better if possible though.

I implemented ^

@dralley select_for_update is 100% needed here to prevent related tables (resource reservation) from modification if another transaction holds a lock. An alternative to double query can be rewriting the query on line 48 to eliminate DISTINCT. However as a workaround and immediate solution it is acceptable in my opinion.

fao89 · 2021-04-06T14:21:33Z

@cutwater

cutwater · 2021-04-07T10:55:52Z

pulpcore/tasking/tasks.py

+            except Worker.DoesNotExist:
+                # from _acquire_worker
+                # no worker is ready so we need to wait
+                time.sleep(0.25)
+                continue
+            except IntegrityError:
+                # we have a worker but we can't create the reservations so wait
+                time.sleep(0.25)
+                continue


Suggested change

except Worker.DoesNotExist:

# from _acquire_worker

# no worker is ready so we need to wait

time.sleep(0.25)

continue

except IntegrityError:

# we have a worker but we can't create the reservations so wait

time.sleep(0.25)

continue

except (Worker.DoesNotExist, IntegrityError):

time.sleep(0.25)

continue

cutwater · 2021-04-07T12:31:31Z

pulpcore/tasking/tasks.py

-            # no worker is ready so we need to wait
-            time.sleep(0.25)
-            continue
+        with transaction.atomic():


@dralley select_for_update is 100% needed here to prevent related tables (resource reservation) from modification if another transaction holds a lock. An alternative to double query can be rewriting the query on line 48 to eliminate DISTINCT. However as a workaround and immediate solution it is acceptable in my opinion.

daviddavis · 2021-04-07T12:47:52Z

pulpcore/tasking/tasks.py

+            try:
+                worker = _acquire_worker(resources)
+
+                worker = Worker.objects.select_for_update().get(pk=worker.pk)


I wonder if this is still susceptible to a race condition?

Worker is returned from _acquire_worker()

Worker is cleaned up

Worker is fetched here with get() and assigned reservations

Good catch. Yes, it will. Worker status is not checked here.

Alternatively, I believe this should work:

Worker.objects.select_for_update().filter(pk__in=Worker.objects.filter(reservations__resource__in=resources))

It performs a single query by using a subselect.

Apart from this question, the rest looks good to me.

@daviddavis Is this meant to go on line 132 or on line 48? It has different semantics from line 48.

I think this could potentially work for line 48:

return Worker.objects.select_for_update().filter(pk__in=Worker.objects.filter(reservations__resource__in=resources)).get()

cutwater

LGTM

dralley · 2021-04-07T16:13:32Z

@daviddavis This change appears as though it might be causing deadlocks? If I run the tests locally it hangs on the very first one.

And the CI has been running for nearly an hour

[noissue]

dralley · 2021-04-07T18:26:16Z

@cutwater @mdellweg Please re-review the last patch

dralley · 2021-04-07T19:04:38Z

pulpcore/tasking/tasks.py

    try:
        # A randomly-selected Worker instance that has zero ReservedResource entries
        # associated with it.
-        return workers_qs_with_counts.filter(reservations__count=0).order_by("?")[0]
+        return Worker.objects.select_for_update().filter(pk__in=workers_qs_no_res).first()


I guess the selected worker isn't necessarily random anymore.

Let's fix that. What if we did this:

workers_qs_no_res = ( workers_qs.annotate(models.Count("reservations")) .filter(reservations__count=0) .order_by("?")[0:1] )

Actually I think this is better:

Suggested change

return Worker.objects.select_for_update().filter(pk__in=workers_qs_no_res).first()

return Worker.objects.select_for_update().filter(pk__in=workers_qs_no_res[0:1]).first()

cutwater · 2021-04-08T09:14:03Z

pulpcore/tasking/tasks.py

+            .order_by("?")
+            .first()
+        )
+    except IndexError:


If query fetches no rows, .first() will not throw IndexError, but return None

limit(1).get() ?

There is no limit() method for queryset. You have to use [0:1].get().

my experience says, that get raises the MultipleReturned exception.

Why do you want to get an exception here instead of simply checking first() return value?

Also .get() doesn't raise IndexError

@mdellweg yes, my bad. .get() raises MultipleReturned indeed.

Early morning drowsiness :) That's probably the simplest solution, I can do that.

cutwater · 2021-04-08T13:49:41Z

pulpcore/tasking/tasks.py

+    try:
+        # A randomly-selected Worker instance that has zero ReservedResource entries
+        # associated with it.
+        return (
+            Worker.objects.select_for_update()
+            .filter(pk__in=workers_qs_no_res)
+            .order_by("?")[0:1]
+            .get()
+        )
+    except IndexError:
+        # If all Workers have at least one ReservedResource entry.
+        raise Worker.model.DoesNotExist()


Suggested change

try:

# A randomly-selected Worker instance that has zero ReservedResource entries

# associated with it.

return (

Worker.objects.select_for_update()

.filter(pk__in=workers_qs_no_res)

.order_by("?")[0:1]

.get()

)

except IndexError:

# If all Workers have at least one ReservedResource entry.

raise Worker.model.DoesNotExist()

# A randomly-selected Worker instance that has zero ReservedResource entries

# associated with it.

return (

Worker.objects.select_for_update()

.filter(pk__in=workers_qs_no_res)

.order_by("?")[0:1]

.get()

)

While this approach (queryset[0:1].get()) is offered by official Django docs, it looks odd.

@cutwater I went with your recommendation to just check the value of what is returned by first()

daviddavis · 2021-04-08T13:57:05Z

pulpcore/tasking/tasks.py

+
+    if not worker:
+        # If all Workers have at least one ReservedResource entry.
+        raise Worker.model.DoesNotExist()


Should this be Worker.DoesNotExist()?

I think there's an alias - this is copied over so if it didn't work it would have broken before now. But I'll change it anyway

cutwater · 2021-04-08T13:55:58Z

pulpcore/tasking/tasks.py

+        Worker.objects.select_for_update().filter(pk__in=workers_without_res).order_by("?").first()
+    )
+
+    if not worker:


Minor comment: I'd rather use an explicit check here
if worker is not None

Updated to if worker is None

cutwater · 2021-04-08T14:01:03Z

pulpcore/tasking/tasks.py

+                            worker=worker, resource=resource
+                        )
+                    TaskReservedResource.objects.create(resource=reservation, task=task_status)
+        except (Worker.DoesNotExist, IntegrityError):


Having break statement at the end of the loop looks odd.

IMO this is more readable:

except (Worker.DoesNotExist, IntegrityError): # if worker is ready, or we have a worker but we can't create the reservations, wait time.sleep(0.25) else: break

cutwater · 2021-04-08T14:03:24Z

pulpcore/tasking/tasks.py

+                        )
+                    TaskReservedResource.objects.create(resource=reservation, task=task_status)
+        except (Worker.DoesNotExist, IntegrityError):
+            # if worker is ready, or we have a worker but we can't create the reservations, wait
            time.sleep(0.25)
            continue


I meant that continue is not needed here anymore.

cutwater

LGTM

cutwater · 2021-04-08T14:14:03Z

@dralley Thank you for addressing my comments.

dralley · 2021-04-08T14:22:43Z

CI failure looks unrelated

closes: #8352 https://pulp.plan.io/issues/8352

dralley · 2021-04-08T14:54:49Z

@cutwater Merged. Do you need a new build with this patch or do you plan to upgrade to 3.12?

dralley force-pushed the tasking-race branch 3 times, most recently from 268672b to 9e3b9ff Compare April 2, 2021 17:23

dralley force-pushed the tasking-race branch 5 times, most recently from 643bb52 to d9b481f Compare April 2, 2021 18:04

dralley commented Apr 5, 2021

View reviewed changes

daviddavis requested a review from mdellweg April 6, 2021 14:20

dralley force-pushed the tasking-race branch from d9b481f to 2bee126 Compare April 7, 2021 00:32

cutwater suggested changes Apr 7, 2021

View reviewed changes

daviddavis reviewed Apr 7, 2021

View reviewed changes

dralley force-pushed the tasking-race branch 2 times, most recently from 4971456 to eb445bb Compare April 7, 2021 15:14

cutwater approved these changes Apr 7, 2021

View reviewed changes

daviddavis approved these changes Apr 7, 2021

View reviewed changes

mdellweg approved these changes Apr 7, 2021

View reviewed changes

Inline functions

6689f70

[noissue]

dralley force-pushed the tasking-race branch from eb445bb to 59f0016 Compare April 7, 2021 18:25

dralley force-pushed the tasking-race branch 3 times, most recently from 746e14e to 777c476 Compare April 7, 2021 18:35

dralley commented Apr 7, 2021

View reviewed changes

dralley force-pushed the tasking-race branch from 777c476 to cbcbb0b Compare April 7, 2021 19:16

dralley force-pushed the tasking-race branch from cbcbb0b to 019a40d Compare April 7, 2021 19:19

mdellweg approved these changes Apr 8, 2021

View reviewed changes

cutwater suggested changes Apr 8, 2021

View reviewed changes

dralley force-pushed the tasking-race branch 2 times, most recently from c4b3b0c to 33c1694 Compare April 8, 2021 13:49

cutwater suggested changes Apr 8, 2021

View reviewed changes

dralley force-pushed the tasking-race branch 2 times, most recently from 1285b09 to 32a2742 Compare April 8, 2021 13:53

daviddavis reviewed Apr 8, 2021

View reviewed changes

cutwater suggested changes Apr 8, 2021

View reviewed changes

dralley force-pushed the tasking-race branch 2 times, most recently from 61b4564 to fe73534 Compare April 8, 2021 13:59

cutwater suggested changes Apr 8, 2021

View reviewed changes

dralley force-pushed the tasking-race branch from fe73534 to 69bc192 Compare April 8, 2021 14:01

cutwater reviewed Apr 8, 2021

View reviewed changes

dralley force-pushed the tasking-race branch from 69bc192 to 8a5667b Compare April 8, 2021 14:04

cutwater approved these changes Apr 8, 2021

View reviewed changes

daviddavis approved these changes Apr 8, 2021

View reviewed changes

Fix race condition in handling of reserved resources

27f78e2

closes: #8352 https://pulp.plan.io/issues/8352

dralley force-pushed the tasking-race branch from 8a5667b to 27f78e2 Compare April 8, 2021 14:28

dralley merged commit 289c1cd into pulp:master Apr 8, 2021

dralley deleted the tasking-race branch April 8, 2021 14:53

	return Worker.objects.select_for_update().filter(pk__in=workers_qs_no_res).first()
	return Worker.objects.select_for_update().filter(pk__in=workers_qs_no_res[0:1]).first()

Fix race condition in handling of reserved resources #1228

Fix race condition in handling of reserved resources #1228

Conversation

dralley commented Apr 2, 2021

pulpbot commented Apr 2, 2021

daviddavis commented Apr 5, 2021

dralley Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

dralley Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cutwater Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

daviddavis Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cutwater left a comment

Choose a reason for hiding this comment

dralley commented Apr 7, 2021 • edited Loading

dralley commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddavis Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cutwater Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cutwater left a comment

Choose a reason for hiding this comment

cutwater commented Apr 8, 2021

dralley commented Apr 8, 2021

dralley commented Apr 8, 2021

dralley Apr 5, 2021 •

edited

Loading

dralley Apr 6, 2021 •

edited

Loading

cutwater Apr 7, 2021 •

edited

Loading

daviddavis Apr 7, 2021 •

edited

Loading

dralley commented Apr 7, 2021 •

edited

Loading

daviddavis Apr 8, 2021 •

edited

Loading

cutwater Apr 8, 2021 •

edited

Loading