[pbs-devel] [PATCH proxmox-backup 1/1] api: ui: add transfer-last parameter to pull and sync-job

[Wolfgang Bumiller]

needs a rebase + some comments inline

On Fri, Oct 14, 2022 at 12:30:31PM +0200, Stefan Hanreich wrote:
> partially fixes #3701
> 
> Specifying this parameter limits the amount of backups that get
> synced via the pull command or sync job. The parameter specifies how many
> of the N latest backups should get pulled/synced. All other backups will
> get skipped during the pull/sync-job.
> 
> Additionally added a field in the sync job UI to configure this value.
> 
> Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
> ---
>  pbs-api-types/src/jobs.rs         | 12 ++++++++++++
>  src/api2/config/sync.rs           |  9 +++++++++
>  src/api2/pull.rs                  | 10 +++++++++-
>  src/bin/proxmox-backup-manager.rs | 13 +++++++++++--
>  src/server/pull.rs                | 16 ++++++++++++++--
>  www/config/SyncView.js            |  9 ++++++++-
>  www/window/SyncJobEdit.js         | 13 +++++++++++++
>  7 files changed, 76 insertions(+), 6 deletions(-)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 7f029af7..4ac84372 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -433,6 +433,12 @@ pub const GROUP_FILTER_SCHEMA: Schema = StringSchema::new(
>  pub const GROUP_FILTER_LIST_SCHEMA: Schema =
>      ArraySchema::new("List of group filters.", &GROUP_FILTER_SCHEMA).schema();
>  
> +pub const TRANSFER_LAST_SCHEMA: Schema = IntegerSchema::new(
> +    "The maximum amount of snapshots to be transferred (per group)."
> +)
> +    .minimum(1)
> +    .schema();
> +
>  #[api(
>      properties: {
>          id: {
> @@ -482,6 +488,10 @@ pub const GROUP_FILTER_LIST_SCHEMA: Schema =
>              schema: GROUP_FILTER_LIST_SCHEMA,
>              optional: true,
>          },
> +        "transfer-last": {
> +            schema: TRANSFER_LAST_SCHEMA,
> +            optional: true,
> +        },
>      }
>  )]
>  #[derive(Serialize, Deserialize, Clone, Updater)]
> @@ -511,6 +521,8 @@ pub struct SyncJobConfig {
>      pub group_filter: Option<Vec<GroupFilter>>,
>      #[serde(flatten)]
>      pub limit: RateLimitConfig,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub transfer_last: Option<u64>,

`usize` might need less casting and be good enough unless we worry about
having 4mio snapshots in a group on a 32-bit system?

>  }
>  
>  impl SyncJobConfig {
(...)
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 77caf327..9be82669 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -647,6 +651,8 @@ async fn pull_group(
>          count: 0,
>      };
>  
> +    let transfer_amount = params.transfer_last.unwrap_or(list.len() as u64);
> +
>      for (pos, item) in list.into_iter().enumerate() {
>          let snapshot = item.backup;
>  
> @@ -669,6 +675,12 @@ async fn pull_group(
>              }
>          }
>  
> +        let i = pos as u64;

Please don't rename `pos` to a single letter somewhere in the middle all
the way to the end. Keep it named `pos`.

> +        if progress.group_snapshots - i > transfer_amount {

I find the connection between `group_snapshots` and `list.len()` to be
non-obvious at this point, particularly because its name (with the
`progres.` prefix) sounds more like a counter while it's actually `i`
that is doing the counting (and "done_snapshots") and think

    if pos < (list.len() - transfer_amount)

would be much easier to grasp?

> +            skip_info.update(snapshot.time);
> +            continue;
> +        }
> +
>          // get updated auth_info (new tickets)
>          let auth_info = client.login().await?;
>  
> @@ -697,7 +709,7 @@ async fn pull_group(
>  
>          let result = pull_snapshot_from(worker, reader, &snapshot, downloaded_chunks.clone()).await;
>  
> -        progress.done_snapshots = pos as u64 + 1;
> +        progress.done_snapshots = i + 1;

:-S

>          task_log!(worker, "percentage done: {}", progress);
>  
>          result?; // stop on error