From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <g.goller@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id CF61C9151
 for <pbs-devel@lists.proxmox.com>; Thu, 24 Aug 2023 14:47:43 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id ABFB4326FC
 for <pbs-devel@lists.proxmox.com>; Thu, 24 Aug 2023 14:47:13 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pbs-devel@lists.proxmox.com>; Thu, 24 Aug 2023 14:47:13 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ECD0443BFB
 for <pbs-devel@lists.proxmox.com>; Thu, 24 Aug 2023 14:47:12 +0200 (CEST)
Message-ID: <8d54012c-c9a4-7d62-c7d0-9ea0fd91240e@proxmox.com>
Date: Thu, 24 Aug 2023 14:47:12 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
References: <20230822103603.130998-1-g.goller@proxmox.com>
 <jemfnay3zof2ajkmryrig3ux4mko2ja6bvmcfbx5qhdy76f3qh@5kplrkapc5j3>
Content-Language: en-US
From: Gabriel Goller <g.goller@proxmox.com>
In-Reply-To: <jemfnay3zof2ajkmryrig3ux4mko2ja6bvmcfbx5qhdy76f3qh@5kplrkapc5j3>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.840 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -2.684 Looks like a legit reply (A)
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated
 `view_task_result` to bail on task failure
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 24 Aug 2023 12:47:43 -0000

On 8/24/23 10:57, Wolfgang Bumiller wrote:

> On Tue, Aug 22, 2023 at 12:36:03PM +0200, Gabriel Goller wrote:
>> Now we make an additional request on `api2/json/.../tasks/{upid}/status` to
>> get the `exitstatus` of the task. This allows us to `bail` and thus
>> get a non-zero exit code in the cli.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   pbs-client/src/task_log.rs | 27 ++++++++++++++++++++++++---
>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/pbs-client/src/task_log.rs b/pbs-client/src/task_log.rs
>> index 0bbb928d..4fb31fea 100644
>> --- a/pbs-client/src/task_log.rs
>> +++ b/pbs-client/src/task_log.rs
>> @@ -64,10 +64,24 @@ pub async fn display_task_log(
>>               let path = format!("api2/json/nodes/localhost/tasks/{upid_encoded}/log");
>>               let result = client.get(&path, Some(param)).await?;
>>   
>> +            let status_path = format!("api2/json/nodes/localhost/tasks/{upid_encoded}/status");
>> +            let status_result = client.get(&status_path, None).await?;
>> +
> ^ shouldn't `active` become false before this can happen?
> so can we not just do this once after the loop?
>
>>               let active = result["active"].as_bool().unwrap();
>>               let total = result["total"].as_u64().unwrap();
>>               let data = result["data"].as_array().unwrap();
>>   
>> +            if status_result["data"]["status"].as_str() == Some("stopped")
>> +                && status_result["data"]["exitstatus"].as_str() != Some("OK")
>> +            {
>> +                bail!(
>> +                    "{}",
>> +                    data.iter()
>> +                        .map(|d| try_strip_date(d["t"].as_str().unwrap()))
>> +                        .fold(String::new(), |a, b| a + " " + b)
> ^ not a fan of `+` for string concatenation.
> `format!()` has at least a chance to figure out the lengths first,
> whereas with `+` you're technically doing multiple independent
> operations.
> Further, you're not adding the newlines in between like it happens from
> the use of `print<ln>` in the loop down below ;-)
>
> Also this seems to just use "whatever rest we had not printed yet" as an
> _error_ message. If we check the status after the loop, we could just use
> a generic bail!("task failed") as the output was already there.
>
> Unless there's a reason to do it this way instead? But I don't think we
> can really know how much of it even _is_ error text. We might be getting
> up to 500 (`limit` is 500) lines of random text :-)
No, you are right, I submitted a new patch. I just didn't want to print 
multiple
error messages... but I guess that's alright.
>> +                );
>> +            }
>> +
>>               let lines = data.len();
>>   
>>               for item in data {
>> @@ -76,9 +90,8 @@ pub async fn display_task_log(
>>                   if n != start {
>>                       bail!("got wrong line number in response data ({n} != {start}");
>>                   }
>> -                if strip_date && t.len() > 27 && &t[25..27] == ": " {
>> -                    let line = &t[27..];
>> -                    println!("{line}");
>> +                if strip_date {
>> +                    println!("{}", try_strip_date(t));
>>                   } else {
>>                       println!("{t}");
>>                   }
>> @@ -127,3 +140,11 @@ pub async fn view_task_result(
>>   
>>       Ok(())
>>   }
>> +
>> +fn try_strip_date(log_msg: &str) -> &str {
>> +    if log_msg.len() > 27 && &log_msg[25..27] == ": " {
>> +        &log_msg[27..]
>> +    } else {
>> +        log_msg
>> +    }
>> +}
>> -- 
>> 2.39.2