* [pbs-devel] [PATCH v2 proxmox-backup 0/3] handle reader client disconnects
@ 2024-12-04 8:31 Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader Christian Ebner
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Christian Ebner @ 2024-12-04 8:31 UTC (permalink / raw)
To: pbs-devel
These patches attempt to improve the server side error handling for
reader client disconnects.
During regular operation, the server currently cannot distinguish a
disconnect because of error from a disconnect because of finished
operation. This leaves behind a task in failed state, which is
unexpected and might cause confusion [0].
To improve error handling, follow the approach taken for the backup
writer tasks, letting the client signal it has successfully finished
via an api call and catch the disconnect error for that case.
Reported in the community forum:
[0] https://forum.proxmox.com/threads/158306/
Version 1 of the patches:
https://lore.proxmox.com/pbs-devel/20241203112756.63872-1-c.ebner@proxmox.com/T/
Christian Ebner (3):
client: backup: remove unnecessary clone for backup reader
api: reader: handle reader client disconnects
client: reader: signal server before client disconnect
pbs-client/src/backup_reader.rs | 2 ++
proxmox-backup-client/src/main.rs | 4 ++--
src/api2/reader/environment.rs | 20 +++++++++++++++++++-
src/api2/reader/mod.rs | 30 +++++++++++++++++++++++++++---
4 files changed, 50 insertions(+), 6 deletions(-)
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader
2024-12-04 8:31 [pbs-devel] [PATCH v2 proxmox-backup 0/3] handle reader client disconnects Christian Ebner
@ 2024-12-04 8:31 ` Christian Ebner
2024-12-04 13:50 ` [pbs-devel] applied: " Fabian Grünbichler
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] api: reader: handle reader client disconnects Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect Christian Ebner
2 siblings, 1 reply; 10+ messages in thread
From: Christian Ebner @ 2024-12-04 8:31 UTC (permalink / raw)
To: pbs-devel
This was introduced by commit fdea4e53 ("client: implement prepare
reference method") to read a reference metadata archive for detection
of unchanged, reusable files when using change detection mode set to
`metadata`.
Avoid unnecessary cloning of the atomic reference counted
`BackupReader` instance, as it is used exclusively for this codepath.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- fix incorrect commit title by s/writer/reader/
proxmox-backup-client/src/main.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 9daa513f9..632a29170 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -1107,7 +1107,7 @@ async fn create_backup(
&target,
manifest.clone(),
&client,
- backup_reader.clone(),
+ backup_reader,
crypt_config.clone(),
crypto.mode,
)
@@ -1310,7 +1310,7 @@ async fn prepare_reference(
let most_used = metadata_ref_index.find_most_used_chunks(8);
let file_info = manifest.lookup_file_info(&target)?;
let chunk_reader = RemoteChunkReader::new(
- backup_reader.clone(),
+ backup_reader,
crypt_config.clone(),
file_info.chunk_crypt_mode(),
most_used,
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH v2 proxmox-backup 2/3] api: reader: handle reader client disconnects
2024-12-04 8:31 [pbs-devel] [PATCH v2 proxmox-backup 0/3] handle reader client disconnects Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader Christian Ebner
@ 2024-12-04 8:31 ` Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect Christian Ebner
2 siblings, 0 replies; 10+ messages in thread
From: Christian Ebner @ 2024-12-04 8:31 UTC (permalink / raw)
To: pbs-devel
Currently, if a reader client disconnects after finishing its work,
the connection will be closed by the client without notifying the
server. The future handling the connection on then server side will
then return with a connection error, and in consequence the reader
worker task will log with error state. This can cause confusion [0],
as this is not an error but normal behaviour.
Instead of failing, provide an api endpoint for the client to signal
it has finished operation. The server sets the connection environment
state accordingly, and the connection error is suppressed if the flag
has been set. This follows the same logic used for the backup writer,
introduced by commit b428af97 ("backup: avoid Transport endpoint is
not connected error").
Report in the community forum:
[0] https://forum.proxmox.com/threads/158306/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- Use the same approach as used for the backup writer, as the
connection graceful_shutdown did not resolve the issue at hand.
src/api2/reader/environment.rs | 20 +++++++++++++++++++-
src/api2/reader/mod.rs | 30 +++++++++++++++++++++++++++---
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/src/api2/reader/environment.rs b/src/api2/reader/environment.rs
index 3b2f06f43..3cdc8e394 100644
--- a/src/api2/reader/environment.rs
+++ b/src/api2/reader/environment.rs
@@ -1,5 +1,5 @@
use std::collections::HashSet;
-use std::sync::{Arc, RwLock};
+use std::sync::{Arc, Mutex, RwLock};
use serde_json::{json, Value};
@@ -24,6 +24,11 @@ pub struct ReaderEnvironment {
pub datastore: Arc<DataStore>,
pub backup_dir: BackupDir,
allowed_chunks: Arc<RwLock<HashSet<[u8; 32]>>>,
+ connection_state: Arc<Mutex<ConnectionState>>,
+}
+
+struct ConnectionState {
+ client_finished: bool,
}
impl ReaderEnvironment {
@@ -44,6 +49,9 @@ impl ReaderEnvironment {
formatter: JSON_FORMATTER,
backup_dir,
allowed_chunks: Arc::new(RwLock::new(HashSet::new())),
+ connection_state: Arc::new(Mutex::new(ConnectionState {
+ client_finished: false,
+ })),
}
}
@@ -69,6 +77,16 @@ impl ReaderEnvironment {
pub fn check_chunk_access(&self, digest: [u8; 32]) -> bool {
self.allowed_chunks.read().unwrap().contains(&digest)
}
+
+ pub(crate) fn client_finished(&self) -> bool {
+ let state = self.connection_state.lock().unwrap();
+ state.client_finished
+ }
+
+ pub(crate) fn finish(&self) {
+ let mut state = self.connection_state.lock().unwrap();
+ state.client_finished = true;
+ }
}
impl RpcEnvironment for ReaderEnvironment {
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index 50f80de43..cb53f6b5e 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -192,9 +192,16 @@ fn upgrade_to_backup_reader_protocol(
http.http2_initial_connection_window_size(window_size);
http.http2_max_frame_size(4 * 1024 * 1024);
- http.serve_connection(conn, service)
- .map_err(Error::from)
- .await
+ if let Err(err) = http.serve_connection(conn, service).await {
+ // Avoid Transport endpoint is not connected (os error 107)
+ // fixme: find a better way to test for that error
+ if !(err.to_string().starts_with("connection error")
+ && env2.client_finished())
+ {
+ return Err(Error::from(err));
+ }
+ }
+ Ok(())
};
futures::select! {
@@ -228,6 +235,7 @@ const READER_API_SUBDIRS: SubdirMap = &[
"download",
&Router::new().download(&API_METHOD_DOWNLOAD_FILE),
),
+ ("finish", &Router::new().post(&API_METHOD_FINISH)),
("speedtest", &Router::new().download(&API_METHOD_SPEEDTEST)),
];
@@ -347,6 +355,22 @@ fn download_chunk(
.boxed()
}
+#[sortable]
+pub const API_METHOD_FINISH: ApiMethod = ApiMethod::new(
+ &ApiHandler::Sync(&finish),
+ &ObjectSchema::new("Signal the reader instance is finished", &[]),
+);
+
+fn finish(
+ _param: Value,
+ _info: &ApiMethod,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
+ let env: &ReaderEnvironment = rpcenv.as_ref();
+ env.finish();
+ Ok(Value::Null)
+}
+
/* this is too slow
fn download_chunk_old(
_parts: Parts,
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-04 8:31 [pbs-devel] [PATCH v2 proxmox-backup 0/3] handle reader client disconnects Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] api: reader: handle reader client disconnects Christian Ebner
@ 2024-12-04 8:31 ` Christian Ebner
2024-12-04 13:49 ` Fabian Grünbichler
2 siblings, 1 reply; 10+ messages in thread
From: Christian Ebner @ 2024-12-04 8:31 UTC (permalink / raw)
To: pbs-devel
Signal the server that the client has successfully finished its
operation and is about to close the connection. This allows the server
side to react and clean up the connection, without returning and
logging an error state, as that can cause confusion [0], as this is
not an error but normal behaviour.
Report in the community forum:
[0] https://forum.proxmox.com/threads/158306/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- no changes
Note:
I am not sure this is the best approach, as this might block the
thread until the server responds or it runs into a time out.
The alternative would require completely reworking all backup reader
related call sides. Or maybe there is another alternative?
pbs-client/src/backup_reader.rs | 2 ++
1 file changed, 2 insertions(+)
diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
index 88cba599b..63106c999 100644
--- a/pbs-client/src/backup_reader.rs
+++ b/pbs-client/src/backup_reader.rs
@@ -27,6 +27,8 @@ pub struct BackupReader {
impl Drop for BackupReader {
fn drop(&mut self) {
+ // Ignore errors
+ let _ = proxmox_async::runtime::block_on(self.post("finish", None));
self.abort.abort();
}
}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect Christian Ebner
@ 2024-12-04 13:49 ` Fabian Grünbichler
2024-12-04 14:13 ` Christian Ebner
0 siblings, 1 reply; 10+ messages in thread
From: Fabian Grünbichler @ 2024-12-04 13:49 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On December 4, 2024 9:31 am, Christian Ebner wrote:
> Signal the server that the client has successfully finished its
> operation and is about to close the connection. This allows the server
> side to react and clean up the connection, without returning and
> logging an error state, as that can cause confusion [0], as this is
> not an error but normal behaviour.
>
> Report in the community forum:
> [0] https://forum.proxmox.com/threads/158306/
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 1:
> - no changes
>
> Note:
> I am not sure this is the best approach, as this might block the
> thread until the server responds or it runs into a time out.
>
> The alternative would require completely reworking all backup reader
> related call sides. Or maybe there is another alternative?
>
> pbs-client/src/backup_reader.rs | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
> index 88cba599b..63106c999 100644
> --- a/pbs-client/src/backup_reader.rs
> +++ b/pbs-client/src/backup_reader.rs
> @@ -27,6 +27,8 @@ pub struct BackupReader {
>
> impl Drop for BackupReader {
> fn drop(&mut self) {
> + // Ignore errors
> + let _ = proxmox_async::runtime::block_on(self.post("finish", None));
should we maybe make this explicit, like we do in the BackupWriter? I
know that it's a bit less "obvious" here compared to writer sessions
what constitutes success/being finished ;)
means a bit more churn now to adapt the users of BackupReader, but would
make it possible to differentiate server side whether a reader session
was exited normally or via an error?
we could even provide some sort of message via the finish API call that
the server could log if desired, differentiating between:
- regular finish (no error/warning)
- finish called with a warning message (warning)
- finish not called, reader went away (error)
?
> self.abort.abort();
> }
> }
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [pbs-devel] applied: [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader Christian Ebner
@ 2024-12-04 13:50 ` Fabian Grünbichler
0 siblings, 0 replies; 10+ messages in thread
From: Fabian Grünbichler @ 2024-12-04 13:50 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
this one already, thanks!
On December 4, 2024 9:31 am, Christian Ebner wrote:
> This was introduced by commit fdea4e53 ("client: implement prepare
> reference method") to read a reference metadata archive for detection
> of unchanged, reusable files when using change detection mode set to
> `metadata`.
>
> Avoid unnecessary cloning of the atomic reference counted
> `BackupReader` instance, as it is used exclusively for this codepath.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 1:
> - fix incorrect commit title by s/writer/reader/
>
> proxmox-backup-client/src/main.rs | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 9daa513f9..632a29170 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -1107,7 +1107,7 @@ async fn create_backup(
> &target,
> manifest.clone(),
> &client,
> - backup_reader.clone(),
> + backup_reader,
> crypt_config.clone(),
> crypto.mode,
> )
> @@ -1310,7 +1310,7 @@ async fn prepare_reference(
> let most_used = metadata_ref_index.find_most_used_chunks(8);
> let file_info = manifest.lookup_file_info(&target)?;
> let chunk_reader = RemoteChunkReader::new(
> - backup_reader.clone(),
> + backup_reader,
> crypt_config.clone(),
> file_info.chunk_crypt_mode(),
> most_used,
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-04 13:49 ` Fabian Grünbichler
@ 2024-12-04 14:13 ` Christian Ebner
2024-12-05 9:40 ` Fabian Grünbichler
0 siblings, 1 reply; 10+ messages in thread
From: Christian Ebner @ 2024-12-04 14:13 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
On 12/4/24 14:49, Fabian Grünbichler wrote:
> On December 4, 2024 9:31 am, Christian Ebner wrote:
>> Signal the server that the client has successfully finished its
>> operation and is about to close the connection. This allows the server
>> side to react and clean up the connection, without returning and
>> logging an error state, as that can cause confusion [0], as this is
>> not an error but normal behaviour.
>>
>> Report in the community forum:
>> [0] https://forum.proxmox.com/threads/158306/
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 1:
>> - no changes
>>
>> Note:
>> I am not sure this is the best approach, as this might block the
>> thread until the server responds or it runs into a time out.
>>
>> The alternative would require completely reworking all backup reader
>> related call sides. Or maybe there is another alternative?
>>
>> pbs-client/src/backup_reader.rs | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
>> index 88cba599b..63106c999 100644
>> --- a/pbs-client/src/backup_reader.rs
>> +++ b/pbs-client/src/backup_reader.rs
>> @@ -27,6 +27,8 @@ pub struct BackupReader {
>>
>> impl Drop for BackupReader {
>> fn drop(&mut self) {
>> + // Ignore errors
>> + let _ = proxmox_async::runtime::block_on(self.post("finish", None));
>
> should we maybe make this explicit, like we do in the BackupWriter? I
> know that it's a bit less "obvious" here compared to writer sessions
> what constitutes success/being finished ;)
>
> means a bit more churn now to adapt the users of BackupReader, but would
> make it possible to differentiate server side whether a reader session
> was exited normally or via an error?
Yes, that is basically what I meant in the note:
The current approach is more of a catch all. Explicitly calling the
finish for each reader instance might be prove difficult because of all
the Arc instances being passed around and/or the reader instantiation
happening in a helper which then only returns the wrapped consumer of
the reader instance, e.g. a `RemoteChunkReader` or `pxar::Accessor`.
Will have a look on how to handle this, but it will require some bigger
refactoring.
> we could even provide some sort of message via the finish API call that
> the server could log if desired, differentiating between:
>
> - regular finish (no error/warning)
> - finish called with a warning message (warning)
> - finish not called, reader went away (error)
>
> ?
Not sure about this: What warnings do we even want to tell the server
about? I think the reader instance will either work without issues or
fail with a hard error? Or do you have some specific use-case in mind here?
>
>> self.abort.abort();
>> }
>> }
>> --
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-04 14:13 ` Christian Ebner
@ 2024-12-05 9:40 ` Fabian Grünbichler
2024-12-05 9:56 ` Christian Ebner
0 siblings, 1 reply; 10+ messages in thread
From: Fabian Grünbichler @ 2024-12-05 9:40 UTC (permalink / raw)
To: Christian Ebner, Proxmox Backup Server development discussion
On December 4, 2024 3:13 pm, Christian Ebner wrote:
> On 12/4/24 14:49, Fabian Grünbichler wrote:
>> On December 4, 2024 9:31 am, Christian Ebner wrote:
>>> Signal the server that the client has successfully finished its
>>> operation and is about to close the connection. This allows the server
>>> side to react and clean up the connection, without returning and
>>> logging an error state, as that can cause confusion [0], as this is
>>> not an error but normal behaviour.
>>>
>>> Report in the community forum:
>>> [0] https://forum.proxmox.com/threads/158306/
>>>
>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>> ---
>>> changes since version 1:
>>> - no changes
>>>
>>> Note:
>>> I am not sure this is the best approach, as this might block the
>>> thread until the server responds or it runs into a time out.
>>>
>>> The alternative would require completely reworking all backup reader
>>> related call sides. Or maybe there is another alternative?
>>>
>>> pbs-client/src/backup_reader.rs | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
>>> index 88cba599b..63106c999 100644
>>> --- a/pbs-client/src/backup_reader.rs
>>> +++ b/pbs-client/src/backup_reader.rs
>>> @@ -27,6 +27,8 @@ pub struct BackupReader {
>>>
>>> impl Drop for BackupReader {
>>> fn drop(&mut self) {
>>> + // Ignore errors
>>> + let _ = proxmox_async::runtime::block_on(self.post("finish", None));
>>
>> should we maybe make this explicit, like we do in the BackupWriter? I
>> know that it's a bit less "obvious" here compared to writer sessions
>> what constitutes success/being finished ;)
>>
>> means a bit more churn now to adapt the users of BackupReader, but would
>> make it possible to differentiate server side whether a reader session
>> was exited normally or via an error?
>
> Yes, that is basically what I meant in the note:
> The current approach is more of a catch all. Explicitly calling the
> finish for each reader instance might be prove difficult because of all
> the Arc instances being passed around and/or the reader instantiation
> happening in a helper which then only returns the wrapped consumer of
> the reader instance, e.g. a `RemoteChunkReader` or `pxar::Accessor`.
>
> Will have a look on how to handle this, but it will require some bigger
> refactoring.
>
>> we could even provide some sort of message via the finish API call that
>> the server could log if desired, differentiating between:
>>
>> - regular finish (no error/warning)
>> - finish called with a warning message (warning)
>> - finish not called, reader went away (error)
>>
>> ?
>
> Not sure about this: What warnings do we even want to tell the server
> about? I think the reader instance will either work without issues or
> fail with a hard error? Or do you have some specific use-case in mind here?
well it would be a way to record on the server that that reader "failed"
from a client's point of view, which would then be reflected in the task
lists/warning counts/..
e.g., if you do a restore, but abort it in the middle for some reason,
you could tell the server so that an admin looking at the server side
tasks can tell that this was an "incomplete" read session (this would
just be warning case, not an error).
not sure it's worth it / makes sense, but wanted to put it out there :)
if we just want to disambiguate "client vanished" vs "client said good
bye", then the approach in this patch is probably fine.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-05 9:40 ` Fabian Grünbichler
@ 2024-12-05 9:56 ` Christian Ebner
2024-12-05 11:29 ` Fabian Grünbichler
0 siblings, 1 reply; 10+ messages in thread
From: Christian Ebner @ 2024-12-05 9:56 UTC (permalink / raw)
To: Fabian Grünbichler, Proxmox Backup Server development discussion
On 12/5/24 10:40, Fabian Grünbichler wrote:
>>> we could even provide some sort of message via the finish API call that
>>> the server could log if desired, differentiating between:
>>>
>>> - regular finish (no error/warning)
>>> - finish called with a warning message (warning)
>>> - finish not called, reader went away (error)
>>>
>>> ?
>>
>> Not sure about this: What warnings do we even want to tell the server
>> about? I think the reader instance will either work without issues or
>> fail with a hard error? Or do you have some specific use-case in mind here?
>
> well it would be a way to record on the server that that reader "failed"
> from a client's point of view, which would then be reflected in the task
> lists/warning counts/..
>
> e.g., if you do a restore, but abort it in the middle for some reason,
> you could tell the server so that an admin looking at the server side
> tasks can tell that this was an "incomplete" read session (this would
> just be warning case, not an error).
Hmm, indeed. An intentional abort might be better handled as warning.
Ideally the same would be possible for the backup writer path as well,
so signaling this via additional api endpoint `abort` might even be
better? Since the `finish` is already intended to close and check
consistency of the backup snapshot for the backup writer case. Just
adding a flag to differentiate this seems less declarative.
Any thoughts on that?
> not sure it's worth it / makes sense, but wanted to put it out there :)
>
> if we just want to disambiguate "client vanished" vs "client said good
> bye", then the approach in this patch is probably fine.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect
2024-12-05 9:56 ` Christian Ebner
@ 2024-12-05 11:29 ` Fabian Grünbichler
0 siblings, 0 replies; 10+ messages in thread
From: Fabian Grünbichler @ 2024-12-05 11:29 UTC (permalink / raw)
To: Christian Ebner, Proxmox Backup Server development discussion
On December 5, 2024 10:56 am, Christian Ebner wrote:
> On 12/5/24 10:40, Fabian Grünbichler wrote:
>>>> we could even provide some sort of message via the finish API call that
>>>> the server could log if desired, differentiating between:
>>>>
>>>> - regular finish (no error/warning)
>>>> - finish called with a warning message (warning)
>>>> - finish not called, reader went away (error)
>>>>
>>>> ?
>>>
>>> Not sure about this: What warnings do we even want to tell the server
>>> about? I think the reader instance will either work without issues or
>>> fail with a hard error? Or do you have some specific use-case in mind here?
>>
>> well it would be a way to record on the server that that reader "failed"
>> from a client's point of view, which would then be reflected in the task
>> lists/warning counts/..
>>
>> e.g., if you do a restore, but abort it in the middle for some reason,
>> you could tell the server so that an admin looking at the server side
>> tasks can tell that this was an "incomplete" read session (this would
>> just be warning case, not an error).
>
> Hmm, indeed. An intentional abort might be better handled as warning.
> Ideally the same would be possible for the backup writer path as well,
> so signaling this via additional api endpoint `abort` might even be
> better? Since the `finish` is already intended to close and check
> consistency of the backup snapshot for the backup writer case. Just
> adding a flag to differentiate this seems less declarative.
>
> Any thoughts on that?
would have to look at the/a concrete implementation, but I think that
should also work for the writer case, since whenever we'd want to call
abort(), we'd not call finish(), but just close the connection, which
already triggers an error on the server side anyway? so even when a
(new) writer calls abort on an old server, just the error message would
change (from whatever it is right now to invalid request or something
like that), but the server-side task would still end up in an error
state anyway.. and for new client + new server, we'd get a nicer state
and message.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-12-05 11:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-04 8:31 [pbs-devel] [PATCH v2 proxmox-backup 0/3] handle reader client disconnects Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 1/3] client: backup: remove unnecessary clone for backup reader Christian Ebner
2024-12-04 13:50 ` [pbs-devel] applied: " Fabian Grünbichler
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 2/3] api: reader: handle reader client disconnects Christian Ebner
2024-12-04 8:31 ` [pbs-devel] [PATCH v2 proxmox-backup 3/3] client: reader: signal server before client disconnect Christian Ebner
2024-12-04 13:49 ` Fabian Grünbichler
2024-12-04 14:13 ` Christian Ebner
2024-12-05 9:40 ` Fabian Grünbichler
2024-12-05 9:56 ` Christian Ebner
2024-12-05 11:29 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox