Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors base/clone handling so bases can be prepared/managed on remote nodes (not implicitly tied to localhost), and adjusts ISO handling to use a derived device_re field across backend/frontend. It also expands the test suite to cover these node-independent base workflows and download paths.
Changes:
- Refactor domain/base lifecycle across nodes (prepare base, set/unset base per node, remove instances) and adjust request chaining (
after_request_ok). - Update ISO matching from
file_retodevice_rein frontend and download/refresh logic. - Add/extend tests for node bases, downloads, and cleanup/leftover verification; adjust
file_existsto return consistent boolean values.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| t/vm/h10_hybernate.t | Ensure domains are shut down before cloning in hibernate test. |
| t/vm/60_new_args.t | Add a download request test for ISOs per VM backend. |
| t/vm/20_base.t | Align file_exists expectations to boolean 0/1. |
| t/vm/11_base.t | New test covering base removal behavior and backing files. |
| t/storage_move.t | Test cleanup adjustments (but currently contains merge conflict markers). |
| t/request/15_download.t | Add negative download test and require uid in download requests. |
| t/repository/10_iso.t | Make ISO download test exit early if output mismatch. |
| t/nodes/10_basic.t | Expand node migration/base reuse tests and tighten remote file checks. |
| t/lib/Test/Ravada.pm | Add helpers (import_clone, check_leftovers) and improve pool cleanup. |
| t/kvm/a10_pools.t | Make pool-volume test stricter and ensure it runs. |
| t/front/20_create_domain.t | Switch KVM create-domain test ISO to Alpine. |
| public/js/admin.js | Use iso.device_re for ISO filename matching in UI. |
| lib/Ravada/WebSocket.pm | Add channel validation logic for list_isos subscriptions. |
| lib/Ravada/Volume/QCOW2.pm | Use VM-aware file existence checks and safer temp cleanup. |
| lib/Ravada/Volume/Class.pm | Return destination path from copy_file. |
| lib/Ravada/Volume.pm | Only remove volume file if it exists on the VM. |
| lib/Ravada/VM/Void.pm | Standardize file_exists return values; adjust ISO row processing; extend copy_file signature. |
| lib/Ravada/VM/KVM.pm | Standardize file_exists; adjust ISO url/filename handling and download logic. |
| lib/Ravada/VM.pm | Change DB handle init; adjust balancing/instance tracking; update remote command timeout logic; add helper migrations. |
| lib/Ravada/Request.pm | Expand args for remove/set_base_vm; mark refresh_storage as “important”; use after_request_ok. |
| lib/Ravada/Front/Domain.pm | Add remove_instance placeholder in front-domain API. |
| lib/Ravada/Front.pm | Replace _fix_iso_file_re with _get_device_re; ensure refresh_storage uses user_daemon. |
| lib/Ravada/Domain/Void.pm | Split remove into remove + remove_instance; remove lock file via VM abstraction. |
| lib/Ravada/Domain/KVM.pm | Add remove_instance; adjust XML access and stats gating; tweak UUID checks. |
| lib/Ravada/Domain.pm | Major node/base refactor: prepare_base overwrite support, remove_instance cascade, clone behavior when active, parent-base migration requests, and instance metadata. |
| lib/Ravada.pm | Harden internal network insert; update ISO regex; add stats settings defaults; allow remove by id_domain; adjust download rsync destination. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (!exists $self->clients->{$ws}) { | ||
| die "Error: missig id_node in ".$args{channel} | ||
| if $args{channel} eq 'list_isos'; | ||
|
|
There was a problem hiding this comment.
This validation is ineffective: the client subscribes to channels like list_isos/<id_vm>, so if $args{channel} eq 'list_isos' will never trigger. Also the error message has a typo ('missig'). Consider parsing the channel (or checking for /\d+$/) and returning an error response instead of die, to avoid taking down the WebSocket handler.
| remove_domain($domain); | ||
| $vm->remove_storage_pool($sp); | ||
| <<<<<<< HEAD | ||
| ======= | ||
|
|
||
| >>>>>>> develop | ||
| } |
There was a problem hiding this comment.
Unresolved Git merge conflict markers (<<<<<<<, =======, >>>>>>>) are present in this test file, which will break parsing/execution of the test suite. Please resolve the conflict and remove these markers.
| $vm->remove_storage_pool($sp); | ||
|
|
||
| $vm->remove_storage_pool($sp); | ||
| rmdir($dir) or die "$! $dir"; |
There was a problem hiding this comment.
$vm->remove_storage_pool($sp); is executed twice in a row, which can cause a second removal attempt to throw or mask earlier failures. Keep a single call (and ensure cleanup order is correct).
| my $vm = Ravada::VM->open($domain->_data('id_vm')); | ||
| for my $vol (@volumes) { | ||
| next if $vol->file =~ /\.iso$/; | ||
| $vm->remove_file($vol->file) | ||
| } | ||
| Ravada::Domain::_remove_domain_data_db($id); |
There was a problem hiding this comment.
This loop is missing a statement terminator: $vm->remove_file($vol->file) has no trailing semicolon, which will cause a syntax error. Add the semicolon (and consider handling errors from remove_file if it can fail).
| sub _internal_uuid($self) { | ||
| confess "ERROR: Missing internal domain" if !$self->domain; | ||
| return $self->domain->get_id(); | ||
| } |
There was a problem hiding this comment.
_internal_uuid currently returns get_id(), which is the libvirt numeric domain ID, not the UUID. This will produce incorrect behavior anywhere that relies on UUID stability/uniqueness. Return get_uuid_string() (or equivalent) instead.
| my @cpu_stats; | ||
| @cpu_stats = $self->domain->get_cpu_stats(-1,1) | ||
| if Ravada::Front->setting(undef,'/backend/stats/cpu'); | ||
|
|
||
| $cpu_time = int($cpu_stats[0]->{cpu_time}/1024/1024); |
There was a problem hiding this comment.
Ravada::Front->setting is called with two arguments here (undef, '/backend/stats/cpu'), but setting($self, $name, $new_value=undef) expects the setting path as the first argument when called as a class method. This likely makes the CPU stats toggle ineffective. Call Ravada::Front->setting('/backend/stats/cpu') instead.
| sub is_local($self) { | ||
| if (ref($self)) { | ||
| return 1 if !$self->host | ||
| || $self->host eq 'localhost' | ||
| || $self->host eq '127.0.0,1' | ||
| ; | ||
| } elsif ($self =~ /^\d+$/) { | ||
| my $id=$self; | ||
| my $sth = _dbh->prepare("SELECT hostname " | ||
| ." FROM vms " | ||
| ." WHERE id=?" | ||
| ); | ||
| $sth->execute($id); | ||
| my ($host) = $sth->fetchrow; | ||
| return 1 if !$host | ||
| || $host eq 'localhost' | ||
| || $host eq '127.0.0,1' | ||
| ; | ||
|
|
There was a problem hiding this comment.
is_local checks for 127.0.0,1 (comma) instead of the standard loopback 127.0.0.1 (dot). This will incorrectly classify loopback-hosted VMs/nodes as remote (and vice versa). Please correct the IP literal in both the object and numeric-id branches.
| my $timeout = 10; | ||
|
|
||
| $timeout = 60*60 if $command[0] =~ /cp|qemu/; | ||
| my $t0 = time; | ||
| my ($out, $err) = $ssh->capture2({timeout => $timeout},join " ",@command); | ||
| warn ''.(time -$t0)." @command" if $command[0] =~ /cp|qemu/i && $command[1] ne 'info'; | ||
|
|
There was a problem hiding this comment.
$command[1] ne 'info' can warn when $command[1] is undef (common for single-arg commands). Guard with defined $command[1] (or use ($command[1]// '') ne 'info') before the string comparison.
Allow the creation of bases in nodes independent from the main host.