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

[Thomas Lamprecht]

On 12/10/19 9:51 AM, Stefan Reiter wrote:
> 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.
> 

I mean, for that they need host root, for checking the task log not
necessarily. Also, it's not that *they* may know for what to look for,
but an more experienced PVE user in the Forum or the list - that's what
my point was.

> 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.

Not 100% sure my self, maybe someone else has better pros/cons.

> 
>>> 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;
>>>           };
>>>       };
>>>  
>>