-
-
Notifications
You must be signed in to change notification settings - Fork 117
Handle global disposable preload feature #686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
==========================================
+ Coverage 70.59% 70.74% +0.15%
==========================================
Files 61 61
Lines 13306 13374 +68
==========================================
+ Hits 9393 9462 +69
+ Misses 3913 3912 -1
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:
|
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025070402-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 tests10 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 12 fixed
Unstable testsDetailsPerformance TestsPerformance degradation:No issues Remaining performance tests:No remaining performance tests |
|
There are some results in https://openqa.qubes-os.org/tests/142046 already. I'm not sure what is causing some of those failures yet. |
I don't see this PR included on OpenQA: |
3afd031 to
bb71368
Compare
|
Not ready, some corner cases when modifying either global or local qube feature. |
bb71368 to
3a10acd
Compare
qubes/vm/mix/dvmtemplate.py
Outdated
| global preload feature is set.""" | ||
| assert isinstance(self, qubes.vm.BaseVM) | ||
| if ( | ||
| self == getattr(self, "default_dispvm", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self == getattr(self, "default_dispvm", None) | |
| self == getattr(self.app, "default_dispvm", None) |
otherwise you'll check VM's "default_dispvm" property, not the global one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
c5cf131 to
f105a73
Compare
qubes/vm/mix/dvmtemplate.py
Outdated
| value = None | ||
| if ( | ||
| not force_local | ||
| and not getattr(self, "preload_max_ignore_global", False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case this attribute is needed? When event handler sets it, the global default_dispvm is changed already, so the below condition would handle it already, no?
But if there is a case where it's needed, please add it properly in __init__ (mixin class can have __init__ too, just don't forget to call super()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears to be misconception that I presumed it would be needed and didn't test without it, plus it was before force_local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be a case where it is needed. I tested without it and it worked.
**app**:
property-(re)?set:default_dispvm:
* old_appvm = True
* new_appvm = False
**vm.adminvm**:
domain-feature-delete:preload-dispvm-max:
* True
domain-feature-set:preload-dispvm-max:
* False
qubes/tests/app.py
Outdated
| mock_new.assert_called_once() | ||
| self.assertEqual( | ||
| "domain-preload-dispvm-start", mock_new.call_args[0][0] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mock_new.assert_called_once() | |
| self.assertEqual( | |
| "domain-preload-dispvm-start", mock_new.call_args[0][0] | |
| ) | |
| mock_new.assert_called_once_with("domain-preload-dispvm-start") |
If there are other args that are not interesting for test, you can use unittest.mock.ANY as a placeholder.
Besides being easier to read, it's also easier to spot mistakes like too much copy&paste - see the one below checks mock_new again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented with mock.ANY locally, much better.
qubes/tests/app.py
Outdated
| ) as mock_new: | ||
| self.app.default_dispvm = self.appvm_alt | ||
| mock_old.assert_not_called() | ||
| mock_new.assert_called_once() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_called_once_with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
Now this needs a rebase |
4c9cbdb to
e45fee0
Compare
|
The global preload test is quite large for a single test but it has the advantage of having less code duplication, as later tests depends on previous tests plus there is a clear distinction between tests with logging. |
|
Liberated for OpenQA. |
|
e45fee0 to
befb836
Compare
qubes/tests/integ/dispvm.py
Outdated
| # each test function is applied. | ||
| if "_preload_" not in self._testMethodName: | ||
| self.app.default_dispvm = self.disp_base | ||
| self.cleanup_preload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self.cleanup_preload | |
| self.cleanup_preload() |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, when running the test directly, it doesn't skip like it did on OpenQA. Fixing this mistake led to find another wrong function call.
qubes/tests/integ/dispvm.py
Outdated
| tasks = [self.app.domains[x].cleanup() for x in old_preload] | ||
| self.loop.run_until_complete(asyncio.gather(*tasks)) | ||
| self.disp_base.features["preload-dispvm-max"] = False | ||
| self.cleanup_preload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
befb836 to
e9151ea
Compare
|
Ran with the latest commit |
Finally :) |
For: QubesOS/qubes-issues#1512
For: QubesOS/qubes-desktop-linux-manager#262