[pbs-devel] applied: [PATCH backup 1/4] log rotate: do NOT overwrite file with possible writers

[pbs-devel] applied: [PATCH backup 1/4] log rotate: do NOT overwrite file with possible writers

[Thomas Lamprecht]

this is not the job of logrotate, and the real 20+ years battle
tested log rotate binary does not do so either as it's actually
pretty dangerous.

If we "replace" the file we break any logger which already opened a
new one here, e.g., a dameon starting up, and thus that writer would
log to nirvana.

It's the job of a logger to create a file if not existing, it makes
no sense to do it here.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/tools/logrotate.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/tools/logrotate.rs b/src/tools/logrotate.rs
index ce311fbe..f7d1a693 100644
--- a/src/tools/logrotate.rs
+++ b/src/tools/logrotate.rs
@@ -6,7 +6,7 @@ use std::io::Read;
 use anyhow::{bail, Error};
 use nix::unistd;
 
-use proxmox::tools::fs::{CreateOptions, make_tmp_file, replace_file};
+use proxmox::tools::fs::{CreateOptions, make_tmp_file};
 
 /// Used for rotating log files and iterating over them
 pub struct LogRotate {
@@ -108,8 +108,6 @@ impl LogRotate {
             rename(&filenames[0], &filenames[1])?;
         }
 
-        // create empty original file
-        replace_file(&filenames[0], b"", options)?;
 
         if let Some(max_files) = max_files {
             // delete all files > max_files
-- 
2.27.0

[pbs-devel] applied: [PATCH backup 2/4] log rotate: factor out compression in private function

[Thomas Lamprecht]

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/tools/logrotate.rs | 58 ++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/src/tools/logrotate.rs b/src/tools/logrotate.rs
index f7d1a693..28e3b7bb 100644
--- a/src/tools/logrotate.rs
+++ b/src/tools/logrotate.rs
@@ -46,6 +46,35 @@ impl LogRotate {
         }
     }
 
+    fn compress(file: &PathBuf, options: &CreateOptions) -> Result<(), Error> {
+        let mut source = File::open(file)?;
+        let (fd, tmp_path) = make_tmp_file(file, options.clone())?;
+        let target = unsafe { File::from_raw_fd(fd) };
+        let mut encoder = match zstd::stream::write::Encoder::new(target, 0) {
+            Ok(encoder) => encoder,
+            Err(err) => {
+                let _ = unistd::unlink(&tmp_path);
+                bail!("creating zstd encoder failed - {}", err);
+            }
+        };
+
+        if let Err(err) = std::io::copy(&mut source, &mut encoder) {
+            let _ = unistd::unlink(&tmp_path);
+            bail!("zstd encoding failed for file {:?} - {}", file, err);
+        }
+
+        if let Err(err) = encoder.finish() {
+            let _ = unistd::unlink(&tmp_path);
+            bail!("zstd finish failed for file {:?} - {}", file, err);
+        }
+
+        if let Err(err) = rename(&tmp_path, file) {
+            let _ = unistd::unlink(&tmp_path);
+            bail!("rename failed for file {:?} - {}", file, err);
+        }
+        Ok(())
+    }
+
     /// Rotates the files up to 'max_files'
     /// if the 'compress' option was given it will compress the newest file
     ///
@@ -77,38 +106,13 @@ impl LogRotate {
         }
 
         if self.compress {
-            let mut source = File::open(&filenames[0])?;
-            let (fd, tmp_path) = make_tmp_file(&filenames[1], options.clone())?;
-            let target = unsafe { File::from_raw_fd(fd) };
-            let mut encoder = match zstd::stream::write::Encoder::new(target, 0) {
-                Ok(encoder) => encoder,
-                Err(err) => {
-                    let _ = unistd::unlink(&tmp_path);
-                    bail!("creating zstd encoder failed - {}", err);
-                }
-            };
-
-            if let Err(err) = std::io::copy(&mut source, &mut encoder) {
-                let _ = unistd::unlink(&tmp_path);
-                bail!("zstd encoding failed for file {:?} - {}", &filenames[1], err);
+            if filenames[0].extension().unwrap_or(std::ffi::OsStr::new("")) != "zst" {
+                Self::compress(&filenames[0], &options)?;
             }
-
-            if let Err(err) = encoder.finish() {
-                let _ = unistd::unlink(&tmp_path);
-                bail!("zstd finish failed for file {:?} - {}", &filenames[1], err);
-            }
-
-            if let Err(err) = rename(&tmp_path, &filenames[1]) {
-                let _ = unistd::unlink(&tmp_path);
-                bail!("rename failed for file {:?} - {}", &filenames[1], err);
-            }
-
-            unistd::unlink(&filenames[0])?;
         } else {
             rename(&filenames[0], &filenames[1])?;
         }
 
-
         if let Some(max_files) = max_files {
             // delete all files > max_files
             for file in filenames.iter().skip(max_files) {
-- 
2.27.0

[pbs-devel] applied: [PATCH backup 3/4] log rotate: do NOT compress first rotation

[Thomas Lamprecht]

The first rotation is normally the one still opened by one or more
processes for writing, so it must NOT be replaced, removed, ..., as
this then makes the remaining logging, until those processes are
noticed that they should reopen the logfile due to rotation, goes
into nirvana, which is far from ideal for a log.

Only rotating (renaming) is OK for this active file, as this does not
invalidates the file and keeps open FDs intact.

So start compressing with the second rotation, which should be clear
to use, as all writers must have been told to reopen the log during
the last rotation, reopen is a fast operation and normally triggered
at least day ago (at least if one did not dropped the state file
manually), so we are fine to archive that one for real.
If we plan to allow faster rotation the whole rotation+reopen should
be locked, so that we can guarantee that all writers switched over,
but this is unlikely to be needed.

Again, this is was logrotate sanely does by default since forever.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/tools/logrotate.rs | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/tools/logrotate.rs b/src/tools/logrotate.rs
index 28e3b7bb..54fe4bfe 100644
--- a/src/tools/logrotate.rs
+++ b/src/tools/logrotate.rs
@@ -100,17 +100,16 @@ impl LogRotate {
         filenames.push(PathBuf::from(next_filename));
         let count = filenames.len();
 
-        // rotate all but the first, that we maybe have to compress
-        for i in (1..count-1).rev() {
+        for i in (0..count-1).rev() {
             rename(&filenames[i], &filenames[i+1])?;
         }
 
         if self.compress {
-            if filenames[0].extension().unwrap_or(std::ffi::OsStr::new("")) != "zst" {
-                Self::compress(&filenames[0], &options)?;
+            for i in 2..count-1 {
+                if filenames[i].extension().unwrap_or(std::ffi::OsStr::new("")) != "zst" {
+                    Self::compress(&filenames[i], &options)?;
+                }
             }
-        } else {
-            rename(&filenames[0], &filenames[1])?;
         }
 
         if let Some(max_files) = max_files {
-- 
2.27.0

[pbs-devel] applied: [PATCH backup 4/4] log rotate: move basic rotation logic into module for reuse

[Thomas Lamprecht]

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/server/worker_task.rs | 24 ++++--------------------
 src/tools/logrotate.rs    | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/src/server/worker_task.rs b/src/server/worker_task.rs
index ab8e2bf6..8ef0fde7 100644
--- a/src/server/worker_task.rs
+++ b/src/server/worker_task.rs
@@ -1,6 +1,5 @@
 use std::collections::{HashMap, VecDeque};
 use std::fs::File;
-use std::path::Path;
 use std::io::{Read, Write, BufRead, BufReader};
 use std::panic::UnwindSafe;
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -349,26 +348,11 @@ fn lock_task_list_files(exclusive: bool) -> Result<std::fs::File, Error> {
 /// rotates it if it is
 pub fn rotate_task_log_archive(size_threshold: u64, compress: bool, max_files: Option<usize>) -> Result<bool, Error> {
     let _lock = lock_task_list_files(true)?;
-    let path = Path::new(PROXMOX_BACKUP_ARCHIVE_TASK_FN);
-    let metadata = match path.metadata() {
-        Ok(metadata) => metadata,
-        Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false),
-        Err(err) =>  bail!("unable to open task archive - {}", err),
-    };
 
-    if metadata.len() > size_threshold {
-        let mut logrotate = LogRotate::new(PROXMOX_BACKUP_ARCHIVE_TASK_FN, compress).ok_or_else(|| format_err!("could not get archive file names"))?;
-        let backup_user = crate::backup::backup_user()?;
-        logrotate.rotate(
-            CreateOptions::new()
-                .owner(backup_user.uid)
-                .group(backup_user.gid),
-            max_files,
-        )?;
-        Ok(true)
-    } else {
-        Ok(false)
-    }
+    let mut logrotate = LogRotate::new(PROXMOX_BACKUP_ARCHIVE_TASK_FN, compress)
+        .ok_or(format_err!("could not get archive file names"))?;
+
+    logrotate.rotate(size_threshold, None, max_files)
 }
 
 // atomically read/update the task list, update status of finished tasks
diff --git a/src/tools/logrotate.rs b/src/tools/logrotate.rs
index 54fe4bfe..dc703922 100644
--- a/src/tools/logrotate.rs
+++ b/src/tools/logrotate.rs
@@ -83,7 +83,7 @@ impl LogRotate {
     /// foo.1.zst => foo.2.zst
     /// foo       => foo.1.zst
     ///           => foo
-    pub fn rotate(&mut self, options: CreateOptions, max_files: Option<usize>) -> Result<(), Error> {
+    pub fn do_rotate(&mut self, options: CreateOptions, max_files: Option<usize>) -> Result<(), Error> {
         let mut filenames: Vec<PathBuf> = self.file_names().collect();
         if filenames.is_empty() {
             return Ok(()); // no file means nothing to rotate
@@ -123,6 +123,35 @@ impl LogRotate {
 
         Ok(())
     }
+
+    pub fn rotate(
+        &mut self,
+        max_size: u64,
+        options: Option<CreateOptions>,
+        max_files: Option<usize>
+    ) -> Result<bool, Error> {
+
+        let options = match options {
+            Some(options) => options,
+            None => {
+                let backup_user = crate::backup::backup_user()?;
+                CreateOptions::new().owner(backup_user.uid).group(backup_user.gid)
+            },
+        };
+
+        let metadata = match self.base_path.metadata() {
+            Ok(metadata) => metadata,
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false),
+            Err(err) =>  bail!("unable to open task archive - {}", err),
+        };
+
+        if metadata.len() > max_size {
+            self.do_rotate(options, max_files)?;
+            Ok(true)
+        } else {
+            Ok(false)
+        }
+    }
 }
 
 /// Iterator over logrotated file names
-- 
2.27.0