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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Aug 24, 2023 at 10:57:08AM +0200, 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?

(by "this" I actually mean the `status = "stopped"` from down below ;-) )

> 
> >              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 :-)
> 
> 
> > +                );
> > +            }
> > +
> >              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