From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 2A24E1FF136 for ; Mon, 23 Mar 2026 16:22:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9B68C3097D; Mon, 23 Mar 2026 16:22:47 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Mon, 23 Mar 2026 16:22:13 +0100 Message-Id: To: =?utf-8?q?Fabian_Gr=C3=BCnbichler?= , "Christian Ebner" , =?utf-8?q?Michael_K=C3=B6ppl?= , Subject: Re: [PATCH proxmox-backup v2 1/3] api: move statefile loading into compute_schedule_status From: =?utf-8?q?Michael_K=C3=B6ppl?= Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0 References: <20260319110318.70346-1-m.koeppl@proxmox.com> <20260319110318.70346-2-m.koeppl@proxmox.com> <75768b61-8ff5-4e0a-b7e4-3b5fafbdd1c9@proxmox.com> <3b18d413-11f2-4179-aabd-4e9f000924ea@proxmox.com> <1774270312.uf8ttrfndd.astroid@yuna.none> In-Reply-To: <1774270312.uf8ttrfndd.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774279287154 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.088 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: RLF2KBUZ6CV224KPLWKHP7MRIDS6BPWJ X-Message-ID-Hash: RLF2KBUZ6CV224KPLWKHP7MRIDS6BPWJ X-MailFrom: m.koeppl@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: On Mon Mar 23, 2026 at 1:55 PM CET, Fabian Gr=C3=BCnbichler wrote: [snip] >>>> This alters behavior as now it is never tried to load the state file i= f >>>> status_in_memory.upid is None, so there is no error logged. >>>> >>>> So this must be expanded by an else branch where the loading is >>>> attempted also for that case and the potential error logged. >>>=20 >>> Missed that while refactoring, sorry for the oversight. Also noticed >>> that there is an additional change in behavior regarding the handling o= f >>> any *other* error that might occur in compute_schedule_status because >>> previously, we would use basically ignore any error and return the >>> default status here, e.g. if the UPID could not be parsed for a started >>> job. To match this behavior, I could just do >>>=20 >>> let computed_schedule: JobScheduleStatus =3D >>> compute_schedule_status("garbage_collection", &store, Some(upi= d)) >>> .unwrap_or_else(|_| JobScheduleStatus::default()); >>>=20 >>> But the question is if the behavior here *should* differ from all other >>> endpoints if the UPID could not be parsed? Because everywhere else we'd >>> still return an error in that case. >>=20 >> True: this was introduced with commit fe1d34d2e ("api: garbage collect= =20 >> job status") and then adapted with commit 3ae21d87c ("GC: flatten=20 >> existing status into job status"). So I guess this is related to the=20 >> mentioned renaming. >>=20 >> Maybe Fabian can give us a clue? > > If there is a sensible way to handle compute_schedule_status failing > (AFAICT, currently that only triggers if the UPID can't be parsed), we > should do it consistently across the board - the same is true if that is > not possible ;) Thanks for your feedback and input! So I would keep the behavior implemented in the v2 version of this patch and return an error to the user if parsing the UPID fails, matching the behavior of all other endpoints. > > maybe we should introduce another JobState variant that is not persisted > ever, but used as a placeholder for partially-invalid entries? we > already do quite a bit of mangling of other invalid data there anyway.. That's actually how I initially planned to implement it. I introduced an `Unknown` JobState and returned that if parsing the statefile failed (again, could also do this if parsing the UPID failed). Not quite sure about the name, though. Open to suggestions.