Refresh preloaded disposables on outdated volumes#707
Conversation
ben-grande
force-pushed
the
preload-refresh
branch
from
bb9ef8b to
8aa6f18
Compare
ben-grande
force-pushed
the
preload-refresh
branch
4 times, most recently
from
b1a4cde to
0e44908
Compare
|
Integration test is working and that was easy, the unit tests are more difficult to get the magic right. |
ben-grande
force-pushed
the
preload-refresh
branch
4 times, most recently
from
bc7bbff to
4d56b5f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #707 +/- ##
==========================================
- Coverage 70.53% 70.37% -0.16%
==========================================
Files 61 61
Lines 13546 13587 +41
==========================================
+ Hits 9554 9562 +8
- Misses 3992 4025 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # TODO: ben: cleanup with delay? Delay can help because the | ||
| # cleanup is not relevant to this run. | ||
| asyncio.ensure_future(qube.cleanup()) | ||
| dispvm = qube |
There was a problem hiding this comment.
This loop doesn't stop checking. There should be some break/continue to stop at first non-outdated.
ben-grande
force-pushed
the
preload-refresh
branch
from
4d56b5f to
b5b18d0
Compare
| dispvm = qube | ||
| break |
There was a problem hiding this comment.
Should it be under else:? otherwise you will try to use a dispvm that you just scheduled to be cleaned up...
Or maybe add continue after scheduling cleanup?
There was a problem hiding this comment.
I didn't test the latest version since the TODOs yet, focusing on the benchmark PR to test delays on different stages (somehow put a delay in those places).
ben-grande
force-pushed
the
preload-refresh
branch
from
b5b18d0 to
b612182
Compare
|
With preload of No delay on Delay of |
ben-grande
force-pushed
the
preload-refresh
branch
2 times, most recently
from
ddeb253 to
1c0fb1f
Compare
| "domain-preload-dispvm-start", reason="there is a gap" | ||
| "domain-preload-dispvm-start", | ||
| reason="there is a gap", | ||
| delay=5, |
There was a problem hiding this comment.
This is also relevant on refresh because the old preloads might be removed already and there is a gap, but because of the delay in refresh_preload, there is a higher chance of this event being called. Therefore, add a delay to not attempt preload.
| appvm.remove_preload_from_list([qube.name]) | ||
| # Delay to not affect this run. | ||
| asyncio.ensure_future( | ||
| qube.delay(delay=2, coros=[qube.cleanup()]) |
There was a problem hiding this comment.
2 seconds seems to be enough according to qubesd logs.
| ) | ||
| # Delay to not affect this run. | ||
| asyncio.ensure_future( | ||
| dispvm.delay(delay=2, coros=[dispvm.cleanup()]) |
There was a problem hiding this comment.
2 seconds seems to be enough according to qubesd logs.
| self.fire_event_async( | ||
| "domain-preload-dispvm-start", | ||
| reason="of outdated volume(s)", | ||
| delay=4, |
There was a problem hiding this comment.
This delay gives enough time for the old preloads to be cleaned up before trying to preload. It is intended to avoid overload of the system because cleanup and preload can be a bit intensive when combined with many preloaded disposables. There is no perfect delay here, basing of what my tests told were enough, but my hardware is not the same as any other user's hardware.
ben-grande
force-pushed
the
preload-refresh
branch
from
1c0fb1f to
6865b51
Compare
qubes/vm/qubesvm.py
Outdated
| return qubes.qmemman.algo.pref_mem(domain) / 1024 | ||
|
|
||
| async def delay(self, delay: float | int, coros: list[Awaitable]) -> None: | ||
| self.log.info( |
Reduce load when viable with delays, which can help on the current run or overall health of the system to not refreshing (cleanup+preload) in a short time. Fixes: QubesOS/qubes-issues#10026 For: QubesOS/qubes-issues#1512
ben-grande
force-pushed
the
preload-refresh
branch
from
6865b51 to
fcbbac9
Compare
| dispvm = None | ||
| for item in preload_dispvm: | ||
| qube = app.domains[item] | ||
| if any(vol.is_outdated() for vol in qube.volumes.values()): |
There was a problem hiding this comment.
Just to be clear of why this is needed, receiving the event domain-shutdown, as far as I know, does not enforce that it runs before anything else, including running before this section.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025080809-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061004-4.3&flavor=update
Failed tests15 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 11 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:3 performance degradations
Remaining performance tests:130 tests
|
Being fixed on 1201890 |
3 participants
Fixes: QubesOS/qubes-issues#10026
For: QubesOS/qubes-issues#1512
Missing unit tests.