add `qvm-restart` by RandyTheOtter · Pull Request #470 · QubesOS/qubes-core-admin-client

RandyTheOtter · 2026-05-23T21:48:04Z

An attempt to finalize Ali's work: #387

I have decided to not handle shutdown in situ because I would like to avoid duplication

codecov · 2026-05-25T00:39:49Z

Codecov Report

❌ Patch coverage is 76.31579% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.85%. Comparing base (bb41ca5) to head (5dcd115).

Files with missing lines	Patch %	Lines
qubesadmin/tools/qvm_restart.py	72.72%	9 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #470   +/-   ##
=======================================
  Coverage   76.84%   76.85%           
=======================================
  Files          53       54    +1     
  Lines        9434     9472   +38     
=======================================
+ Hits         7250     7280   +30     
- Misses       2184     2192    +8

☔ View full report in Codecov by Harness.
📢 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.

RandyTheOtter · 2026-05-25T01:28:03Z

That should address the issues that are obvious-ish for me to fix.

Only thing left is the qvm-start issue. @marmarek it seems to me qvm-start can raise the error mid-loop only if it is executed with --drive option (which we don't pass here), or domain is already running. It also errors out if some domains don't exist, but that shoudn't be a concern for qvm-restart as qvm-shutdown fails the same way.

Am I missing something?

marmarek · 2026-05-25T01:37:41Z

it seems to me qvm-start can raise the error mid-loop only

There is a bunch of cases where qvm-start can fail. Startup timeout, not enough memory, not enough disk space, and many more.

BTW #469 does refactor to make at least shutdown function reusable. You might want to wait for it to be merged, and then use the new function.

RandyTheOtter · 2026-05-25T19:50:17Z

There is a bunch of cases where qvm-start can fail. Startup timeout, not enough memory, not enough disk space, and many more.

Shouldn't it get caught here?:
https://github.com/QubesOS/qubes-core-admin-client/blob/main/qubesadmin/tools/qvm_start.py#L201

        except (IOError, OSError, qubesadmin.exc.QubesException,
                ValueError) as e:
            if drive_assignment:
                try:
                    domain.devices['block'].detach(drive_assignment)
                except qubesadmin.exc.QubesException:
                    pass
            exit_code = 1
            parser.print_error(str(e))

    return exit_code

It returns status after the loop ends, so all start-able domains should start. They definitely do when I try a group with some of them having far too much memory to start.

marmarek · 2026-05-25T20:18:56Z

Right, this case might be okay. But still, invoking other tool's main from another tool, parsing arguments again etc is not a good idea. This will also create several instances of Qubes() objects, which will cause issues with caching if that become relevant at some point...

RandyTheOtter · 2026-05-25T20:49:31Z

Right, this case might be okay. But still, invoking other tool's main from another tool, parsing arguments again etc is not a good idea.

What's wrong with that? If anything using the same qvm-start every time a qube is started feels more reliable

This will also create several instances of Qubes() objects, which will cause issues with caching if that become relevant at some point...

Okay, should I create a separate pr for that, similar to Ben's qvm-shutdown refactor?

ben-grande · 2026-05-25T22:54:21Z

Right, this case might be okay. But still, invoking other tool's main from another tool, parsing arguments again etc is not a good idea.

What's wrong with that? If anything using the same qvm-start every time a qube is started feels more reliable

main() is intended for script invocation from the shell, sometimes, it may raise SystemExit, sometimes indirectly via parser.error_runtime(). Even if not the case now for some tools, don't rely on it.
Most of that code is not necessary for a restart, only for a "start with drive for installation". You will end up just needing domain.start()

This will also create several instances of Qubes() objects, which will cause issues with caching if that become relevant at some point...

Okay, should I create a separate pr for that, similar to Ben's qvm-shutdown refactor?

No need for a separate PR, unless Github auto-closes this one when you rebase to a different upstream branch, and doesn't allow anyone to reopen it...

Add my fork as a remote
Rebase your PR on mine
Use qubesadmin.utils.(shutdown|start)
When my PR is merged, rebase your branch on the QubesOS remote back to the main branch

ben-grande

Looking better. Please see CI failures.

ben-grande · 2026-06-17T07:51:06Z

+        )
+    )
+    if failed['start']:
+        sys.stderr.write("Failed to start domains back up:\n")


See parser.print_error.

Wow that method looks useless, and requires making parser available. Why does it exist?

Regardless, I can't use it if I want to have output without "Error:" in each print. Adopted for pre-report messages.

RandyTheOtter · 2026-06-19T01:50:49Z

Pylint is complaining that test lines too long, when broken apart they're less readable.
Tests are failing due to qubesadmin.vm failing to get_power_state(). Doesn't seem like my fault, not sure what to think of this.

RandyTheOtter · 2026-06-19T02:21:22Z

Seems like get_power_state() needs something else in the mock response, I should look into this later on

ben-grande · 2026-06-22T09:48:26Z

Pylint is complaining that test lines too long, when broken apart they're less readable.

Use black -l80 FILE.

ben-grande · 2026-06-22T09:51:14Z

+    if not args.start:
+        target_domains = [
+            vm for vm in target_domains
+            if vm.get_power_state() == "Running"


Change this to if vm.is_running()?

ben-grande · 2026-07-03T13:54:09Z

I have a use case for this to be in "utils" module (without expecting a command-line client), so I can easily use here (see how other actions were done):

Add top-like monitoring tool #333

marmarek

Besides comments below, can you squash the commits (and possibly add co-authored-by tag to the commit message to retain info about Ali)?

marmarek · 2026-07-16T15:10:41Z

+        )
+        self.app.expected_calls[
+            ("some-vm", "admin.vm.Shutdown", "wait", None)
+        ] = b"0\x00"


This should be:

Suggested change

] = b"0\x00"

] = b"2\x00QubesVMNotStartedErrorDomain is powered off: 'some-vm'"

CurrentState responses are too long even with black -l 80, looking at the other tests it seems like I can cheat a little and mock only power_state part of the response. It fixed some failing tests as well

Whoops, I replied to the wrong conversation...

Anyway, QubesVMNotStartedErrorDomain is wrong, shutdown doesn't fail if target is down. Correct me if it's an upcoming change I haven't seen yet, but with current admin it replies 0\x00.

marmarek · 2026-07-16T15:12:01Z

+        )
+        self.app.expected_calls[
+            ("dormant-vm", "admin.vm.Shutdown", "force+wait", None)
+        ] = b"0\x00"


And similar here.

ben-grande · 2026-08-02T05:30:46Z

+            b"0\x00some-vm class=AppVM state=Running\n"
+        )
+        self.app.expected_calls[("some-vm", "admin.vm.CurrentState", None, None)] = (
+            b"0\x00some-vm mem=42069 mem_static_max=42069 cputime=1337 power_state=Running\n"


Example return type of admin.vm.CurrentState

% qubesd-query -e dom0 admin.vm.CurrentState dom0 | cat -A 0^@mem=4000000 mem_static_max=16000 cputime=440000000 power_state=Running%

So know you know where the null is, just first character. My shell shows that there is no newline, by printing a % with reversevideo at the end.

If there were newline, it would have printed $:

% printf '\n' | cat -A $

But on your shell, if it is not customized to handle output missing newline, your prompt will be on the same line as the output like this:

% bash --norc bash-5.2$ printf '\0' | cat -A ^@bash-5.2$

If you want to see null representation and no newline without newline:

% printf '\0' | cat -A ^@%

If you want to see precise bytes, I'd recommend xxd or hexdump -C.

ben-grande · 2026-08-04T09:00:40Z

For the line-too-long pylint issue, use black formater with black -l80 FILE. Don't use black -l80 . as a lot of files on this repo are not formatted and there are open PRs that may be impacted.

RandyTheOtter · 2026-08-05T02:49:33Z

For the line-too-long pylint issue, use black formater with black -l80 FILE. Don't use black -l80 . as a lot of files on this repo are not formatted and there are open PRs that may be impacted.

CurrentState responses are too long even with black -l 80, looking at the other tests it seems like I can cheat a little and mock only power_state part of the response. It have fixed some failing tests as well... Somehow...

ben-grande · 2026-08-05T07:35:27Z

I was going to say that this is wrong, as you can break strings into multiple lines, but there are other tests, such as tests.tools.qvm_start.py, doing the same thing as only returning the necessary keys. It's still wrong, as it may cause problem in the future, but maybe it's fine for now?

ben-grande · 2026-08-12T13:46:08Z

+
+    if failed['shutdown']:
+        parser.print_error(
+            "Failed to restart domains:\n",


I think the word "shutdown" fits better here.

Why? Shutdown failure implies startup failure, thus it is a restart failure.

Restart is made of "shutdown + start". Being specific that it is the "shutdown" is to be explicit and avoid ambiguous meaning.

ben-grande · 2026-08-12T13:51:16Z

+            ]
+        )
+    )
+    return failed


We have to consider that every other method using generic_action has the same return type, which this functions changes from dict[QubesVM, BaseException] to dict[str, dict[QubesVM, BaseException]]. I am not sure what to do here, I don't like inconsistencies and would like that calling any of these actions returned the same type, so we don't need separate evaluation of the result depending on the method called, but I also understand that you use it for logging in qvm_restart. Maybe you could evaluate based on known exceptions for each call, and then do a safe fallback to generic logging when the exception is not known.

Do we have a purpose for consistency here? Exception matching looks like avoidable maintenance burden considering that we already know which instruction returns what.

#333

A single function that deals with all qube state wrappers. If the restart wrapper has a return signature, I'd need to create a separate handling just for that.

Ideally, there would be a parent class QubesVMShutdownError and QubesVMStartError, so that you could check if QubesVMShutdownTimeoutError or QubesMemoryError is part of which, so you would return with the same signature, and the command-line tool that intendes to log the information, would us isinstance to check hierarchy.

Co-Authored-By: Ali Mirjamali <ali@mirjamali.com>

marmarek

CI complains about undocumented option:

Undocumented arguments for command 'qvm-restart': '--kill, -k'

marmarek · 2026-09-03T01:57:35Z

+            *report_failure(failed_start),
+            end=""
+        )
+


restart generator needs to be closed:

await restart.aclose()

RandyTheOtter force-pushed the restart branch from 07b6c1c to c451b70 Compare May 23, 2026 21:58

RandyTheOtter force-pushed the restart branch from abb0997 to cb68ef0 Compare May 25, 2026 00:54

RandyTheOtter closed this Jun 8, 2026

RandyTheOtter force-pushed the restart branch from cb68ef0 to 0c7b921 Compare June 8, 2026 02:49

RandyTheOtter reopened this Jun 8, 2026

RandyTheOtter marked this pull request as draft June 8, 2026 05:25

marmarek mentioned this pull request Jun 10, 2026

Add qvm-restart #387

Closed

RandyTheOtter marked this pull request as ready for review June 17, 2026 04:43

ben-grande suggested changes Jun 17, 2026

View reviewed changes

RandyTheOtter force-pushed the restart branch 5 times, most recently from 46d45b5 to 626ae1f Compare June 19, 2026 01:50

RandyTheOtter requested a review from ben-grande June 19, 2026 01:51

RandyTheOtter force-pushed the restart branch from 626ae1f to e044634 Compare June 19, 2026 01:55

ben-grande reviewed Jun 22, 2026

View reviewed changes

RandyTheOtter force-pushed the restart branch from 447e147 to e3fa402 Compare July 10, 2026 17:58

marmarek requested changes Jul 16, 2026

View reviewed changes

RandyTheOtter force-pushed the restart branch 2 times, most recently from 9e9793a to 79f3a32 Compare August 1, 2026 02:45

ben-grande reviewed Aug 2, 2026

View reviewed changes

RandyTheOtter force-pushed the restart branch from a9629c5 to 169d1a9 Compare August 5, 2026 03:44

ben-grande suggested changes Aug 5, 2026

View reviewed changes

Comment thread qubesadmin/utils/__init__.py Outdated

Comment thread qubesadmin/utils/__init__.py Outdated

marmarek reviewed Aug 9, 2026

View reviewed changes

Comment thread qubesadmin/tools/qvm_restart.py Outdated

RandyTheOtter force-pushed the restart branch from 169d1a9 to 5dcd115 Compare August 10, 2026 22:05

ben-grande suggested changes Aug 12, 2026

View reviewed changes

Add qvm-restart

4f1f8b0

Co-Authored-By: Ali Mirjamali <ali@mirjamali.com>

RandyTheOtter force-pushed the restart branch from 5dcd115 to 4f1f8b0 Compare August 23, 2026 23:53

marmarek requested changes Sep 3, 2026

View reviewed changes

	] = b"0\x00"
	] = b"2\x00QubesVMNotStartedErrorDomain is powered off: 'some-vm'"

Uh oh!

Conversation

RandyTheOtter commented May 23, 2026

Uh oh!

codecov Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RandyTheOtter commented May 25, 2026

Uh oh!

marmarek commented May 25, 2026

Uh oh!

RandyTheOtter commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmarek commented May 25, 2026

Uh oh!

RandyTheOtter commented May 25, 2026

Uh oh!

ben-grande commented May 25, 2026

Uh oh!

ben-grande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandyTheOtter Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RandyTheOtter commented Jun 19, 2026

Uh oh!

RandyTheOtter commented Jun 19, 2026

Uh oh!

ben-grande commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-grande commented Jul 3, 2026

Uh oh!

marmarek left a comment

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!

Choose a reason for hiding this comment

Uh oh!

ben-grande commented Aug 4, 2026

Uh oh!

RandyTheOtter commented Aug 5, 2026

Uh oh!

ben-grande commented Aug 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

RandyTheOtter Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov Bot commented May 25, 2026 •

edited

Loading

RandyTheOtter commented May 25, 2026 •

edited

Loading

ben-grande left a comment •

edited

Loading

RandyTheOtter Jun 17, 2026 •

edited

Loading

RandyTheOtter Aug 23, 2026 •

edited

Loading