* [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure
@ 2023-08-22 10:36 Gabriel Goller
2023-08-24 8:57 ` Wolfgang Bumiller
0 siblings, 1 reply; 4+ messages in thread
From: Gabriel Goller @ 2023-08-22 10:36 UTC (permalink / raw)
To: pbs-devel
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?;
+
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)
+ );
+ }
+
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure
2023-08-22 10:36 [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure Gabriel Goller
@ 2023-08-24 8:57 ` Wolfgang Bumiller
2023-08-24 9:01 ` Wolfgang Bumiller
2023-08-24 12:47 ` Gabriel Goller
0 siblings, 2 replies; 4+ messages in thread
From: Wolfgang Bumiller @ 2023-08-24 8:57 UTC (permalink / raw)
To: Gabriel Goller; +Cc: pbs-devel
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 :-)
> + );
> + }
> +
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure
2023-08-24 8:57 ` Wolfgang Bumiller
@ 2023-08-24 9:01 ` Wolfgang Bumiller
2023-08-24 12:47 ` Gabriel Goller
1 sibling, 0 replies; 4+ messages in thread
From: Wolfgang Bumiller @ 2023-08-24 9:01 UTC (permalink / raw)
To: Gabriel Goller; +Cc: pbs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure
2023-08-24 8:57 ` Wolfgang Bumiller
2023-08-24 9:01 ` Wolfgang Bumiller
@ 2023-08-24 12:47 ` Gabriel Goller
1 sibling, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2023-08-24 12:47 UTC (permalink / raw)
To: Wolfgang Bumiller; +Cc: pbs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-24 12:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-22 10:36 [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure Gabriel Goller
2023-08-24 8:57 ` Wolfgang Bumiller
2023-08-24 9:01 ` Wolfgang Bumiller
2023-08-24 12:47 ` Gabriel Goller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox