Refactor queue req by frankiejol · Pull Request #2290 · UPC/ravada

frankiejol · 2026-02-10T12:21:51Z

Queue long requests using system command at

…tor/queue_req

Copilot

Pull request overview

This PR introduces asynchronous queuing for long-running VM operations (notably prepare_base) by scheduling host commands via at and tracking completion through new backend requests, alongside related refactors to base-preparation hooks and tests.

Changes:

Added command queueing via at (queue_command) and a new wait_job request to poll job completion.
Refactored base preparation flow to support deferred finalization (post_prepare_base) and renamed the subclass hook to after_prepare_base.
Added/updated tests to exercise queued base preparation and removed an older download-related test.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
t/vm/d10_not_download.t	Removes `test_dont_download` coverage and its invocation.
t/q20_queue_req.t	Adds new tests for queued requests (`wait_job`/`post_prepare_base`) including failure and success paths.
t/30_request.t	Updates base-prep test to go through `Ravada::Request->prepare_base` and adds a Void-specific extra volume.
lib/Ravada/Volume/Void.pm	Adjusts `prepare_base` signature to accept an extra argument.
lib/Ravada/Volume/QCOW2.pm	Adds request-aware `prepare_base` path that queues work instead of running synchronously.
lib/Ravada/Volume/ISO.pm	Adjusts `prepare_base` signature for consistency with request-aware calls.
lib/Ravada/Volume/Class.pm	Updates `prepare_base` wrapper to pass through request context and defers post-processing if base file doesn’t exist yet.
lib/Ravada/VM/Void.pm	Refactors `remove_file` to use the new `_remove_file_os` helper.
lib/Ravada/VM/KVM.pm	Enhances `remove_file` to delete existing non-volume files via `_remove_file_os`.
lib/Ravada/VM.pm	Introduces `queue_command`, `wait_job` plumbing, `_remove_file_os`, and queue directory helpers.
lib/Ravada/Request.pm	Adds `wait_job` and `post_prepare_base` request args support plus `check_requests` validation behavior.
lib/Ravada/Domain/KVM.pm	Renames `post_prepare_base` hook to `after_prepare_base`.
lib/Ravada/Domain.pm	Refactors `prepare_base` workflow to schedule `post_prepare_base` and support queued per-volume base creation.
lib/Ravada.pm	Adds backend handlers for `wait_job` and `post_prepare_base`, and integrates `check_requests` error propagation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T12:41:07Z

    refresh_storage
    set_time
    open_exposed_ports
    manage_pools
    screenshot
    prepare_base
+    wait_job
 );


wait_job is added as a non-duplicate command, but it is not added to any %COMMAND type group. As a result, Ravada::Request->type will classify it as 'long', which may bypass the intended disk/IO concurrency limits during base preparation. Consider adding wait_job (and likely post_prepare_base) to the appropriate command group (e.g. secondary for polling or disk for the finalization step) so scheduling/limits are consistent.

Copilot · 2026-02-10T12:41:08Z

+        $domain->add_volume( format => 'qcow2', size => 1*1024*1024);
+    }
+
+    my $n_vols = scalar( grep {$_ =~ /img|qcow2|raw/ } $domain->list_volumes);


The volume count regex grep { $_ =~ /img|qcow2|raw/ } ... is very loose (it matches any path containing those substrings) and can miscount volumes, making the subsequent is(scalar(@req), $n_vols+1) assertion flaky. Consider tightening it to match extensions explicitly (e.g., \.(img|qcow2|raw)$).

Suggested change

my $n_vols = scalar( grep {$_ =~ /img|qcow2|raw/ } $domain->list_volumes);

my $n_vols = scalar( grep { $_ =~ /\.(?:img|qcow2|raw)$/ } $domain->list_volumes );

Copilot · 2026-02-10T12:41:08Z

+    my ($job) = $err =~ m{^job (\d+) at}m;
+    die $err if !$job;
+    if ($err && $err =~ /warning: commands will be executed/) {
+        $err = '';
+    }
+


queue_command parses the scheduled job id from stderr ($err), but at typically prints the “job N at …” line to stdout. With the current logic, $out will contain the job line (and gets warned), $job will be undef, and the method will die even on success. Parse $job from $out (and treat real errors from $err separately).

Suggested change

my ($job) = $err =~ m{^job (\d+) at}m;

die $err if !$job;

if ($err && $err =~ /warning: commands will be executed/) {

$err = '';

}

warn "err='$err'" if $err;

# 'at' typically prints "job N at ..." to stdout; try there first.

my ($job) = $out =~ m{^job (\d+) at}m;

# Fallback in case some implementations print the job line to stderr.

if (!$job && $err) {

($job) = $err =~ m{^job (\d+) at}m;

}

if ($err) {

# Ignore common informational warning about missing tty / atd behavior.

if ($err =~ /warning: commands will be executed/) {

$err = '';

# Ignore lines that only contain the job information.

} elsif ($err !~ m{^job \d+ at}m) {

die $err;

}

}

die "Unable to queue job with at: could not parse job id" if !$job;

Copilot · 2026-02-10T12:41:09Z

+        my $file = $volume->file;
+        my $file_base = $base{$volume->info->{target}};
+        next if !$file_base;
+
+        my $base = Ravada::Volume->new(
+            file => $file_base
+            ,is_base => 1
+            ,domain => $self
+        );
+        $base->clone(file => $volume->file);
+


_clone_volumes_base clones the base into $volume->file without removing/replacing the existing volume file first. For formats like QCOW2, qemu-img create ... $file_clone will fail if $file_clone already exists, which is the normal case here. This needs to remove/rename the existing volume file before cloning (similar to Ravada::Volume::Class::_post_prepare_base which does vm->remove_file($self->file) before cloning).

Copilot · 2026-02-10T12:41:09Z

    }
-    $self->post_prepare_base();
+    $self->after_prepare_base();
    return @base_img;
 }

-=head2 post_prepare_base
+=head2 after_prepare_base

 Placeholder for optional method implemented in subclasses. This will
 run after preparing the base files.

 =cut

-sub post_prepare_base($self) {}
+sub after_prepare_base($self) {}
+
+sub _before_post_prepare_base($self) {
+    $self->_data('is_base' => 0 );
+    $self->_clone_volumes_base();
+    $self->_data('is_base' => 1 );
+}
+
+sub post_prepare_base($self) { }
+
+sub _after_post_prepare_base($self) {
+    $self->after_prepare_base();
+}


after_prepare_base is invoked during _do_prepare_base and then invoked again via the post_prepare_base after-modifier (_after_post_prepare_base). This means subclass implementations (e.g. KVM’s backing-store setup) can run twice, and the first run may occur before queued base files exist. Consider running after_prepare_base only in one place, or making the first call conditional on whether async queueing is being used.

Copilot · 2026-02-10T12:41:09Z

-sub prepare_base($self) {
+sub prepare_base($self, $req=undef) {

+    cluck if !defined $req;


cluck if !defined $req; will emit stack traces whenever prepare_base is invoked without a request object, which is noisy in production and can make real warnings harder to spot. If this is just for debugging, gate it behind a debug flag or remove it once the new call path is stable.

Suggested change

cluck if !defined $req;

Copilot · 2026-02-10T12:41:09Z

+    $self->write_file("$file_queue.sh",
+    "#!/bin/sh\n"
+    ."@$command > $file_queue.out 2> $file_queue.err"
+    );


The generated queue script interpolates @$command directly into a /bin/sh script without any shell-escaping/quoting, which can break when args contain spaces and can become command-injection if any arg is influenced by external input. Build the script with proper shell escaping (or avoid the shell entirely by writing a wrapper that execs the command safely).

@copilot open a new pull request to apply changes based on this feedback

Copilot · 2026-02-10T12:41:10Z

+        chmod(oct(744),"$file_queue.sh") or die "$! chmod $file_queue";
+    } else {
+        my ( $out, $err)  = $self->run_command("chmod",'744',"$file_queue.sh");


The queued script is chmod’d to 0744, making it readable/executable by group/others and potentially exposing command lines and output paths under /run/user/.... Using a more restrictive mode like 0700 (and ensuring the queue dir has tight permissions) would reduce information exposure.

Suggested change

chmod(oct(744),"$file_queue.sh") or die "$! chmod $file_queue";

} else {

my ( $out, $err) = $self->run_command("chmod",'744',"$file_queue.sh");

chmod(oct(700),"$file_queue.sh") or die "$! chmod $file_queue";

} else {

my ( $out, $err) = $self->run_command("chmod",'700',"$file_queue.sh");

Copilot · 2026-03-06T14:37:20Z

@frankiejol I've opened a new pull request, #2301, to work on those changes. Once the pull request is ready, I'll request review from you.

* Initial plan * Fix shell injection in queue_command via _shell_quote helper Co-authored-by: frankiejol <136835+frankiejol@users.noreply.github.com> * Update lib/Ravada/VM.pm Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: frankiejol <136835+frankiejol@users.noreply.github.com> Co-authored-by: Francesc Guasch <frankie@etsetb.upc.edu> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…tor/queue_req

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

frankiejol added 7 commits November 24, 2025 13:26

wip: queue commands using AT

9dbd34a

wip: queue prepare base for volumes

5c9a5f9

wip: test queue

e55a9d4

wip: req prepare base

c1ad270

wip: queue prepare base

1f8e39e

Merge branch 'refactor/queue_req' of github.com:UPC/ravada into refac…

5b8a2e5

…tor/queue_req

wip: test unnecessary

7e428b7

frankiejol added this to the v2.5.0 milestone Feb 10, 2026

Copilot AI review requested due to automatic review settings February 10, 2026 12:21

Copilot started reviewing on behalf of frankiejol February 10, 2026 12:22 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

frankiejol added 4 commits February 11, 2026 13:01

Merge branch 'develop' into refactor/queue_req

093e71d

wip: document API

53db245

wip: allow post prepare base req

763540d

wip: updated volumes and filesystem tests

b4117f4

Copilot AI mentioned this pull request Mar 6, 2026

Fix shell injection in queue_command script generation #2301

Merged

Copilot AI and others added 5 commits March 6, 2026 16:12

wip: removed duplicated call and fixed file mode

e37fd2e

Merge branch 'refactor/queue_req' of github.com:UPC/ravada into refac…

fb9f18c

…tor/queue_req

Update lib/Ravada/Domain.pm

8bff9ef

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update lib/Ravada/VM.pm

52ab3e9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

frankiejol merged commit 76cdeed into develop Mar 6, 2026
1 check passed

frankiejol deleted the refactor/queue_req branch March 6, 2026 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor queue req#2290

Refactor queue req#2290
frankiejol merged 16 commits intodevelopfrom
refactor/queue_req

frankiejol commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

frankiejol Mar 6, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	my $n_vols = scalar( grep {$_ =~ /img\|qcow2\|raw/ } $domain->list_volumes);
	my $n_vols = scalar( grep { $_ =~ /\.(?:img\|qcow2\|raw)$/ } $domain->list_volumes );

-    my ($job) = $err =~ m{^job (\d+) at}m;
-    die $err if !$job;
-    if ($err && $err =~ /warning: commands will be executed/) {
-        $err = '';
-    }
+    warn "err='$err'" if $err;
+    # 'at' typically prints "job N at ..." to stdout; try there first.
+    my ($job) = $out =~ m{^job (\d+) at}m;
+    # Fallback in case some implementations print the job line to stderr.
+    if (!$job && $err) {
+        ($job) = $err =~ m{^job (\d+) at}m;
+    }
+    if ($err) {
+        # Ignore common informational warning about missing tty / atd behavior.
+        if ($err =~ /warning: commands will be executed/) {
+            $err = '';
+        # Ignore lines that only contain the job information.
+        } elsif ($err !~ m{^job \d+ at}m) {
+            die $err;
+        }
+    }
+    die "Unable to queue job with at: could not parse job id" if !$job;

Conversation

frankiejol commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

frankiejol Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants