Re: [PATCH proxmox-backup v3 2/3] api: add heartbeat endpoint for backup reader/writer http/2 clients

[Robert Obkircher <r.obkircher@proxmox.com>]

> Backup readers and writers can potentially have long periods of idle
> connections, e.g. a reader if a backup snapshot has been mounted and
> all relevant chunks are locally cached, a backup session with
> previous metadata archive not needing to fetch new contents while the
> backup is ongoing or a backup writer to a datastore with slow backend
> storage.
> 
> Proxies like e.g. HAProxy might however close idle connections for
> better resource handling [0,1], even multiplexed HTTP/2 connections as
> are being used for the Proxmox Backup Sever backup reader/writer
> protocol.
> 
> Therefore, perform heartbeat traffic in the HTTP/2 client when no
> other requests are being send, tracked by an internal timeout,
> being rest before sending a request.
nit: typo: "rest"
> 
> Since older servers do not provide the new API path, ignore errors
> as the response is not strictly necessary for the connection to
> remain established.
> 
> The heartbeat is currently only being performed if the timeout value
> in seconds is given via the PBS_READER_HEARTBEAT_TIMEOUT for readers
> and PBS_WRITER_HEARTBEAT_TIMEOUT for writers.
> 
> Testing was performed using HAProxy with the Proxmox Backup Server
> as backend using the following 5 second connection idle timeouts
> as configuration parameters in haproxy.cfg:
> ```
> ...
> defaults
>     ...
>     timeout connect 5000
>     timeout client  5000
>     timeout server  5000
>     ..
> 
> frontend http-in
>     bind *:8007
>     mode tcp
>     default_backend pbs
> 
> backend pbs
>     mode tcp
>     http-reuse always
>     server pbs <PBS-IP>:8007 verify none
> ```
> 
> As command invocation:
> ```
> PBS_READER_HEARTBEAT_TIMEOUT=1 proxmox-backup-client mount <snapshot> <archive> \
> <mountpoint> --repository <user-and-realm>@<PROXY-IP>:<datastore> --verbose
> ```
> 
> [0] https://www.haproxy.com/documentation/haproxy-configuration-manual/latest/#4-timeout%20client
> [1] https://www.haproxy.com/documentation/haproxy-configuration-manual/latest/#4.2-timeout%20http-keep-alive
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6373
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/pbs-client/src/backup_reader.rs b/pbs-client/src/backup_reader.rs
> index 139659479..cd9071310 100644
> --- a/pbs-client/src/backup_reader.rs
> +++ b/pbs-client/src/backup_reader.rs
> @@ -5,6 +5,7 @@ use std::sync::Arc;
>  
>  use futures::future::AbortHandle;
>  use serde_json::{Value, json};
> +use tokio::sync::mpsc;
>  
>  use pbs_api_types::{BackupArchiveName, BackupDir, BackupNamespace, MANIFEST_BLOB_NAME};
>  use pbs_datastore::data_blob::DataBlob;
> @@ -16,28 +17,61 @@ use pbs_datastore::{BackupManifest, PROXMOX_BACKUP_READER_PROTOCOL_ID_V1};
>  use pbs_tools::crypt_config::CryptConfig;
>  use pbs_tools::sha::sha256;
>  
> -use super::{H2Client, HttpClient};
> +use super::{H2Client, HeartBeatMsg, HttpClient, parse_optional_heartbeat_env};
>  
>  /// Backup Reader
>  pub struct BackupReader {
>      h2: H2Client,
>      abort: AbortHandle,
>      crypt_config: Option<Arc<CryptConfig>>,
> +    heartbeat: Option<mpsc::Sender<HeartBeatMsg>>,
>  }
>  
>  impl Drop for BackupReader {
>      fn drop(&mut self) {
> +        self.send_msg_heartbeat(HeartBeatMsg::Abort);
>          self.abort.abort();
>      }
>  }
>  
>  impl BackupReader {
> -    fn new(h2: H2Client, abort: AbortHandle, crypt_config: Option<Arc<CryptConfig>>) -> Arc<Self> {
> -        Arc::new(Self {
> -            h2,
> -            abort,
> -            crypt_config,
> -        })
> +    fn new(
> +        h2: H2Client,
> +        abort: AbortHandle,
> +        crypt_config: Option<Arc<CryptConfig>>,
> +    ) -> Result<Arc<Self>, Error> {
> +        if let Some(timeout) = parse_optional_heartbeat_env("PBS_READER_HEARTBEAT_TIMEOUT")? {
> +            let (send, mut recv) = mpsc::channel(1);
> +            let backup_reader = Arc::new(Self {
> +                h2,
> +                abort,
> +                crypt_config,
> +                heartbeat: Some(send),
> +            });
> +            let reader_cloned = Arc::clone(&backup_reader);
> +
> +            tokio::spawn(async move {
> +                loop {
> +                    match tokio::time::timeout(timeout, recv.recv()).await {
> +                        Ok(Some(HeartBeatMsg::Reset)) => (),
> +                        Ok(Some(HeartBeatMsg::Abort)) | Ok(None) => break,
> +                        Err(_elapsed) => {
> +                            // connection idle timeout reached, send heatbeat
> +                            let _ = reader_cloned.h2.get("heartbeat", None).await;
> +                        }
> +                    }
Wouldn't this spam the server if someone sets timeout to 0?

-- 
Robert Obkircher <r.obkircher@proxmox.com>