Integer comparison on qube volume size by ben-grande · Pull Request #775 · QubesOS/qubes-core-admin

ben-grande · 2026-01-14T14:53:43Z

Disposable size is integer and disposable template size is string.

For: QubesOS/qubes-issues#1512
For: QubesOS/qubes-issues#10525
Fixes: QubesOS/qubes-issues#10572
For: #771

qubes/tests/integ/dispvm.py

ben-grande · 2026-01-14T15:40:38Z

openQArun TEST=system_tests_dispvm

OpenQA

qubesos-bot · 2026-01-14T19:03:37Z

OpenQA test summary

Complete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=202601221354-4.3&flavor=pull-requests

Test run included the following:

Integer comparison on qube volume size #775

New failures, excluding unstable

Compared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026011214-devel&flavor=update

system_tests_network
- VmNetworking_debian-13-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmNetworking_fedora-42-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
system_tests_network_ipv6
- VmIPv6Networking_debian-13-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_debian-13-xfce: test_501_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_fedora-42-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_fedora-42-xfce: test_501_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None

Failed tests

7 failures

system_tests_network
- VmNetworking_debian-13-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmNetworking_fedora-42-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
system_tests_network_ipv6
- VmIPv6Networking_debian-13-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_debian-13-xfce: test_501_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_fedora-42-xfce: test_001_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None
- VmIPv6Networking_fedora-42-xfce: test_110_dynamic_attach (failure)
  ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 125 != 0
- VmIPv6Networking_fedora-42-xfce: test_501_simple_networking_paused_from_existent_to_none (error)
  qubes.exc.QubesVMError: netvm should not be None

Fixed failures

Compared to: https://openqa.qubes-os.org/tests/165203#dependencies

6 fixed

system_tests_network
- VmNetworking_debian-13-xfce: test_110_dynamic_attach (failure)
  ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^... AssertionError: 125 != 0
- VmNetworking_fedora-42-xfce: test_001_simple_networking_paused_from_none_to_existent (error)
  subprocess.CalledProcessError: Command 'qubes.WaitForNetworkUplink'...
system_tests_dispvm
- system_tests: Fail (unknown)
  Tests qubes.tests.integ.dispvm failed (exit code 1), details report...
- system_tests: Failed (test died)
  # Test died: Some tests failed at qubesos/tests/system_tests.pm lin...
- TC_20_DispVM_whonix-workstation-18: test_015_preload_race_more (error)
  raise TimeoutError from exc_val... TimeoutError
- TC_20_DispVM_whonix-workstation-18: test_016_preload_race_less (error)
  raise TimeoutError from exc_val... TimeoutError

Unstable tests

Details

Performance Tests

Performance degradation:

No issues

Remaining performance tests:

No remaining performance tests

ben-grande · 2026-01-15T09:33:06Z

Just would like to note that it also didn't fail openqa last time: #771 (comment), but was broken on real use case, which is sad... it requires some qubesd restart to change the state, which I don't think I can do mid test run.

Anyway, new integration tests introduced for the cases that should discard.

HW42 · 2026-01-20T08:22:48Z

qubes/vm/dispvm.py

-            self.volume_config["private"]["size"]
-            != appvm.volume_config["private"]["size"]
+        if int(self.volume_config["private"]["size"]) != int(
+            appvm.volume_config["private"]["size"]


Is there a reason to use .volume_config at all here? Using .volumes["private"].size would avoid this issue (and should remove the hack in your test).

Cause I didn't know...

Now that I changed to it, unit tests became a bit harder because I wasn't creating the volumes, the storage was being mocked. There are a few dispvm tests doing it, will try to use that.

HW42 · 2026-01-20T08:39:34Z

While strictly speaking out of scope of this PR, what is the intended behavior for the next lines?

        if any(vol for vol in self.volumes.values() if vol.is_outdated()):
            # Volume name is irrelevant. We use any() to return fast.
            differed["volumes"] = ["root"]
            return differed

Either only the "root" volume matters, then the iterator is unnecessary, or the return value is (potentially) wrong.

ben-grande · 2026-01-20T08:42:47Z

While strictly speaking out of scope of this PR, what is the intended behavior for the next lines?
        if any(vol for vol in self.volumes.values() if vol.is_outdated()):
            # Volume name is irrelevant. We use any() to return fast.
            differed["volumes"] = ["root"]
            return differed
Either only the "root" volume matters, then the iterator is unnecessary, or the return value is (potentially) wrong.

The iterator stops on the first is_outdated, but we don't need to know which. I just placed "root" there to inform that it was from a template shutdown (volumes become outdated). It is not that only "root" matters, any matters, so any outdated should report it as such. Reporting as "root" as it just go to logs. If you don't think this is sane, I can return all the outdated volumes instead.

qubes/vm/mix/dvmtemplate.py

ben-grande · 2026-01-20T10:05:31Z

openQArun TEST=system_tests_dispvm

HW42 · 2026-01-20T11:08:39Z

[...]

Either only the "root" volume matters, then the iterator is unnecessary, or the return value is (potentially) wrong.

The iterator stops on the first is_outdated, but we don't need to know which. I just placed "root" there to inform that it was from a template shutdown (volumes become outdated). It is not that only "root" matters, any matters, so any outdated should report it as such. Reporting as "root" as it just go to logs. If you don't think this is sane, I can return all the outdated volumes instead.

I'm fine with returning on the first outdated volume, that also matched the other checks. But then the return value should accurately report which volume is outdated, otherwise things can get very confusing when debugging. Given that we are talking about a DispVM, it's actually a normal case that the private volume of the AppVM template gets outdated without the root volume being outdated. Should be very straight forward to implement:

        for vol_name, vol in self.volumes.items():
            if vol.is_outdated():
                differed["volumes"] = [vol_name]
                return differed

[This should go into a separate PR, just commented here since this jumped my into my eye, while trying to understand the original issue]

HW42 · 2026-01-20T11:12:16Z

qubes/vm/dispvm.py

-            self.volume_config["private"]["size"]
-            != appvm.volume_config["private"]["size"]
+        if int(self.volumes["private"].size) != int(
+            appvm.volumes["private"].size


No need for int() here. Here we have a proper Volume object, which should always return an int for it's size property.

ben-grande · 2026-01-20T11:16:51Z

[...]

Either only the "root" volume matters, then the iterator is unnecessary, or the return value is (potentially) wrong.

The iterator stops on the first is_outdated, but we don't need to know which. I just placed "root" there to inform that it was from a template shutdown (volumes become outdated). It is not that only "root" matters, any matters, so any outdated should report it as such. Reporting as "root" as it just go to logs. If you don't think this is sane, I can return all the outdated volumes instead.

I'm fine with returning on the first outdated volume, that also matched the other checks. But then the return value should accurately report which volume is outdated, otherwise things can get very confusing when debugging. Given that we are talking about a DispVM, it's actually a normal case that the private volume of the AppVM template gets outdated without the root volume being outdated. Should be very straight forward to implement:
        for vol_name, vol in self.volumes.items():
            if vol.is_outdated():
                differed["volumes"] = [vol_name]
                return differed
[This should go into a separate PR, just commented here since this jumped my into my eye, while trying to understand the original issue]

If the first outdated volume is "private", than the logs can't distinguish between the current check of outdated volume and the prior check of volume size... So if I do your way, I'd prefer returning every volume, which yes, doesn't guarantee that only "private" will appear. And I think it makes sense to include in this PR, if you agree. OpenQA didn't run yet because of temporary build failure.

codecov · 2026-01-20T11:31:12Z

Codecov Report

❌ Patch coverage is 30.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (756ff94) to head (71d268f).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
qubes/vm/mix/dvmtemplate.py	0.00%	4 Missing ⚠️
qubes/vm/dispvm.py	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #775      +/-   ##
==========================================
- Coverage   70.23%   70.17%   -0.07%     
==========================================
  Files          61       61              
  Lines       13975    13977       +2     
==========================================
- Hits         9815     9808       -7     
- Misses       4160     4169       +9

Flag	Coverage Δ
unittests	`70.17% <30.00%> (-0.07%)`	⬇️

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.

HW42 · 2026-01-20T19:11:17Z

If the first outdated volume is "private", than the logs can't distinguish between the current check of outdated volume and the prior check of volume size... So if I do your way, I'd prefer returning every volume, [...]

If this your concern then listing every volume doesn't help. If you resize "private" is will return {"volumes": "private"} due to the first check. If you started the DispVMs template AppVM, but not the TemplateVM it's based on, it will return {"volumes": "private"} due to the second check.

So if you want to be able to distinguish those cases in the return value you need to use different keys (for example "volumes_size" and "volume_outdated").

And I think it makes sense to include in this PR, if you agree.

As noted above I would have put it into a separate, but fine with me to keep it here if you like.

The size type on volume_config, depends how the qube is loaded, from disk or from the store. The volumes have a size property, use that instead, but still convert to integer, no cost operation. For: QubesOS/qubes-issues#1512 For: QubesOS/qubes-issues#10525 Fixes: QubesOS/qubes-issues#10572 For: QubesOS#771

Makes it easier to debug, even for volumes, which may have been for different reasons, template shutdown or resize. For: QubesOS/qubes-issues#1512 For: QubesOS/qubes-issues#10525 Fixes: QubesOS/qubes-issues#10572 For: QubesOS#771

ben-grande · 2026-01-21T12:03:53Z

openQArun TEST=system_tests_dispvm

Didn't create a gitlab or openqa pipeline.

ben-grande · 2026-01-21T12:45:40Z

openQArun TEST=system_tests_dispvm

openqa

ben-grande · 2026-01-22T13:54:53Z

openQArun TEST=system_tests_dispvm,system_tests_network,system_tests_network_ipv6

openqa

HW42 · 2026-01-24T04:50:18Z

Merged as a7c6042

I updated the commit message to match the current version. Also dropped the "Not needed to return all volumes" comment (only describes what the code does not why. Also why comment the early return here and not in the other cases).

(As a nitpick on the commits: For a fix like this I would have put the changed test into a separate commit.)

The failures we saw on some openQA are not related to this change as far as I can see.

Merged this now to get the fix to users (although it only affected the testing repo). But what I think we want to address in some future change:

Look at the code coverage report. Some of the complaints look like real issues.
Add a test that would have caught the regression
Don't unnecessarily mix types in volumes_config

ben-grande · 2026-01-26T10:25:07Z

(As a nitpick on the commits: For a fix like this I would have put the changed test into a separate commit.)

I normally put tests in the same commit, it allows automatic git bisect using run-tests to check. Is there a resource that recommends tests to be in separate commits?

This was referenced Jan 14, 2026

Integer comparison on qube volume size #774

Closed

Preloaded disposables QubesOS/qubes-issues#1512

Open

ben-grande commented Jan 14, 2026

View reviewed changes

qubes/tests/integ/dispvm.py Outdated Show resolved Hide resolved

ben-grande requested a review from HW42 January 14, 2026 14:59

HW42 reviewed Jan 20, 2026

View reviewed changes

qubes/vm/mix/dvmtemplate.py Show resolved Hide resolved

ben-grande force-pushed the preload-fix-vol-size branch from 8dee80e to aee1e29 Compare January 20, 2026 09:59

HW42 reviewed Jan 20, 2026

View reviewed changes

ben-grande force-pushed the preload-fix-vol-size branch from aee1e29 to 7f126d3 Compare January 20, 2026 11:18

ben-grande force-pushed the preload-fix-vol-size branch from 7f126d3 to 958dcec Compare January 21, 2026 04:21

Log outdated preloaded disp volumes and properties

71d268f

Makes it easier to debug, even for volumes, which may have been for different reasons, template shutdown or resize. For: QubesOS/qubes-issues#1512 For: QubesOS/qubes-issues#10525 Fixes: QubesOS/qubes-issues#10572 For: QubesOS#771

ben-grande force-pushed the preload-fix-vol-size branch from 958dcec to 71d268f Compare January 21, 2026 12:03

This comment was marked as outdated.

Sign in to view

ben-grande mentioned this pull request Jan 22, 2026

Skip unecessary event #778

Merged

HW42 closed this Jan 24, 2026

Uh oh!

Conversation

ben-grande commented Jan 14, 2026

Uh oh!

Uh oh!

ben-grande commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qubesos-bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

OpenQA test summary

New failures, excluding unstable

Failed tests

Fixed failures

Unstable tests

Performance Tests

Performance degradation:

Remaining performance tests:

Uh oh!

ben-grande commented Jan 15, 2026

Uh oh!

HW42 Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

ben-grande Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

HW42 commented Jan 20, 2026

Uh oh!

ben-grande commented Jan 20, 2026

Uh oh!

Uh oh!

ben-grande commented Jan 20, 2026

Uh oh!

HW42 commented Jan 20, 2026

Uh oh!

HW42 Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

ben-grande commented Jan 20, 2026

Uh oh!

codecov bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HW42 commented Jan 20, 2026

Uh oh!

ben-grande commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben-grande commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

ben-grande commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HW42 commented Jan 24, 2026

Uh oh!

ben-grande commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ben-grande commented Jan 14, 2026 •

edited

Loading

qubesos-bot commented Jan 14, 2026 •

edited

Loading

codecov bot commented Jan 20, 2026 •

edited

Loading

ben-grande commented Jan 21, 2026 •

edited

Loading

ben-grande commented Jan 21, 2026 •

edited

Loading

ben-grande commented Jan 22, 2026 •

edited

Loading