Add disposable preload setting to advanced settings #425
Conversation
ben-grande
force-pushed
the
preload-dispvm
branch
6 times, most recently
from
8a9c8b1 to
9369545
Compare
ben-grande
changed the title
ben-grande
force-pushed
the
preload-dispvm
branch
2 times, most recently
from
c639372 to
315926b
Compare
There was a problem hiding this comment.
One black thing - while I do not love it, apparently we are to be using it, so could you put in an option to revert to default 88 characters per line? Marek approved it, and it really cuts down on black being absolutely ridiculous. I do it in this PR: https://github.com/QubesOS/qubes-desktop-linux-manager/pull/258/files#diff-037ea159eb0a7cb0ac23b851e66bee30fb838ee8d0d99fa331a1ba65283d37f7
qubesmanager/settings.py
Outdated
| @@ -984,6 +997,9 @@ def __init_advanced_tab__(self): | |||
|
|
|||
| vm_memory = getattr(self.vm, "memory", None) | |||
| vm_maxmem = getattr(self.vm, "maxmem", None) | |||
| vm_preload_dispvm = int( | |||
| self.vm.features.get("preload-dispvm-max", 0) or 0 | |||
There was a problem hiding this comment.
I'm a little bit confused about this, why is there or 0 here?
There was a problem hiding this comment.
The , 0 is for when there is no value, the or 0 is for when the value is empty ''. I need to use the , 0 to not fail the .get() if there is no value to retrieve.
There was a problem hiding this comment.
I checked again, the get(..., 0) is not necessary if there is or 0. It also doesn't fail the get() method without it. The or 0 covers more cases.
ui/settingsdlg.ui
Outdated
| @@ -6,7 +6,7 @@ | |||
| <rect> | |||
| <x>0</x> | |||
| <y>0</y> | |||
| <width>836</width> | |||
| <width>858</width> | |||
There was a problem hiding this comment.
I can revert this and other settings.
ui/settingsdlg.ui
Outdated
| @@ -29,7 +29,7 @@ | |||
| <locale language="English" country="UnitedStates"/> | |||
| </property> | |||
| <property name="currentIndex"> | |||
| <number>0</number> | |||
There was a problem hiding this comment.
Not on purpose, see comment below.
ui/settingsdlg.ui
Outdated
| @@ -65,7 +65,6 @@ | |||
| <widget class="QLabel" name="type_label"> | |||
| <property name="font"> | |||
| <font> | |||
| <weight>50</weight> | |||
There was a problem hiding this comment.
why this and other font changes?
There was a problem hiding this comment.
Not on purpose, changed automatically by Qt Designer 6.4.2 on Debian, maybe I need to switch to Fedora to get the 6.9.0... https://packages.fedoraproject.org/pkgs/qt6-qttools/qt6-designer/.
qubesmanager/settings.py
Outdated
| @@ -481,7 +481,10 @@ def __init_basic_tab__(self): | |||
| QtGui.QRegularExpressionValidator( | |||
| QtCore.QRegularExpression( | |||
| "[a-zA-Z0-9_-]*", | |||
| QtCore.QRegularExpression.PatternOption.CaseInsensitiveOption, | |||
| # fmt: off | |||
There was a problem hiding this comment.
why turning off formatting in such an awkward place (the middle of function declaration)? I do not like black myself, but I don't think this is a good approach to deal with its... opinions
There was a problem hiding this comment.
I used black -l80, with -l88, this is possibly unnecessary..
qubesmanager/settings.py
Outdated
|
|
||
| vm_memory = getattr(self.vm, "memory", None) | ||
| vm_maxmem = getattr(self.vm, "maxmem", None) | ||
| vm_preload_dispvm = int( |
There was a problem hiding this comment.
I think the preload-disposables changes should all be in one commit (and the qrexec and black stuff in their own commits)
There was a problem hiding this comment.
Yes, that is just for easier review and reverting if some tests show regression. By the end, it will be squashed.
qubesmanager/settings.py
Outdated
| self.preload_dispvm.setMinimum(0) | ||
| self.preload_dispvm.setMaximum(9999) |
There was a problem hiding this comment.
That does not seem like a sensible maximum... (Also, max and min can be set in the ui file; easiest way to edit it is with designer-qt)
There was a problem hiding this comment.
Also set on the UI. I don't know a sensible maximum for this, the user can go crazy but the system won't preload if there is not enough memory. Is 50 a reasonable amount? I don't want to discourage users with a lot of memory in the future to not use the UI and use the terminal.
ui/settingsdlg.ui
Outdated
| <item row="6" column="0"> | ||
| <widget class="QLabel" name="preload_dispvm_label"> | ||
| <property name="text"> | ||
| <string>Preload disposables:</string> |
There was a problem hiding this comment.
I think this needs an explaining tooltip that clarifies what this setting does, exactly.
I though Qubes project was using |
ben-grande
force-pushed
the
preload-dispvm
branch
3 times, most recently
from
942f71a to
ab68aa1
Compare
|
Now I used |
ben-grande
force-pushed
the
preload-dispvm
branch
from
ab68aa1 to
b580cc1
Compare
ben-grande
force-pushed
the
preload-dispvm
branch
5 times, most recently
from
4087322 to
2680b07
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 69.08% 69.20% +0.12%
==========================================
Files 17 17
Lines 3891 3910 +19
==========================================
+ Hits 2688 2706 +18
- Misses 1203 1204 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ben-grande
force-pushed
the
preload-dispvm
branch
2 times, most recently
from
a555607 to
207745b
Compare
|
PipelineRetryFailed |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025061118-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 tests30 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/142375#dependencies 10 fixed
Unstable testsPerformance TestsPerformance degradation:6 performance degradations
Remaining performance tests:66 tests
|
|
No failed tests on openqa? :) Not much to see on screenshots, but you can see that on a qube with https://openqa.qubes-os.org/tests/141540#step/desktop_linux_manager_create_qube/31 |
|
Ok, openqa bot updated the results, there are failures but they are only on
Whonix-Workstation now. I will switch from "$HOSTNAME" to "qubesdb-read
/name".
It failed on race_more only for that OS also, probably out of memory, I
will read the hypervisor.log later.
…On Thu, Jun 5, 2025, 7:07 PM ben-grande ***@***.***> wrote:
*ben-grande* left a comment (QubesOS/qubes-manager#425)
<#425 (comment)>
No failed tests on openqa? :)
Not much to see on screenshots, but you can see that on a qube with Disposable
template disabled, the Preload disposable is greyed out:
https://openqa.qubes-os.org/tests/141540#step/desktop_linux_manager_create_qube/31
—
Reply to this email directly, view it on GitHub
<#425 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BCE2O4L4FBR6GIFHGCWMJJ33CC5STAVCNFSM6AAAAAB5V6J5R2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNBWGUYDINBWGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
qubesmanager/settings.py
Outdated
| self.warn_too_much_mem_label.setVisible(True) | ||
|
|
||
| def check_disp_changes(self): |
There was a problem hiding this comment.
what does this function actually do?
ben-grande
force-pushed
the
preload-dispvm
branch
from
207745b to
051b7f6
Compare
ben-grande
force-pushed
the
preload-dispvm
branch
from
051b7f6 to
44147ca
Compare
ben-grande
force-pushed
the
preload-dispvm
branch
from
44147ca to
e866884
Compare
For: QubesOS/qubes-issues#1512