Re: [pbs-devel] [PATCH v3 proxmox-backup 2/5] fix #3847: datastore: test FixedIndexWriter

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

Quoting Robert Obkircher (2026-01-16 13:48:04)
> 
> On 1/14/26 14:13, Fabian Grünbichler wrote:
> 
>     On January 9, 2026 6:35 pm, Robert Obkircher wrote:
> 
>         Create a dummy chunk store and write fidx files with fixed and
>         dynamically sized writers. Compare the resulting binary files directly
>         (ignoring uuid and ctime) and also read them back using the reader.
> 
>         The chunk hashes are made up and don't actually exist in the chunk
>         store.
> 
>         Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>         ---
>          pbs-datastore/src/fixed_index.rs | 184 +++++++++++++++++++++++++++++++
>          1 file changed, 184 insertions(+)
> 
>         diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
>         index 8036a519..a20edc94 100644
>         --- a/pbs-datastore/src/fixed_index.rs
>         +++ b/pbs-datastore/src/fixed_index.rs
>         @@ -530,3 +530,187 @@ impl FixedIndexWriter {
>                  Ok(())
>              }
>          }
>         +
>         +#[cfg(test)]
>         +mod tests {
>         +    use super::*;
>         +    use crate::chunk_store::ChunkStore;
>         +    use pbs_api_types::DatastoreFSyncLevel;
>         +    use std::fs;
>         +    use std::sync::Arc;
>         +
>         +    const CS: usize = 4096;
>         +
>         +    #[test]
>         +    fn test_fixed_index_writer() {
> 
>     this should be a setup method, and not annotated as #[test]
> 
> 
>         +        let mut testdir = fs::canonicalize(".").unwrap();
> 
>     NACK, please use a tmpdir instead of writing to the current dir.. cargo
>     only provides a mechanism for this for integration tests, so for unit
>     tests you need to take care of it yourself, or move these to an
>     integration test..
> 
>     we have discussed dropping the chunk store entirely in the past:
> 
>     https://lore.proxmox.com/pbs-devel/1761822027.sm41v71zmk.astroid@yuna.none/
> 
>     maybe we should do that and make unit testing here more
>     straight-forward? would still require a writable path though..
> 
> Ok, I'll also send a separate patch for `chunk_store::test_chunk_store1`.
> 
> 
>         +        testdir.push(".testdir_fixed_index");
>         +
>         +        if let Err(_e) = std::fs::remove_dir_all(&testdir) { /* ignore */ }
>         +
>         +        // this is a lot faster than ChunkStore::create, which takes almost 3 seconds
>         +        let store = {
>         +            let mut chunks = PathBuf::from(&testdir);
>         +            chunks.push(".chunks");
>         +            fs::create_dir_all(chunks).unwrap();
>         +            Arc::new(ChunkStore::open("test", &testdir, DatastoreFSyncLevel::None).unwrap())
>         +        };
>         +
>         +        test_empty(store.clone());
>         +        test_smaller_than_initial_capacity(store.clone());
>         +        test_grow_to_multiples_of_chunk_size(store.clone());
>         +        test_grow_to_fixed_size(store);
> 
>     these should not be called here, but rather call the setup method
> 
> The only reason I didn't do that was because burning 3 seconds per test to 
> create 65k directories for each chunk store seemed a bit excessive. But if
> my workaround is ok, or if we remove it entirely, then separate tests are
> definitely better.

yes, I think the solution for that is getting rid of the ChunkStore reference
there, and just pass in the index path (base).

> 
>         +
>         +        std::fs::remove_dir_all(&testdir).unwrap();
> 
>     and this should be handled with some kind of auto-cleanup-on-drop
>     mechanism, unless the whole thing is moved to an integration test, in
>     which case it will be somewhere under `target/` anyway..
> 
> The idea was to preserve files for debugging in case of failure, but I guess
> in that case one can just patch the code and re-run the test.

leaving the files around in case of failure is less of an issue if they are in
some tmpdir somewhere, but it's bad to pollute the source dir/CWD (and in some
build environments, that dir might not even be writable).

> 
>         +    }
>         +
>         +    fn test_empty(store: Arc<ChunkStore>) {
>         +        let path = Path::new("test_empty");
>         +        let mut w = FixedIndexWriter::create(store, path, None, CS).unwrap();
>         +
>         +        assert_eq!(0, w.index_length(), "returns length, not capacity");
> 
>     should also check the capacity and size?
> 
> 
>         +        assert!(w.add_digest(0, &[1u8; 32]).is_err(), "out of bounds");
>         +        assert!(w.close().is_err(), "should refuse to create empty file");
>         +    }
>         +
>         +    fn test_smaller_than_initial_capacity(store: Arc<ChunkStore>) {
> 
>     the test_grow_to_multiples_of_chunk_size also tests sizes smaller than
>     the initial capacity.. this tests a very special edge case - an index
>     made up of a single, partial chunk..
> 
> 
>         +        let path = Path::new("test_smaller_than_initial_capacity");
>         +        let mut w = FixedIndexWriter::create(store.clone(), path, None, CS).unwrap();
>         +
>         +        let size = CS - 1;
>         +        let expected = test_data(size);
>         +        w.grow_to_size(size).unwrap();
>         +        expected[0].add_to(&mut w);
>         +
>         +        w.close().unwrap();
>         +        drop(w);
>         +
>         +        check_with_reader(&store.relative_path(path), size, &expected);
>         +        compare_to_known_size_writer(store, path, size, &expected);
>         +    }
>         +
>         +    fn test_grow_to_multiples_of_chunk_size(store: Arc<ChunkStore>) {
>         +        let path = Path::new("test_grow_to_multiples_of_chunk_size");
>         +        let mut w = FixedIndexWriter::create(store.clone(), path, None, CS).unwrap();
>         +
>         +        let initial = FixedIndexWriter::INITIAL_CAPACITY;
>         +        let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
>         +        let expected = test_data(steps.last().unwrap() * CS);
>         +
>         +        let mut begin = 0;
>         +        for chunk_count in steps {
>         +            let last = &expected[chunk_count - 1];
>         +            w.grow_to_size(last.end).unwrap();
>         +            assert_eq!(last.index + 1, w.index_length());
> 
>     if index_length is changed to capacity, then this needs to be adapted,
>     unless index_length() continues to return the length based on size and
>     chunk size
> 
> The function index_length is public and is also used by the backup environment.
> It must obviously return the length and not the internal capacity.

see comments on the first patch :)


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel