Cache libvirt domain state - #819
Conversation
|
PipelineRetryFailed |
|
Didn't run openqa locally, only experiment some things, start, shutdown, kill, checking Domains Widget. |
|
PipelineRetryFailed |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #819 +/- ##
==========================================
- Coverage 70.72% 70.67% -0.05%
==========================================
Files 61 61
Lines 14315 14401 +86
==========================================
+ Hits 10124 10178 +54
- Misses 4191 4223 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5eff83f to
aa2e823
Compare
|
PipelineRetryFailed |
1 similar comment
|
PipelineRetryFailed |
|
Marked as draft as there still is some caching issues for the |
|
If necessary (or simpler), it's IMO okay to use cache only for |
Experimenting with cache a bit more and it's getting better, but if I don't get it to a hundred percent, will not cache the power state. |
|
From my tests, it seems to be working. My nemesis, OpenQA, should try me. Do you know a subset of tests that are interesting before joining this PR in the full run? Looking at this recent test:
Line to use after this PR is not draft anymore: |
|
I'd include also at least one of the guivm tests |
9538d21 to
6011ca3
Compare
|
PipelineRetryFailed |
|
Last CI failed on Fedora 43 to due to: So it finally reached this repo. |
|
PipelineRetryFailed |
|
openQArun TEST=system_tests_suspend,system_tests_usbproxy,system_tests_devices,system_tests_gui_tools,system_tests_guivm_gui_interactive |
7d9eb1b to
524ee8e
Compare
01ed08c to
d9da451
Compare
|
PipelineRetryFailed |
d9da451 to
fd3bc89
Compare
|
Some tests are failing here |
fd3bc89 to
50baa1f
Compare
| if self.__waiter is None: | ||
| self.__waiter = asyncio.get_running_loop().create_future() | ||
| waiter = self.__waiter | ||
| if self._power_state_waiter is None: | ||
| self._power_state_waiter = asyncio.get_running_loop().create_future() |
There was a problem hiding this comment.
The local variable (waiter) was important part. Using attribute directly does not guarantee you wait for the same operation you initiated here - it might be something else that happened later (like, somebody calling kill just after shutdown, or maybe start just after kill).
This is the commit that introduced the issue, but the same issue applies to later change_libvirt_state context manager.
There was a problem hiding this comment.
The local variable (
waiter) was important part. Using attribute directly does not guarantee you wait for the same operation you initiated here - it might be something else that happened later (like, somebody calling kill just after shutdown, or maybe start just after kill).
Hm.. right. I think I will change for action specific waiters, so a waiter for shutdown, a wait for suspend etc.
A use case for attribute instead of local variable is https://github.com/QubesOS/qubes-core-admin/pull/876/changes#r3914042842, as I think an attribute might prevent the shutdown from being called twice when there is an awaitable before the libvirt shutdown.
I think the same thing that is being done for shutdown on the other PR, should be done to the other power state actions also, although I understand that the shutdown is more critical.
| yield | ||
| finally: | ||
| if wait: | ||
| await asyncio.wait_for(self._lifecycle_waiter, timeout=timeout) |
There was a problem hiding this comment.
Using the same event object for different events feels risky. What happens for example if kill() is called while start() is running? It might return early for example when VIR_DOMAIN_EVENT_DEFINED or VIR_DOMAIN_EVENT_RESUMED is delivered.
This would be even more complicated in connection with #867 that relies more on waiting for an operation started by somebody else (like cancel_start() does). But shutdown() already has similar case.
There was a problem hiding this comment.
Yes, it's wrong, will fix it.
| await self.fire_event_async( | ||
| "domain-start-failed", reason=str(exc) | ||
| ) | ||
| await self.notify_failed_startup(exc=exc) |
There was a problem hiding this comment.
This will set self._power_state too early, no?
There was a problem hiding this comment.
Yes, seems too early.
50baa1f to
8c2b880
Compare
| finally: | ||
| if wait: | ||
| await asyncio.wait_for( | ||
| self._lifecycle_waiter[event], timeout=timeout |
There was a problem hiding this comment.
As said in #819 (comment), this should use waiter saved in a local variable, before yield. Otherwise you may hit None here if the event is delivered early (or in a very unlucky situation, even wait for another instance of that event, make it wait too long or even forever).
There was a problem hiding this comment.
Now I understand what you mean by that, thanks for the explanation.
Since there is "resume", and there is a moment when resuming that domain might still be suspended, or event suspending a domain on purposed, allow clients to know the domain state.
Useful when debugging to know what's happening. I thought of logging the pretty name of the detail, but that got really big and I think it's out of scope from the Qubes OS project and in scope of the python-libvirt package.
- Keep events related to domain state in the same place - Shorter exception messages, as it's already prefixed with qube name - Much easier to read with less body on the if statements - Will help on a future commit to access private attributes without having to disable pylint checks - The events are related to a domain, not to the larger app instance
There are no other logs emitted on some power state events, such as shutdown. Seems reasonable to increase the log level so developers can have at least an idea of what is happening.
These data only changes once when domain is running.
The state changes rarely, but querying it can take a considerable blocking time, worse when looping through the state of multiple domains. For: QubesOS/qubes-issues#10569 For: QubesOS/qubes-issues#9902
Intended to allow callbacks to be executed early instead of having to wait for the next cycle.
On commit 29f3c9b, this repeated handling was introduced because "libvirt_domain" called "self._update_libvirt_domain()" if domain was not found by libvirt. This is no longer the case and we can rely on cached property.
"Transient" state was used by clients to indicate an impermanent state, while on the server, to indicate that Qrexec should be running but is not. One of the issues is that this is not a libvirt state, The state aren't always as transient (short duration) as we think it is, clients were never never notified of state changes from "Running/Paused/Suspended" to "Transient", which can happen on "qrexec" feature change and on qube's qrexec agent stopping. A replacement for "Transient" state that clients will be satisfied is "Starting" and "Halting". I judged introducing these new states, although not libvirt states, as a better replacement because they can be dealt with events already received by the client "domain-(pre-)?(start|shutdown)(-failed)?", and don't need to learn a new argument for "domain-(resumed|unpaused)" to set the state from "Paused/Suspended" to "Transient". In case the old "Transient" meaning of qrexec not being ready, is needed in the future, add it somewhere else, with another more insightful name, and not as a power state. Note that "Transient" is a temporary name and it will be replaced by "Starting" on R5.0. For: QubesOS/qubes-issues#10966
8c2b880 to
4656652
Compare
|
openQArun TEST=system_tests_suspend,system_tests_usbproxy,system_tests_devices,system_tests_gui_tools,system_tests_guivm_gui_interactive,system_tests_basic_vm_qrexec_gui |
|
openQArun TEST=system_tests_suspend,system_tests_usbproxy,system_tests_devices,system_tests_gui_tools,system_tests_guivm_gui_interactive,system_tests_basic_vm_qrexec_gui,system_tests_dispvm |
This branch was previously deployed
4 participants
The state changes rarely, but querying it can take a considerable time, that is blocking, when looping through the state of multiple domains.
For: QubesOS/qubes-issues#10569
For: QubesOS/qubes-issues#9902
Didn't run openqa locally, only experiment some things:
qubed-query -e dom0 admin.vm.CurrentState QUBETODO: