From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id E88711FF0BC for ; Thu, 15 Oct 2026 13:14:56 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B8FD121400; Thu, 15 Oct 2026 13:14:56 +0200 (CEST) Message-ID: Date: Thu, 15 Oct 2026 13:14:49 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH proxmox v3 01/10] fix #7904: Add 'disable' to {sync,verify,tape,gc} job schema and datastore config To: Jonas Theisen , pbs-devel@lists.proxmox.com References: <20261009145328.281554-1-j.theisen@proxmox.com> <20261009145328.281554-2-j.theisen@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20261009145328.281554-2-j.theisen@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792062889647 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.507 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: G3UIA2VY34MMRW5L4PTHPVAGW3UTBPQE X-Message-ID-Hash: G3UIA2VY34MMRW5L4PTHPVAGW3UTBPQE X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: nit: this patch by itself does not fix the issue, so IMO the subject prefix is not warranted for this one. I would suggest to rather stick to pbs-api-types here. On 10/9/26 4:53 PM, Jonas Theisen wrote: > To be able to en-/disable the respective jobs this patch adds the > "disable" field to the job config and the needed schema. > > This also adds the gc_disable parameter to the datastore config > to allow this type of job to be disabled. nit: The commit message might also include the reasoning for why `disable` was choosen here, i.e.: ``` `disable` was chosen as parameter name for the flag on all jobs for compatibility with the pre-existing flag for prune job config. > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7904 > Signed-off-by: Jonas Theisen > --- > pbs-api-types/src/datastore.rs | 17 +++++++++++++++++ > pbs-api-types/src/jobs.rs | 27 +++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs > index 93ccaf07..b8ef7509 100644 > --- a/pbs-api-types/src/datastore.rs > +++ b/pbs-api-types/src/datastore.rs > @@ -460,6 +460,11 @@ pub const COUNTER_RESET_SCHEDULE_SCHEMA: Schema = > optional: true, > schema: SINGLE_LINE_COMMENT_SCHEMA, > }, > + "gc-disable": { > + description: "Disable scheduled execution of garbage collection.", > + optional: true, > + type: Boolean, > + }, > "gc-schedule": { > optional: true, > schema: GC_SCHEDULE_SCHEMA, > @@ -524,6 +529,10 @@ pub struct DataStoreConfig { > #[serde(skip_serializing_if = "Option::is_none")] > pub comment: Option, > > + #[serde(default, skip_serializing_if = "is_false")] > + #[updater(serde(skip_serializing_if = "Option::is_none"))] > + pub gc_disable: bool, > + > #[serde(skip_serializing_if = "Option::is_none")] > pub gc_schedule: Option, > > @@ -605,6 +614,7 @@ impl DataStoreConfig { > name, > path, > comment: None, > + gc_disable: Default::default(), > gc_schedule: None, > gc_on_unmount: None, > prune_schedule: None, > @@ -1704,6 +1714,9 @@ pub struct GarbageCollectionJobStatus { > pub store: String, > #[serde(flatten)] > pub status: GarbageCollectionStatus, > + /// Is this job disabled nit: Above docstring sounds more like a question and should rather be aligned with the other descriptions. Given that, I wonder why not use the SCHEMA as suggested in the previous version of the patch series? > + #[serde(default)] > + pub disable: bool, > /// Schedule of the gc job > #[serde(skip_serializing_if = "Option::is_none")] > pub schedule: Option, > @@ -1721,6 +1734,10 @@ pub struct GarbageCollectionJobStatus { > pub duration: Option, > } > > +fn is_false(b: &bool) -> bool { > + !b > +} comment: This is now already the 3rd implementation of this helper for skipping serde deserialization. It should rather move to lib.rs and made be accessible by all call sites. Ran into this myself lately, so the proposed fix is https://lore.proxmox.com/pbs-devel/20261014144357.771051-2-c.ebner@proxmox.com/ > + > #[api( > properties: { > "gc-status": { > diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs > index 9a5d2b77..73d7c13f 100644 > --- a/pbs-api-types/src/jobs.rs > +++ b/pbs-api-types/src/jobs.rs > @@ -228,6 +228,11 @@ pub const VERIFICATION_OUTDATED_AFTER_SCHEMA: Schema = > optional: true, > schema: VERIFICATION_SCHEDULE_SCHEMA, > }, > + disable: { > + type: Boolean, > + optional: true, > + default: false, > + }, > ns: { > optional: true, > schema: BACKUP_NAMESPACE_SCHEMA, > @@ -267,6 +272,10 @@ pub struct VerificationJobConfig { > #[serde(skip_serializing_if = "Option::is_none")] > /// when to schedule this job in calendar event notation > pub schedule: Option, > + /// Disable this job. > + #[serde(default, skip_serializing_if = "is_false")] > + #[updater(serde(skip_serializing_if = "Option::is_none"))] > + pub disable: bool, > #[serde(skip_serializing_if = "Option::is_none", default)] > /// on which backup namespace to run the verification recursively > pub ns: Option, > @@ -403,6 +412,11 @@ pub struct TapeBackupJobSetup { > optional: true, > schema: SYNC_SCHEDULE_SCHEMA, > }, > + disable: { > + type: Boolean, > + optional: true, > + default: false, > + }, > } > )] > #[derive(Serialize, Deserialize, Clone, Updater, PartialEq)] > @@ -417,6 +431,10 @@ pub struct TapeBackupJobConfig { > pub comment: Option, > #[serde(skip_serializing_if = "Option::is_none")] > pub schedule: Option, > + /// Disable this job. > + #[serde(default, skip_serializing_if = "is_false")] > + #[updater(serde(skip_serializing_if = "Option::is_none"))] > + pub disable: bool, > } > > #[api( > @@ -642,6 +660,11 @@ pub const UNMOUNT_ON_SYNC_DONE_SCHEMA: Schema = > optional: true, > schema: SYNC_SCHEDULE_SCHEMA, > }, > + disable: { > + type: Boolean, > + optional: true, > + default: false, > + }, > "group-filter": { > schema: GROUP_FILTER_LIST_SCHEMA, > optional: true, > @@ -717,6 +740,10 @@ pub struct SyncJobConfig { > pub comment: Option, > #[serde(skip_serializing_if = "Option::is_none")] > pub schedule: Option, > + /// Disable this job. > + #[serde(default, skip_serializing_if = "is_false")] > + #[updater(serde(skip_serializing_if = "Option::is_none"))] > + pub disable: bool, > #[serde(skip_serializing_if = "Option::is_none")] > pub group_filter: Option>, > #[serde(flatten)]