Ensuring cache presence before starting borg create. Fixes #7450 #7475
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #7475 +/- ##
==========================================
+ Coverage 83.90% 83.93% +0.03%
==========================================
Files 67 67
Lines 11740 11762 +22
Branches 2139 2146 +7
==========================================
+ Hits 9850 9872 +22
+ Misses 1321 1319 -2
- Partials 569 571 +2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -900,6 +909,13 @@ def create_master_idx(chunk_idx): | |||
| self.do_cache = os.path.isdir(archive_path) | |||
| self.chunks = create_master_idx(self.chunks) | |||
|
|
|||
| def check_cache_integrity(self): | |||
There was a problem hiding this comment.
this only tests existence, not integrity.
how about:
return all(os.path.exists(os.path.join(self.path, file)) for file in "chunks", "config")
There was a problem hiding this comment.
also: only called from one place.
maybe rather do the joins only once and have the rest in-line?
| @@ -514,6 +514,15 @@ def __init__( | |||
| self.txn_active = False | |||
|
|
|||
| self.path = cache_dir(self.repository, path) | |||
| if os.path.exists(self.path) and not self.check_cache_integrity(): | |||
There was a problem hiding this comment.
isn't the first term redundant as the check call would also return False in that case?
| @@ -514,6 +514,15 @@ def __init__( | |||
| self.txn_active = False | |||
|
|
|||
| self.path = cache_dir(self.repository, path) | |||
| if os.path.exists(self.path) and not self.check_cache_integrity(): | |||
| logger.warning("Rebuilding cache as some cache-files have gone missing") | |||
There was a problem hiding this comment.
i guess this should be more precise.
There was a problem hiding this comment.
also: no "-" in english.
| self.chunks = ChunkIndex() | ||
| self.chunks.write(os.path.join(self.path, "chunks")) |
There was a problem hiding this comment.
this will create an empty (and thus incorrect) chunks index on disk, how is that helpful?
also creation of that file is done in a rather unsafe way.
There was a problem hiding this comment.
Well, I was assuming that creating an empty chunks cache should be enough for borg create to continue and re-populate it as it normally would.
- Should we be entirely rebuilding it here?
- How else would be creating the file. Is there another piece of code that I can take a look at?
There was a problem hiding this comment.
borg create needs the chunks cache in a valid state (refcounts, sizes for all chunks in the repo) and updates refcounts on that basis.
| @@ -921,6 +937,15 @@ def wipe_cache(self): | |||
| self.chunks = ChunkIndex() | |||
| with SaveFile(os.path.join(self.path, files_cache_name()), binary=True): | |||
| pass # empty file | |||
| self.recreate_cache_config() | |||
|
|
|||
| def recreate_cache_config(self, delete_existing=False): | |||
There was a problem hiding this comment.
maybe call this create... - it only mutates to the more special recreate_... in you give delete_existing=True.
| info = json.loads(self.cmd(f"--repo={self.repository_location}", "rinfo", "--json")) | ||
| repository = info["repository"] | ||
| cache_dir = os.path.join(get_cache_dir(), repository["id"]) | ||
| config_cache_path = os.path.join(cache_dir, "config") |
There was a problem hiding this comment.
this is the cache config, not the config cache.
| repository = info["repository"] | ||
| cache_dir = os.path.join(get_cache_dir(), repository["id"]) | ||
| config_cache_path = os.path.join(cache_dir, "config") | ||
| chunk_cache_path = os.path.join(cache_dir, "chunks") |
| self.cmd(f"--repo={self.repository_location}", "create", "archive1", "input") | ||
| info = json.loads(self.cmd(f"--repo={self.repository_location}", "rinfo", "--json")) | ||
| repository = info["repository"] | ||
| cache_dir = os.path.join(get_cache_dir(), repository["id"]) |
@ThomasWaldmann These are the changes for #7450. Right now, it recreates the config-cache if not found, and recreates the chunk-cache and the config-cache(to remove integrity data), if the chunk-cache is not found. Think there is a better/more efficient way to do this?