[pve-devel] [PATCH qemu-server 2/4] hide long commandline on vm_start/migrate failure

[Stefan Reiter]

On 12/10/19 9:13 AM, Thomas Lamprecht wrote:
> On 12/9/19 4:14 PM, Stefan Reiter wrote:
>> By default run_command prints the entire commandline executed when an
>> error occurs, but QEMU and our migrate command are not only
>> uninteresting to the user[*] but also annoyingly long. Hide them and only
>> print the exit code.
>>
>> [*] Especially our migrate command, since it can't be manually executed
>> anyway. QEMU's commandline *might* contain something interesting, but is
>> so long that it's tricky to parse anyway, any a user can always call 'qm
>> showcmd --pretty'.
>>
> 
> But it can hold interesting info about why the command failed.. And my
> "pretty" option is just a basic SED script, so could be also nicely formatted
> post-error. I mean, I'm tempted to apply it, just a bit concerns about removing
> infos for users which just post the whole error and have not much clue for
> debugging, for those helping them the command line /could/ be of help - maybe?
> 

I mean, we already print the QEMU errors (with patch 3 even to the 
migrate log), and any errors that occur on our end. If the user knows 
what to look for in a QEMU command line, a call to 'qm showcmd' should 
be within their reach as well.

On the other hand, I'm fine with leaving it as well. The following 
patches apply pretty cleanly (one auto-merge) without this one for me, 
so you could just apply the rest of the series.

LMK if I should send a v2, with or without this patch. The change from 
your other mail would be included too, just an oversight on my end.

>> Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
>> ---
>>   PVE/QemuMigrate.pm |  6 ++++--
>>   PVE/QemuServer.pm  | 10 ++++++++--
>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index f909873..b5ec45c 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -595,7 +595,7 @@ sub phase2 {
>>   
>>       # Note: We try to keep $spice_ticket secret (do not pass via command line parameter)
>>       # instead we pipe it through STDIN
>> -    PVE::Tools::run_command($cmd, input => $spice_ticket, outfunc => sub {
>> +    my $exitcode = PVE::Tools::run_command($cmd, input => $spice_ticket, outfunc => sub {
>>   	my $line = shift;
>>   
>>   	if ($line =~ m/^migration listens on tcp:(localhost|[\d\.]+|\[[\d\.:a-fA-F]+\]):(\d+)$/) {
>> @@ -629,7 +629,9 @@ sub phase2 {
>>       }, errfunc => sub {
>>   	my $line = shift;
>>   	$self->log('info', $line);
>> -    });
>> +    }, noerr => 1);
>> +
>> +    die "remote command failed with exit code $exitcode\n" if $exitcode;
>>   
>>       die "unable to detect remote migration address\n" if !$raddr;
>>   
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 13798a1..01b677b 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -5396,7 +5396,11 @@ sub vm_start {
>>   	                                          : $defaults->{cpuunits};
>>   
>>   	my $start_timeout = ($conf->{hugepages} || $is_suspended) ? 300 : 30;
>> -	my %run_params = (timeout => $statefile ? undef : $start_timeout, umask => 0077);
>> +	my %run_params = (
>> +	    timeout => $statefile ? undef : $start_timeout,
>> +	    umask => 0077,
>> +	    noerr => 1,
>> +	);
>>   
>>   	my %properties = (
>>   	    Slice => 'qemu.slice',
>> @@ -5412,7 +5416,9 @@ sub vm_start {
>>   	my $run_qemu = sub {
>>   	    PVE::Tools::run_fork sub {
>>   		PVE::Systemd::enter_systemd_scope($vmid, "Proxmox VE VM $vmid", %properties);
>> -		run_command($cmd, %run_params);
>> +
>> +		my $exitcode = run_command($cmd, %run_params);
>> +		die "QEMU exited with code $exitcode\n" if $exitcode;
>>   	    };
>>   	};
>>   
>>
>