Gc tests #1585

rklaehn · 2023-10-06T09:01:29Z

Description

Add a number of basic tests as well as a stress test to gc

Questions

I had some consistent windows failures on Friday. But now it seems to work on windows. Not sure what is going on - was windows CI just broken on Friday?

Missing (this or next PR):

test that docs are protected
actually change gc default to enabled

Change checklist

Self-review.
~~Documentation updates if relevant.~~
Tests if relevant.

first batch is basic smoke tests

this adds 1000 files via download while gc is running at high frequency. 1/100 of these files are tagged, the others not. the tagged files should survive, the untagged files should be gone.

# Conflicts: # iroh/src/node.rs

rklaehn · 2023-10-09T12:12:09Z

iroh-bytes/src/baomap.rs

@@ -251,7 +251,7 @@ pub trait Store: ReadableStore + PartialMap {
    ///
    /// Sweeping might take long, but it can safely be done in the background.
    fn gc_sweep(&self) -> LocalBoxStream<'_, GcSweepEvent> {
-        let blobs = self.blobs();
+        let blobs = self.blobs().chain(self.partial_blobs());


this was an actual bug. I was not considering partial blogs for cleanup.

rklaehn · 2023-10-09T12:14:53Z

iroh-bytes/src/util.rs

@@ -124,6 +130,18 @@ pub enum SetTagOption {
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
 pub struct HashAndFormat(pub Hash, pub BlobFormat);

+impl HashAndFormat {
+    /// Create a new hash and format pair, using the default (raw) format.
+    pub fn raw(hash: Hash) -> Self {


just for convenience

rklaehn · 2023-10-09T12:15:24Z

iroh/src/baomap/flat.rs

@@ -895,42 +895,65 @@ impl Store {
        Ok((tag, size))
    }

+    fn import_bytes_sync(&self, data: Bytes, format: BlobFormat) -> io::Result<TempTag> {


Use finalize_import_sync from import_bytes_sync for DRY

rklaehn · 2023-10-09T12:16:59Z

iroh/src/baomap/flat.rs

+        // all writes here are protected by the temp tag
+        let complete_io_guard = self.0.complete_io_mutex.lock().unwrap();
+        // move the data file into place, or create a reference to it
+        let new = match file {


This was a bug before. You must pass through the format.

if somebody adds a hashseq using import_stream or import_reader, the resulting temp tag must still be with format HashSeq.

rklaehn · 2023-10-09T12:55:33Z

iroh/src/baomap/flat.rs

            let outboard_path = self.owned_outboard_path(&hash);
-            std::fs::write(outboard_path, outboard)?;
+            std::fs::rename(temp_outboard_path, outboard_path)?;


this was a bug before. You must never write to the final outboard file. It can only come into existence by an atomic mv. It would have been really hard to observe this though...

dignifiedquire · 2023-10-09T20:23:31Z

iroh/tests/gc.rs

@@ -0,0 +1,440 @@
+#![cfg(all(feature = "mem-db", feature = "iroh-collection"))]


What about also running the tests with the fs db? Would probably be good to make sure deletions there work similarly good

The "gc_flat_stress" and "gc_flat_partial" tests are using the flat file db.

The idea was to use a "perfect" db in the smoke tests to see if the gc_loop / driver is implemented correctly, and then use db specific tests like gc_flat_xxx to make sure that the db actually works.

I guess I can make the tests generic.

dignifiedquire · 2023-10-09T20:25:13Z

iroh/tests/gc.rs

+}
+
+async fn step() {
+    tokio::time::sleep(Duration::from_millis(100)).await;


why not use tokios timer https://docs.rs/tokio/latest/tokio/time/fn.pause.html#auto-advance testing functionality, this actually worked quite well for me when testing other timer based behaviour?

I hate it.

It depends on the notion that every single place in your code base and your dependencies that deals with time is using the tokio stuff, otherwise it will break in weird ways. Have a single place in your own code or your library code where you compute differences between SystemTime, and it is going to fail. But not outright fail. Fail in a subtle way, or succeed where it should not succeed.

It's another of these attempts of the tokio people to make tokio the default stdlib of rust. Is Lennart Poettering of systemd a secret tokio contributor?

I had thought about having an event stream from the db that you could use to detect when a GC has been performed instead of a fixed delay. I did not do it bc I thought that would lock in the fact that we are using a certain GC method (mark and sweep). But I would prefer to do this over using that tokio stuff.

Is Lennart Poettering of systemd a secret tokio contributor?

🤣

# Conflicts: # iroh-bytes/src/util.rs

Windows does not allow you to delete files when they are open. You don't get an error, deletion via remove_file just does nothing.

rklaehn added 5 commits October 5, 2023 15:59

tests(iroh-bytes): add tests for gc

5a7c3bc

first batch is basic smoke tests

add a minimal stress test for the gc.

80b8225

this adds 1000 files via download while gc is running at high frequency. 1/100 of these files are tagged, the others not. the tagged files should survive, the untagged files should be gone.

clippy

6fbbab6

Merge branch 'main' into gc-tests

96666a2

fmt

31167bc

rklaehn force-pushed the gc-tests branch from 9ac85c7 to 31167bc Compare October 6, 2023 09:44

rklaehn added 3 commits October 6, 2023 15:31

Merge branch 'main' into gc-tests

0562442

# Conflicts: # iroh/src/node.rs

rework import_bytes to use finalize_import_sync as well

46e3230

Merge branch 'main' into gc-tests

cd36837

rklaehn commented Oct 9, 2023

View reviewed changes

rklaehn marked this pull request as ready for review October 9, 2023 12:17

rklaehn requested a review from Frando October 9, 2023 12:18

rklaehn commented Oct 9, 2023

View reviewed changes

rklaehn requested a review from dignifiedquire October 9, 2023 15:03

dignifiedquire reviewed Oct 9, 2023

View reviewed changes

dignifiedquire approved these changes Oct 10, 2023

View reviewed changes

rklaehn added 4 commits October 10, 2023 12:08

clippy

6a88c70

Merge branch 'main' into gc-tests

8138bf0

# Conflicts: # iroh-bytes/src/util.rs

Make sure the partial files get dropped before gc is performed.

47358ca

Windows does not allow you to delete files when they are open. You don't get an error, deletion via remove_file just does nothing.

Merge branch 'main' into gc-tests

0146754

rklaehn added this pull request to the merge queue Oct 10, 2023

Merged via the queue into main with commit 079bb9e Oct 10, 2023
15 checks passed

rklaehn deleted the gc-tests branch October 10, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gc tests #1585

Gc tests #1585

rklaehn commented Oct 6, 2023 •

edited

Loading

rklaehn Oct 9, 2023

rklaehn Oct 9, 2023

rklaehn Oct 9, 2023

rklaehn Oct 9, 2023

rklaehn Oct 9, 2023 •

edited

Loading

dignifiedquire Oct 9, 2023

rklaehn Oct 10, 2023

dignifiedquire Oct 9, 2023

rklaehn Oct 10, 2023 •

edited

Loading

dignifiedquire Oct 10, 2023

		@@ -0,0 +1,440 @@
		#![cfg(all(feature = "mem-db", feature = "iroh-collection"))]

Gc tests #1585

Gc tests #1585

Conversation

rklaehn commented Oct 6, 2023 • edited Loading

Description

Questions

Missing (this or next PR):

Change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rklaehn commented Oct 6, 2023 •

edited

Loading

rklaehn Oct 9, 2023 •

edited

Loading

rklaehn Oct 10, 2023 •

edited

Loading