Re: [pbs-devel] [RFC proxmox/proxmox-backup] refactor common server code from pbs

[Lukas Wagner <l.wagner@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