[pve-devel] [PATCH v3 qemu-server 3/8] refactor: split check_running into _exists_ and _running_
Fabian Grünbichler
f.gruenbichler at proxmox.com
Tue Nov 19 10:30:09 CET 2019
On November 4, 2019 2:57 pm, Stefan Reiter wrote:
> vm_exists_on_node in PVE::QemuConfig checks if a config file for a vmid
> exists
>
> vm_running_locally in PVE::QemuServer::Helpers checks if a VM is running
> on the local machine by probing its pidfile and checking /proc/.../cmdline
>
> check_running is left in QemuServer for compatibility, but changed to
> simply call the two new helper functions.
>
> Both methods are also correctly mocked for testing snapshots.
>
> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
> ---
>
> No users of check_running are changed for now, but this could be done in a
> future series.
>
>
> PVE/QemuConfig.pm | 16 ++++++++-
> PVE/QemuServer.pm | 67 ++------------------------------------
> PVE/QemuServer/Helpers.pm | 68 +++++++++++++++++++++++++++++++++++++++
> test/snapshot-test.pm | 16 +++++++--
> 4 files changed, 100 insertions(+), 67 deletions(-)
>
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 499d5e3..bca2725 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -16,6 +16,20 @@ my $nodename = PVE::INotify::nodename();
>
> my $MAX_UNUSED_DISKS = 256;
>
> +sub vm_exists_on_node {
> + my ($vmid, $node, $noerr) = @_;
> +
> + my $filename = __PACKAGE__->config_file($vmid, $node);
> + my $exists = -f $filename;
> +
> + if (!$noerr && !$exists) {
> + my $node_err = $node ? "on node '$node'" : "on local node";
> + die "unable to find configuration file for VM $vmid $node_err - no such machine\n";
why not simple set $node to $nodename? we already have it anyway, it's
what gets used down the call stack anyway, and 'local node' for
something that potentially gets called on other nodes during migration
is always a potential source of confusion ;)
is there a use-case for $noerr? existing $nocheck callers probably skip
this altogether since they don't care whether the config exists or not..
we could still introduce it later if we find a caller that needs it?
> + }
> +
> + return $exists;
> +}
> +
> # BEGIN implemented abstract methods from PVE::AbstractConfig
IMHO, vm_exists_on_node is a prime candidate for being put into
AbstractConfig - it's not qemu-specific at all, we probably want to do a
similar split for pve-container as well, and we want to use it when
appropriate in AbstractConfig.
any objections to moving it there? maybe adapt the name?
s/vm/guest_config/ ? actual moving could also be done at a latter point
to avoid the bump in + depends on pve-guest-common
>
> sub guest_type {
> @@ -161,7 +175,7 @@ sub __snapshot_save_vmstate {
>
> sub __snapshot_check_running {
> my ($class, $vmid) = @_;
> - return PVE::QemuServer::check_running($vmid);
> + return PVE::QemuServer::Helpers::vm_running_locally($vmid);
> }
>
> sub __snapshot_check_freeze_needed {
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ac9abd0..cc9288f 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2952,73 +2952,12 @@ sub check_local_storage_availability {
> return $nodehash
> }
>
> -sub parse_cmdline {
> - my ($pid) = @_;
> -
> - my $fh = IO::File->new("/proc/$pid/cmdline", "r");
> - if (defined($fh)) {
> - my $line = <$fh>;
> - $fh->close;
> - return undef if !$line;
> - my @param = split(/\0/, $line);
> -
> - my $cmd = $param[0];
> - return if !$cmd || ($cmd !~ m|kvm$| && $cmd !~ m@(?:^|/)qemu-system-[^/]+$@);
> -
> - my $phash = {};
> - my $pending_cmd;
> - for (my $i = 0; $i < scalar (@param); $i++) {
> - my $p = $param[$i];
> - next if !$p;
> -
> - if ($p =~ m/^--?(.*)$/) {
> - if ($pending_cmd) {
> - $phash->{$pending_cmd} = 1;
> - }
> - $pending_cmd = $1;
> - } elsif ($pending_cmd) {
> - $phash->{$pending_cmd} = $p;
> - $pending_cmd = undef;
> - }
> - }
> -
> - return $phash;
> - }
> - return undef;
> -}
> -
> +# Compat only, use vm_exists_on_node and vm_running_locally where possible
> sub check_running {
> my ($vmid, $nocheck, $node) = @_;
>
> - my $filename = PVE::QemuConfig->config_file($vmid, $node);
> -
> - die "unable to find configuration file for VM $vmid - no such machine\n"
> - if !$nocheck && ! -f $filename;
> -
> - my $pidfile = PVE::QemuServer::Helpers::pidfile_name($vmid);
> -
> - if (my $fd = IO::File->new("<$pidfile")) {
> - my $st = stat($fd);
> - my $line = <$fd>;
> - close($fd);
> -
> - my $mtime = $st->mtime;
> - if ($mtime > time()) {
> - warn "file '$filename' modified in future\n";
> - }
> -
> - if ($line =~ m/^(\d+)$/) {
> - my $pid = $1;
> - my $cmdline = parse_cmdline($pid);
> - if ($cmdline && $cmdline->{pidfile} eq $pidfile) {
> - if (my $pinfo = PVE::ProcFSTools::check_process_running($pid)) {
> - return $pid;
> - }
> - }
> - }
> - }
> -
> - return undef;
> + PVE::QemuConfig::vm_exists_on_node($vmid, $node, $nocheck);
> + return PVE::QemuServer::Helpers::vm_running_locally($vmid);
> }
>
> sub vzlist {
> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
> index 91e2c83..1f472a9 100644
> --- a/PVE/QemuServer/Helpers.pm
> +++ b/PVE/QemuServer/Helpers.pm
> @@ -3,7 +3,10 @@ package PVE::QemuServer::Helpers;
> use strict;
> use warnings;
>
> +use File::stat;
> +
> use PVE::INotify;
> +use PVE::ProcFSTools;
>
> my $nodename = PVE::INotify::nodename();
>
> @@ -34,4 +37,69 @@ sub vnc_socket {
> return "${var_run_tmpdir}/$vmid.vnc";
> }
>
> +# Parse the cmdline of a running kvm/qemu process and return arguments as hash
> +sub parse_cmdline {
> + my ($pid) = @_;
> +
> + my $fh = IO::File->new("/proc/$pid/cmdline", "r");
> + if (defined($fh)) {
> + my $line = <$fh>;
> + $fh->close;
> + return undef if !$line;
> + my @param = split(/\0/, $line);
> +
> + my $cmd = $param[0];
> + return if !$cmd || ($cmd !~ m|kvm$| && $cmd !~ m@(?:^|/)qemu-system-[^/]+$@);
> +
> + my $phash = {};
> + my $pending_cmd;
> + for (my $i = 0; $i < scalar (@param); $i++) {
> + my $p = $param[$i];
> + next if !$p;
> +
> + if ($p =~ m/^--?(.*)$/) {
> + if ($pending_cmd) {
> + $phash->{$pending_cmd} = 1;
> + }
> + $pending_cmd = $1;
> + } elsif ($pending_cmd) {
> + $phash->{$pending_cmd} = $p;
> + $pending_cmd = undef;
> + }
> + }
> +
> + return $phash;
> + }
> + return undef;
> +}
> +
> +sub vm_running_locally {
> + my ($vmid) = @_;
> +
> + my $pidfile = pidfile_name($vmid);
> +
> + if (my $fd = IO::File->new("<$pidfile")) {
> + my $st = stat($fd);
> + my $line = <$fd>;
> + close($fd);
> +
> + my $mtime = $st->mtime;
> + if ($mtime > time()) {
> + warn "file '$pidfile' modified in future\n";
> + }
> +
> + if ($line =~ m/^(\d+)$/) {
> + my $pid = $1;
> + my $cmdline = parse_cmdline($pid);
> + if ($cmdline && $cmdline->{pidfile} eq $pidfile) {
> + if (my $pinfo = PVE::ProcFSTools::check_process_running($pid)) {
> + return $pid;
> + }
> + }
> + }
> + }
> +
> + return undef;
> +}
> +
> 1;
> diff --git a/test/snapshot-test.pm b/test/snapshot-test.pm
> index c95e7f3..a76b4fd 100644
> --- a/test/snapshot-test.pm
> +++ b/test/snapshot-test.pm
> @@ -297,14 +297,22 @@ sub __snapshot_save_vmstate {
> $snap->{vmstate} = "somestorage:state-volume";
> $snap->{runningmachine} = "somemachine"
> }
> +
> +sub vm_exists_on_node {
> + my ($vmid, $node) = @_;
> + return -f cfs_config_path(undef, $vmid, $node);
> +}
> # END mocked PVE::QemuConfig methods
>
> -# BEGIN redefine PVE::QemuServer methods
> +# BEGIN mocked PVE::QemuServer::Helpers methods
>
> -sub check_running {
> +sub vm_running_locally {
> return $running;
> }
>
> +# END mocked PVE::QemuServer::Helpers methods
> +
> +# BEGIN redefine PVE::QemuServer methods
>
> sub do_snapshots_with_qemu {
> return 0;
> @@ -369,6 +377,9 @@ sub vm_stop {
> PVE::Tools::run_command("rm -rf snapshot-working");
> PVE::Tools::run_command("cp -a snapshot-input snapshot-working");
>
> +my $qemu_helpers_module = new Test::MockModule('PVE::QemuServer::Helpers');
> +$qemu_helpers_module->mock('vm_running_locally', \&vm_running_locally);
> +
> my $qemu_config_module = new Test::MockModule('PVE::QemuConfig');
> $qemu_config_module->mock('config_file_lock', \&config_file_lock);
> $qemu_config_module->mock('cfs_config_path', \&cfs_config_path);
> @@ -376,6 +387,7 @@ $qemu_config_module->mock('load_config', \&load_config);
> $qemu_config_module->mock('write_config', \&write_config);
> $qemu_config_module->mock('has_feature', \&has_feature);
> $qemu_config_module->mock('__snapshot_save_vmstate', \&__snapshot_save_vmstate);
> +$qemu_config_module->mock('vm_exists_on_node', \&vm_exists_on_node);
>
> # ignore existing replication config
> my $repl_config_module = new Test::MockModule('PVE::ReplicationConfig');
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list