add qvm-restart

[RandyTheOtter]

An attempt to finalize Ali's work: #387

Closes QubesOS/qubes-issues#4747

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

[codecov]

Codecov Report

❌ Patch coverage is 82.85714% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.85%. Comparing base (64cd714) to head (cb68ef0).

Files with missing lines	Patch %	Lines
qubesadmin/tools/qvm_restart.py	87.50%	4 Missing ⚠️
qubesadmin/tools/qvm_shutdown.py	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #470      +/-   ##
==========================================
+ Coverage   76.83%   76.85%   +0.01%     
==========================================
  Files          53       54       +1     
  Lines        9365     9399      +34     
==========================================
+ Hits         7196     7224      +28     
- Misses       2169     2175       +6     

☔ 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.

[RandyTheOtter]

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]

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]

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]

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]

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]

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]

See parser.print_error.

[RandyTheOtter]

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]

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]

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

[ben-grande]

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

Use black -l80 FILE.

[ben-grande]

Change this to if vm.is_running()?

[ben-grande]

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