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

[Robert Obkircher <r.obkircher@proxmox.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9988 bytes --]


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.
>> +
>> +        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.
>> +    }
>> +
>> +    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.
>
>> +            assert!(w.add_digest(last.index + 1, &[1u8; 32]).is_err());
>> +
>> +            for c in expected[begin..chunk_count].iter().rev() {
>> +                c.add_to(&mut w);
>> +            }
>> +            begin = chunk_count;
>> +        }
>> +        w.close().unwrap();
>> +        drop(w);
>> +
>> +        let size = expected.len() * CS;
>> +        check_with_reader(&store.relative_path(path), size, &expected);
>> +        compare_to_known_size_writer(store, path, size, &expected);
>> +    }
>> +
>> +    fn test_grow_to_fixed_size(store: Arc<ChunkStore>) {
> s/fixed/misaligned/ ?
>
>> +        let path = Path::new("test_grow_to_fixed_size");
>> +        let mut w = FixedIndexWriter::create(store.clone(), path, None, CS).unwrap();
>> +
>> +        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
>> +        let expected = test_data(size);
>> +
>> +        w.grow_to_size(size).unwrap();
>> +        assert!(w.grow_to_size(size + 1).is_err(), "size must be fixed now");
>> +        assert_eq!(expected.len(), w.index_length());
>> +        assert!(w.add_digest(expected.len(), &[1u8; 32]).is_err());
>> +
>> +        for c in expected.iter().rev() {
>> +            c.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);
>> +    }
>> +
>> +    struct TestChunk {
>> +        digest: [u8; 32],
>> +        index: usize,
>> +        size: usize,
>> +        end: usize,
>> +    }
>> +
>> +    impl TestChunk {
>> +        fn add_to(&self, w: &mut FixedIndexWriter) {
>> +            assert_eq!(
>> +                self.index,
>> +                w.check_chunk_alignment(self.end, self.size).unwrap()
>> +            );
>> +            w.add_digest(self.index, &self.digest).unwrap();
>> +        }
>> +    }
>> +
>> +    fn test_data(size: usize) -> Vec<TestChunk> {
>> +        (0..size.div_ceil(CS))
>> +            .map(|index| {
>> +                let mut digest = [0u8; 32];
>> +                let i = &(index as u64).to_le_bytes();
>> +                for c in digest.chunks_mut(i.len()) {
>> +                    c.copy_from_slice(i);
>> +                }
>> +                let size = if ((index + 1) * CS) <= size {
>> +                    CS
>> +                } else {
>> +                    size % CS
>> +                };
>> +                TestChunk {
>> +                    digest,
>> +                    index,
>> +                    size,
>> +                    end: index * CS + size,
>> +                }
>> +            })
>> +            .collect()
>> +    }
>> +
>> +    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
>> +        let reader = FixedIndexReader::open(path).unwrap();
>> +        assert_eq!(size as u64, reader.index_bytes());
>> +        assert_eq!(chunks.len(), reader.index_count());
>> +        for c in chunks {
>> +            assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
>> +        }
>> +    }
>> +
>> +    fn compare_to_known_size_writer(
>> +        store: Arc<ChunkStore>,
>> +        name: &Path,
>> +        size: usize,
>> +        chunks: &[TestChunk],
>> +    ) {
>> +        let mut path = PathBuf::from(name);
>> +        path.set_extension("reference");
>> +        let mut w = FixedIndexWriter::create(store.clone(), &path, Some(size), CS).unwrap();
>> +        for c in chunks {
>> +            c.add_to(&mut w);
>> +        }
>> +        w.close().unwrap();
>> +        drop(w);
>> +
>> +        let mut reference = fs::read(store.relative_path(&path)).unwrap();
>> +        let mut tested = fs::read(store.relative_path(name)).unwrap();
>> +
>> +        // ignore uuid and ctime
>> +        reference[8..32].fill(0);
>> +        tested[8..32].fill(0);
>> +
>> +        assert_eq!(reference, tested);
>> +    }
>> +}
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>

[-- Attachment #1.2: Type: text/html, Size: 12404 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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