qui-devices: Improve partition handling#310
Conversation
This is the problem. @piotrbartman you had some idea how to fix it, but I can't find it now |
marmarta
left a comment
marmarta
left a comment
There was a problem hiding this comment.
I'm not sure if this is a good idea, in terms of where the bug is - the amount of clearing cache and re-iterating over the devices suggests to me the problem is in some bug in core-admin-client?
Plus, some pylint complaining.
| parent_dev = dev | ||
| for dev_id, device in self.devices.items(): | ||
| if device.parent == parent_dev.port: | ||
| vm.devices[parent_dev.device_class].clear_cache() |
There was a problem hiding this comment.
I think this and the following lines means that you clear cache and re-query the backend vm multiple times, once per each parent device. That's not very effective, I think this should only be done once when needed.
| if device.attachments | ||
| ] | ||
| if dev_id != backend.Device.id_from_device(dev): | ||
| to_delete.append(device._full_id) |
There was a problem hiding this comment.
pylint complains about this (rightly, I think)
| return | ||
|
|
||
| if not isinstance(device, qubesadmin.device_protocol.DeviceInfo): | ||
| device.backend_domain.devices[device.devclass].clear_cache() |
There was a problem hiding this comment.
I am somewhat worried by the amount of clear_cache here. One, this will potentially be quire slow, I think, but two, doesn't this point to an underlying issue in core-admin-client that should be fixed instead of working around it?
There was a problem hiding this comment.
I am somewhat worried by the amount of clear_cache here. One, this will potentially be quire slow, I think,
if the backend vm is holding onto a lot of block devices ?
but two, doesn't this point to an underlying issue in core-admin-client that should be fixed instead of working around it?
I agree. As @marmarek pointed out in the first comment as well, it needs to be fixed at the root.
However, since some time has passed since this message (QubesOS/qubes-issues#10180 (comment)) of yours and there has been no progress, I wrote a workaround to be used as a temporary stopgap until that is completed. (In other words, my brain finds it hard to tinker with APIs)
There was a problem hiding this comment.
I am somewhat worried by the amount of clear_cache here. One, this will potentially be quire slow, I think,
if the backend vm is holding onto a lot of block devices ?
yeah, for example in the default case of sys-usb, just having a bunch of USB sticks with multiple partitions each will cause the amount of calls to skyrocket.
In general, I am very worried about putting a slow workaround to an API bug in the frontend... It is ultimately @marmarek's call here.
There was a problem hiding this comment.
Your concern is entirely justified. I appreciate you taking the time to wrote a review despite your busy workload.
At device_attached() function: When a partition is attached, the parent device’s entry is immediately removed from the QubesDB, the sub-device’s infomation is changed, and an device-attach event is triggered. However, due to concurrency issues, the API may not return information about the attached device. In that case, let’s query the backend domain to get the proper device information. At device_removed() function: If the parent device has sub-devices, updates their internal information. Fixes: QubesOS/qubes-issues#10828 Fixes: QubesOS/qubes-issues#10732
3 participants
At device_attached() function:
When a partition is attached, the parent device’s entry is immediately removed from the QubesDB, the sub-device’s infomation is changed, and an device-attach event is triggered. However, due to concurrency issues, the API may not return information about the attached device. In that case, let’s query the backend domain to get the proper device information.
At device_removed() function:
If the parent device has sub-devices, updates their internal information.
Fixes: QubesOS/qubes-issues#10828
Fixes: QubesOS/qubes-issues#10732