Refactor VMCollection#445
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
+ Coverage 76.79% 76.82% +0.03%
==========================================
Files 53 53
Lines 9367 9359 -8
==========================================
- Hits 7193 7190 -3
+ Misses 2174 2169 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hippalectryon-0
force-pushed
the
refactor-vmcollection
branch
2 times, most recently
from
75691a8 to
c7365b7
Compare
|
This needs a rebase |
hippalectryon-0
force-pushed
the
refactor-vmcollection
branch
from
c7365b7 to
ebffe8c
Compare
|
Rebased |
|
See failures at https://openqa.qubes-os.org/tests/174895/file/system_tests-tests-qubes.tests.integ.dispvm_perf.log, looks like some tests in https://github.com/QubesOS/qubes-core-admin/tree/main/tests use an API you changed. But TBH, I'm concerned with such changes, this isn't really an internal function, there may be more users of it, so we shouldn't really break it. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026042500-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=2026032404-devel&flavor=update
Failed tests67 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/170766#dependencies 33 fixed
Unstable testsDetails
Performance TestsPerformance degradation:9 performance degradations
Remaining performance tests:75 tests
|
3 participants
This PR is a rewrite of
VMCollection.Requires #438
Goals:
Main changes:
Improved docstring
VMCollectionis actually a fairly counter-intuitive object. It's not a true collection (as a dict or set or Mapping) since it holds two different collections - one for thevm.Listcache and one containing blind objects - but depending on the method called one or the other may be used.The new docstring aims to fairly convey that.
Better variable names
Previous variables names did not properly convey what the variables did.
Rewrite of the
vm_objects/vm_dictsplitThe new code uses
vms/known_namesinstead, which 1) is clearer 2) allows for simpler logic in e.g.get_blind()Minor changes:
Split and renamed
refresh_cacheThe old
refresh_cacheby default did not refresh the cache (force=False). It has been split into_ensure_cache_loaded(=force=False) andrefresh_cache(=force=True).More direct vm access in
__iter__,valuesSince we already call
_ensure_cache_loadedat the start of the method, we can useget_blindto avoidself._vms[name] > __getitem__ > __contains __ > get_blindRemove
app.cache_enabledlogicThe
VMCollectionshould not have to interact withapp.cache_enabled, this should be internal toapp. In other words, even if we pass apower_state=in__init__, ifcache_enabled=False, then theappobject should not used the passed value.Looking at
QubeBase's code, this is already the case:_power_state_cacheis only not-Noneifcache_enabled=True- so in practice this should not change any behavior, only cleanup the code.Note (see test
test_101_list_selected): this "fixes" the fact that callingqvm-lson a selected subset of VMs callsadmin.vm.CurrentStateeven though this data is already returned byadmin.vm.List. If I understand correctly this behavior was due to the fact that whenArgParserfetches a subset of VMs, it creates a newappobject which by default has cache disabled, before the calling method can itself enable the cache.