[pbs-devel] [PATCH v5 proxmox-backup 31/31] server: sync job: use delete stats provided by the api
Christian Ebner
c.ebner at proxmox.com
Wed Oct 30 14:44:34 CET 2024
On 10/25/24 12:17, Fabian Grünbichler wrote:
> 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?
Okay, dropped the Option wrapper here and adapted the
PushParameters::new() method accordingly to always async fetch the
version. This extra API call even when `remove_vanished` is not set and
therefore the remote api version is not required does not hurt.
>
>> +
>> + 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 ?
Yes, I did follow your suggestion and added a `supports_feature` instead
of the `is_min_required` method. That is way more ergonomic and allows
to append new feature strings without issues at certain commits.
>
> 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..
True, made the unwrap_or_default conditionally here and the other
mentions below.
>
>> +
>> + 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?
Yes, added the warning in case of unsupported feature and
remove_vanished flag set.
>
> 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
>>
>>
>>
>
>
> _______________________________________________
> 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