Ensuring cache presence before starting borg create. Fixes #7450#7475
Ensuring cache presence before starting borg create. Fixes #7450#7475Michael-Girma wants to merge 1 commit intoborgbackup:masterfrom
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. |
ThomasWaldmann
left a comment
ThomasWaldmann
left a comment
There was a problem hiding this comment.
thanks for the PR - some stuff i noticed.
| 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?
| 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?
|
|
||
| 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.
| 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"]) |
|
Closing this because there was a recent PR rebuilding missing/corrupt cache files. |
3 participants
@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?