[pbs-devel] [PATCH proxmox-backup 0/5] various bug fixes/improvements for gc

[pbs-devel] [PATCH proxmox-backup 0/5] various bug fixes/improvements for gc

[Dominik Csapak]

these are some improvements/fixes for the garbage_collection

Dominik Csapak (5):
  garbage collect: improve index error messages
  backup/{dynamic,fixed}_index: improve error message for small index
    files
  server/gc_job: add 'to_stdout'
  api2/admin/datastore: start the garbage_collection task with our
    helper
  proxmox-backup-proxy: use only jobstate for garbage_collection
    schedule

 src/api2/admin/datastore.rs     | 17 +++++------------
 src/backup/datastore.rs         | 22 +++++++++++++++++++---
 src/backup/dynamic_index.rs     | 20 +++++++++++++-------
 src/backup/fixed_index.rs       | 19 +++++++++++++------
 src/bin/proxmox-backup-proxy.rs | 23 ++++++-----------------
 src/server/gc_job.rs            |  3 ++-
 6 files changed, 58 insertions(+), 46 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/5] garbage collect: improve index error messages

[Dominik Csapak]

so that in case of a broken index file, the user knows which it is

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/backup/datastore.rs | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 94d0cc04..36ff32c8 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -475,10 +475,22 @@ impl DataStore {
                 Ok(file) => {
                     if let Ok(archive_type) = archive_type(&path) {
                         if archive_type == ArchiveType::FixedIndex {
-                            let index = FixedIndexReader::new(file)?;
+                            let index = FixedIndexReader::new(file).map_err(|err| {
+                                    format_err!(
+                                        "cannot read fixed index {}: {}",
+                                        full_path.to_string_lossy(),
+                                        err
+                                    )
+                            })?;
                             self.index_mark_used_chunks(index, &path, status, worker)?;
                         } else if archive_type == ArchiveType::DynamicIndex {
-                            let index = DynamicIndexReader::new(file)?;
+                            let index = DynamicIndexReader::new(file).map_err(|err| {
+                                    format_err!(
+                                        "cannot read dynamic index {}: {}",
+                                        full_path.to_string_lossy(),
+                                        err
+                                    )
+                            })?;
                             self.index_mark_used_chunks(index, &path, status, worker)?;
                         }
                     }
@@ -487,7 +499,11 @@ impl DataStore {
                     if err.kind() == std::io::ErrorKind::NotFound {
                         // simply ignore vanished files
                     } else {
-                        return Err(err.into());
+                        return Err(format_err!(
+                            "cannot open index {}: {}",
+                            full_path.to_string_lossy(),
+                            err
+                        ));
                     }
                 }
             }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/5] backup/{dynamic, fixed}_index: improve error message for small index files

[Dominik Csapak]

index files that were smaller than their respective header size,
would fail with

"failed to fill whole buffer"

instead now check explicitely for the size and fail with
"index too small (size)"

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/backup/dynamic_index.rs | 20 +++++++++++++-------
 src/backup/fixed_index.rs   | 19 +++++++++++++------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/backup/dynamic_index.rs b/src/backup/dynamic_index.rs
index 8731a418..08f9cdff 100644
--- a/src/backup/dynamic_index.rs
+++ b/src/backup/dynamic_index.rs
@@ -95,6 +95,18 @@ impl DynamicIndexReader {
 
         let header_size = std::mem::size_of::<DynamicIndexHeader>();
 
+        let rawfd = file.as_raw_fd();
+        let stat = match nix::sys::stat::fstat(rawfd) {
+            Ok(stat) => stat,
+            Err(err) => bail!("fstat failed - {}", err),
+        };
+
+        let size = stat.st_size as usize;
+
+        if size < header_size {
+            bail!("index too small ({})", stat.st_size);
+        }
+
         let header: Box<DynamicIndexHeader> = unsafe { file.read_host_value_boxed()? };
 
         if header.magic != super::DYNAMIC_SIZED_CHUNK_INDEX_1_0 {
@@ -103,13 +115,7 @@ impl DynamicIndexReader {
 
         let ctime = proxmox::tools::time::epoch_i64();
 
-        let rawfd = file.as_raw_fd();
-
-        let stat = nix::sys::stat::fstat(rawfd)?;
-
-        let size = stat.st_size as usize;
-
-        let index_size = size - header_size;
+        let index_size = stat.st_size as usize - header_size;
         let index_count = index_size / 40;
         if index_count * 40 != index_size {
             bail!("got unexpected file size");
diff --git a/src/backup/fixed_index.rs b/src/backup/fixed_index.rs
index eff50055..44ebfabe 100644
--- a/src/backup/fixed_index.rs
+++ b/src/backup/fixed_index.rs
@@ -68,6 +68,19 @@ impl FixedIndexReader {
         file.seek(SeekFrom::Start(0))?;
 
         let header_size = std::mem::size_of::<FixedIndexHeader>();
+
+        let rawfd = file.as_raw_fd();
+        let stat = match nix::sys::stat::fstat(rawfd) {
+            Ok(stat) => stat,
+            Err(err) => bail!("fstat failed - {}", err),
+        };
+
+        let size = stat.st_size as usize;
+
+        if size < header_size {
+            bail!("index too small ({})", stat.st_size);
+        }
+
         let header: Box<FixedIndexHeader> = unsafe { file.read_host_value_boxed()? };
 
         if header.magic != super::FIXED_SIZED_CHUNK_INDEX_1_0 {
@@ -81,12 +94,6 @@ impl FixedIndexReader {
         let index_length = ((size + chunk_size - 1) / chunk_size) as usize;
         let index_size = index_length * 32;
 
-        let rawfd = file.as_raw_fd();
-
-        let stat = match nix::sys::stat::fstat(rawfd) {
-            Ok(stat) => stat,
-            Err(err) => bail!("fstat failed - {}", err),
-        };
 
         let expected_index_size = (stat.st_size as usize) - header_size;
         if index_size != expected_index_size {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/5] server/gc_job: add 'to_stdout'

[Dominik Csapak]

we will use this for the manual api call

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

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 39254504..8d28aecb 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -329,7 +329,7 @@ async fn schedule_datastore_garbage_collection() {
 
         let auth_id = Authid::backup_auth_id();
 
-        if let Err(err) = crate::server::do_garbage_collection_job(job, datastore, auth_id, Some(event_str)) {
+        if let Err(err) = crate::server::do_garbage_collection_job(job, datastore, auth_id, Some(event_str), false) {
             eprintln!("unable to start garbage collection job on datastore {} - {}", store, err);
         }
     }
diff --git a/src/server/gc_job.rs b/src/server/gc_job.rs
index dabbb237..0128a33e 100644
--- a/src/server/gc_job.rs
+++ b/src/server/gc_job.rs
@@ -14,6 +14,7 @@ pub fn do_garbage_collection_job(
     datastore: Arc<DataStore>,
     auth_id: &Authid,
     schedule: Option<String>,
+    to_stdout: bool,
 ) -> Result<String, Error> {
 
     let email = crate::server::lookup_user_email(auth_id.user());
@@ -25,7 +26,7 @@ pub fn do_garbage_collection_job(
         &worker_type,
         Some(store.clone()),
         auth_id.clone(),
-        false,
+        to_stdout,
         move |worker| {
             job.start(&worker.upid().to_string())?;
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/5] api2/admin/datastore: start the garbage_collection task with our helper

[Dominik Csapak]

instead of manually, this has the advantage that we now set
the jobstate correctly and can return with an error if it is
currently running (instead of failing in the task)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/admin/datastore.rs | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 220f06ae..a5d3e979 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -29,7 +29,7 @@ use crate::backup::*;
 use crate::config::datastore;
 use crate::config::cached_user_info::CachedUserInfo;
 
-use crate::server::WorkerTask;
+use crate::server::{jobstate::Job, WorkerTask};
 use crate::tools::{
     self,
     zip::{ZipEncoder, ZipEntry},
@@ -856,20 +856,13 @@ fn start_garbage_collection(
     let datastore = DataStore::lookup_datastore(&store)?;
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    println!("Starting garbage collection on store {}", store);
+    let job =  Job::new("garbage_collection", &store)
+        .map_err(|_| format_err!("garbage collection already running"))?;
 
     let to_stdout = if rpcenv.env_type() == RpcEnvironmentType::CLI { true } else { false };
 
-    let upid_str = WorkerTask::new_thread(
-        "garbage_collection",
-        Some(store.clone()),
-        auth_id.clone(),
-        to_stdout,
-        move |worker| {
-            worker.log(format!("starting garbage collection on store {}", store));
-            datastore.garbage_collection(&*worker, worker.upid())
-        },
-    )?;
+    let upid_str = crate::server::do_garbage_collection_job(job, datastore, &auth_id, None, to_stdout)
+        .map_err(|err| format_err!("unable to start garbage collection job on datastore {} - {}", store, err))?;
 
     Ok(json!(upid_str))
 }
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 4/5] api2/admin/datastore: start the garbage_collection task with our helper

[Dietmar Maurer]

> instead of manually, this has the advantage that we now set
> the jobstate correctly and can return with an error if it is
> currently running (instead of failing in the task)

We always send notifications now, even for manually triggered task.

So the behavior is finally inconsistent, i.e. we do not send notifications for
manually triggered verify. Maybe we should change that too?

Re: [pbs-devel] [PATCH proxmox-backup 4/5] api2/admin/datastore: start the garbage_collection task with our helper

[Dominik Csapak]

On 11/3/20 6:45 AM, Dietmar Maurer wrote:
>> instead of manually, this has the advantage that we now set
>> the jobstate correctly and can return with an error if it is
>> currently running (instead of failing in the task)
> 
> We always send notifications now, even for manually triggered task.
> 
> So the behavior is finally inconsistent, i.e. we do not send notifications for
> manually triggered verify. Maybe we should change that too?
> 

ok well that was not really my intention, so we could simply add an 
option to do_garbage_collection_job ?

or as you said add them to manual verify too, im ok with both..

[pbs-devel] [PATCH proxmox-backup 5/5] proxmox-backup-proxy: use only jobstate for garbage_collection schedule

[Dominik Csapak]

in case the garbage_collection errors out, we never set the in-memory
state, so if it failed, the last 'good' starttime was considered
for the schedule

this could lead to the job running every minute instead of the
correct schedule

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/bin/proxmox-backup-proxy.rs | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 8d28aecb..bf121213 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -290,22 +290,11 @@ async fn schedule_datastore_garbage_collection() {
 
         let worker_type = "garbage_collection";
 
-        let stat = datastore.last_gc_status();
-        let last = if let Some(upid_str) = stat.upid {
-            match upid_str.parse::<UPID>() {
-                Ok(upid) => upid.starttime,
-                Err(err) => {
-                    eprintln!("unable to parse upid '{}' - {}", upid_str, err);
-                    continue;
-                }
-            }
-        } else {
-            match jobstate::last_run_time(worker_type, &store) {
-                Ok(time) => time,
-                Err(err) => {
-                    eprintln!("could not get last run time of {} {}: {}", worker_type, store, err);
-                    continue;
-                }
+        let last = match jobstate::last_run_time(worker_type, &store) {
+            Ok(time) => time,
+            Err(err) => {
+                eprintln!("could not get last run time of {} {}: {}", worker_type, store, err);
+                continue;
             }
         };
 
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup 0/5] various bug fixes/improvements for gc

[Thomas Lamprecht]

On 02.11.20 12:34, Dominik Csapak wrote:
> these are some improvements/fixes for the garbage_collection
> 
> Dominik Csapak (5):
>   garbage collect: improve index error messages
>   backup/{dynamic,fixed}_index: improve error message for small index
>     files
>   server/gc_job: add 'to_stdout'
>   api2/admin/datastore: start the garbage_collection task with our
>     helper
>   proxmox-backup-proxy: use only jobstate for garbage_collection
>     schedule
> 
>  src/api2/admin/datastore.rs     | 17 +++++------------
>  src/backup/datastore.rs         | 22 +++++++++++++++++++---
>  src/backup/dynamic_index.rs     | 20 +++++++++++++-------
>  src/backup/fixed_index.rs       | 19 +++++++++++++------
>  src/bin/proxmox-backup-proxy.rs | 23 ++++++-----------------
>  src/server/gc_job.rs            |  3 ++-
>  6 files changed, 58 insertions(+), 46 deletions(-)
> 



applied series, thanks!

Die two followups.
1. due to a patch of mine the UPID import in proxmox-backup-proxy
   was now unused, dropped that
2. This:

return Err(format_err!(
    "cannot open index {}: {}",
    full_path.to_string_lossy(),
    err,
));

is:

bail!("can't open index {} - {}", path.to_string_lossy(), err),


On another note, I had the feeling that we did not sent out mails for manually
triggered GCs before this change, not sure if I'm just imagine or if you changed
that, possibly unwillingly?

Re: [pbs-devel] applied: [PATCH proxmox-backup 0/5] various bug fixes/improvements for gc

[Dietmar Maurer]

> return Err(format_err!(
>     "cannot open index {}: {}",
>     full_path.to_string_lossy(),
>     err,
> ));
> 
> is:
> 
> bail!("can't open index {} - {}", path.to_string_lossy(), err),

btw, I normally use the following instead:

bail!("can't open index {:?} - {}", path, err),

Re: [pbs-devel] applied: [PATCH proxmox-backup 0/5] various bug fixes/improvements for gc

[Dominik Csapak]

On 11/3/20 6:11 AM, Dietmar Maurer wrote:
>> return Err(format_err!(
>>      "cannot open index {}: {}",
>>      full_path.to_string_lossy(),
>>      err,
>> ));
>>
>> is:
>>
>> bail!("can't open index {} - {}", path.to_string_lossy(), err),
> 
> btw, I normally use the following instead:
> 
> bail!("can't open index {:?} - {}", path, err),
> 

yes that is better... (i somehow thought that would print the path as
byte array, idk why i thought that)