Background work to prepare move to v2 storage API in the client #2017

ogayot · 2024-06-19T12:01:08Z

Going forward, the client storage/filesystem controller should perform all of the storage actions by making async calls to the API. These calls will be implemented using coroutines functions that will call aiohttp.

At the start, these new coroutine functions will override the methods from the manipulator. However, the functions defined by the manipulator are not coroutines functions so there is a mismatch.

Move the needed functions to coroutine functions so that we can properly override them with coroutine functions.

This PR introduces:

a way to connect a signal to a coroutine connect_async_signal. This is similar in essence to what urwid.connect_signal except that it creates a background task to execute the coroutine
a way to declare storage actions as coroutine functions.

NOTE: This may feel like a wasted effort to move manipulator functions to coroutines functions since eventually, the client will not inherit from the manipulator anymore. However, it is necessary to implement the client using async functions so this should allow us to make a simpler transition.

mwhudson

A very interesting PR! I have a bunch of quibbles and questions I'm afraid but hopefully they are interesting to think about :-)

subiquitycore/async_helpers.py

mwhudson · 2024-06-20T03:49:15Z

subiquitycore/async_helpers.py

@@ -45,6 +48,14 @@ def schedule_task(coro, propagate_errors=True):
 background_tasks = set()


+def connect_async_signal(obj, name, callback, **kwargs):


Is it worth typing the callback argument here?

I don't think we can do much better than Callable[..., Awaitable[Any]] since the handler can take a variadic number of arguments. At least, it "forces" the use of a coroutine function rather than a function.

mwhudson · 2024-06-20T03:49:58Z

subiquitycore/async_helpers.py

+    connect_signal(obj, name, partial(async_signal_callback, callback), **kwargs)
+
+
+def async_signal_callback(callback, *args, **kwargs):


Maybe this should have a leading underscore? I think I would be tempted to use a closure rather than partial here but its not very important I guess.

mwhudson · 2024-06-20T03:55:31Z

subiquity/common/filesystem/boot.py

@@ -64,7 +64,7 @@ class MakeBootDevicePlan(abc.ABC):
    """


Modifying this file feels like it's pushing things too far? It's API is not going to be part of the boundary between the client and server, surely?

Agreed... I haven't found a good way around it yet though.

mwhudson · 2024-06-20T03:57:22Z

subiquity/common/filesystem/tests/test_manipulator.py

        disk = make_disk(self.manipulator.model, ptable=None)
-        self.manipulator.reformat(disk, "msdos")
+        await self.manipulator.reformat(disk, "msdos")
        self.assertEqual("msdos", disk.ptable)




I hope you didn't do all this by hand!

I actually did 🙃

mwhudson · 2024-06-20T04:03:01Z

subiquity/server/controllers/filesystem.py

        device = gap.device
        part_align = device.alignment_data().part_align
        bootfs_size = align_up(sizes.get_bootfs_size(gap.size), part_align)
        gap_boot, gap_rest = gap.split(bootfs_size)
        spec = dict(fstype="ext4", mount="/boot")
-        self.create_partition(device, gap_boot, spec)
-        part = self.create_partition(device, gap_rest, dict(fstype=None))
+        await self.create_partition(device, gap_boot, spec)


Now I wonder about things like this. What do we expect the code to look like in the end? Will we still have the FilesystemManipulator methods like create_partition? Will they be async? I realize this is asking after you've done a bunch of work but did you consider the approach of having the client FilesystemController inherit from an async wrapper of FilesystemManipulator full of methods like:

async def create_partition(self, ...): return self._fsm.create_partition(...)

?

(why did I have both FilesystemControllers inherit from FilesystemManipulator rather than given them both an instance of the latter? that was lazy of me)

My intention was that at the end, the client would not use (and not inherit anymore from) the FilesystemManipulator. And if only the server uses the manipulator, then there should be no need for all the async stuff. create_partition would still be implemented in the manipulator but used by the server only.

But unless I can replace all the calls from the client to the manipulator with API calls in one go, I will run into problems with code that is not async.

I did consider a few ways to avoid making everything suddenly async but couldn't come up with something that would theoretically work.

I'm happy to try other solutions. It feels like the wrapper will not be enough to make it possible to call async code from methods of the manipulator ; which is what we'd need in practice AFAICT.

mwhudson · 2024-06-20T04:08:56Z

subiquity/ui/views/filesystem/filesystem.py

@@ -168,7 +170,9 @@ def refresh_model_inputs(self):
                        ]
            actions = [(_("Unmount"), mi.mount.can_delete(), "unmount")]
            menu = ActionMenu(actions)
-            connect_signal(menu, "action", self._mount_action, mi.mount)
+            connect_async_signal(
+                menu, "action", self._mount_action, user_args=[mi.mount]


This sort of thing is interesting. Are the semantics of async callbacks what we want here? In that the task of updating the model happens in the background to some extent and then the UI is redrawn some (in theory) arbitrary time later. Is that what we want? What do we want? Do we want to do TuiApplication._wait_with_indication games to block the UI for MAX_BLOCK_TIME and then do something like grey out the widget if the API call takes longer than that?

Am I overthinking this?

Yes, I think we should not update the UI until the API call returns. Whether we should _wait_with_indication is a good point to consider implementing at some point (so far, I don't have any specific use case in mind where the API would be that slow to finish).

I think redrawing the UI after the background task finishes would be ideal. We'd need access to view (or the urwid handler) to request a redraw though ; so I need to think about what that means.

mwhudson · 2024-06-20T04:11:09Z

subiquity/ui/views/filesystem/partition.py

        self.parent.refresh_model_inputs()
        self.parent.remove_overlay()
+        # This should probably be moved somewhere else
+        self.parent.request_redraw_if_visible()


This is the same sort of concern as my last comment I guess. "Doing an API call when a dialog is closed" seems likely to be a common enough operation to erect some infrastructure around!

Agreed, I'll give this more thoughts.

subiquity/ui/views/filesystem/tests/test_partition.py

mwhudson · 2024-06-21T00:25:28Z

I was going to respond to each comment but I think perhaps it's best for me to wait until you ping again?

dbungert · 2024-06-24T21:35:42Z

subiquity/ui/views/filesystem/delete.py

-        self.parent.refresh_model_inputs()
-        self.parent.remove_overlay()
+        async def async_confirm() -> None:
+            await self.parent.controller.reformat(self.obj)


minor: these two confirms are almost the same, suggest a refactor to unify them

Later, a call to request_redraw_if_visible was added, does it make sense here? Do all cleanup operations need that? Should that just be part of remove_overlay?

subiquitycore/async_helpers.py

Chris-Peterson444 · 2024-06-28T22:29:23Z

subiquity/ui/views/filesystem/filesystem.py

+            await self.parent.controller.remove_boot_disk(disk)
+            self.parent.refresh_model_inputs()
        else:
-            self.parent.controller.add_boot_disk(disk)
-        self.parent.refresh_model_inputs()
+            await self.parent.controller.add_boot_disk(disk)
+            self.parent.refresh_model_inputs()


nit: is there any reason for adding self.parent.refresh_model_inputs() to both clauses?

asyncio.create_task only accepts one positional argument and passing more than one results in the following exception: TypeError: create_task() takes 1 positional argument but 2 were given Therefore, it does not make sense for run_bg_task to forward *args to it. We now use explicit keyword argument names. Signed-off-by: Olivier Gayot <[email protected]>

Signed-off-by: Olivier Gayot <[email protected]>

Going forward, the client storage/filesystem controller should perform all of the storage actions by making async calls to the API. These calls will be implemented using coroutines functions that will call aiohttp. At the start, these new coroutine functions will override the methods from the manipulator. However, the functions defined by the manipulator are not coroutines functions so there is a mismatch. Move the needed functions to coroutine functions so that we can properly override them with coroutine functions. Signed-off-by: Olivier Gayot <[email protected]>

ogayot force-pushed the storage-v2-manipulator-async branch 2 times, most recently from 4440207 to 587cc90 Compare June 19, 2024 14:50

mwhudson reviewed Jun 20, 2024

View reviewed changes

ogayot force-pushed the storage-v2-manipulator-async branch from 587cc90 to 5ecaba6 Compare June 20, 2024 08:42

ogayot force-pushed the storage-v2-manipulator-async branch from 5ecaba6 to c33fdc5 Compare June 24, 2024 09:18

dbungert reviewed Jun 24, 2024

View reviewed changes

ogayot force-pushed the storage-v2-manipulator-async branch 2 times, most recently from 60d3bd1 to 7ca47b2 Compare June 27, 2024 07:24

Chris-Peterson444 reviewed Jun 28, 2024

View reviewed changes

ogayot force-pushed the storage-v2-manipulator-async branch from 7ca47b2 to 0d7640c Compare July 3, 2024 08:48

ogayot added 4 commits July 25, 2024 10:34

async_helpers: add way to associate a coroutine to an urwid signal

e6c8823

Signed-off-by: Olivier Gayot <[email protected]>

storage: client: accept coroutines as actions

ca8c522

Signed-off-by: Olivier Gayot <[email protected]>

ogayot force-pushed the storage-v2-manipulator-async branch from 0d7640c to 6df6d68 Compare July 25, 2024 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Background work to prepare move to v2 storage API in the client #2017

Background work to prepare move to v2 storage API in the client #2017

ogayot commented Jun 19, 2024 •

edited

Loading

mwhudson left a comment

mwhudson Jun 20, 2024

ogayot Jun 20, 2024 •

edited

Loading

mwhudson Jun 20, 2024

mwhudson Jun 20, 2024

ogayot Jun 20, 2024

mwhudson Jun 20, 2024

ogayot Jun 20, 2024

mwhudson Jun 20, 2024

ogayot Jun 20, 2024

mwhudson Jun 20, 2024

ogayot Jun 20, 2024

mwhudson Jun 20, 2024

ogayot Jun 20, 2024

mwhudson commented Jun 21, 2024

dbungert Jun 24, 2024

dbungert Jun 24, 2024

Chris-Peterson444 Jun 28, 2024

		@@ -45,6 +48,14 @@ def schedule_task(coro, propagate_errors=True):
		background_tasks = set()


		def connect_async_signal(obj, name, callback, **kwargs):

		connect_signal(obj, name, partial(async_signal_callback, callback), **kwargs)


		def async_signal_callback(callback, args, *kwargs):

Background work to prepare move to v2 storage API in the client #2017

Are you sure you want to change the base?

Background work to prepare move to v2 storage API in the client #2017

Conversation

ogayot commented Jun 19, 2024 • edited Loading

mwhudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogayot Jun 20, 2024 • 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

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

mwhudson commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogayot commented Jun 19, 2024 •

edited

Loading

ogayot Jun 20, 2024 •

edited

Loading