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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 37C099C443 for ; Tue, 24 Oct 2023 10:18:15 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 089211CD53 for ; Tue, 24 Oct 2023 10:17:45 +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 ; Tue, 24 Oct 2023 10:17:43 +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 BB33D44979 for ; Tue, 24 Oct 2023 10:17:43 +0200 (CEST) Message-ID: <7025757f-af16-40bc-90dc-3fda2775b828@proxmox.com> Date: Tue, 24 Oct 2023 10:17:42 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: de-AT, en-US To: Proxmox Backup Server development discussion , Dominik Csapak References: <20231018103911.3798182-1-d.csapak@proxmox.com> From: Lukas Wagner In-Reply-To: <20231018103911.3798182-1-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -2.526 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 KAM_SOMETLD_ARE_BAD_TLD 5 .bar, .beauty, .buzz, .cam, .casa, .cfd, .club, .date, .guru, .link, .live, .monster, .online, .press, .pw, .quest, .rest, .sbs, .shop, .stream, .top, .trade, .wiki, .work, .xyz TLD abuse SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox-backup-api.rs, proxmox-backup-manager.rs, proxmox-backup-proxy.rs, jobs.rs, jobstate.rs, foo.bar, lib.rs, mod.rs] Subject: Re: [pbs-devel] [RFC proxmox/proxmox-backup] refactor common server code from pbs 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: Tue, 24 Oct 2023 08:18:15 -0000 In general these changes look good to me. I think we should take the opportunity to write some unit tests for the jobstate/cert-management crates though. I guess with the new server-config crate it should relatively straightforward to use some temporary directory as a base directory for the test runs. Reviewed-by: Lukas Wagner On 10/18/23 12:39, Dominik Csapak wrote: > # General > > Sending this as RFC because of the multiple ways (see 'Alternatives' > section) we could implement this (or something similar). I'm not sure > what the general opinion about this pattern is. > > # Goal > > When creating a new daemon/server, we currently have much code in pbs > that would have to be duplicated, mostly relating to setting up the > server directories/files. Some crates features already have an interface > to be slightly configurable, but not all of them. > > This change adds a new crate 'proxmox-server-config' that provides an > interface which should be easy to use and reusable across multiple > daemons/server applications for our use-cases. > > In this series i opted to factor out the two main remaining crates that > are not seperated yet: the jobstates and the certificate management. > (Though i guess there are more, depending on what another daemon might > want to do) > > # Implementation > > I use a static OnceLock to save a global `ServerConfig` struct, that > contains some configurations available, which are either setup, or > derived from the minimum required configuration (base_dir, name, user). > > This can then be used to access them from any point in the program or in > crates where needed. > > With this, we don't have to carry around the different directories as > parameters, nor do we have to sprinkle things like `config!("foo.bar")`, > around the code to get the path we want. > > # Alternatives > > The main alternatives to this approaches are: > > * copy the code across different server/daemons: > obviously the easiest way, but doesn't scale well and prone to > diverging implementations. > > * parameters: > this has two ways of working: > - all functions must take the configuration (dir(s)/user(s)) > blows up the parameters, and we have to use our current dir macros > even more > - each crate/module has an initializer that takes the config, which > would then e.g. save that into a static OnceLock once again, > disadvantage here is that we may duplicate values across multiple > crates (should not be much though) > > There are probably more ways to implement this (e.g. maybe a global > trait object that must be implemented by each server?) > > # Future work > > We could extend this pattern for other crates (e.g. rest-server) so > that we don't have to carry around the parameters anymore. > > Also we could make the ServerConfig extendable, where each server could > configure it's own necessary variables (e.g with a hashmap) > > proxmox: > > Dominik Csapak (3): > new proxmox-server-config crate > new proxmox-jobstate crate > new proxmox-cert-management crate > > Cargo.toml | 5 + > proxmox-cert-management/Cargo.toml | 23 ++ > proxmox-cert-management/debian/changelog | 5 + > proxmox-cert-management/debian/control | 53 ++++ > proxmox-cert-management/debian/copyright | 18 ++ > proxmox-cert-management/debian/debcargo.toml | 7 + > proxmox-cert-management/src/lib.rs | 182 +++++++++++ > proxmox-jobstate/Cargo.toml | 26 ++ > proxmox-jobstate/debian/changelog | 5 + > proxmox-jobstate/debian/control | 55 ++++ > proxmox-jobstate/debian/copyright | 18 ++ > proxmox-jobstate/debian/debcargo.toml | 7 + > proxmox-jobstate/src/api_types.rs | 40 +++ > proxmox-jobstate/src/jobstate.rs | 315 +++++++++++++++++++ > proxmox-jobstate/src/lib.rs | 46 +++ > proxmox-server-config/Cargo.toml | 16 + > proxmox-server-config/debian/changelog | 5 + > proxmox-server-config/debian/control | 37 +++ > proxmox-server-config/debian/copyright | 18 ++ > proxmox-server-config/debian/debcargo.toml | 7 + > proxmox-server-config/src/lib.rs | 302 ++++++++++++++++++ > 21 files changed, 1190 insertions(+) > create mode 100644 proxmox-cert-management/Cargo.toml > create mode 100644 proxmox-cert-management/debian/changelog > create mode 100644 proxmox-cert-management/debian/control > create mode 100644 proxmox-cert-management/debian/copyright > create mode 100644 proxmox-cert-management/debian/debcargo.toml > create mode 100644 proxmox-cert-management/src/lib.rs > create mode 100644 proxmox-jobstate/Cargo.toml > create mode 100644 proxmox-jobstate/debian/changelog > create mode 100644 proxmox-jobstate/debian/control > create mode 100644 proxmox-jobstate/debian/copyright > create mode 100644 proxmox-jobstate/debian/debcargo.toml > create mode 100644 proxmox-jobstate/src/api_types.rs > create mode 100644 proxmox-jobstate/src/jobstate.rs > create mode 100644 proxmox-jobstate/src/lib.rs > create mode 100644 proxmox-server-config/Cargo.toml > create mode 100644 proxmox-server-config/debian/changelog > create mode 100644 proxmox-server-config/debian/control > create mode 100644 proxmox-server-config/debian/copyright > create mode 100644 proxmox-server-config/debian/debcargo.toml > create mode 100644 proxmox-server-config/src/lib.rs > > proxmox-backup: > > Dominik Csapak (1): > use proxmox_jobstate and proxmox_cert_management crates > > Cargo.toml | 8 + > pbs-api-types/Cargo.toml | 1 + > pbs-api-types/src/jobs.rs | 41 +--- > src/auth_helpers.rs | 184 +--------------- > src/bin/proxmox-backup-api.rs | 8 +- > src/bin/proxmox-backup-manager.rs | 2 + > src/bin/proxmox-backup-proxy.rs | 2 + > src/server/jobstate.rs | 347 +----------------------------- > src/server/mod.rs | 21 ++ > 9 files changed, 52 insertions(+), 562 deletions(-) > -- - Lukas