* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal