Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure

[Gabriel Goller <g.goller@proxmox.com>]

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