Thaw queues on awaiting stuck futures #365

mdellweg · 2019-11-06T09:39:31Z

[noissue] Required-PR: pulp/pulpcore#365

[noissue] Required PR: pulp/pulpcore#365

bmbouter · 2019-11-06T17:01:12Z

@mdellweg can you give me more insight into this change? It's a plugin writer facing feature so we also need a pulp.plan.io entry for it.

mdellweg · 2019-11-06T18:29:10Z

@gmbnomis care to take a look?

[noissue] Required PR: pulp/pulpcore#365

bmbouter · 2019-11-13T18:34:32Z

CHANGES/5668.feature

@@ -0,0 +1 @@
+Allow get_or_create_future without deactivating does_batch on DeclarativeContent from the beginning.


This release note is for the plugin API, can it be moved to the plugin API folder in CHANGES?

bmbouter · 2019-11-13T18:57:57Z

pulpcore/plugin/stages/models.py

@@ -86,6 +86,25 @@ def __init__(self, artifact=None, url=None, relative_path=None, remote=None, ext
        return download_result


+class DCFuture(asyncio.Future):


This will need an autodoc entry so it shows in the docs also somewhere on this page https://github.com/pulp/pulpcore/blob/master/docs/plugins/api-reference/stages.rst

That is actually something we need to discuss, because as i see it, this is inner plumbing. The user does not even need to know about that class.

bmbouter · 2019-11-13T19:05:45Z

pulpcore/plugin/stages/models.py

@@ -113,7 +132,7 @@ class DeclarativeContent:
        ValueError: If `content` is not specified.
    """

-    __slots__ = ('content', 'd_artifacts', 'extra_data', 'does_batch', 'future')
+    __slots__ = ('content', 'd_artifacts', 'extra_data', 'does_batch', 'future', 'thaw_queue_event')


The future and the thaw_queue_event seem very similar. I'd like to think more about how we can provide this value to plugin writers without adding more facilities they need to use. Part of the concern is that if the plugin writer needs to take extra care to use this facility, how is that different than the plugin writer taking extra care to clear the does_batch attribute like we document here.

I wonder whether thaw_queue_event needs to be stored in content at all. If I understand your PR correctly, we need thaw_queue_event only if there is a future. As we use our own future type here anyway, couldn't we store it in the future?

@bmbouter The way this change works, the user isn't supposed to do more than before. The only thing that is new, is that he can omit to set does_batch=False. So we can even safely remove that warning, that using futures can jam the queue.
@gmbnomis That is a matter of perspective, but this version allows to create the future, even after the d_content has been injected into the queue. Also just setting it might not even be slower than looking if there is a future and then conditionally set it.

@mdellweg I thought about this difference, but injecting the d_content into the queue and adding the future later is a bug IMO. The queue operation should be async and we don't know when it comes back. The DC may have passed the entire pipeline before one can actually attach a future to it.

My argument was not about speed (which is probably in the same ball park), but about isolation. If the implementation of the "thaw queue" feature can be self-contained in DCFuture, I think we should do that.

bmbouter · 2019-11-13T19:07:26Z

@mdellweg how would you feel in us including this in 3.1? I see the benefits so that is clear, but I'm concerned about how we're offering them. I want to work to find a way to bring this benefit to plugin writers without adding the the options/facilities they need to think about.

gmbnomis · 2019-11-13T19:55:43Z

pulpcore/plugin/stages/models.py

@@ -86,6 +86,25 @@ def __init__(self, artifact=None, url=None, relative_path=None, remote=None, ext
        return download_result


+class DCFuture(asyncio.Future):


This class looks like a mixture between a proxy to a future and a subclassed future. But I don't get why. Wouldn't the following work?

class DCFuture(asyncio.Future): """A special future, that can thaw the queue it is stuck in.""" __slots__ = ["d_content"] def __init__(self, d_content): self.d_content = d_content def __await__(self): self.d_content.does_batch = False if self.d_content.thaw_queue_event: self.d_content.thaw_queue_event.set() return super().__await__()

Right. I wanted to make it a subclass (so that is is a future). But then i didn't see a way to inject it into:
self.future = asyncio.get_event_loop().create_future()
So it became a proxy, and i forgot about the inheritance. Thinking about this now, we can probably make the DC the proxy itself.

Thinking about this now, we can probably make the DC the proxy itself.

This would turn the DeclarativeContent into an awaitable future-like object itself. Though i like the concept, it would be backwards incompatible, and rather involved. I am not sure it's worth persuing it today.

I even like the proper proxy you changed it into now better. The async trigger mechanism is an optional feature of DC, but not its main feature.

I've always seen the DC as some kind of promised future, so i thinks it feels kind of natural to await them. As a result of a discussion with @bmbouter i prepared another (breaking) solution, where the DC's can be awaited directly.
https://github.com/mdellweg/pulpcore/tree/awaitable_dc

Oh, we are in "we can have a nicer (but breaking) API" land?

It's great that your change gets rid of the exposed Future, since Futures are considered low-level objects that should not be exposed in user level APIs (https://docs.python.org/3/library/asyncio-future.html#future-object).

But, as said, let's please not await DC directly:

It is a pretty uncommon thing to do. The basic building block of an async API seems to be the coroutine, not an awaitable object instance. See for example the high-level APIs offered by asyncio (https://docs.python.org/3/library/asyncio.html). None of them seem to offer a directly awaitable object instance.

It is more explicit, we could e.g. have async def wait_for_resolution(self) which is clearer IMHO than just awaiting the DC (and can have a nice docstring)

It allows us to enhance the API later, we could e.g. add a async def wait_for_download(self) to await an earlier event. Or we can add optional parameters if the need arises.

wdyt?

Oh, we are in "we can have a nicer (but breaking) API" land?

Since it was rescheduled to 3.1 that is at least an option. And i am throwing ideas here (which is why i did not just replace this branch).
If what you are saying is you liked to use it like

await self.put(d_content) content = await d_content.resolution()

instead of

await self.put(d_content) content = await d_content

or the current version

fut = d_content.get_or_create_future() await self.put(d_content) content = await fut

i think, i can have a working example in no time.

bmbouter · 2019-11-13T21:02:52Z

I'm branching 3.0 now. If we are able to resolve this before the actual GA I'd be OK cherry picking it since it's a) additive only and b) only affects plugin writers. Let's continue the collaboration!

[noissue] Required PR: pulp/pulpcore#365

mdellweg · 2019-11-21T15:18:45Z

@gmbnomis is this what you expected to see?
Sure documentation still needs to be adjusted...

bmbouter · 2019-11-21T15:28:10Z

pulpcore/plugin/stages/models.py


-    def __init__(self, content=None, d_artifacts=None, extra_data=None, does_batch=True):


@ipanova does docker use does_batch?

bmbouter · 2019-11-21T15:29:08Z

pulpcore/plugin/stages/api.py

+                [thaw_event_listener, get_listener], return_when=asyncio.FIRST_COMPLETED
+            )
+            if thaw_event_listener in done:
+                await thaw_event_listener


Like we mentioned on the call, I don't see how awaiting here productive since thaw_event_listener is already in done.

simon-baatz · 2019-11-21T20:15:55Z

pulpcore/plugin/stages/api.py

            if content is None:
                shutdown = True
                log.debug(_('%(name)s - shutdown.'), {'name': self})
            else:
-                if not content.does_batch:
+                if not content._resolved and content._future is not None:


I think the stage should not access _resolved and _future directly (this is different from _thaw_queue_event which serves as the communication means between Stage and DC).

A does_batch property in DC would be a nice way to keep this in DC I guess. A plugin writer could even derive from DC and use a different mechanism to decide what to batch.

Ah, i see now, where this is going. But i do not want to leave it as a variable on the DC, because we cannot just clear it after resolving without surprise. So i'm going to make it a property. A derived DC can then alter the logic to it's need (for example wait only til artifacts are ready).

gmbnomis · 2019-11-21T21:03:02Z

@gmbnomis is this what you expected to see?

Yes, I like the interface much more than the old one.

mdellweg · 2019-11-25T14:56:52Z

@gmbnomis, @bmbouter ^

gmbnomis · 2019-11-28T18:23:19Z

pulpcore/plugin/stages/models.py

+
+    @property
+    def does_batch(self):
+        return not self._resolved and self._future is None


Should we add a note here that an overridden method must never 'overrule' the original implementation (i.e. never turn a "does not batch" into a "does batch")

gmbnomis · 2019-11-28T18:28:32Z

pulpcore/plugin/stages/content_stages.py


        ...

-        foo_a_content = await foo_a_future  # awaits until the foo_a reaches this stage
+        foo_a_content = await foo_a.resolution()  # awaits until the foo_a reaches this stage


Should we mention somewhere that, as a general rule, the pipeline will work more efficient if waiting for resolution is done as late as possible? (that's a terrible wording, but hopefully you understand what I am trying to say).

I know exactly what you mean, and i will try to write something.

gmbnomis · 2019-11-28T18:40:10Z

@gmbnomis, @bmbouter ^

As said, I like the new interface much better than the old one. It's @bmbouter's call whether this backwards incompatible change is ok at this point.

Regarding the "thaw queue" feature, I am still struggling with the added complexity (given that the benefit will only be seen in very specific situations and if the plugin writer knows what she/he is doing). OTOH, this may be the price we have to pay to ease the use of pipeline feedback loops without risking hangs all the time.

bmbouter · 2019-12-02T20:31:49Z

@mdellweg Travis is failing, maybe try rebasing?

I'm ok adding it at this point in time because we are still going to have breaking changes in the plugin API for a few Y releases. When merged this would just go into 3.1 and not backported to 3.0.

This overall seems safer so I'm inclined to accept it. Last call for blocking concerns.

mdellweg · 2019-12-03T09:39:19Z

@bmbouter rebasing helped.

bmbouter · 2019-12-07T17:45:21Z

I want to do a final review on this soon. Sorry for the delay

[noissue] Required PR: pulp/pulpcore#365

If it is currently waiting in a queue, unfreeze that. Remove old interfaces does_batch and get_or_create_future. fixes #5668

bmbouter

@mdellweg sorry this has taken so long. Now that 3.0.0 is released I had enough time to review this once more. I'm going to add a 5668.feature in another release note, but I'm merging this for now. Thank you!

bmbouter · 2019-12-13T20:54:00Z

@mdellweg to complete this work please send an email to pulp-dev identifying this as a breaking change with the subject starting with [breaking]. All plugin API breaking changes need to follow that policy once merged.

[noissue] Required PR: pulp/pulpcore#365

See the PR pulp/pulpcore#365

See the PR pulp/pulpcore#365 and https://www.redhat.com/archives/pulp-dev/2019-December/msg00050.html to learn more. [noissue]

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Nov 6, 2019

Refactor sync first_stage to create metadata units

50105bd

[noissue] Required-PR: pulp/pulpcore#365

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Nov 6, 2019

Refactor sync first_stage to create metadata units

cfdc67e

[noissue] Required PR: pulp/pulpcore#365

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Nov 13, 2019

Refactor sync first_stage to create metadata units

67a2235

[noissue] Required PR: pulp/pulpcore#365

bmbouter reviewed Nov 13, 2019

View reviewed changes

gmbnomis reviewed Nov 13, 2019

View reviewed changes

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Nov 14, 2019

Refactor sync first_stage to create metadata units

c88892f

[noissue] Required PR: pulp/pulpcore#365

bmbouter reviewed Nov 21, 2019

View reviewed changes

simon-baatz reviewed Nov 21, 2019

View reviewed changes

gmbnomis reviewed Nov 28, 2019

View reviewed changes

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Dec 10, 2019

Refactor sync first_stage to create metadata units

4913926

[noissue] Required PR: pulp/pulpcore#365

Add resolution callback to DeclarativeContent

338deb9

If it is currently waiting in a queue, unfreeze that. Remove old interfaces does_batch and get_or_create_future. fixes #5668

bmbouter approved these changes Dec 13, 2019

View reviewed changes

bmbouter merged commit 2d415c3 into pulp:master Dec 13, 2019

mdellweg deleted the thaw_queues branch December 16, 2019 10:21

mdellweg pushed a commit to mdellweg/pulp_deb that referenced this pull request Dec 16, 2019

Refactor sync first_stage to create metadata units

442560d

[noissue] Required PR: pulp/pulpcore#365

lubosmj added a commit to lubosmj/pulp_container that referenced this pull request Dec 16, 2019

Adopt changes on awaiting futures from pulpcore

abe6067

See the PR pulp/pulpcore#365

lubosmj added a commit to lubosmj/pulp_container that referenced this pull request Dec 16, 2019

Adopt changes for awaiting futures from pulpcore

28dc0bc

See the PR pulp/pulpcore#365

lubosmj added a commit to lubosmj/pulp_container that referenced this pull request Dec 16, 2019

Adopt changes for awaiting futures from pulpcore

13bc0e2

See the PR pulp/pulpcore#365 and https://www.redhat.com/archives/pulp-dev/2019-December/msg00050.html to learn more. [noissue]

lubosmj mentioned this pull request Dec 16, 2019

Adopt changes for awaiting futures from pulpcore pulp/pulp_container#38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thaw queues on awaiting stuck futures #365

Thaw queues on awaiting stuck futures #365

mdellweg commented Nov 6, 2019 •

edited

Loading

bmbouter commented Nov 6, 2019

mdellweg commented Nov 6, 2019

bmbouter Nov 13, 2019

bmbouter Nov 13, 2019

mdellweg Nov 14, 2019

bmbouter Nov 13, 2019

gmbnomis Nov 13, 2019

mdellweg Nov 14, 2019

gmbnomis Nov 14, 2019

bmbouter commented Nov 13, 2019

gmbnomis Nov 13, 2019

mdellweg Nov 14, 2019

mdellweg Nov 14, 2019

gmbnomis Nov 14, 2019

mdellweg Nov 18, 2019

gmbnomis Nov 19, 2019 •

edited

Loading

mdellweg Nov 20, 2019

bmbouter commented Nov 13, 2019

mdellweg commented Nov 21, 2019

bmbouter Nov 21, 2019

bmbouter Nov 21, 2019

simon-baatz Nov 21, 2019 •

edited

Loading

mdellweg Nov 22, 2019

gmbnomis commented Nov 21, 2019

mdellweg commented Nov 25, 2019

gmbnomis Nov 28, 2019 •

edited

Loading

gmbnomis Nov 28, 2019

mdellweg Nov 29, 2019

gmbnomis commented Nov 28, 2019

bmbouter commented Dec 2, 2019

mdellweg commented Dec 3, 2019

bmbouter commented Dec 7, 2019

bmbouter left a comment

bmbouter commented Dec 13, 2019

		@@ -0,0 +1 @@
		Allow get_or_create_future without deactivating does_batch on DeclarativeContent from the beginning.

		@@ -86,6 +86,25 @@ def __init__(self, artifact=None, url=None, relative_path=None, remote=None, ext
		return download_result


		class DCFuture(asyncio.Future):


		def __init__(self, content=None, d_artifacts=None, extra_data=None, does_batch=True):

Thaw queues on awaiting stuck futures #365

Thaw queues on awaiting stuck futures #365

Conversation

mdellweg commented Nov 6, 2019 • edited Loading

bmbouter commented Nov 6, 2019

mdellweg commented Nov 6, 2019

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

bmbouter commented Nov 13, 2019

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

gmbnomis Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Nov 13, 2019

mdellweg commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-baatz Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmbnomis commented Nov 21, 2019

mdellweg commented Nov 25, 2019

gmbnomis Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmbnomis commented Nov 28, 2019

bmbouter commented Dec 2, 2019

mdellweg commented Dec 3, 2019

bmbouter commented Dec 7, 2019

bmbouter left a comment

Choose a reason for hiding this comment

bmbouter commented Dec 13, 2019

mdellweg commented Nov 6, 2019 •

edited

Loading

gmbnomis Nov 19, 2019 •

edited

Loading

simon-baatz Nov 21, 2019 •

edited

Loading

gmbnomis Nov 28, 2019 •

edited

Loading