[pve-devel] [PATCH] convert monitor_command to qmp_command - part1
Alexandre DERUMIER
aderumier at odiso.com
Wed Jun 6 12:47:59 CEST 2012
Hi dietmar, sorry for this big patch.
>>Can't we us '1', or JSON::true ?
"1" don't work when passing to "to_json". I'll try JSON::true.
>> - if ($stat ne $lstat) {
>>We only want to print changes. Can we get back that functionality somehow?
ok, I miss that. I'll reput it.
>>That code look quite ugly for me. Maybe we can add wrappers for vm_qmp_command():
>>
>>for example:
>>
>>vm_mon_cmd($vmid, $execute, %params)
>>vm_mon_cmd_nocheck($vmid, $execute, %params)
>>
>>so we can call it like:
>>
>>vm_mon_cmd($vmid, 'block_set_io_throttle', device => $deviceid, bps => $bps, iops => $iops, ...)
>>
>>What do you think?
good idea,I'll change that. Should works fine.
> + $timeout = 3600;
>>Why do you overwrite timeout here? value is passed as parameter!
sorry,I was for debug, I forgot to remove it
>>Wow. I simply do not understand that code. For example $event is set but never used?
>>I guess this is worth a separate patch including some docs (inline comments).
with some qmp query, we can have more than 1 json response.
they are some events response (informative infos), I don't use them for now.
I think I'll add it to the return like
return ($return, $event), so we can use them later if needed.
>>Would you mind to explain that?
>>
>>For me it looks easier to catch the error in QemuMigrate.pm, and remove that
>>whole returnerror code:
>>
>>eval { PVE::QemuServer::vm_qmp_command($vmid, $migratecommand, 1); }
>>my $merr = $@;
>>
>>or do we need that functionality somewhere else?
I put that because when calling PVE::QemuServer::vm_qmp_command($vmid, $migratecommand, 1), I have error in json response, then it's dying.
Don't remember the message, but it's was false message as migration started fine.
(I'll do test again to be sure of message)
but yes,your are right (I think I was too tired yesterday....)
eval { PVE::QemuServer::vm_qmp_command($vmid, $migratecommand, 1); }
my $merr = $@;
is the right way !
I'll cleanup all the code and resubmit them in small patchs, and I'll made comments in each patch
Thanks for your comments,
-Alexandre
----- Mail original -----
De: "Dietmar Maurer" <dietmar at proxmox.com>
À: "Alexandre Derumier" <aderumier at odiso.com>, pve-devel at pve.proxmox.com
Envoyé: Mercredi 6 Juin 2012 12:16:20
Objet: RE: [pve-devel] [PATCH] convert monitor_command to qmp_command - part1
There are too many question in this patch, and still white space errors:
Applying: convert monitor_command to qmp_command - part1
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:15: space before tab in indent.
my $ejectcmd = {};
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:30: trailing whitespace, space before tab in indent.
my $changecmd = {};
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:84: space before tab in indent.
$self->log('info', "migration status: $ms (transferred ${trans}KB, " .
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:163: trailing whitespace.
/home/dietmar/pve2-devel/qemu-server/.git/rebase-apply/patch:252: trailing whitespace.
warning: 5 lines add whitespace errors.
So I will not commit that.
Also, this patch is quite large. If possible, try to assemble smaller ones.
Comments are inline:
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index
> c172fde..d10db8c 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -673,12 +673,24 @@ my $vmconfig_update_disk = sub {
> if (PVE::QemuServer::drive_is_cdrom($drive)) { # cdrom
>
> if (PVE::QemuServer::check_running($vmid)) {
> + my $ejectcmd = {};
> + $ejectcmd->{execute} = "eject";
> + $ejectcmd->{arguments}->{force} = bless( do{\(my $o = 1)},
> 'JSON::XS::Boolean' );
Can't we us '1', or JSON::true ?
> + $ejectcmd->{arguments}->{device} = "drive-$opt";
> +
> if ($drive->{file} eq 'none') {
> - PVE::QemuServer::vm_monitor_command($vmid, "eject -f
> drive-$opt", 0);
> + PVE::QemuServer::vm_qmp_command($vmid, $ejectcmd, 0);
> } else {
> + PVE::QemuServer::vm_qmp_command($vmid, $ejectcmd, 0);
> #force eject if
> +locked
> my $path = PVE::QemuServer::get_iso_path($storecfg, $vmid,
> $drive->{file});
> - PVE::QemuServer::vm_monitor_command($vmid, "eject -f
> drive-$opt", 0); #force eject if locked
> - PVE::QemuServer::vm_monitor_command($vmid, "change
> drive-$opt \"$path\"", 0) if $path;
> +
> + if($path) {
> + my $changecmd = {};
> + $changecmd->{execute} = "change";
> + $changecmd->{arguments}->{device} = "drive-$opt";
> + $changecmd->{arguments}->{target} = $path;
> + PVE::QemuServer::vm_qmp_command($vmid,
> $changecmd, 0);
> + }
> }
> }
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm index
> 0dc81a5..2a3d3a5 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -324,29 +324,28 @@ sub phase2 {
> # start migration
>
> my $start = time();
> + my $migratecommand->{execute} = "migrate";
> + $migratecommand->{arguments}->{uri} = "tcp:localhost:$lport";
> + my $merr = PVE::QemuServer::vm_qmp_command($vmid,
> $migratecommand,
> + 1);
>
> - my $merr = PVE::QemuServer::vm_monitor_command($vmid, "migrate -d
> \"tcp:localhost:$lport\"", 1);
> -
> - my $lstat = '';
> while (1) {
> sleep (2);
> - my $stat = PVE::QemuServer::vm_monitor_command($vmid, "info
> migrate", 1);
> - if ($stat =~ m/^Migration status:
> (active|completed|failed|cancelled)$/im) {
> + my $stat = PVE::QemuServer::vm_qmp_command($vmid, "query-
> migrate", 1);
> +
> + if ($stat->{status} =~ m/^(active|completed|failed|cancelled)$/im) {
> $merr = undef;
> - my $ms = $1;
> -
> - if ($stat ne $lstat) {
We only want to print changes. Can we get back that functionality somehow?
> - if ($ms eq 'active') {
> - my ($trans, $rem, $total) = (0, 0, 0);
> - $trans = $1 if $stat =~ m/^transferred ram: (\d+)
> kbytes$/im;
> - $rem = $1 if $stat =~ m/^remaining ram: (\d+) kbytes$/im;
> - $total = $1 if $stat =~ m/^total ram: (\d+) kbytes$/im;
> -
> - $self->log('info', "migration status: $ms (transferred
> ${trans}KB, " .
> - "remaining ${rem}KB), total ${total}KB)");
> - } else {
> - $self->log('info', "migration status: $ms");
> - }
> + my $ms = $stat->{status};
> +
> + if ($ms eq 'active') {
> + my ($trans, $rem, $total) = (0, 0, 0);
> + $trans = sprintf "%.2f", $stat->{ram}->{transferred}/1024 if $stat-
> >{ram}->{transferred};
> + $rem = sprintf "%.2f", $stat->{ram}->{remaining}/1024 if $stat-
> >{ram}->{remaining};
> + $total = sprintf "%.2f", $stat->{ram}->{total}/1024 if
> +$stat->{ram}->{total};
> +
> + $self->log('info', "migration status: $ms (transferred ${trans}KB, "
> .
> + "remaining ${rem}KB), total ${total}KB)");
> + } else {
> + $self->log('info', "migration status: $ms");
> }
>
> if ($ms eq 'completed') {
> @@ -363,10 +362,9 @@ sub phase2 {
>
> last if $ms ne 'active';
> } else {
> - die $merr if $merr;
> - die "unable to parse migration status '$stat' - aborting\n";
> + die $merr->{desc} if $merr->{desc};
> + die "unable to parse migration status '$stat->{status}' -
> +aborting\n";
> }
> - $lstat = $stat;
> };
> }
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index
> 47c122e..c608312 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2593,7 +2593,17 @@ sub qemu_block_set_io_throttle {
> $iops_rd = 0 if !$iops_rd;
> $iops_wr = 0 if !$iops_wr;
>
> - my $ret = vm_monitor_command($vmid, "block_set_io_throttle $deviceid
> $bps $bps_rd $bps_wr $iops $iops_rd $iops_wr");
> + my $cmd = {};
> + $cmd->{execute} = "block_set_io_throttle";
> + $cmd->{arguments}->{device} = $deviceid;
> + $cmd->{arguments}->{bps} = $bps;
> + $cmd->{arguments}->{bps_rd} = $bps_rd;
> + $cmd->{arguments}->{bps_wr} = $bps_wr;
> + $cmd->{arguments}->{iops} = $iops;
> + $cmd->{arguments}->{iops_rd} = $iops_rd;
> + $cmd->{arguments}->{iops_wr} = $iops_wr;
That code look quite ugly for me. Maybe we can add wrappers for vm_qmp_command():
for example:
vm_mon_cmd($vmid, $execute, %params)
vm_mon_cmd_nocheck($vmid, $execute, %params)
so we can call it like:
vm_mon_cmd($vmid, 'block_set_io_throttle', device => $deviceid, bps => $bps, iops => $iops, ...)
What do you think?
> +
> + my $ret = vm_qmp_command($vmid, $cmd);
> $ret =~ s/^\s+//;
> return 1 if $ret eq "";
> syslog("err", "error setting block_set_io_throttle: $ret"); @@ -2653,7
> +2663,7 @@ sub vm_start {
> } else {
> unlink $statefile;
> # fixme: send resume - is that necessary ?
> - eval { vm_monitor_command($vmid, "cont"); };
> + eval { vm_qmp_command($vmid, "cont"); };
> }
> }
>
> @@ -2662,31 +2672,38 @@ sub vm_start {
> my $migrate_speed = $defaults->{migrate_speed} || 8192;
> $migrate_speed = $conf->{migrate_speed} || $migrate_speed;
> eval {
> - my $cmd = "migrate_set_speed ${migrate_speed}m";
> - vm_monitor_command($vmid, $cmd);
> + my $cmd = {};
> + $cmd->{execute} = "migrate_set_speed";
> + $cmd->{arguments}->{value} = ($migrate_speed * 1048576);
> #value
> +in bytes with qmp
> +
> + vm_qmp_command($vmid, $cmd);
> };
>
> my $migrate_downtime = $defaults->{migrate_downtime};
> $migrate_downtime = $conf->{migrate_downtime} if defined($conf-
> >{migrate_downtime});
> if (defined($migrate_downtime)) {
> - my $cmd = "migrate_set_downtime ${migrate_downtime}";
> - eval { vm_monitor_command($vmid, $cmd); };
> + my $cmd = {};
> + $cmd->{execute} = "migrate_set_downtime";
> + $cmd->{arguments}->{value} = $migrate_downtime;
> +
> + eval { vm_qmp_command($vmid, $cmd); };
> }
>
> vm_balloonset($vmid, $conf->{balloon}) if $conf->{balloon};
> -
> +
> + vm_qmp_command($vmid,"query-commands");
> });
> }
>
> my $qmp_read_avail = sub {
> - my ($fh, $timeout) = @_;
> + my ($fh, $timeout, $returnerror) = @_;
>
> my $sel = new IO::Select;
> $sel->add($fh);
>
> my $res = '';
> my $buf;
> -
> + $timeout = 3600;
Why do you overwrite timeout here? value is passed as parameter!
> my @ready;
> while (scalar (@ready = $sel->can_read($timeout))) {
> my $count;
> @@ -2702,8 +2719,18 @@ my $qmp_read_avail = sub {
> }
>
> die "qmp read timeout\n" if !scalar(@ready);
> - my $obj = from_json($res);
> - return $obj;
> + my @jsons = split("\n", $res);
> + my $obj = {};
> + my $event = '';
> + foreach my $json (@jsons) {
> + my $obj = from_json($json);
> + $event = $obj->{event} if exists $obj->{event};
> +
> + return $obj->{error} if exists $obj->{error} && $returnerror;
> + die $obj->{error}->{desc} if exists $obj->{error}->{desc};
> + return $obj->{QMP} if exists $obj->{QMP};
> + return $obj->{"return"} if exists $obj->{"return"};
> + }
Wow. I simply do not understand that code. For example $event is set but never used?
I guess this is worth a separate patch including some docs (inline comments).
> };
>
> sub __read_avail {
> @@ -2831,6 +2858,9 @@ sub vm_qmp_command {
>
> # hack: migrate sometime blocks the monitor (when
> migrate_downtime
> # is set)
> + if (ref($cmdstr) eq "HASH") {
> + $timeout = 60*60 if ($cmdstr->{execute} =~ m/(migrate)$/);
> + }
> #if ($cmdstr =~ m/^(info\s+migrate|migrate\s)/) {
> # $timeout = 60*60; # 1 hour
> #}
> @@ -2838,7 +2868,7 @@ sub vm_qmp_command {
> # read banner;
> my $data = &$qmp_read_avail($sock, $timeout);
> # '{"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1},
> "package": " (qemu-kvm-devel)"}, "capabilities": []}} ';
> - die "got unexpected qemu qmp banner\n" if !$data->{QMP};
> + die "got unexpected qemu qmp banner\n" if !$data;
>
> my $sel = new IO::Select;
> $sel->add($sock);
> @@ -2857,18 +2887,19 @@ sub vm_qmp_command {
>
> $res = &$qmp_read_avail($sock, $timeout);
> # res = '{"return": {}}
> - die "qmp negociation error\n" if !$res->{return};
> + die "qmp negociation error\n" if !$res;
>
> $timeout = 20;
>
> my $fullcmd;
> + my $returnerror;
> if (ref($cmdstr) eq "HASH") {
> #generate json from hash for complex cmd
> $fullcmd = to_json($cmdstr);
> -
> - if ($fullcmd->{execute} =~ m/^(info\s+migrate|migrate\s)/) {
> + if ($cmdstr->{execute} =~ m/(migrate)$/) {
> $timeout = 60*60; # 1 hour
> - } elsif ($fullcmd->{execute} =~ m/^(eject|change)/) {
> + $returnerror = 1;
Would you mind to explain that?
For me it looks easier to catch the error in QemuMigrate.pm, and remove that
whole returnerror code:
eval { PVE::QemuServer::vm_qmp_command($vmid, $migratecommand, 1); }
my $merr = $@;
or do we need that functionality somewhere else?
> + } elsif ($cmdstr->{execute} =~ m/^(eject|change)/) {
> $timeout = 60; # note: cdrom mount command is slow
> }
> } else {
> @@ -2883,8 +2914,8 @@ sub vm_qmp_command {
> if (ref($cmdstr) ne "HASH") {
> return if ($cmdstr eq 'q') || ($cmdstr eq 'quit');
> }
> -
> - $res = &$qmp_read_avail($sock, $timeout);
> +
> + $res = &$qmp_read_avail($sock, $timeout, $returnerror);
> };
>
- Dietmar
--
--
Alexandre D erumier
Ingénieur Système
Fixe : 03 20 68 88 90
Fax : 03 20 68 90 81
45 Bvd du Général Leclerc 59100 Roubaix - France
12 rue Marivaux 75002 Paris - France
More information about the pve-devel
mailing list