Easy dependency switch if only preload is in chain #733

ben-grande · 2025-09-16T12:32:09Z

Fixes: QubesOS/qubes-issues#10227
For: QubesOS/qubes-issues#1512

Missing some tests.

PR is based on top of:

Remove preloaded disposables on test tearDown #748

marmarek · 2025-09-16T12:43:01Z

qubes/app.py

                        and getattr(obj, prop.__name__) == vm
                    ):
+                        if getattr(obj, "is_preload") and (
+                            prop.__name__ in ["default_dispvm", "template"]


You sure about default_dispvm here?

Yes, see logs from qubesmanager tests:

'This qube cannot be removed. It is used as: - default_dispvm for qube anon-whonix

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.

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.

marmarek · 2025-09-16T12:45:41Z

qubes/vm/mix/dvmtemplate.py

        if newvalue:
            return
-        if any(self.dispvms):
+        if any(True for disp in self.dispvms if not disp.is_preload):


Suggested change

if any(True for disp in self.dispvms if not disp.is_preload):

if any(not disp.is_preload for disp in self.dispvms):

?

marmarek · 2025-09-16T21:24:56Z

CI says typo:

AttributeError: 'DispVM' object has no attribute 'is_prelaod'. Did you mean: 'is_preload'?

codecov · 2025-09-17T05:53:36Z

Codecov Report

❌ Patch coverage is 87.35632% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.70%. Comparing base (335c908) to head (67b8fcb).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
qubes/vm/mix/dvmtemplate.py	88.70%	7 Missing ⚠️
qubes/vm/dispvm.py	63.63%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #733      +/-   ##
==========================================
+ Coverage   70.43%   70.70%   +0.26%     
==========================================
  Files          61       61              
  Lines       13755    13814      +59     
==========================================
+ Hits         9689     9767      +78     
+ Misses       4066     4047      -19

Flag	Coverage Δ
unittests	`70.70% <87.35%> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ben-grande · 2025-09-17T11:48:57Z

qubes/app.py

                )

    @qubes.events.handler("domain-pre-delete")
    def on_domain_pre_deleted(self, event, vm):


I don't like the code duplication.

qubes.app

qubesadmin.tools.qvm_remove

qubesmanager.settings

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?

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.

Make sense, thanks for the explanation.

ben-grande · 2025-09-17T17:17:16Z

Are integration tests necessary seen that coverage is high?

ben-grande · 2025-09-18T07:02:09Z

qubes/app.py

+                "Cannot remove %s as it is used by %s",
+                vm,
+                ", ".join(
+                    ":".join(str(i) for i in tup) for tup in dependencies


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.

ben-grande · 2025-09-18T07:04:57Z

qubes/app.py

+                        if isinstance(obj, qubes.app.Qubes):
+                            dependencies.append(('"GLOBAL"', prop.__name__))
+                        elif not obj.property_is_default(prop):
+                            dependencies.append((obj.name, prop.__name__))


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

I don't like quotes in here - it isn't really obvious it means the term is special, it looks more like a bug (inconsistent quoting)... Maybe @GLOBAL, as we use @ already for keywords?

marmarek · 2025-10-26T00:29:14Z

mypy is not happy

ben-grande · 2025-10-29T09:03:05Z

Didn't identify any related failure on openqa.

marmarek · 2025-11-03T02:47:04Z

qubes/app.py

+                        if isinstance(obj, qubes.app.Qubes):
+                            dependencies.append(('"GLOBAL"', prop.__name__))
+                        elif not obj.property_is_default(prop):
+                            dependencies.append((obj.name, prop.__name__))


I don't like quotes in here - it isn't really obvious it means the term is special, it looks more like a bug (inconsistent quoting)... Maybe @GLOBAL, as we use @ already for keywords?

marmarek · 2025-11-03T11:05:45Z

qubes/app.py

            )

+        if preloads:
+            vm.remove_preload_excess(0, reason="domain will be deleted")


I don't like the asynchronous nature of this handling. It means, by the time qvm-remove completes, there may still be some (preloaded) disposables referencing this template. While you can't await in this function, you can in domain-remove-from-disk handler.

If it is not done on the test, it will be handled by "qubes/tests/__init__.py", which will attempt to kill the domain. If the preloaded disposable was still starting, exceptions will be handled by also attempting to kill the domain. Both methods will trigger the "_bare_cleanup()", sometimes indirectly via "cleanup()" or "_auto_cleanup()", but if "_bare_cleanup()" happens on the other call, not the one that called kill, it will not await and will attempt to remove the domain from the disk while it is still running (not completely killed). For: QubesOS#742 For: QubesOS/qubes-issues#1512 Fixes: QubesOS/qubes-issues#10369

Fixes: QubesOS/qubes-issues#10227 For: QubesOS/qubes-issues#1512

ben-grande mentioned this pull request Sep 16, 2025

Preloaded disposables QubesOS/qubes-issues#1512

Open

32 tasks

marmarek reviewed Sep 16, 2025

View reviewed changes

ben-grande force-pushed the preload-template-switch branch from e427a69 to 0947f51 Compare September 16, 2025 20:01

ben-grande force-pushed the preload-template-switch branch from 0947f51 to a30519b Compare September 17, 2025 05:33

ben-grande force-pushed the preload-template-switch branch 2 times, most recently from 28276e3 to 0e2a25d Compare September 17, 2025 08:06

ben-grande commented Sep 17, 2025

View reviewed changes

ben-grande force-pushed the preload-template-switch branch 7 times, most recently from c202c55 to c2093bf Compare September 17, 2025 15:54

ben-grande force-pushed the preload-template-switch branch from c2093bf to b52c6c7 Compare September 17, 2025 17:17

ben-grande marked this pull request as ready for review September 17, 2025 17:19

ben-grande commented Sep 18, 2025

View reviewed changes

ben-grande force-pushed the preload-template-switch branch from b52c6c7 to ed344a6 Compare October 6, 2025 10:43

ben-grande mentioned this pull request Oct 13, 2025

Rename disp template if only preloads are running QubesOS/qubes-manager#429

Closed

ben-grande requested a review from marmarek October 16, 2025 09:07

ben-grande force-pushed the preload-template-switch branch 3 times, most recently from 04d170e to 8181764 Compare October 24, 2025 12:45

ben-grande force-pushed the preload-template-switch branch from 8181764 to ff0fa1e Compare October 27, 2025 10:22

qubesos-bot mentioned this pull request Oct 29, 2025

Allow audio qube to query preload property QubesOS/qubes-mgmt-salt-dom0-virtual-machines#86

Merged

qubesos-bot mentioned this pull request Oct 30, 2025

Revert "Use socks5 proxy on Whonix Gateway to fix onion urls resolution" QubesOS/qubes-core-agent-linux#618

Merged

marmarek requested changes Nov 3, 2025

View reviewed changes

ben-grande force-pushed the preload-template-switch branch from 36008ef to f316eb8 Compare November 3, 2025 09:14

This was referenced Nov 3, 2025

Cache GUIVM keyboard layout in qvm-start-daemon QubesOS/qubes-core-admin-client#393

Merged

Set proper log file perms on sudo qubes-vm-update QubesOS/qubes-core-admin-linux#204

Merged

Update qubesdb first on keyboard layout change #747

Merged

marmarek reviewed Nov 3, 2025

View reviewed changes

marmarek removed the openqa-pending label Nov 3, 2025

ben-grande force-pushed the preload-template-switch branch 4 times, most recently from b0c6f95 to e032059 Compare November 3, 2025 16:31

ben-grande force-pushed the preload-template-switch branch 2 times, most recently from 981b6d5 to 4e1ef76 Compare November 3, 2025 17:08

Easy dependency switch if only preload is in chain

67b8fcb

Fixes: QubesOS/qubes-issues#10227 For: QubesOS/qubes-issues#1512

ben-grande force-pushed the preload-template-switch branch from 4e1ef76 to 67b8fcb Compare November 3, 2025 17:13

marmarek added the openqa-pending label Nov 3, 2025

marmarek approved these changes Nov 9, 2025

View reviewed changes

marmarek merged commit 182f907 into QubesOS:main Nov 9, 2025
5 of 6 checks passed

	if any(True for disp in self.dispvms if not disp.is_preload):
	if any(not disp.is_preload for disp in self.dispvms):

Uh oh!

Easy dependency switch if only preload is in chain #733

Easy dependency switch if only preload is in chain #733

Conversation

ben-grande commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmarek commented Sep 16, 2025

Uh oh!

codecov bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-grande commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmarek commented Oct 26, 2025

Uh oh!

ben-grande commented Oct 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ben-grande commented Sep 16, 2025 •

edited

Loading

codecov bot commented Sep 17, 2025 •

edited

Loading