From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 2D51E975E1 for ; Wed, 17 Apr 2024 09:46:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0C276379E8 for ; Wed, 17 Apr 2024 09:46:59 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 17 Apr 2024 09:46:58 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2E6E54179C for ; Wed, 17 Apr 2024 09:46:58 +0200 (CEST) Date: Wed, 17 Apr 2024 09:46:54 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Gabriel Goller , Proxmox Backup Server development discussion References: <20240412100631.94218-1-l.wagner@proxmox.com> <20240412100631.94218-15-l.wagner@proxmox.com> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1713339434.lmgwlqq4ff.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.056 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 Subject: Re: [pbs-devel] [PATCH proxmox-backup 14/33] server: notifications: send GC notifications via notification system X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2024 07:46:59 -0000 On April 16, 2024 2:13 pm, Lukas Wagner wrote: >=20 >=20 > On 2024-04-16 11:37, Gabriel Goller wrote: >> On Fri Apr 12, 2024 at 12:06 PM CEST, Lukas Wagner wrote: >>> diff --git a/src/server/gc_job.rs b/src/server/gc_job.rs >>> index 41375d72..ff5bdccf 100644 >>> --- a/src/server/gc_job.rs >>> +++ b/src/server/gc_job.rs >>> @@ -19,8 +19,6 @@ pub fn do_garbage_collection_job( >>> ) -> Result { >>> let store =3D datastore.name().to_string(); >>> =20 >>> - let (email, notify) =3D crate::server::lookup_datastore_notify_set= tings(&store); >>> - >>> let worker_type =3D job.jobtype().to_string(); >>> let upid_str =3D WorkerTask::new_thread( >>> &worker_type, >>> @@ -43,11 +41,9 @@ pub fn do_garbage_collection_job( >>> eprintln!("could not finish job state for {}: {err}", = job.jobtype()); >>> } >>> =20 >>> - if let Some(email) =3D email { >>> - let gc_status =3D datastore.last_gc_status(); >>> - if let Err(err) =3D send_gc_status(&email, notify, &st= ore, &gc_status, &result) { >>> - eprintln!("send gc notification failed: {err}"); >>> - } >>> + let gc_status =3D datastore.last_gc_status(); >>> + if let Err(err) =3D send_gc_status(&store, &gc_status, &re= sult) { >>> + eprintln!("send gc notification failed: {err}"); >>=20 >> I think we should use 'task_err!()' here. I know eprintln is used above, >> and technically works because we redirect stderr in the service setup=20 >> but it's still slow and kinda the legacy method of printing task errors. >=20 > I think the reason why the original code does not use task_log is because= the > job is already marked as finished at that point: >=20 > if let Err(err) =3D job.finish(status) { > eprintln!("could not finish job state for {}: {err}", job= .jobtype()); > } >=20 > let gc_status =3D datastore.last_gc_status(); > if let Err(err) =3D send_gc_status(&store, &gc_status, &resul= t) { > eprintln!("send gc notification failed: {err}"); > } >=20 > Other jobs seem to follow the same pattern - use task_log! before, and ep= rintln/log::error! > after the job is finished. > A `task_log!` after the job is finished still seems to work, but I'm not = sure if that > might lead to problems. What do you think? I don't think this is a problem per se - job.finish() just marks the job as finished in the job state, it's not related to the worker task finish/exit itself. since task_log (and friends) require the worker to still exist, and the last thing that happens when the worker fn returns its result is that result being logged via the same logging mechanism, this should be fine. but we might still want to switch the order around, so that a warning for notification failure actually gets counted? if it's such a common pattern to do job.start at the start, and job.finish near/at the end of a worker task, I wonder whether we couldn't handle that in a uniform fashion ;) then we might also not end up calling worker.create_state twice (once for job.finish, and once for worker.log_result as part of the worker exiting) with slightly different states..