Repository: borgbackup/borgstore

Issue:
how to deal with nesting directories
Initially, no nesting directories were pre-created and for every `store.store` operation, the code basically always did:
- a `mkdir -p` to make sure the nesting dir is present and
- a file write to that directory

For the `file:` backend, this was not a problem, but for `sftp:` we need to optimize as it suffers from latency for each operation we do.

So, as this was doing a lot of unnecessary `mkdir` ops once after all these dirs were created, this was optimized to:
- only create all the nesting dirs once, at store creation time
- expect that the nesting dirs are all present, write the file to the nesting directory

This saves quite some time, assuming that store creation happens rarely, but data is written to the same store frequently.

BUT:
- the unit tests create stores frequently and do not write a lot of data to them. this was worked around by using less (0) nesting levels for the unit tests.
- for borgbackup repos:
  - repos are often created **interactively**.
  - repos are used **non-interactively** to do scheduled backups.
  - store creation being super slow (like 20-30 minutes) is annoying if done interactively.

Improvement idea:

Change the code to get the best of both approaches:
- give the option to pre-create the nesting dirs, but do not require it
- when storing objects to nesting dirs:
  - first try to directly write to the nesting dir. if the nesting dir is present, we are quickly done.
  - if that fails, because the nesting dir is not there, do a second try and first create the nesting dir and then write to the nesting dir.

That way:
- interactive store creation can be fast
- storing into the store will also be fast (after most of the dirs were created)
- it is more fault-tolerant, e.g. does not fail if a directory was "lost"

Maybe we even don't need the nesting dir pre-creation that way:
the initial backups are usually rather slow anyway, so guess that first try / second try overhead would maybe not make a big difference.

Discussion:

Patch:
diff --git a/src/borgstore/backends/_base.py b/src/borgstore/backends/_base.py
index 645d2b9..db01f08 100644
--- a/src/borgstore/backends/_base.py
+++ b/src/borgstore/backends/_base.py
@@ -46,6 +46,10 @@ class BackendBase(ABC):
     # have to care for ad-hoc directory creation for every store or move call. of course, create will take
     # significantly longer, especially if nesting on levels > 1 is used.
     # otoh, for some backends this might be completely pointless, e.g. if mkdir is a NOP (is ignored).
+    # for the unit tests, precreate_dirs should be set to False, otherwise they get slowed down to much.
+    # for interactive usage, precreate_dirs = False is often the less annoying, quicker option.
+    # code in .store and .move methods can deal with mkdir in the exception handler, after first just
+    # assuming that the directory is usually already there.
     precreate_dirs: bool = False
 
     @abstractmethod
diff --git a/src/borgstore/backends/posixfs.py b/src/borgstore/backends/posixfs.py
index ddd85ea..ab52cf1 100644
--- a/src/borgstore/backends/posixfs.py
+++ b/src/borgstore/backends/posixfs.py
@@ -35,10 +35,8 @@ def get_file_backend(url):
 
 
 class PosixFS(BackendBase):
-    # PosixFS implementation supports precreate = True as well as = False,
-    # but be careful: if backend creation was with precreate_dirs = False,
-    # backend usage must not be with precreate_dirs = True.
-    precreate_dirs: bool = True
+    # PosixFS implementation supports precreate = True as well as = False.
+    precreate_dirs: bool = False
 
     def __init__(self, path, *, do_fsync=False):
         self.base_path = Path(path)
@@ -125,21 +123,31 @@ class PosixFS(BackendBase):
             raise ObjectNotFound(name) from None
 
     def store(self, name, value):
+        def _write_to_tmpfile():
+            with tempfile.NamedTemporaryFile(suffix=TMP_SUFFIX, dir=tmp_dir, delete=False) as f:
+                f.write(value)
+                if self.do_fsync:
+                    f.flush()
+                    os.fsync(f.fileno())
+                tmp_path = Path(f.name)
+            return tmp_path
+
         if not self.opened:
             raise BackendMustBeOpen()
         path = self._validate_join(name)
         tmp_dir = path.parent
-        if not self.precreate_dirs:
-            # note: tmp_dir already exists, if it was pre-created by Store.create_levels.
-            tmp_dir.mkdir(parents=True, exist_ok=True)
         # write to a differently named temp file in same directory first,
         # so the store never sees partially written data.
-        with tempfile.NamedTemporaryFile(suffix=TMP_SUFFIX, dir=tmp_dir, delete=False) as f:
-            f.write(value)
-            if self.do_fsync:
-                f.flush()
-                os.fsync(f.fileno())
-            tmp_path = Path(f.name)
+        try:
+            # try to do it quickly, not doing the mkdir. fs ops might be slow, esp. on network fs (latency).
+            # this will frequently succeed, because the dir is already there.
+            tmp_path = _write_to_tmpfile()
+        except FileNotFoundError:
+            # retry, create potentially missing dirs first. this covers these cases:
+            # - either the dirs were not precreated
+            # - a previously existing directory was "lost" in the filesystem
+            tmp_dir.mkdir(parents=True, exist_ok=True)
+            tmp_path = _write_to_tmpfile()
         # all written and synced to disk, rename it to the final name:
         try:
             tmp_path.replace(path)
@@ -157,17 +165,26 @@ class PosixFS(BackendBase):
             raise ObjectNotFound(name) from None
 
     def move(self, curr_name, new_name):
+        def _rename_to_new_name():
+            curr_path.rename(new_path)
+
         if not self.opened:
             raise BackendMustBeOpen()
         curr_path = self._validate_join(curr_name)
         new_path = self._validate_join(new_name)
         try:
-            if not self.precreate_dirs:
-                # note: new_path.parent dir already exists, if it was pre-created by Store.create_levels.
-                new_path.parent.mkdir(parents=True, exist_ok=True)
-            curr_path.replace(new_path)
+            # try to do it quickly, not doing the mkdir. fs ops might be slow, esp. on network fs (latency).
+            # this will frequently succeed, because the dir is already there.
+            _rename_to_new_name()
         except FileNotFoundError:
-            raise ObjectNotFound(curr_name) from None
+            # retry, create potentially missing dirs first. this covers these cases:
+            # - either the dirs were not precreated
+            # - a previously existing directory was "lost" in the filesystem
+            new_path.parent.mkdir(parents=True, exist_ok=True)
+            try:
+                _rename_to_new_name()
+            except FileNotFoundError:
+                raise ObjectNotFound(curr_name) from None
 
     def list(self, name):
         if not self.opened:
diff --git a/src/borgstore/backends/sftp.py b/src/borgstore/backends/sftp.py
index b21b447..54b4aea 100644
--- a/src/borgstore/backends/sftp.py
+++ b/src/borgstore/backends/sftp.py
@@ -40,10 +40,8 @@ def get_sftp_backend(url):
 
 
 class Sftp(BackendBase):
-    # Sftp implementation supports precreate = True as well as = False,
-    # but be careful: if backend creation was with precreate_dirs = False,
-    # backend usage must not be with precreate_dirs = True.
-    precreate_dirs: bool = True
+    # Sftp implementation supports precreate = True as well as = False.
+    precreate_dirs: bool = False
 
     def __init__(self, hostname: str, path: str, port: int = 0, username: Optional[str] = None):
         self.username = username
@@ -219,19 +217,28 @@ class Sftp(BackendBase):
             raise ObjectNotFound(name) from None
 
     def store(self, name, value):
+        def _write_to_tmpfile():
+            with self.client.open(tmp_name, mode="w") as f:
+                f.set_pipelined(True)  # speeds up the following write() significantly!
+                f.write(value)
+
         if not self.opened:
             raise BackendMustBeOpen()
         validate_name(name)
         tmp_dir = Path(name).parent
-        if not self.precreate_dirs:
-            # note: tmp_dir already exists, if it was pre-created by Store.create_levels.
-            self._mkdir(str(tmp_dir), parents=True, exist_ok=True)
         # write to a differently named temp file in same directory first,
         # so the store never sees partially written data.
         tmp_name = str(tmp_dir / ("".join(random.choices("abcdefghijklmnopqrstuvwxyz", k=8)) + TMP_SUFFIX))
-        with self.client.open(tmp_name, mode="w") as f:
-            f.set_pipelined(True)  # speeds up the following write() significantly!
-            f.write(value)
+        try:
+            # try to do it quickly, not doing the mkdir. each sftp op might be slow due to latency.
+            # this will frequently succeed, because the dir is already there.
+            _write_to_tmpfile()
+        except FileNotFoundError:
+            # retry, create potentially missing dirs first. this covers these cases:
+            # - either the dirs were not precreated
+            # - a previously existing directory was "lost" in the filesystem
+            self._mkdir(str(tmp_dir), parents=True, exist_ok=True)
+            _write_to_tmpfile()
         # rename it to the final name:
         try:
             self.client.posix_rename(tmp_name, name)
@@ -249,22 +256,27 @@ class Sftp(BackendBase):
             raise ObjectNotFound(name) from None
 
     def move(self, curr_name, new_name):
+        def _rename_to_new_name():
+            self.client.posix_rename(curr_name, new_name)
+
         if not self.opened:
             raise BackendMustBeOpen()
         validate_name(curr_name)
         validate_name(new_name)
-        if not self.precreate_dirs:
-            # note: the parent dir of new_name already exists, if it was pre-created by Store.create_levels.
-            try:
-                parent_dir = Path(new_name).parent
-                self._mkdir(str(parent_dir), parents=True, exist_ok=True)
-            except OSError:
-                # exists already?
-                pass
+        parent_dir = Path(new_name).parent
         try:
-            self.client.posix_rename(curr_name, new_name)
+            # try to do it quickly, not doing the mkdir. each sftp op might be slow due to latency.
+            # this will frequently succeed, because the dir is already there.
+            _rename_to_new_name()
         except FileNotFoundError:
-            raise ObjectNotFound(curr_name) from None
+            # retry, create potentially missing dirs first. this covers these cases:
+            # - either the dirs were not precreated
+            # - a previously existing directory was "lost" in the filesystem
+            self._mkdir(str(parent_dir), parents=True, exist_ok=True)
+            try:
+                _rename_to_new_name()
+            except FileNotFoundError:
+                raise ObjectNotFound(curr_name) from None
 
     def list(self, name):
         if not self.opened:


Test patch:
diff --git a/tests/test_backends.py b/tests/test_backends.py
index 6050564..1e28a36 100644
--- a/tests/test_backends.py
+++ b/tests/test_backends.py
@@ -26,7 +26,6 @@ from borgstore.constants import ROOTNS, TMP_SUFFIX
 
 def get_posixfs_test_backend(tmp_path):
     be = PosixFS(tmp_path / "store")
-    be.precreate_dirs = False  # True (default) makes tests super slow.
     return be
 
 
@@ -48,8 +47,6 @@ def get_sftp_test_backend():
     url = os.environ.get("BORGSTORE_TEST_SFTP_URL")
     if url:
         be = get_sftp_backend(url)
-        if be is not None:
-            be.precreate_dirs = False  # True (default) makes tests super slow.
         return be
 
 
@@ -405,3 +402,31 @@ def test_must_not_be_open(tested_backends, request):
     with pytest.raises(BackendMustNotBeOpen):
         backend.destroy()
     backend.close()  # needed for test teardown to succeed
+
+
+def test_missing_nesting_dir_store(tested_backends, request):
+    with get_backend_from_fixture(tested_backends, request) as backend:
+        # for the unit tests to be fast, sftp and posixfs backends are created with
+        # .precreate_dirs = False, so we do not have precreated nesting dirs.
+        #
+        # test so the code does not expect pre-created dirs, .store should do mkdir and write and succeed:
+        assert not backend.precreate_dirs
+        backend.store("namespace1/nest1/key1", b"value1")
+        # test so the code expects pre-created dirs (but we do not have them!) and
+        # initially does not do the mkdir. then it retries, does mkdir and write and succeeds:
+        backend.precreate_dirs = True
+        backend.store("namespace2/nest2/key2", b"value2")
+
+
+def test_missing_nesting_dir_move(tested_backends, request):
+    with get_backend_from_fixture(tested_backends, request) as backend:
+        # similar as previous test for .store, but this tests .move method.
+        # test so the code does not expect pre-created dirs, .move should do mkdir and move and succeed:
+        assert not backend.precreate_dirs
+        backend.store("namespace1/nest1/key1", b"value1")
+        backend.move("namespace1/nest1/key1", "namespace1a/nest1a/key1a")
+        # test so the code expects pre-created dirs (but we do not have them!) and
+        # initially does not do the mkdir. then it retries, does mkdir and move and succeeds:
+        backend.precreate_dirs = True
+        backend.store("namespace2/nest2/key2", b"value2")
+        backend.move("namespace2/nest2/key2", "namespace2a/nest2a/key2a")