[pbs-devel] [PATCH proxmox{, -backup} v8 0/4] proxmox-log introduction
Max Carrara
m.carrara at proxmox.com
Wed Jul 10 12:59:03 CEST 2024
On Tue Jul 9, 2024 at 4:20 PM CEST, Gabriel Goller wrote:
> Removed the task_log! (and friends task_warn!, task_debug!, etc.) macro
> and introduced the `tracing` crate. We now initiate the tracing crate
> using two layer, which are logging to the syslog and the tasklog.
> It uses the `tracing-journald` crate and the original `FileLogger`.
>
> To write to the task logs from the worker threads and tasks, we now
> have a task_local logger (and warning counter), which
> will get instantiated when a task/thread is created. This means that
> when we call `info!` or any other `tracing` log macros, it will get
> the file_logger from TLS and write to the file.
Building & Testing
------------------
* patches applied fine on the latest master branches of both proxmox and
proxmox-backup
* rebuilt `proxmox-backup` and installed the new binaries on my PBS test VM
* for a quick smoke test, ran a couple of tasks:
- GC
- sync (local)
- backup VM from PVE
Nothing out of the ordinary here; the logs work and look just like
before, except that under the hood `proxmox-log` was being used.
Pretty neat.
Code Review
-----------
Consider this "proofreading" again, as most things appear to have been
addressed in previous versions of this series.
Patches 01 and 02 are rather straightforward, the code is easy to
follow, even though the whole `task_log!` thing from tokio is something
I'm personally not too familiar with. After reading a little into it, it
makes total sense, especially after I saw that every worker gets their
own `LOGGER` and `WARN_COUNTER` (as you had also pointed out off-list).
So, all is fine here.
Patches 03 and 04 are quite sizeable, but nevertheless I think that's
totally fine in this context, as you're essentially replacing all
`task_log!` and related invocations. It's very nice to see a lot of
functions and coroutines being slimmed down that way; logging seems much
more convenient now.
I haven't caught any replacement that looks off; even though there are a
lot of changes in patch 03 & 04 in total, they're fairly simple.
Also, your code is formatted with `cargo fmt` and `cargo clippy` doesn't
complain either; very nice.
Conclusion
----------
The series passed a decent smoke test and I haven't found anything to
complain about in your code.
LGTM.
Tested-by: Max Carrara <m.carrara at proxmox.com>
Reviewed-by: Max Carrara <m.carrara at proxmox.com>
>
> v8, thanks @Wolfgang:
> - remove unnecessary filter on TasklogLayer
> we don't need to check the TLS in the filter **and** in `on_event`, one
> time is enough
> - remove `flog` macro from `FileLogger`
> - move string building in TasklogLayer `on_event` into TLS closure
> - rebase
>
> v7, thanks @Lukas:
> - run cargofmt
>
> v6, thanks @Lukas:
> - rebase
> - reorder imports, inline variables
> - introduce `tracing-journald` and throw out `syslog`
> - split single layer into two, one for syslog and for tasklog
>
> v5:
> - minor rebase
>
> v4:
> - rebase
> - reword commit messages (thanks @Thomas)
> - split into multiple patches
>
> v3, thanks @Sterzy:
> - updated debian/control files
> - downgraded tracing-log
>
> v2:
> - Rebase onto master
> - Split proxmox-backup commit into two
>
> v1, thanks @Wolfgang, @Lukas:
> - Combined syslog and tasklog to single layer
> - Infer the logging target from the FileLogger TLS
>
> RFC v2, thanks @Dominik, @Thomas:
> - Remove the 'tasklog = true' attribute and infer the context
> - Wrap the worker_thread or worker_task in a span with name
> 'worker_task'
> - All events in the span with name 'worker_task' get logged to the
> file_logger, everything else goes to syslog (Error events go to
> both)
> - Remove the `Option<>` around the `FileLogger` in TLS
> - Clippy fixes
>
> proxmox:
>
> Gabriel Goller (2):
> proxmox-log: add tracing infrastructure
> enable tracing logger, remove task_log macros
>
> Cargo.toml | 6 ++
> proxmox-log/Cargo.toml | 23 +++++
> proxmox-log/debian/changelog | 5 +
> proxmox-log/debian/control | 52 +++++++++++
> proxmox-log/debian/copyright | 18 ++++
> proxmox-log/debian/debcargo.toml | 7 ++
> .../src/file_logger.rs | 34 +++----
> proxmox-log/src/lib.rs | 48 ++++++++++
> proxmox-log/src/tasklog_layer.rs | 58 ++++++++++++
> proxmox-rest-server/Cargo.toml | 2 +
> proxmox-rest-server/src/api_config.rs | 3 +-
> proxmox-rest-server/src/lib.rs | 3 -
> proxmox-rest-server/src/rest.rs | 4 +-
> proxmox-rest-server/src/worker_task.rs | 93 ++++++++-----------
> proxmox-sys/src/worker_task_context.rs | 47 ----------
> 15 files changed, 278 insertions(+), 125 deletions(-)
> create mode 100644 proxmox-log/Cargo.toml
> create mode 100644 proxmox-log/debian/changelog
> create mode 100644 proxmox-log/debian/control
> create mode 100644 proxmox-log/debian/copyright
> create mode 100644 proxmox-log/debian/debcargo.toml
> rename {proxmox-rest-server => proxmox-log}/src/file_logger.rs (83%)
> create mode 100644 proxmox-log/src/lib.rs
> create mode 100644 proxmox-log/src/tasklog_layer.rs
>
>
> proxmox-backup:
>
> Gabriel Goller (2):
> switch from task_log! macro to tracing
> api: switch from task_log! macro to tracing
>
> Cargo.toml | 7 +
> debian/control | 2 +
> pbs-datastore/Cargo.toml | 1 +
> pbs-datastore/src/chunk_store.rs | 30 +---
> pbs-datastore/src/datastore.rs | 82 ++++------
> src/api2/admin/datastore.rs | 26 ++--
> src/api2/config/acme.rs | 21 +--
> src/api2/config/datastore.rs | 16 +-
> src/api2/config/prune.rs | 14 +-
> src/api2/node/apt.rs | 1 +
> src/api2/node/certificates.rs | 67 ++++----
> src/api2/node/disks/directory.rs | 13 +-
> src/api2/node/disks/mod.rs | 12 +-
> src/api2/node/disks/zfs.rs | 31 ++--
> src/api2/node/mod.rs | 11 +-
> src/api2/pull.rs | 37 ++---
> src/api2/tape/backup.rs | 75 ++++-----
> src/api2/tape/drive.rs | 152 +++++++-----------
> src/api2/tape/restore.rs | 259 ++++++++++---------------------
> src/backup/verify.rs | 105 ++++---------
> src/bin/proxmox-backup-api.rs | 13 +-
> src/bin/proxmox-backup-proxy.rs | 42 ++---
> src/server/gc_job.rs | 6 +-
> src/server/prune_job.rs | 29 ++--
> src/server/pull.rs | 243 +++++++++--------------------
> src/server/realm_sync_job.rs | 44 ++----
> src/server/verify_job.rs | 10 +-
> src/tape/drive/mod.rs | 36 ++---
> src/tape/pool_writer/mod.rs | 92 ++++-------
> src/tools/disks/mod.rs | 21 +--
> tests/worker-task-abort.rs | 9 +-
> 31 files changed, 514 insertions(+), 993 deletions(-)
>
>
> Summary over all repositories:
> 46 files changed, 792 insertions(+), 1118 deletions(-)
More information about the pbs-devel
mailing list