[pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Dominik Csapak]

we parse the task result from the last line, so we should not print a
new line in the task result, else we get an 'unknown' task state

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes
 proxmox-rest-server/src/worker_task.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index 51394549..a8899ab9 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -494,7 +494,7 @@ impl TaskState {
         match self {
             TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
             other => format!("TASK {}", other),
-        }
+        }.replace('\n', "\\n") // no newline in task result
     }
 
     fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 2/2] backup-proxy: fix api log reopen send_command calls

[Dominik Csapak]

if we give a string to send_command, it gets to the receiving end as a
string again (not an object), and the command fails with:

'unable to parse parameters (expected json object)'

Instead, use send_raw_command which expects the string. Also refactor
the identical command strings.

Fixes: commit 45b8a0327f21f048bb4384bafc292954358b5651

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* include fixed commit in commit message
* use send_raw_command with the original strings

 src/bin/proxmox-backup-proxy.rs | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 4c879483..3529f586 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -852,14 +852,15 @@ async fn schedule_task_log_rotate() {
 }
 
 async fn command_reopen_access_logfiles() -> Result<(), Error> {
+    let cmd = "{\"command\":\"api-access-log-reopen\"}\n";
     // only care about the most recent daemon instance for each, proxy & api, as other older ones
     // should not respond to new requests anyway, but only finish their current one and then exit.
     let sock = proxmox_rest_server::our_ctrl_sock();
-    let f1 = proxmox_rest_server::send_command(sock, "{\"command\":\"api-access-log-reopen\"}\n");
+    let f1 = proxmox_rest_server::send_raw_command(sock, cmd);
 
     let pid = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_API_PID_FN)?;
     let sock = proxmox_rest_server::ctrl_sock_from_pid(pid);
-    let f2 = proxmox_rest_server::send_command(sock, "{\"command\":\"api-access-log-reopen\"}\n");
+    let f2 = proxmox_rest_server::send_raw_command(sock, cmd);
 
     match futures::join!(f1, f2) {
         (Err(e1), Err(e2)) => Err(format_err!("reopen commands failed, proxy: {}; api: {}", e1, e2)),
@@ -870,14 +871,15 @@ async fn command_reopen_access_logfiles() -> Result<(), Error> {
 }
 
 async fn command_reopen_auth_logfiles() -> Result<(), Error> {
+    let cmd = "{\"command\":\"api-auth-log-reopen\"}\n";
     // only care about the most recent daemon instance for each, proxy & api, as other older ones
     // should not respond to new requests anyway, but only finish their current one and then exit.
     let sock = proxmox_rest_server::our_ctrl_sock();
-    let f1 = proxmox_rest_server::send_command(sock, "{\"command\":\"api-auth-log-reopen\"}\n");
+    let f1 = proxmox_rest_server::send_raw_command(sock, cmd);
 
     let pid = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_API_PID_FN)?;
     let sock = proxmox_rest_server::ctrl_sock_from_pid(pid);
-    let f2 = proxmox_rest_server::send_command(sock, "{\"command\":\"api-auth-log-reopen\"}\n");
+    let f2 = proxmox_rest_server::send_raw_command(sock, cmd);
 
     match futures::join!(f1, f2) {
         (Err(e1), Err(e2)) => Err(format_err!("reopen commands failed, proxy: {}; api: {}", e1, e2)),
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup v2 2/2] backup-proxy: fix api log reopen send_command calls

[Thomas Lamprecht]

On 11.10.21 14:14, Dominik Csapak wrote:
> if we give a string to send_command, it gets to the receiving end as a
> string again (not an object), and the command fails with:
> 
> 'unable to parse parameters (expected json object)'
> 
> Instead, use send_raw_command which expects the string. Also refactor
> the identical command strings.
> 
> Fixes: commit 45b8a0327f21f048bb4384bafc292954358b5651
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * include fixed commit in commit message
> * use send_raw_command with the original strings
> 
>  src/bin/proxmox-backup-proxy.rs | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
>

applied this one for now but dropped the factoring out of the command, not really
worth it IMO and I prefer to keep (initial) change noise small for those things, thanks!

Also did a semantic backport to the stable-1 branch, cherry-picking is rather out of
the window with that big bunch of restructuring that happend in the last weeks/months.

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Thomas Lamprecht]

On 11.10.21 14:14, Dominik Csapak wrote:
> we parse the task result from the last line, so we should not print a
> new line in the task result, else we get an 'unknown' task state
> 

isn't the issue that we already print the newline in the file_logger's log method
this is using underneath and we could get two newlines (i.e. a last empty line)
which then failed to parse?

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> no changes
>  proxmox-rest-server/src/worker_task.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
> index 51394549..a8899ab9 100644
> --- a/proxmox-rest-server/src/worker_task.rs
> +++ b/proxmox-rest-server/src/worker_task.rs
> @@ -494,7 +494,7 @@ impl TaskState {
>          match self {
>              TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>              other => format!("TASK {}", other),
> -        }
> +        }.replace('\n', "\\n") // no newline in task result

Why not `.trim_end()` ? A literal \n seems rather odd to me..

Comment suggestion:

// our consumer already add \n where required, so avoid double-newline

>      }
>  
>      fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
> 

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Dominik Csapak]

On 10/11/21 15:12, Thomas Lamprecht wrote:
> On 11.10.21 14:14, Dominik Csapak wrote:
>> we parse the task result from the last line, so we should not print a
>> new line in the task result, else we get an 'unknown' task state
>>
> 
> isn't the issue that we already print the newline in the file_logger's log method
> this is using underneath and we could get two newlines (i.e. a last empty line)
> which then failed to parse?

no in my case the issue is that the message contained a newline in the 
middle:

--8<--
2021-09-28T08:34:00+02:00: TASK ERROR: reopen commands failed, proxy: 
unable to parse parameters (expected json object)
; api: unable to parse parameters (expected json object)
-->8--

> 
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> no changes
>>   proxmox-rest-server/src/worker_task.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>> index 51394549..a8899ab9 100644
>> --- a/proxmox-rest-server/src/worker_task.rs
>> +++ b/proxmox-rest-server/src/worker_task.rs
>> @@ -494,7 +494,7 @@ impl TaskState {
>>           match self {
>>               TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>               other => format!("TASK {}", other),
>> -        }
>> +        }.replace('\n', "\\n") // no newline in task result
> 
> Why not `.trim_end()` ? A literal \n seems rather odd to me..
> 
> Comment suggestion:
> 
> // our consumer already add \n where required, so avoid double-newline

again that was not the issue

> 
>>       }
>>   
>>       fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>
> 

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Thomas Lamprecht]

On 11.10.21 15:14, Dominik Csapak wrote:
> On 10/11/21 15:12, Thomas Lamprecht wrote:
>> On 11.10.21 14:14, Dominik Csapak wrote:
>>> we parse the task result from the last line, so we should not print a
>>> new line in the task result, else we get an 'unknown' task state
>>>
>>
>> isn't the issue that we already print the newline in the file_logger's log method
>> this is using underneath and we could get two newlines (i.e. a last empty line)
>> which then failed to parse?
> 
> no in my case the issue is that the message contained a newline in the middle:

hmm, ok,  and example where/how it happens could have helped then here ;)

> --8<--
> 2021-09-28T08:34:00+02:00: TASK ERROR: reopen commands failed, proxy: unable to parse parameters (expected json object)
> ; api: unable to parse parameters (expected json object)
> -->8--

Yeah, but why replacing it with a literal \n then? Why not percent encoding
or fixing it at the root in create_state?

>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>> no changes
>>>   proxmox-rest-server/src/worker_task.rs | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>>> index 51394549..a8899ab9 100644
>>> --- a/proxmox-rest-server/src/worker_task.rs
>>> +++ b/proxmox-rest-server/src/worker_task.rs
>>> @@ -494,7 +494,7 @@ impl TaskState {
>>>           match self {
>>>               TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>>               other => format!("TASK {}", other),
>>> -        }
>>> +        }.replace('\n', "\\n") // no newline in task result
>>
>> Why not `.trim_end()` ? A literal \n seems rather odd to me..
>>
>> Comment suggestion:
>>
>> // our consumer already add \n where required, so avoid double-newline
> 
> again that was not the issue

well, the comment was not really helpful in that case either..

Also, in that case it would make more sense to operate on the TaskState::Error branch
or avoid setting such a bad state in the first place..

> 
>>
>>>       }
>>>         fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>>
>>
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Dominik Csapak]

On 10/11/21 15:23, Thomas Lamprecht wrote:
> On 11.10.21 15:14, Dominik Csapak wrote:
>> On 10/11/21 15:12, Thomas Lamprecht wrote:
>>> On 11.10.21 14:14, Dominik Csapak wrote:
>>>> we parse the task result from the last line, so we should not print a
>>>> new line in the task result, else we get an 'unknown' task state
>>>>
>>>
>>> isn't the issue that we already print the newline in the file_logger's log method
>>> this is using underneath and we could get two newlines (i.e. a last empty line)
>>> which then failed to parse?
>>
>> no in my case the issue is that the message contained a newline in the middle:
> 
> hmm, ok,  and example where/how it happens could have helped then here ;)

thats fair ;)

> 
>> --8<--
>> 2021-09-28T08:34:00+02:00: TASK ERROR: reopen commands failed, proxy: unable to parse parameters (expected json object)
>> ; api: unable to parse parameters (expected json object)
>> -->8--
> 
> Yeah, but why replacing it with a literal \n then? Why not percent encoding
> or fixing it at the root in create_state?

i was going to find the root cause (i do not think its in create state, 
but where the error is generated)

but wanted to have a generic method to not have a wrong task state even
when it happens again somewhere

imho, percent encoding(%0A) is not nicer in this context here than \n

> 
>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>> no changes
>>>>    proxmox-rest-server/src/worker_task.rs | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>>>> index 51394549..a8899ab9 100644
>>>> --- a/proxmox-rest-server/src/worker_task.rs
>>>> +++ b/proxmox-rest-server/src/worker_task.rs
>>>> @@ -494,7 +494,7 @@ impl TaskState {
>>>>            match self {
>>>>                TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>>>                other => format!("TASK {}", other),
>>>> -        }
>>>> +        }.replace('\n', "\\n") // no newline in task result
>>>
>>> Why not `.trim_end()` ? A literal \n seems rather odd to me..
>>>
>>> Comment suggestion:
>>>
>>> // our consumer already add \n where required, so avoid double-newline
>>
>> again that was not the issue
> 
> well, the comment was not really helpful in that case either..
> 
> Also, in that case it would make more sense to operate on the TaskState::Error branch
> or avoid setting such a bad state in the first place..

you're right, thats a better place

> 
>>
>>>
>>>>        }
>>>>          fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>>>
>>>
>>
>>
> 

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Thomas Lamprecht]

On 11.10.21 15:30, Dominik Csapak wrote:
> On 10/11/21 15:23, Thomas Lamprecht wrote:
>> On 11.10.21 15:14, Dominik Csapak wrote:
>>> On 10/11/21 15:12, Thomas Lamprecht wrote:
>>>> On 11.10.21 14:14, Dominik Csapak wrote:
>>>>> we parse the task result from the last line, so we should not print a
>>>>> new line in the task result, else we get an 'unknown' task state
>>>>>
>>>>
>>>> isn't the issue that we already print the newline in the file_logger's log method
>>>> this is using underneath and we could get two newlines (i.e. a last empty line)
>>>> which then failed to parse?
>>>
>>> no in my case the issue is that the message contained a newline in the middle:
>>
>> hmm, ok,  and example where/how it happens could have helped then here ;)
> 
> thats fair ;)
> 
>>
>>> --8<--
>>> 2021-09-28T08:34:00+02:00: TASK ERROR: reopen commands failed, proxy: unable to parse parameters (expected json object)
>>> ; api: unable to parse parameters (expected json object)
>>> -->8--
>>
>> Yeah, but why replacing it with a literal \n then? Why not percent encoding
>> or fixing it at the root in create_state?
> 
> i was going to find the root cause (i do not think its in create state, but where the error is generated)
> 

I do not mean the root cause that logged a newline at the end, we cannot avoid the future,
but the one that crates the TaskError enum, and if I'm not completely sleep deprived, that 
is create_state.

> but wanted to have a generic method to not have a wrong task state even
> when it happens again somewhere
> 
> imho, percent encoding(%0A) is not nicer in this context here than \n

percent encoding is an actual format than can be rendered again nicely, just randomly
mapping the \n control code (0x0A) to 0x5C 0x6E, without escaping existing (literal) \n
just isn't a used format, at least FWICR, that's my point in provoking that question.

> 
>>
>>>>
>>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>>> ---
>>>>> no changes
>>>>>    proxmox-rest-server/src/worker_task.rs | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>>>>> index 51394549..a8899ab9 100644
>>>>> --- a/proxmox-rest-server/src/worker_task.rs
>>>>> +++ b/proxmox-rest-server/src/worker_task.rs
>>>>> @@ -494,7 +494,7 @@ impl TaskState {
>>>>>            match self {
>>>>>                TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>>>>                other => format!("TASK {}", other),
>>>>> -        }
>>>>> +        }.replace('\n', "\\n") // no newline in task result
>>>>
>>>> Why not `.trim_end()` ? A literal \n seems rather odd to me..
>>>>
>>>> Comment suggestion:
>>>>
>>>> // our consumer already add \n where required, so avoid double-newline
>>>
>>> again that was not the issue
>>
>> well, the comment was not really helpful in that case either..
>>
>> Also, in that case it would make more sense to operate on the TaskState::Error branch
>> or avoid setting such a bad state in the first place..
> 
> you're right, thats a better place
> 
>>
>>>
>>>>
>>>>>        }
>>>>>          fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>>>>
>>>>
>>>
>>>
>>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Dominik Csapak]

On 10/11/21 15:37, Thomas Lamprecht wrote:
>>
> 
> I do not mean the root cause that logged a newline at the end, we cannot avoid the future,
> but the one that crates the TaskError enum, and if I'm not completely sleep deprived, that
> is create_state.

sorry, i misunderstood you, yes, the TaskState::Error gets created in 
'create_state', and yes, it makes sense to do it already there...

> 
>> but wanted to have a generic method to not have a wrong task state even
>> when it happens again somewhere
>>
>> imho, percent encoding(%0A) is not nicer in this context here than \n
> 
> percent encoding is an actual format than can be rendered again nicely, just randomly
> mapping the \n control code (0x0A) to 0x5C 0x6E, without escaping existing (literal) \n
> just isn't a used format, at least FWICR, that's my point in provoking that question.
> 

while we could decode it again, existing tasks did not get encoded, so 
we would potentially decode some strings that shouldn't be decoded...

we could also simply remove the newlines at all, i don't think we would
benefit from having multiline error messages in tasks..
since most of the time we want to show a single line per task

or what about trying to parse the task log backwards
until we find a correct date+taskerror or at least a correct date?
(in the first case we can assume that the newlines are part of
the error, in the second the state would still be 'unknown' but
with a correct date)

if you don't like either approach, i'd send a
version with percent-encoded/decoded, it's imho more important
that we get the correct task state on task parsing than how we
solve it exactly

>>
>>>
>>>>>
>>>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>>>> ---
>>>>>> no changes
>>>>>>     proxmox-rest-server/src/worker_task.rs | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
>>>>>> index 51394549..a8899ab9 100644
>>>>>> --- a/proxmox-rest-server/src/worker_task.rs
>>>>>> +++ b/proxmox-rest-server/src/worker_task.rs
>>>>>> @@ -494,7 +494,7 @@ impl TaskState {
>>>>>>             match self {
>>>>>>                 TaskState::Error { message, .. } => format!("TASK ERROR: {}", message),
>>>>>>                 other => format!("TASK {}", other),
>>>>>> -        }
>>>>>> +        }.replace('\n', "\\n") // no newline in task result
>>>>>
>>>>> Why not `.trim_end()` ? A literal \n seems rather odd to me..
>>>>>
>>>>> Comment suggestion:
>>>>>
>>>>> // our consumer already add \n where required, so avoid double-newline
>>>>
>>>> again that was not the issue
>>>
>>> well, the comment was not really helpful in that case either..
>>>
>>> Also, in that case it would make more sense to operate on the TaskState::Error branch
>>> or avoid setting such a bad state in the first place..
>>
>> you're right, thats a better place
>>
>>>
>>>>
>>>>>
>>>>>>         }
>>>>>>           fn from_endtime_and_message(endtime: i64, s: &str) -> Result<Self, Error> {
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 

Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] rest-server/worker-task: replace newlines with '\n' in task result

[Thomas Lamprecht]

On 11.10.21 15:57, Dominik Csapak wrote:
> On 10/11/21 15:37, Thomas Lamprecht wrote:
>>>
>>
>> I do not mean the root cause that logged a newline at the end, we cannot avoid the future,
>> but the one that crates the TaskError enum, and if I'm not completely sleep deprived, that
>> is create_state.
> 
> sorry, i misunderstood you, yes, the TaskState::Error gets created in 'create_state', and yes, it makes sense to do it already there...
> 
>>
>>> but wanted to have a generic method to not have a wrong task state even
>>> when it happens again somewhere
>>>
>>> imho, percent encoding(%0A) is not nicer in this context here than \n
>>
>> percent encoding is an actual format than can be rendered again nicely, just randomly
>> mapping the \n control code (0x0A) to 0x5C 0x6E, without escaping existing (literal) \n
>> just isn't a used format, at least FWICR, that's my point in provoking that question.
>>
> 
> while we could decode it again, existing tasks did not get encoded, so we would potentially decode some strings that shouldn't be decoded...



> 
> we could also simply remove the newlines at all, i don't think we would

no, same loss of information as replacing it with a literal \n...

> benefit from having multiline error messages in tasks..
> since most of the time we want to show a single line per task

We want to have the error, if it's longer and over a few lines then
that's fine too.

> 
> or what about trying to parse the task log backwards
> until we find a correct date+taskerror or at least a correct date?
> (in the first case we can assume that the newlines are part of
> the error, in the second the state would still be 'unknown' but
> with a correct date)

brittle to do in general, one cannot guarantee that the error doesn't also contains
what we'd match, may not be likely but if we want to improve on the current solution
I'd rather go for an actual improvement over an hack..

> 
> if you don't like either approach, i'd send a
> version with percent-encoded/decoded, it's imho more important
> that we get the correct task state on task parsing than how we
> solve it exactly

I'd like to have an actual solution without loss of information, and with our current
approach I see only a two-way encoding doing that, I don't care much for what one we
use, but good would be as human readable as possible as this is a issue that happens
rather rarely.

IMO having log + result intertwined into the log isn't ideal either, having a "$UPID.result"
(or the like) file with the information already present in a structured format would
simplify things a lot..