[pbs-devel] [PATCH v5 proxmox-backup 31/31] server: sync job: use delete stats provided by the api
Fabian Grünbichler
f.gruenbichler at proxmox.com
Fri Oct 25 12:17:55 CEST 2024
On October 18, 2024 10:42 am, Christian Ebner wrote:
> Use the API exposed additional delete statistics to generate the
> task log output for sync jobs in push direction instead of fetching the
> contents before and after deleting.
>
> Detect older api versions of the target server for fallback.
>
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> changes since version 4:
> - no changes
>
> changes since version 3:
> - fetch target api version to remain backwards compatible
>
> src/server/push.rs | 126 +++++++++++++++++++++++++++++----------------
> 1 file changed, 82 insertions(+), 44 deletions(-)
>
> diff --git a/src/server/push.rs b/src/server/push.rs
> index bf6045214..a915b6964 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -8,11 +8,12 @@ use anyhow::{bail, format_err, Error};
> use futures::stream::{self, StreamExt, TryStreamExt};
> use tokio::sync::mpsc;
> use tokio_stream::wrappers::ReceiverStream;
> -use tracing::info;
> +use tracing::{info, warn};
>
> use pbs_api_types::{
> - print_store_and_ns, Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupFilter,
> - GroupListItem, NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem,
> + print_store_and_ns, ApiVersion, ApiVersionInfo, Authid, BackupDir, BackupGroup,
> + BackupGroupDeleteStats, BackupNamespace, CryptMode, GroupFilter, GroupListItem,
> + NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem,
> PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE,
> };
> use pbs_client::{BackupRepository, BackupWriter, HttpClient, MergedChunkInfo, UploadOptions};
> @@ -41,6 +42,8 @@ pub(crate) struct PushTarget {
> ns: BackupNamespace,
> // Http client to connect to remote
> client: HttpClient,
> + // Api version reported by the target
> + api_version: Option<ApiVersion>,
> }
>
> /// Parameters for a push operation
> @@ -106,6 +109,7 @@ impl PushParameters {
> repo,
> ns: remote_ns,
> client,
> + api_version: None,
> };
> let group_filter = group_filter.unwrap_or_default();
>
> @@ -124,6 +128,17 @@ impl PushParameters {
> fn map_to_target(&self, namespace: &BackupNamespace) -> Result<BackupNamespace, Error> {
> namespace.map_prefix(&self.source.ns, &self.target.ns)
> }
> +
> + // Fetch and set the api version info for the target
> + pub(crate) async fn fetch_target_api_version_info(&mut self) -> Result<(), Error> {
> + let api_path = "api2/json/version";
> + let mut result = self.target.client.get(api_path, None).await?;
> + let data = result["data"].take();
> + let version_info: ApiVersionInfo = serde_json::from_value(data)?;
> + self.target.api_version = Some(ApiVersion::try_from(version_info)?);
if the expectation is that this always works, should we do it when
initializing the PushParameters and not wrap it in an Option?
> +
> + Ok(())
> + }
> }
>
> // Check if the job user given in the push parameters has the provided privs on the remote
> @@ -167,7 +182,7 @@ async fn fetch_target_namespaces(params: &PushParameters) -> Result<Vec<BackupNa
> async fn remove_target_namespace(
> params: &PushParameters,
> namespace: &BackupNamespace,
> -) -> Result<(), Error> {
> +) -> Result<BackupGroupDeleteStats, Error> {
> if namespace.is_root() {
> bail!("cannot remove root namespace from target");
> }
> @@ -181,14 +196,23 @@ async fn remove_target_namespace(
> );
>
> let target_ns = params.map_to_target(namespace)?;
> - let args = serde_json::json!({
> + let mut args = serde_json::json!({
> "ns": target_ns.name(),
> "delete-groups": true,
> });
>
> - params.target.client.delete(&api_path, Some(args)).await?;
> + if let Some(version) = ¶ms.target.api_version {
> + if version.is_min_required(ApiVersion::new(3, 2, 8)) {
missed a version already ;) might make sense to either move this up as a
constant, or even add a sort of supports_feature(..) to ApiVersion ?
in any case, something like this should be called out somewhere
prominently so that it can be adapted/bumped if need be when applying
> + args["ignore-protected"] = serde_json::to_value(true)?;
> + }
> + }
>
> - Ok(())
> + let mut result = params.target.client.delete(&api_path, Some(args)).await?;
> + let data = result["data"].take();
> + let delete_stats: BackupGroupDeleteStats =
> + serde_json::from_value(data).unwrap_or_else(|_| BackupGroupDeleteStats::default());
and this here should only be done if the ApiVersion is too old, else it
masks deserialization problems..
> +
> + Ok(delete_stats)
> }
>
> // Fetch the list of groups found on target in given namespace
> @@ -229,7 +253,7 @@ async fn remove_target_group(
> params: &PushParameters,
> namespace: &BackupNamespace,
> backup_group: &BackupGroup,
> -) -> Result<(), Error> {
> +) -> Result<BackupGroupDeleteStats, Error> {
> check_ns_remote_datastore_privs(params, namespace, PRIV_REMOTE_DATASTORE_PRUNE)
> .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?;
>
> @@ -242,14 +266,23 @@ async fn remove_target_group(
> "backup-id": backup_group.id,
> "backup-type": backup_group.ty,
> });
> +
> + if let Some(version) = ¶ms.target.api_version {
> + if version.is_min_required(ApiVersion::new(3, 2, 8)) {
same here
> + args["ignore-protected"] = serde_json::to_value(true)?;
> + }
> + }
> if !namespace.is_root() {
> let target_ns = params.map_to_target(namespace)?;
> args["ns"] = serde_json::to_value(target_ns.name())?;
> }
>
> - params.target.client.delete(&api_path, Some(args)).await?;
> + let mut result = params.target.client.delete(&api_path, Some(args)).await?;
> + let data = result["data"].take();
> + let delete_stats: BackupGroupDeleteStats =
> + serde_json::from_value(data).unwrap_or_else(|_| BackupGroupDeleteStats::default());
and here
>
> - Ok(())
> + Ok(delete_stats)
> }
>
> // Check if the namespace is already present on the target, create it otherwise
> @@ -298,6 +331,7 @@ async fn check_or_create_target_namespace(
> /// Push contents of source datastore matched by given push parameters to target.
> pub(crate) async fn push_store(mut params: PushParameters) -> Result<SyncStats, Error> {
> let mut errors = false;
> + params.fetch_target_api_version_info().await?;
>
> // Generate list of source namespaces to push to target, limited by max-depth
> let mut namespaces = params.source.list_namespaces(&mut params.max_depth).await?;
> @@ -365,9 +399,25 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result<SyncStats,
> if synced_namespaces.contains(&target_namespace) {
> continue;
> }
> - if let Err(err) = remove_target_namespace(¶ms, &target_namespace).await {
> - info!("failed to remove vanished namespace {target_namespace} - {err}");
> - continue;
> + match remove_target_namespace(¶ms, &target_namespace).await {
> + Ok(delete_stats) => {
> + stats.add(SyncStats::from(RemovedVanishedStats {
> + snapshots: delete_stats.removed_snapshots(),
> + groups: delete_stats.removed_groups(),
> + namespaces: 1,
> + }));
should we maybe print a warning once at the start of sync if the
ApiVersion is too old, so that users know that these stats are
incomplete?
because in that case if no snapshots/groups were protected, we'll get a
default BackupGroupDeleteStats here, so we actually lack the information
about removed snapshots and groups..
> + if delete_stats.protected_snapshots() > 0 {
> + warn!(
> + "kept {protected_count} protected snapshots of namespace '{target_namespace}'",
> + protected_count = delete_stats.protected_snapshots(),
> + );
> + continue;
> + }
> + }
> + Err(err) => {
> + warn!("failed to remove vanished namespace {target_namespace} - {err}");
> + continue;
> + }
> }
> info!("removed vanished namespace {target_namespace}");
> }
> @@ -449,38 +499,26 @@ pub(crate) async fn push_namespace(
>
> info!("delete vanished group '{target_group}'");
>
> - let count_before = match fetch_target_groups(params, namespace).await {
> - Ok(snapshots) => snapshots.len(),
> - Err(_err) => 0, // ignore errors
> - };
> -
> - if let Err(err) = remove_target_group(params, namespace, &target_group).await {
> - info!("{err}");
> - errors = true;
> - continue;
> - }
> -
> - let mut count_after = match fetch_target_groups(params, namespace).await {
> - Ok(snapshots) => snapshots.len(),
> - Err(_err) => 0, // ignore errors
> - };
> -
> - let deleted_groups = if count_after > 0 {
> - info!("kept some protected snapshots of group '{target_group}'");
> - 0
> - } else {
> - 1
> - };
> -
> - if count_after > count_before {
> - count_after = count_before;
> + match remove_target_group(params, namespace, &target_group).await {
> + Ok(delete_stats) => {
> + if delete_stats.protected_snapshots() > 0 {
> + warn!(
> + "kept {protected_count} protected snapshots of group '{target_group}'",
> + protected_count = delete_stats.protected_snapshots(),
> + );
> + }
> + stats.add(SyncStats::from(RemovedVanishedStats {
> + snapshots: delete_stats.removed_snapshots(),
> + groups: delete_stats.removed_groups(),
> + namespaces: 0,
> + }));
same here
> + }
> + Err(err) => {
> + info!("failed to delete vanished group - {err}");
> + errors = true;
> + continue;
> + }
> }
> -
> - stats.add(SyncStats::from(RemovedVanishedStats {
> - snapshots: count_before - count_after,
> - groups: deleted_groups,
> - namespaces: 0,
> - }));
> }
> }
>
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list