mirror of
https://github.com/xcat2/xcat-core.git
synced 2026-09-22 00:49:30 +00:00
506069061b
The name of the volume of a node, and the bus of a file-backed disk, could come from a match made by a routine on the call path. A riscv64 node breaks on it: a leaked value that is neither scsi nor virtio gives the node an hd* volume, and the riscv64 virt machine has no IDE controller for that disk. createstorage and build_diskstruct in xCAT-server/lib/xcat/plugins/kvm.pm read the model of the disk out of the vmstorage value with s/=(.*)//, then read $1. The substitution is allowed to fail, because most vmstorage values state no model, and a failed match leaves $1 as the last successful capture. dohyp gives every node the storage model scsi before mkvm runs, and a captured value takes priority over it, so a leaked value can only replace the default that keeps a riscv64 node on sd*. The leak follows the call path, not the history of the process. Perl restores $1 when the block that set it ends, so a match made in a routine that has returned cannot reach createstorage; only a match still live in an enclosing block can, and a later successful match without a group empties $1 again. A long-running xcatd is not what makes this happen, and looking for one is a wrong turn. Both routines now read $1 only when their own substitution matches. A vmstorage value that states a model, and vmstoragemodel, name the volume as before. The default itself moves into default_storagemodel, which dohyp calls, so a test can hold it. It sat inline with a comment, and changing it to ide left every assertion passing. kvm_createstorage_model.t runs each node twice, once with a capture left live in the calling block, because a case that leaves $1 empty passes against the defect. Five of its eleven assertions fail without this change, and a sixth fails if the default changes. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
125 lines
5.7 KiB
Perl
125 lines
5.7 KiB
Perl
#!/usr/bin/env perl
|
|
use strict;
|
|
use warnings;
|
|
|
|
use FindBin;
|
|
use Test::More;
|
|
|
|
# The scratch package below declares these; the test names them once each.
|
|
no warnings 'once';
|
|
|
|
my $source = "$FindBin::Bin/../../xCAT-server/lib/xcat/plugins/kvm.pm";
|
|
open(my $source_fh, '<', $source) or die "open $source: $!";
|
|
my $content = do { local $/; <$source_fh> };
|
|
close($source_fh) or die "close $source: $!";
|
|
|
|
my @routines;
|
|
for my $name (qw(createstorage build_diskstruct guest_arch_profile getUnits
|
|
default_storagemodel)) {
|
|
my ($routine) = $content =~ /^(sub \Q$name\E\s*\{.*?^\})/ms;
|
|
BAIL_OUT("could not extract $name from kvm.pm") unless $routine;
|
|
push(@routines, $routine);
|
|
}
|
|
|
|
# kvm.pm needs a management node to load, so createstorage runs in a scratch package.
|
|
# get_filepath_by_url is the routine that reaches libvirt; it records the device name it is
|
|
# asked for, which is the name createstorage gives the volume of the node.
|
|
my $harness = <<'PERL';
|
|
package KVMStore;
|
|
our ($node, $confdata, $clonemethod, @asked);
|
|
sub getstorageformat { my ($cfginfo) = @_; return $cfginfo->{storageformat}; }
|
|
sub get_filepath_by_url { my %args = @_; push(@asked, $args{dev}); return $args{dev}; }
|
|
sub oldCreateStorage { push(@asked, 'oldCreateStorage'); }
|
|
sub get_multiple_paths_by_url { return {}; }
|
|
PERL
|
|
|
|
eval $harness . join("\n", @routines) . "\n1;\n"; ## no critic (BuiltinFunctions::ProhibitStringyEval)
|
|
BAIL_OUT("could not load the kvm storage routines: $@") if $@;
|
|
|
|
# The name createstorage gives the volume of one node. $stale is a capture left live in this
|
|
# block by an earlier successful match, which is the state createstorage runs in when a
|
|
# routine on the call path matched a pattern that has a group.
|
|
sub volume_dev {
|
|
my (%args) = @_;
|
|
my $storage = $args{storage} // 'dir:///var/lib/libvirt/images/';
|
|
my $cfginfo = {
|
|
node => 'cn1',
|
|
host => 'hyp1',
|
|
storage => $storage,
|
|
storagemodel => $args{storagemodel},
|
|
};
|
|
@KVMStore::asked = ();
|
|
# The match must run in this block, and nothing may match after it: perl restores $1 when
|
|
# the block that set it ends, and any later successful match replaces what it holds.
|
|
my $subject = 'left by an earlier match: ' . ($args{stale} // '');
|
|
$subject =~ /match: (.*)/ if defined $args{stale};
|
|
# A match without a group empties $1, which is the clean state the other cases need.
|
|
$subject =~ /^left/ unless defined $args{stale};
|
|
KVMStore::createstorage($storage, undef, '30G', $cfginfo, 1);
|
|
return $KVMStore::asked[0];
|
|
}
|
|
|
|
# dohyp sets storagemodel to scsi for every node it dispatches, whatever the architecture,
|
|
# before mkvm reaches createstorage. That default is what names the volume of a node whose
|
|
# vmstoragemodel is empty, and a riscv64 node depends on it: the riscv64 virt machine has no
|
|
# IDE controller, so its volume must be sd*.
|
|
is(volume_dev(storagemodel => 'scsi'), 'sda',
|
|
'the scsi storage model names an sd* volume');
|
|
|
|
# A capture from a match made elsewhere must not name the volume. These are the values a
|
|
# routine on the mkvm call path can leave in $1.
|
|
is(volume_dev(storagemodel => 'scsi', stale => '/var/lib/libvirt/images/'), 'sda',
|
|
'a path left by an earlier match does not name the volume');
|
|
is(volume_dev(storagemodel => 'scsi', stale => 'virtio'), 'sda',
|
|
'a model name left by an earlier match does not name the volume');
|
|
is(volume_dev(storagemodel => 'virtio', stale => 'scsi'), 'vda',
|
|
'an earlier match does not override vmstoragemodel either');
|
|
|
|
# The model stated on the vmstorage value, and vmstoragemodel, still name the volume.
|
|
is(volume_dev(storage => 'dir:///var/lib/libvirt/images/=scsi'), 'sda',
|
|
'a model on the vmstorage value names an sd* volume');
|
|
is(volume_dev(storagemodel => 'virtio'), 'vda',
|
|
'vmstoragemodel=virtio names a vd* volume');
|
|
|
|
# createstorage on its own defaults to ide. Nothing in the product reaches this today, because
|
|
# dohyp gives every node the default storage model first.
|
|
is(volume_dev(), 'hda', 'createstorage alone defaults to an hd* volume');
|
|
|
|
# So the sd* name of a node with no vmstoragemodel rests on that default, and a riscv64 node
|
|
# rests on the sd* name. Drive the two together, so a change to the default fails here rather
|
|
# than on a riscv64 node that stops booting.
|
|
is(volume_dev(storagemodel => KVMStore::default_storagemodel()), 'sda',
|
|
'the default storage model names an sd* volume');
|
|
|
|
# build_diskstruct reads $1 the same way, for a disk backed by a plain file. The device name
|
|
# and the bus of that disk must come from the node, not from a match made elsewhere.
|
|
sub file_disk {
|
|
my (%args) = @_;
|
|
local $KVMStore::node = 'cn1';
|
|
local $KVMStore::confdata = {
|
|
vm => { cn1 => [ { host => 'hyp1', storage => '/var/lib/libvirt/images/cn1.img' } ] },
|
|
nodetype => { cn1 => [ { arch => $args{arch} } ] },
|
|
hyp1 => { cpumodel => 'x86_64' },
|
|
};
|
|
my $chatter = '';
|
|
my $disks;
|
|
my $subject = 'left by an earlier match: ' . ($args{stale} // '');
|
|
$subject =~ /match: (.*)/ if defined $args{stale};
|
|
$subject =~ /^left/ unless defined $args{stale};
|
|
{
|
|
open(my $capture, '>', \$chatter) or die "capture stdout: $!";
|
|
local *STDOUT = $capture;
|
|
($disks) = KVMStore::build_diskstruct(undef);
|
|
}
|
|
return $disks->[0];
|
|
}
|
|
|
|
is(file_disk(arch => 'x86_64')->{target}->{bus}, 'ide',
|
|
'a file-backed disk of an x86_64 node is ide');
|
|
is(file_disk(arch => 'x86_64', stale => 'virtio')->{target}->{bus}, 'ide',
|
|
'a model name left by an earlier match does not choose the bus of a file-backed disk');
|
|
is(file_disk(arch => 'riscv64', stale => 'ide')->{target}->{dev}, 'sda',
|
|
'a riscv64 file-backed disk keeps its sd* name whatever an earlier match left behind');
|
|
|
|
done_testing();
|