[pbs-devel] [PATCH proxmox-backup v2 1/3] rest-server: cleanup_old_tasks: improve error handling

[pbs-devel] [PATCH proxmox-backup v2 1/3] rest-server: cleanup_old_tasks: improve error handling

[Dominik Csapak]

by not bubbling up most errors, and continuing on. this avoids that we
stop cleaning up because e.g. one directory was missing.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* ignore only ENOENT error for task log archive dirs,
  log all other errors
* use task_log instead of log::errors, this way the task will end on a
  'WARNINGS' and the user sees that something was wrong with it
  this also has the advantage that the errors really go into the task
  log instead of the syslog
* improve error messages

 proxmox-rest-server/src/worker_task.rs | 38 ++++++++++++++++++++------
 src/bin/proxmox-backup-proxy.rs        |  2 +-
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index 643e1872..cd1a4457 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -16,6 +16,7 @@ use tokio::sync::oneshot;
 use nix::fcntl::OFlag;
 use once_cell::sync::OnceCell;
 
+use proxmox_sys::task_warn;
 use proxmox_sys::linux::procfs;
 use proxmox_sys::fs::{create_path, replace_file, atomic_open_or_create_file, CreateOptions};
 use proxmox_lang::try_block;
@@ -232,7 +233,7 @@ pub fn rotate_task_log_archive(
 
 /// removes all task logs that are older than the oldest task entry in the
 /// task archive
-pub fn cleanup_old_tasks(compressed: bool) -> Result<(), Error> {
+pub fn cleanup_old_tasks(worker: &dyn WorkerTaskContext, compressed: bool) -> Result<(), Error> {
     let setup = worker_task_setup()?;
 
     let _lock = setup.lock_task_list_files(true)?;
@@ -270,18 +271,37 @@ pub fn cleanup_old_tasks(compressed: bool) -> Result<(), Error> {
         for i in 0..256 {
             let mut path = setup.taskdir.clone();
             path.push(format!("{:02X}", i));
-            for file in std::fs::read_dir(path)? {
-                let file = file?;
+            let files = match std::fs::read_dir(path) {
+                Ok(files) => files,
+                Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue,
+                Err(err) => {
+                    task_warn!(worker, "could not check task logs in '{:02X}': {}", i, err);
+                    continue;
+                }
+            };
+            for file in files {
+                let file = match file {
+                    Ok(file) => file,
+                    Err(err) => {
+                        task_warn!(worker, "could not check some task log in '{:02X}': {}", i, err);
+                        continue;
+                    }
+                };
                 let path = file.path();
 
-                let modified = get_modified(file)
-                    .map_err(|err| format_err!("error getting mtime for {:?}: {}", path, err))?;
+                let modified = match get_modified(file) {
+                    Ok(modified) => modified,
+                    Err(err) => {
+                        task_warn!(worker, "error getting mtime for '{:?}': {}", path, err);
+                        continue;
+                    }
+                };
 
                 if modified < cutoff_time {
-                    match std::fs::remove_file(path) {
-                        Ok(()) => {},
-                        Err(err) if err.kind() == std::io::ErrorKind::NotFound => {},
-                        Err(err) => bail!("could not remove file: {}", err),
+                    match std::fs::remove_file(&path) {
+                        Ok(()) => {}
+                        Err(err) if err.kind() == std::io::ErrorKind::NotFound => {}
+                        Err(err) => task_warn!(worker, "could not remove file '{:?}': {}", path, err)
                     }
                 }
             }
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index a4f20af1..0d2271a8 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -900,7 +900,7 @@ async fn schedule_task_log_rotate() {
 
                 if has_rotated {
                     task_log!(worker, "cleaning up old task logs");
-                    if let Err(err) = cleanup_old_tasks(true) {
+                    if let Err(err) = cleanup_old_tasks(&worker, true) {
                         task_warn!(worker, "could not completely cleanup old tasks: {}", err);
                     }
                 }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 2/3] config/node: add 'task_log_max_days' config

[Dominik Csapak]

to be able to configure the maximum days to keep task logs

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* rebased on master
 src/api2/node/config.rs | 4 ++++
 src/config/node.rs      | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 3b267adc..113d7ed8 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -66,6 +66,8 @@ pub enum DeletableProperty {
     default_lang,
     /// Delete any description
     description,
+    /// Delete the task-log-max-days property
+    task_log_max_days,
 }
 
 #[api(
@@ -127,6 +129,7 @@ pub fn update_node_config(
                 DeletableProperty::ciphers_tls_1_2 => { config.ciphers_tls_1_2 = None; },
                 DeletableProperty::default_lang => { config.default_lang = None; },
                 DeletableProperty::description => { config.description = None; },
+                DeletableProperty::task_log_max_days => { config.task_log_max_days = None; },
             }
         }
     }
@@ -143,6 +146,7 @@ pub fn update_node_config(
     if update.ciphers_tls_1_2.is_some() { config.ciphers_tls_1_2 = update.ciphers_tls_1_2; }
     if update.default_lang.is_some() { config.default_lang = update.default_lang; }
     if update.description.is_some() { config.description = update.description; }
+    if update.task_log_max_days.is_some() { config.task_log_max_days = update.task_log_max_days; }
 
     crate::config::node::save_config(&config)?;
 
diff --git a/src/config/node.rs b/src/config/node.rs
index ac6774e3..07e88ee2 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -222,6 +222,10 @@ pub struct NodeConfig {
     /// Node description
     #[serde(skip_serializing_if = "Option::is_none")]
     pub description: Option<String>,
+
+    /// Maximum days to keep Task logs
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub task_log_max_days: Option<usize>,
 }
 
 impl NodeConfig {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 3/3] rest-server: add option to rotate task logs by 'max_days' instead of 'max_files'

[Dominik Csapak]

and use it with the configurable: 'task_log_max_days' of the node config

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-rest-server/src/worker_task.rs | 49 +++++++++++++++++++++++---
 src/bin/proxmox-backup-proxy.rs        |  6 ++++
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index cd1a4457..851f3c7c 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -209,14 +209,16 @@ pub fn init_worker_tasks(basedir: PathBuf, file_opts: CreateOptions) -> Result<(
 }
 
 /// checks if the Task Archive is bigger that 'size_threshold' bytes, and
-/// rotates it if it is
+/// rotates it if it is, keeps only up to 'max_files'.
+/// If 'max_days' is given, 'max_files' is ignored, and all archive files
+/// will be deleted where there are only tasks that are older than the given days.
 pub fn rotate_task_log_archive(
     size_threshold: u64,
     compress: bool,
     max_files: Option<usize>,
+    max_days: Option<usize>,
     options: Option<CreateOptions>,
 ) -> Result<bool, Error> {
-
     let setup = worker_task_setup()?;
 
     let _lock = setup.lock_task_list_files(true)?;
@@ -224,11 +226,50 @@ pub fn rotate_task_log_archive(
     let mut logrotate = LogRotate::new(
         &setup.task_archive_fn,
         compress,
-        max_files,
+        if max_days.is_none() { max_files } else { None },
         options,
     )?;
 
-    logrotate.rotate(size_threshold)
+    let mut rotated = logrotate.rotate(size_threshold)?;
+
+    if let Some(max_days) = max_days {
+        let mut delete = false;
+        let file_names = logrotate.file_names();
+        let mut files = logrotate.files();
+        for file_name in file_names {
+            if !delete {
+                // we only have to check if we did not start deleting already
+
+                // this is ok because the task log files are locked, so no one
+                // else should modify these
+                let reader = match files.next() {
+                    Some(file) => BufReader::new(file),
+                    None => {
+                        bail!("unexpected error: files do not match file_names");
+                    }
+                };
+                if let Some(line) = reader.lines().next() {
+                    if let Ok((_, _, Some(state))) = parse_worker_status_line(&line?) {
+                        // we approximate here with the days, but should be close enough
+                        let cutoff_time =
+                            proxmox_time::epoch_i64() - (max_days * 24 * 60 * 60) as i64;
+                        if state.endtime() < cutoff_time {
+                            // we found the first file that has only older entries, start deleting
+                            delete = true;
+                            rotated = true;
+                        }
+                    }
+                }
+            }
+            if delete {
+                if let Err(err) = std::fs::remove_file(&file_name) {
+                    log::error!("could not remove {:?}: {}", file_name, err);
+                }
+            }
+        }
+    }
+
+    Ok(rotated)
 }
 
 /// removes all task logs that are older than the oldest task entry in the
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 0d2271a8..f0cb29eb 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -847,6 +847,11 @@ async fn schedule_task_log_rotate() {
                 let max_size = 512 * 1024 - 1; // an entry has ~ 100b, so > 5000 entries/file
                 let max_files = 20; // times twenty files gives > 100000 task entries
 
+                let max_days = proxmox_backup::config::node::config()
+                    .map(|(cfg, _)| cfg.task_log_max_days)
+                    .ok()
+                    .flatten();
+
                 let user = pbs_config::backup_user()?;
                 let options = proxmox_sys::fs::CreateOptions::new()
                     .owner(user.uid)
@@ -856,6 +861,7 @@ async fn schedule_task_log_rotate() {
                     max_size,
                     true,
                     Some(max_files),
+                    max_days,
                     Some(options.clone()),
                 )?;
 
-- 
2.30.2

[pbs-devel] applied-series: [PATCH proxmox-backup v2 1/3] rest-server: cleanup_old_tasks: improve error handling

[Thomas Lamprecht]

On 28.03.22 09:54, Dominik Csapak wrote:
> by not bubbling up most errors, and continuing on. this avoids that we
> stop cleaning up because e.g. one directory was missing.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * ignore only ENOENT error for task log archive dirs,
>   log all other errors
> * use task_log instead of log::errors, this way the task will end on a
>   'WARNINGS' and the user sees that something was wrong with it
>   this also has the advantage that the errors really go into the task
>   log instead of the syslog
> * improve error messages
> 
>  proxmox-rest-server/src/worker_task.rs | 38 ++++++++++++++++++++------
>  src/bin/proxmox-backup-proxy.rs        |  2 +-
>  2 files changed, 30 insertions(+), 10 deletions(-)
> 
>

applied, with the merge conflict from a recent rustfmt resolved and the followup
discussed off-list to fix the off-by-one file deletion the series had, thanks!