From: Lukas Wagner <l.wagner@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>,
Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pbs-devel] [RFC proxmox/proxmox-backup] refactor common server code from pbs
Date: Tue, 24 Oct 2023 10:17:42 +0200 [thread overview]
Message-ID: <7025757f-af16-40bc-90dc-3fda2775b828@proxmox.com> (raw)
In-Reply-To: <20231018103911.3798182-1-d.csapak@proxmox.com>
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 <l.wagner@proxmox.com>
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
prev parent reply other threads:[~2023-10-24 8:18 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 10:39 Dominik Csapak
2023-10-18 10:39 ` [pbs-devel] [RFC proxmox 1/3] new proxmox-server-config crate Dominik Csapak
2023-10-25 16:38 ` Thomas Lamprecht
2023-10-30 11:05 ` Dominik Csapak
2023-10-30 11:41 ` Thomas Lamprecht
2023-10-18 10:39 ` [pbs-devel] [RFC proxmox 2/3] new proxmox-jobstate crate Dominik Csapak
2023-10-18 10:39 ` [pbs-devel] [RFC proxmox 3/3] new proxmox-cert-management crate Dominik Csapak
2023-10-18 10:39 ` [pbs-devel] [RFC proxmox-backup 1/1] use proxmox_jobstate and proxmox_cert_management crates Dominik Csapak
2023-10-24 8:17 ` Lukas Wagner [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7025757f-af16-40bc-90dc-3fda2775b828@proxmox.com \
--to=l.wagner@proxmox.com \
--cc=d.csapak@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox