* [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] 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
* [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
* 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