Unverified Commit 51e65ab1 authored by Francesc Guasch's avatar Francesc Guasch Committed by GitHub
Browse files

Fix select disk (#1675)

refactor: remove volume by name

* also lock buttons while changing hardware

closes #1663
parent aadcc6c4
......@@ -4437,7 +4437,8 @@ sub _cmd_remove_hardware {
my $uid = $request->args('uid');
my $hardware = $request->args('name') or confess "Missing argument name";
my $id_domain = $request->defined_arg('id_domain') or confess "Missing argument id_domain";
my $index = $request->args('index');
my $index = $request->defined_arg('index');
my $option = $request->defined_arg('option');
my $domain = $self->search_domain_by_id($id_domain);
......@@ -4446,7 +4447,7 @@ sub _cmd_remove_hardware {
.$domain->name
if !$user->is_admin;
$domain->remove_controller($hardware, $index);
$domain->remove_controller($hardware, $index, $option);
}
sub _cmd_change_hardware {
......
......@@ -5489,6 +5489,20 @@ sub _add_hardware_display($orig, $self, $index, $data) {
$orig->($self, 'display', $index, $data) if $is_builtin;
}
sub _check_duplicated_volume_name($self, $file) {
return if !$file;
my ($name) = $file =~ m{.*/(.*)};
my $sth = $$CONNECTOR->dbh->prepare(
"SELECT id,name FROM volumes WHERE id_domain=? "
." AND (name=? or file=?)"
);
$sth->execute($self->id, $name, $file);
my ($id_found) = $sth->fetchrow;
die "Error: volume '$name' already exists in ".$self->name."\n"
if $id_found;
}
sub _add_hardware_disk($orig, $self, $index, $data) {
my $real_id_vm;
if (!$self->_vm->is_local) {
......@@ -5497,6 +5511,7 @@ sub _add_hardware_disk($orig, $self, $index, $data) {
$self->_set_vm($vm_local, 1);
}
$self->_check_duplicated_volume_name($data->{file});
$orig->($self, 'disk', $index, $data);
if (( defined $index || $data ) && $self->is_known() ) {
......@@ -5540,7 +5555,8 @@ sub _delete_db_display_by_driver($self, $driver) {
$sth->execute($self->id, $driver);
}
sub _around_remove_hardware($orig, $self, $hardware, $index) {
sub _around_remove_hardware($orig, $self, $hardware, $index=undef, $options=undef) {
confess "Error: supply either index or options when removing hardware " if !defined $index && !defined $options;
my $display;
if ( $hardware eq 'display' ) {
$display = $self->_get_display_by_index($index);
......@@ -5569,7 +5585,7 @@ sub _around_remove_hardware($orig, $self, $hardware, $index) {
$self->_delete_db_display_by_driver($driver);
}
} else {
$orig->($self, $hardware, $index)
$orig->($self, $hardware, $index, %$options)
}
if ( $self->is_known() && !$self->is_base ) {
......
......@@ -228,7 +228,7 @@ sub _vol_remove {
my $file = shift;
my $warning = shift;
confess "Error: I won't remove an iso file " if $file =~ /\.iso$/i;
confess "Error: I won't remove an iso file " if $file && $file =~ /\.iso$/i;
my $name;
($name) = $file =~ m{.*/(.*)} if $file =~ m{/};
......@@ -256,6 +256,8 @@ sub _vol_remove {
}
sub remove_volume {
my ($file) = $_[1];
return if !defined $file || $file =~ /\.iso$/;
return _vol_remove(@_);
}
......@@ -1181,7 +1183,7 @@ sub add_volume {
,format => $format
,allocation => ($args{allocation} or undef)
,target => $target_dev
) if !$path;
) if !$path && $device ne 'cdrom';
($name) = $path =~ m{.*/(.*)} if !$name;
# TODO check if <target dev="/dev/vda" bus='virtio'/> widhout dev works it out
......@@ -1215,7 +1217,7 @@ sub add_volume {
eval { $self->domain->attach_device($xml_device,Sys::Virt::Domain::DEVICE_MODIFY_CONFIG) };
die $@ if $@;
$self->_set_boot_order($path, $boot) if $boot;
return $path;
return ( $path or $name);
}
sub _set_boot_hd($self, $value) {
......@@ -1338,7 +1340,7 @@ sub _search_volume_index($self, $file) {
sub _xml_new_device($self , %arg) {
my $bus = delete $arg{bus} or confess "Missing bus.";
my $file = delete $arg{file} or confess "Missing target.";
my $file = ( delete $arg{file} or '');
my $boot = delete $arg{boot};
my $device = delete $arg{device};
......@@ -2327,8 +2329,8 @@ sub remove_controller($self, $name, $index=0,$attribute_name=undef, $attribute_v
my $ret;
#some hardware can be removed searching by attribute
if($name eq 'display') {
$ret = $sub->($self, 0, $attribute_name, $attribute_value);
if($name eq 'display' || defined $attribute_name ) {
$ret = $sub->($self, undef, $attribute_name, $attribute_value);
} else {
$ret = $sub->($self, $index);
}
......@@ -2336,25 +2338,51 @@ sub remove_controller($self, $name, $index=0,$attribute_name=undef, $attribute_v
return $ret;
}
sub _remove_device($self, $index, $device, $attribute_name=undef, $attribute_value=undef) {
sub _find_child($controller, $name) {
return ($controller,$name) if !defined $name || $name !~ m{(.*?)/(.*)};
my ($child_name, $attribute) = ($1,$2);
my ($item) = $controller->findnodes($child_name);
return ($controller,$name) if !$item;
return _find_child($item, $attribute);
}
sub _remove_device($self, $index, $device, $attribute_name0=undef, $attribute_value=undef) {
my $doc = XML::LibXML->load_xml(string => $self->xml_description_inactive);
my ($devices) = $doc->findnodes('/domain/devices');
my $ind=0;
my @found;
for my $controller ($devices->findnodes($device)) {
next if defined $attribute_name
&& $controller->getAttribute($attribute_name) !~ $attribute_value;
my ($item, $attr_name)= _find_child($controller, $attribute_name0);
my $found = 0;
if ( defined $attr_name ) {
my $found_value = $item->getAttribute($attr_name);
push @found,($found_value or '');
$found = 1
if defined $found_value && $found_value =~ $attribute_value;
}
if($found || defined $index && $ind++==$index ){
my ($source ) = $controller->findnodes("source");
my $file;
$file = $source->getAttribute('file') if $source;
if( $ind++==$index ){
$devices->removeChild($controller);
$self->_vm->connect if !$self->_vm->vm;
$self->reload_config($doc);
return;
return $file;
}
}
my $msg = "";
$msg = " $attribute_name=$attribute_value " if defined $attribute_name;
confess "ERROR: $device $msg $index"
$msg = " $attribute_name0=$attribute_value ".join(",",@found)
if defined $attribute_name0;
confess "ERROR: $device $msg ".($index or '<UNDEF>')
." not removed, only ".($ind)." found in ".$self->name."\n";
}
......@@ -2367,20 +2395,27 @@ sub _remove_controller_usb($self, $index) {
$self->_remove_device($index,'redirdev', bus => 'usb');
}
sub _remove_controller_disk($self, $index) {
sub _remove_controller_disk($self, $index, $attribute_name=undef, $attribute_value=undef) {
my @volumes = $self->list_volumes_info();
confess "Error: domain ".$self->name
." trying to remove $index"
." has only ".scalar(@volumes)
if $index >= scalar(@volumes);
if defined $index && $index >= scalar(@volumes);
confess "Error: undefined volume $index ".Dumper(\@volumes)
if !defined $volumes[$index];
if defined $index && !defined $volumes[$index];
$self->_remove_device($index,'disk');
confess "Error: undefined index and attribute"
if !defined $index && !defined $attribute_name;
my $file;
if ($attribute_name) {
$file = $self->_remove_device($index,'disk',$attribute_name => $attribute_value);
} else {
$file = $self->_remove_device($index,'disk');
}
my $file = $volumes[$index]->{file};
$self->remove_volume( $file ) if $file && $file !~ /\.iso$/;
$self->remove_volume( $file );
$self->info(Ravada::Utils::user_daemon);
}
......
......@@ -547,14 +547,15 @@ sub remove_volume($self, $file) {
$self->_vol_remove($file);
}
sub _remove_controller_disk($self,$file) {
sub _remove_controller_disk($self,$index) {
return if ! $self->_vm->file_exists($self->_config_file);
my $data = $self->_load();
my $hardware = $data->{hardware};
my @devices_new;
my $n = 0;
for my $info (@{$hardware->{device}}) {
next if $info->{file} eq $file;
next if $n++ == $index;
push @devices_new,($info);
}
$hardware->{device} = \@devices_new;
......@@ -912,8 +913,9 @@ sub _remove_disk {
my ($self, $index) = @_;
confess "Index is '$index' not number" if !defined $index || $index !~ /^\d+$/;
my @volumes = $self->list_volumes();
$self->remove_volume($volumes[$index]);
$self->_remove_controller_disk($volumes[$index]);
$self->remove_volume($volumes[$index])
if $volumes[$index] && $volumes[$index] !~ /\.iso$/;
$self->_remove_controller_disk($index);
}
sub remove_controller {
......
......@@ -101,7 +101,7 @@ our %VALID_ARG = (
}
,change_owner => {uid => 1, id_domain => 1}
,add_hardware => {uid => 1, id_domain => 1, name => 1, number => 2, data => 2 }
,remove_hardware => {uid => 1, id_domain => 1, name => 1, index => 1}
,remove_hardware => {uid => 1, id_domain => 1, name => 1, index => 2, option => 2}
,change_hardware => {uid => 1, id_domain => 1, hardware => 1, index => 2, data => 1 }
,enforce_limits => { timeout => 2, _force => 2 }
,refresh_machine => { id_domain => 1, uid => 1 }
......@@ -218,6 +218,7 @@ our %CMD_VALIDATE = (
clone => \&_validate_clone
,create => \&_validate_create_domain
,create_domain => \&_validate_create_domain
,remove_hardware => \&_validate_remove_hardware
,start_domain => \&_validate_start_domain
,start => \&_validate_start_domain
,add_hardware=> \&_validate_change_hardware
......@@ -753,6 +754,21 @@ sub _validate($self) {
$method->($self);
}
sub _validate_remove_hardware($self) {
my $name = $self->args('name');
my $args = $self->args();
die "Error: you must pass option or index"
if !exists $args->{option} && !exists $args->{index}
&& !defined $args->{option} && !defined $args->{index};
die "Error: attribute value must be defined ".
join(" ", map { $_ or '<UNDEF>' } %{$args->{option}})
if $args->{option} && grep { !defined } values %{$args->{option}};
}
sub _validate_start_domain($self) {
my $id_domain = $self->defined_arg('id_domain');
......@@ -785,7 +801,6 @@ sub _validate_change_hardware($self) {
$self->after_request($req->id) if $req;
}
sub _validate_create_domain($self) {
my $base;
......
......@@ -236,13 +236,14 @@ sub _get_cached_info($self) {
." ORDER by n_order"
);
$sth->execute($self->name, $self->domain->id);
my $row = $sth->fetchrow_hashref();
return if !$row || !keys %$row;
if ( $row->{info} ) {
$row->{info} = decode_json($row->{info})
}
return $row;
my $found;
while ( my $row = $sth->fetchrow_hashref()) {
if ( $row->{info} ) {
$row->{info} = decode_json($row->{info})
}
return $row;
};
return $found;
}
sub _cache_volume_info($self) {
......@@ -267,7 +268,8 @@ sub _cache_volume_info($self) {
,$name
,$file
,$n_order
,encode_json(\%info));
,encode_json(\%info)
);
};
confess "$name / $n_order \n".$@ if $@;
return;
......
......@@ -67,8 +67,8 @@ sub clone($self, $file_clone) {
sleep 1;
die "Error: ".$self->file." looks active" if $n-- <0;
}
confess if $self->file =~ /ISO/i;
confess if $file_clone =~ /ISO/i;
confess if $self->file =~ /ISO$/i;
confess if $file_clone =~ /ISO$/i;
my $base_format = lc(Ravada::Volume::_type_from_file($self->file, $self->vm));
my ($out, $err);
......
......@@ -612,6 +612,9 @@
if ( hardware == 'disk' && extra.device == 'cdrom') {
extra.driver = 'sata';
}
if ( hardware == 'disk' && extra.device != 'cdrom') {
extra.file= '';
}
if (hardware == 'display' && ! extra) {
$scope.show_new_display = true;
......@@ -630,7 +633,24 @@
item.remove = !item.remove;
return;
}
var file = $scope.showmachine.hardware.disk[index].file;
if (typeof(file) != 'undefined' && file) {
$scope.showmachine.requests++;
$http.post('/request/remove_hardware/'
,JSON.stringify({
'id_domain': $scope.showmachine.id
,'name': 'disk'
,'option': { 'source/file': file }
})
).then(function(response) {
});
item.remove = false;
return;
}
}
$scope.showmachine.requests++;
if(typeof(item) == 'object') {
item.remove = false;
}
......@@ -880,6 +900,7 @@
};
$scope.request = function(request, args) {
$scope.showmachine.requests++;
$scope.pending_request = undefined;
$http.post('/request/'+request+'/'
,JSON.stringify(args)
......
......@@ -1072,6 +1072,7 @@ sub test_fill_memory($vm, $node, $migrate) {
my @clones;
for ( 1 .. 100 ) {
my $clone_name = new_domain_name();
diag("Try $_ , $clone_name may go to ".$node->name);
my $req = Ravada::Request->create_domain(
name => $clone_name
,id_owner => user_admin->id
......@@ -1088,15 +1089,16 @@ sub test_fill_memory($vm, $node, $migrate) {
wait_request(debug => 0);
eval { $clone->start(user_admin) };
$error = $@;
diag($error) if $error;
like($error, qr/(^$|No free memory)/);
exit if $error && $error !~ /No free memory/;
last if $error;
my $clone_2 = Ravada::Domain->open($clone->id);
$nodes{$clone_2->_vm->name}++;
$clone = Ravada::Domain->open($clone->id);
$nodes{$clone->_vm->name}++;
last if $migrate && exists $nodes{$vm->name} && $nodes{$vm->name} > 2;
if (scalar keys(%nodes) > 0) {
if ($migrate || keys(%nodes) > 0) {
$memory = int($memory*1.5);
}
}
......@@ -1591,6 +1593,8 @@ for my $vm_name (vm_names() ) {
start_node($node);
test_fill_memory($vm, $node, 1); # migrate
# test displays with no builtin added
test_displays($vm, $node,1) if $tls;
# test displays with only builtin
......
......@@ -8,6 +8,9 @@ use Data::Dumper;
use Mojo::JSON qw(decode_json);
use Test::More;
no warnings "experimental::signatures";
use feature qw(signatures);
use lib 't/lib';
use Test::Ravada;
......@@ -62,6 +65,187 @@ sub test_frontend_refresh {
$domain->remove(user_admin);
}
sub test_remove_disk($vm, %options) {
diag(Dumper(\%options));
my $make_base = delete $options{make_base};
my $clone = delete $options{clone};
my $remove_by_file = ( delete $options{remove_by_file} or 0);
my $remove_by_index = ( delete $options{remove_by_index} or 1);
my $add_iso_to_clone = delete $options{add_iso_to_clone};
return if $clone && $vm->type eq 'Void';
my $id_iso = ( delete $options{id_iso} or search_id_iso('Alpine%64'));
die "Error: unknown options ".Dumper(\%options)
if keys %options;
for my $index ( 0 .. 3 ) {
diag("\tindex=$index");
my $name = new_domain_name();
my $req = Ravada::Request->create_domain(
name => $name
,id_owner => user_admin->id
,swap => 1024 * 1024
,data => 1024 * 1024
,id_iso => $id_iso
,vm => $vm->type
);
wait_request(debug => 0);
is($req->error,'');
my $domain = rvd_back->search_domain($name);
ok($domain) or return;
if ($make_base) {
$domain->prepare_base(user_admin);
$domain->remove_base(user_admin);
}
if ($clone || $add_iso_to_clone) {
$domain->prepare_base(user_admin);
my $clone = $domain->clone(
name => new_domain_name()
,user => user_admin
);
$domain = $clone;
_add_iso_to_clone($domain) if $add_iso_to_clone;
}
my $info0 = $domain->info(user_admin);
my $n_disks0 = scalar(@{$info0->{hardware}->{disk}});
my %files0 = map { ($_->{file} or '') => 1 } @{$info0->{hardware}->{disk}};
my $expect_removed = $info0->{hardware}->{disk}->[$index];
my @way = ( index => $index );
@way = ( option => { "source/file" => $expect_removed->{file} } )
if $remove_by_file && $expect_removed->{file};
push @way ,( index => $index )
if $remove_by_index;
my $req_rm = Ravada::Request->remove_hardware(
uid => user_admin->id
,id_domain => $domain->id
,name => 'disk'
,@way
);
wait_request(debug => 0);
is($req_rm->status,'done');
is($req_rm->error, '');
my $info = $domain->info(user_admin);
my $n_disks = scalar(@{$info->{hardware}->{disk}});
my %files = map { ($_->{file} or '') => 1 } @{$info->{hardware}->{disk}};
is($n_disks, $n_disks0-1) or exit;
isnt($info->{hardware}->{disk}->[$index]->{file}
,$info0->{hardware}->{disk}->[$index]->{file})
if $index<3;
if ($expect_removed->{file}) {
if ($expect_removed->{device} eq 'cdrom') {
ok(-e $expect_removed->{file},$expect_removed->{file});
} else {
ok(!-e $expect_removed->{file}) or die "$expect_removed->{file} should be removed";
}
}
_check_volumes_table($domain, $n_disks, $expect_removed->{file});
_check_info($domain, $n_disks, $expect_removed->{file});
_check_info_front($domain->id, $n_disks, $expect_removed->{file});
$domain->remove(user_admin);
}
}
sub _add_iso_to_clone($domain) {
my $req = Ravada::Request->add_hardware(
id_domain => $domain->id
,uid => user_admin->id
,'data' => {
'driver' => 'ide',
'type' => 'sys',
'allocation' => '0.1G',
'device' => 'cdrom',
'file' => '/var/lib/libvirt/images/alpine-standard-3.8.1-x86.iso',
'capacity' => '1G'
},
'name' => 'disk',
);
wait_request();
}
sub _check_volumes_table($domain, $n_disks, $file) {
my $sth = connector->dbh->prepare(
"SELECT count(*) FROM volumes WHERE id_domain=?"
);
$sth->execute($domain->id);
my ($count) = $sth->fetchrow;
is($count, $n_disks) or exit;
return if !$file;
$sth = connector->dbh->prepare(
"SELECT * FROM volumes WHERE id_domain=? AND file=?"
);
$sth->execute($domain->id, $file);
my $row = $sth->fetchrow_hashref;
ok(!$row->{id}) or die Dumper($row);
}
sub _check_info($domain, $n_disks, $file) {
my $info = $domain->info(user_admin);
my @disks = @{$info->{hardware}->{disk}};
is(scalar(@disks), $n_disks) or die Dumper(\@disks);
return if !$file;
my ($gone) = grep { defined $_->{file} && $_->{file} eq $file} @disks;
ok(!$gone, "Expecting $file gone") or die Dumper(\@disks);
}
sub _check_info_front($id_domain, $n_disks, $file) {
my $domain = Ravada::Front::Domain->open($id_domain);
my $info = $domain->info(user_admin);
my @disks = @{$info->{hardware}->{disk}};
is(scalar(@disks), $n_disks) or die Dumper(\@disks);
return if !$file;
my ($gone) = grep { defined $_->{file} && $_->{file} eq $file} @disks;
ok(!$gone, "Expecting $file gone") or die Dumper(\@disks);
}
sub test_add_cd($vm, $data) {
my $domain = create_domain($vm);
my $info0 = $domain->info(user_admin);
my $n_disks0 = scalar(@{$info0->{hardware}->{disk}});
my %targets0 = map { $_->{target} => 1 } @{$info0->{hardware}->{disk}};
my $req = Ravada::Request->add_hardware(
id_domain => $domain->id
,name => 'disk'
,uid => user_admin->id
,'data' => $data
);
ok($req);
rvd_back->_process_requests_dont_fork();
is($req->status,'done');
is($req->error,'');
my $info = $domain->info(user_admin);
my $n_disks = scalar(@{$info->{hardware}->{disk}});
is($n_disks, $n_disks0+1);
my $new_dev;
for my $dev ( @{$info->{hardware}->{disk}} ) {
next if $targets0{$dev->{target}};
$new_dev = $dev;