[pbs-devel] [PATCH proxmox-backup 1/3] Add KeepOptions to Sync Job settings
Wolfgang Bumiller
w.bumiller at proxmox.com
Mon Nov 28 11:10:04 CET 2022
On Wed, Nov 23, 2022 at 10:40:20AM +0100, Stefan Hanreich wrote:
>
> On 11/7/22 12:00, Wolfgang Bumiller wrote:
> > On Fri, Nov 04, 2022 at 03:30:52PM +0100, Stefan Hanreich wrote:
> > > This enables users to specify prune options when creating/editing a sync
> > > job. After a Sync Jobs runs successfully, a prune job with the specified
> > > parameters gets started that prunes the synced backup groups. This
> > > behaves exactly the same as a normal prune job.
> > >
> > > Signed-off-by: Stefan Hanreich <s.hanreich at proxmox.com>
> > > ---
> > > pbs-api-types/src/jobs.rs | 7 +++-
> > > src/api2/config/sync.rs | 66 +++++++++++++++++++++++++++--
> > > src/api2/pull.rs | 11 ++++-
> > > src/bin/proxmox-backup-manager.rs | 20 +++++++--
> > > src/server/pull.rs | 70 +++++++++++++++++++++++++++----
> > > 5 files changed, 156 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> > > index 7f029af7..c778039c 100644
> > > --- a/pbs-api-types/src/jobs.rs
> > > +++ b/pbs-api-types/src/jobs.rs
> > > @@ -474,6 +474,9 @@ pub const GROUP_FILTER_LIST_SCHEMA: Schema =
> > > limit: {
> > > type: RateLimitConfig,
> > > },
> > > + keep: {
> > > + type: KeepOptions,
> > > + },
> > > schedule: {
> > > optional: true,
> > > schema: SYNC_SCHEDULE_SCHEMA,
> > > @@ -511,6 +514,8 @@ pub struct SyncJobConfig {
> > > pub group_filter: Option<Vec<GroupFilter>>,
> > > #[serde(flatten)]
> > > pub limit: RateLimitConfig,
> > > + #[serde(flatten)]
> > > + pub keep: KeepOptions,
> > > }
> > > impl SyncJobConfig {
> > > @@ -572,7 +577,7 @@ pub struct SyncJobStatus {
> > > },
> > > }
> > > )]
> > > -#[derive(Serialize, Deserialize, Default, Updater)]
> > > +#[derive(Serialize, Deserialize, Default, Updater, Clone)]
> > > #[serde(rename_all = "kebab-case")]
> > > /// Common pruning options
> > > pub struct KeepOptions {
> > > diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> > > index 6f39b239..82231715 100644
> > > --- a/src/api2/config/sync.rs
> > > +++ b/src/api2/config/sync.rs
> > > @@ -7,9 +7,10 @@ use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
> > > use proxmox_schema::{api, param_bail};
> > > use pbs_api_types::{
> > > - Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT,
> > > - PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT,
> > > - PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
> > > + Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA,
> > > + PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
> > > + PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT, PRIV_REMOTE_READ,
> > > + PROXMOX_CONFIG_DIGEST_SCHEMA
> > > };
> > > use pbs_config::sync;
> > > @@ -216,6 +217,18 @@ pub enum DeletableProperty {
> > > remote_ns,
> > > /// Delete the max_depth property,
> > > max_depth,
> > > + /// Delete keep_last prune option.
> > > + keep_last,
> > > + /// Delete keep_hourly prune option.
> > > + keep_hourly,
> > > + /// Delete keep_daily prune option.
> > > + keep_daily,
> > > + /// Delete keep_weekly prune option.
> > > + keep_weekly,
> > > + /// Delete keep_monthly prune option.
> > > + keep_monthly,
> > > + /// Delete keep_yearly prune option.
> > > + keep_yearly
> > > }
> > > #[api(
> > > @@ -310,6 +323,24 @@ pub fn update_sync_job(
> > > DeletableProperty::max_depth => {
> > > data.max_depth = None;
> > > }
> > > + DeletableProperty::keep_last => {
> > > + data.keep.keep_last = None;
> > > + }
> > > + DeletableProperty::keep_hourly => {
> > > + data.keep.keep_hourly = None;
> > > + }
> > > + DeletableProperty::keep_daily => {
> > > + data.keep.keep_daily = None;
> > > + }
> > > + DeletableProperty::keep_weekly => {
> > > + data.keep.keep_weekly = None;
> > > + }
> > > + DeletableProperty::keep_monthly => {
> > > + data.keep.keep_monthly = None;
> > > + }
> > > + DeletableProperty::keep_yearly => {
> > > + data.keep.keep_yearly = None;
> > > + }
> > > }
> > > }
> > > }
> > > @@ -381,6 +412,25 @@ pub fn update_sync_job(
> > > }
> > > }
> > > + if update.keep.keep_last.is_some() {
> > > + data.keep.keep_last = update.keep.keep_last;
> > > + }
> > > + if update.keep.keep_hourly.is_some() {
> > > + data.keep.keep_hourly = update.keep.keep_hourly;
> > > + }
> > > + if update.keep.keep_daily.is_some() {
> > > + data.keep.keep_daily = update.keep.keep_daily;
> > > + }
> > > + if update.keep.keep_weekly.is_some() {
> > > + data.keep.keep_weekly = update.keep.keep_weekly;
> > > + }
> > > + if update.keep.keep_monthly.is_some() {
> > > + data.keep.keep_monthly = update.keep.keep_monthly;
> > > + }
> > > + if update.keep.keep_yearly.is_some() {
> > > + data.keep.keep_yearly = update.keep.keep_yearly;
> > > + }
> > > +
> > > if !check_sync_job_modify_access(&user_info, &auth_id, &data) {
> > > bail!("permission check failed");
> > > }
> > > @@ -463,6 +513,8 @@ pub const ROUTER: Router = Router::new()
> > > #[test]
> > > fn sync_job_access_test() -> Result<(), Error> {
> > > + use pbs_api_types::KeepOptions;
> > > +
> > > let (user_cfg, _) = pbs_config::user::test_cfg_from_str(
> > > r###"
> > > user: noperm at pbs
> > > @@ -508,6 +560,14 @@ acl:1:/remote/remote1/remotestore1:write at pbs:RemoteSyncOperator
> > > group_filter: None,
> > > schedule: None,
> > > limit: pbs_api_types::RateLimitConfig::default(), // no limit
> > > + keep: KeepOptions {
> > > + keep_last: None,
> > > + keep_hourly: None,
> > > + keep_daily: None,
> > > + keep_weekly: None,
> > > + keep_monthly: None,
> > > + keep_yearly: None
> > ^ This could use `KeepOptions::default()`.
> >
> of course..
> > > + },
> > > };
> > > // should work without ACLs
> > > diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> > > index 193f28fe..f39e0b11 100644
> > > --- a/src/api2/pull.rs
> > > +++ b/src/api2/pull.rs
> > > @@ -9,7 +9,7 @@ use proxmox_schema::api;
> > > use proxmox_sys::task_log;
> > > use pbs_api_types::{
> > > - Authid, BackupNamespace, GroupFilter, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
> > > + Authid, BackupNamespace, GroupFilter, RateLimitConfig, KeepOptions, SyncJobConfig, DATASTORE_SCHEMA,
> > > GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
> > > PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
> > > };
> > > diff --git a/src/server/pull.rs b/src/server/pull.rs
> > > index 77caf327..20fda909 100644
> > > --- a/src/server/pull.rs
> > > +++ b/src/server/pull.rs
> > > @@ -1157,5 +1162,54 @@ pub(crate) async fn pull_ns(
> > > };
> > > }
> > > + if params.keep_options.keeps_something() {
> > > + let result: Result<(), Error> = proxmox_lang::try_block!({
> > ^ While we already go this route for `remove_vanished`, I'd prefer to
> > have both that one and this be a separate function, as this is starting
> > to feel very spaghetti...
> Sounds good, I've already refactored it now.
> > > + task_log!(worker, "running prune job");
> > > +
> > > + for local_group in list.into_iter() {
> > > + let owner = params.store.get_owner(&target_ns, &local_group)?;
> > > + if check_backup_owner(&owner, ¶ms.owner).is_err() {
> > > + continue;
> > > + }
> > > +
> > > + if let Some(ref group_filter) = ¶ms.group_filter {
> > > + if !apply_filters(&local_group, group_filter) {
> > > + continue;
> > > + }
> > > + }
> > > +
> > > + task_log!(worker, "pruning backup group {}", &local_group);
> > > +
> > > + let backup_group = params.store.backup_group(target_ns.clone(), local_group);
> > > + let prune_info = compute_prune_info(backup_group.list_backups()?, ¶ms.keep_options)?;
> > > +
> > > + for (info, mark) in prune_info {
> > I feel like there ought to be a helper for this loop. (probably just
> > with a dry_run and a callback parameter would be enough?)
> >
> > Since we have almost the exact same loop in `src/server/prune_job.rs`
> > and in `src/api2/admin/datastore.rs`
> >
> > Just one has a `dry_run` option and the other want sto collect the info
> > in an array for later.
>
> Not sure I understand 100% correctly. Just adding a helper method that runs
> the loop and destroys marked BackupInfos is relatively clear I think. The
> callback should be called for every BackupInfo and then receive as
> arguments: info & mark.
>
> Should the callback replace the keep/destroy functionality, or should it be
> called additionally? I'd tend towards replacing keep/destroy, but that would
> make the dry_run argument kinda useless in the case of passing a callback.
With this patch we get 3 destroy loops and 1 "shortcut dry-run loop",
destroy should be the main purpose.
Note that one version uses `datastore.remove_backup_dir` which is the
same as `info.backup_dir.destroy()`
Come to think of it, perhaps `compute_prune_info` should return a
`Prune` or `PruneJob` *type* encoding this, with options for dry-run,
logging (also unifies the log messages) and actually executing the
pruning with a callback to collect results.
> Except if dry_run also applies to the callback, in which case passing a
> callback is kinda moot. Should the callback nevertheless be called when
> passing dry_run, or should it get ignored as well - just producing log
> output (print info/mark for every BackupInfo)?
So the callback would always be called in order to collect the result
array.
> Currently I'd tend towards the callback replacing the default functionality
> (destroy marked dirs - which could also just be implemented as the default
> callback if no other callback is supplied)
But then it could almost qualify as a generic `fn for_each()`, so no ;-)
I'm more concerned with multiple different entry points to different
versions of the destroy code and copied possibly-different log messages
;-)
More information about the pbs-devel
mailing list