Easy dependency switch if only preload is in chain #733
Conversation
qubes/app.py
Outdated
| and getattr(obj, prop.__name__) == vm | ||
| ): | ||
| if getattr(obj, "is_preload") and ( | ||
| prop.__name__ in ["default_dispvm", "template"] |
There was a problem hiding this comment.
You sure about default_dispvm here?
There was a problem hiding this comment.
Yes, see logs from qubesmanager tests:
'This qube cannot be removed. It is used as: <br> - <b>default_dispvm</b> for qube <b>anon-whonix</b>
There was a problem hiding this comment.
if it's really used as default_dispvm for anon-whonix the removal should be blocked... the only thing that shouldn't be blocked is being a template (only) for preloaded disposables, that you are going to kill/remove below anyway.
There was a problem hiding this comment.
Hum, then I need to do it another way because a disposable qube has the default_dispvm property set to its template, so it must be only if those two properties matches.
qubes/vm/mix/dvmtemplate.py
Outdated
| if newvalue: | ||
| return | ||
| if any(self.dispvms): | ||
| if any(True for disp in self.dispvms if not disp.is_preload): |
There was a problem hiding this comment.
| if any(True for disp in self.dispvms if not disp.is_preload): | |
| if any(not disp.is_preload for disp in self.dispvms): |
?
ben-grande
force-pushed
the
preload-template-switch
branch
from
e427a69 to
0947f51
Compare
|
CI says typo: |
ben-grande
force-pushed
the
preload-template-switch
branch
from
0947f51 to
a30519b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
==========================================
+ Coverage 70.61% 70.70% +0.09%
==========================================
Files 61 61
Lines 13642 13728 +86
==========================================
+ Hits 9633 9707 +74
- Misses 4009 4021 +12
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:
|
ben-grande
force-pushed
the
preload-template-switch
branch
2 times, most recently
from
28276e3 to
0e2a25d
Compare
| ) | ||
|
|
||
| @qubes.events.handler("domain-pre-delete") | ||
| def on_domain_pre_deleted(self, event, vm): |
There was a problem hiding this comment.
I don't like the code duplication.
Especially the part some lines below "see 'journalctl'" because not everything is returned to the caller. The preloads are introducing one extra discrepancy on top of device assignment only seen from dom0 journal.
I think there should be an API method to check if the qube is deletale and if not, the (vm, prop) is returned via exception. What do you think?
There was a problem hiding this comment.
It's done this way because the caller may not see the whole system (it can be for example a GUIVM with access to a subset of qubes). Returning the whole list may leak info about other qubes that the caller should not see.
The utility function in core-admin-client is supposed to find where the qube is used - based on what the caller actually can see.
There was a problem hiding this comment.
Make sense, thanks for the explanation.
ben-grande
force-pushed
the
preload-template-switch
branch
7 times, most recently
from
c202c55 to
c2093bf
Compare
|
Are integration tests necessary seen that coverage is high? |
ben-grande
force-pushed
the
preload-template-switch
branch
from
c2093bf to
b52c6c7
Compare
| "Cannot remove %s as it is used by %s", | ||
| vm, | ||
| ", ".join( | ||
| ":".join(str(i) for i in tup) for tup in dependencies |
There was a problem hiding this comment.
All the client tools, qvm-remove and qube-manager show a nice view of dependencies, the journal/logs were not showing them. Discovered that qube names can end with a . dot... I guess and hope : will never be allowed in qube names.
| if isinstance(obj, qubes.app.Qubes): | ||
| dependencies.append(('"GLOBAL"', prop.__name__)) | ||
| elif not obj.property_is_default(prop): | ||
| dependencies.append((obj.name, prop.__name__)) |
There was a problem hiding this comment.
I didn't find a better name than "GLOBAL" for the global property, it is using quotes to distinguish from a qube name. It prints as such:
ERROR: Cannot remove default-dvm as it is used by disp-test:template, sys-firewall:template, sys-usb:template, "GLOBAL":default_dispvm
ben-grande
force-pushed
the
preload-template-switch
branch
from
b52c6c7 to
ed344a6
Compare
2 participants
Fixes: QubesOS/qubes-issues#10227
For: QubesOS/qubes-issues#1512
Missing some tests.