Re: [pbs-devel] [PATCH proxmox-backup v8 06/45] datastore: allow to get the backend for a datastore

[Lukas Wagner <l.wagner@proxmox.com>]

With my feedback addressed:

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>


On  2025-07-15 14:52, Christian Ebner wrote:
> Implements an enum with variants Filesystem and S3 to distinguish
> between available backends. Filesystem will be used as default, if no
> backend is configured in the datastores configuration. If the
> datastore has an s3 backend configured, the backend method will
> instantiate and s3 client and return it with the S3 variant.
> 
> This allows to instantiate the client once, keeping and reusing the
> same open connection to the api for the lifetime of task or job, e.g.
> in the backup writer/readers runtime environment.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 7:
> - no changes
> 
>  pbs-datastore/src/datastore.rs | 52 ++++++++++++++++++++++++++++++++--
>  pbs-datastore/src/lib.rs       |  1 +
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 924d8cf9c..90ab80005 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -12,6 +12,7 @@ use pbs_tools::lru_cache::LruCache;
>  use tracing::{info, warn};
>  
>  use proxmox_human_byte::HumanByte;
> +use proxmox_s3_client::{S3Client, S3ClientConfig, S3ClientOptions, S3ClientSecretsConfig};
>  use proxmox_schema::ApiType;
>  
>  use proxmox_sys::error::SysError;
> @@ -23,8 +24,8 @@ use proxmox_worker_task::WorkerTaskContext;
>  
>  use pbs_api_types::{
>      ArchiveType, Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, ChunkOrder,
> -    DataStoreConfig, DatastoreFSyncLevel, DatastoreTuning, GarbageCollectionStatus,
> -    MaintenanceMode, MaintenanceType, Operation, UPID,
> +    DataStoreConfig, DatastoreBackendConfig, DatastoreBackendType, DatastoreFSyncLevel,
> +    DatastoreTuning, GarbageCollectionStatus, MaintenanceMode, MaintenanceType, Operation, UPID,
>  };
>  use pbs_config::BackupLockGuard;
>  
> @@ -127,6 +128,7 @@ pub struct DataStoreImpl {
>      chunk_order: ChunkOrder,
>      last_digest: Option<[u8; 32]>,
>      sync_level: DatastoreFSyncLevel,
> +    backend_config: DatastoreBackendConfig,
>  }
>  
>  impl DataStoreImpl {
> @@ -141,6 +143,7 @@ impl DataStoreImpl {
>              chunk_order: Default::default(),
>              last_digest: None,
>              sync_level: Default::default(),
> +            backend_config: Default::default(),
>          })
>      }
>  }
> @@ -196,6 +199,12 @@ impl Drop for DataStore {
>      }
>  }
>  
> +#[derive(Clone)]
> +pub enum DatastoreBackend {
> +    Filesystem,
> +    S3(Arc<S3Client>),
> +}
> +

Missing doc comments for this public enum

>  impl DataStore {
>      // This one just panics on everything
>      #[doc(hidden)]
> @@ -206,6 +215,39 @@ impl DataStore {
>          })
>      }
>  
> +    /// Get the backend for this datastore based on it's configuration
> +    pub fn backend(&self) -> Result<DatastoreBackend, Error> {
> +        let backend_type = match self.inner.backend_config.ty.unwrap_or_default() {
> +            DatastoreBackendType::Filesystem => DatastoreBackend::Filesystem,
> +            DatastoreBackendType::S3 => {
> +                let s3_client_id = self
> +                    .inner
> +                    .backend_config
> +                    .client
> +                    .as_ref()
> +                    .ok_or_else(|| format_err!("missing client for s3 backend"))?;
> +                let bucket = self
> +                    .inner
> +                    .backend_config
> +                    .bucket
> +                    .clone()
> +                    .ok_or_else(|| format_err!("missing bucket for s3 backend"))?;
> +
> +                let (config, _config_digest) = pbs_config::s3::config()?;
> +                let (secrets, _secrets_digest) = pbs_config::s3::secrets_config()?;
> +                let config: S3ClientConfig = config.lookup("s3client", s3_client_id)?;
> +                let secrets: S3ClientSecretsConfig = secrets.lookup("s3secrets", s3_client_id)?;

Same thing here with regards to the hard-coded section type names.

> +
> +                let options =
> +                    S3ClientOptions::from_config(config, secrets, bucket, self.name().to_owned());
> +                let s3_client = S3Client::new(options)?;
> +                DatastoreBackend::S3(Arc::new(s3_client))
> +            }
> +        };
> +
> +        Ok(backend_type)
> +    }
> +
>      pub fn lookup_datastore(
>          name: &str,
>          operation: Option<Operation>,
> @@ -383,6 +425,11 @@ impl DataStore {
>                  .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
>          )?;
>  
> +        let backend_config: DatastoreBackendConfig = serde_json::from_value(
> +            DatastoreBackendConfig::API_SCHEMA
> +                .parse_property_string(config.backend.as_deref().unwrap_or(""))?,
> +        )?;
> +
>          Ok(DataStoreImpl {
>              chunk_store,
>              gc_mutex: Mutex::new(()),
> @@ -391,6 +438,7 @@ impl DataStore {
>              chunk_order: tuning.chunk_order.unwrap_or_default(),
>              last_digest,
>              sync_level: tuning.sync_level.unwrap_or_default(),
> +            backend_config,
>          })
>      }
>  
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index ffd0d91b2..ca6fdb7d8 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -204,6 +204,7 @@ pub use store_progress::StoreProgress;
>  mod datastore;
>  pub use datastore::{
>      check_backup_owner, ensure_datastore_is_mounted, get_datastore_mount_status, DataStore,
> +    DatastoreBackend,
>  };
>  
>  mod hierarchy;

-- 
- Lukas



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel