Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10379 +/- ##
==========================================
+ Coverage 88.14% 88.15% +0.01%
==========================================
Files 103 103
Lines 18896 18946 +50
Branches 2931 2944 +13
==========================================
+ Hits 16655 16702 +47
- Misses 1557 1561 +4
+ Partials 684 683 -1 ☔ View full report in Codecov by Harness. |
0a6e74a to
74fcb2b
Compare
74fcb2b to
c25f8d1
Compare
Every check --repair rebuild of the chunk index has an object validator (borgbackup#10369), so drop_corrupt_tail was only reachable from tests. - PackReader.iter_headers: remove the drop_corrupt_tail parameter. Without a validator, a corrupt object header raises IntegrityError. - build_chunkindex_from_repo: remove the drop_corrupt_tail parameter. - Repository: remove chunkindex_drop_corrupt_tail and chunkindex_validate. Since borgbackup#10368 the checker hands its index to the repository, so the lazy .chunks rebuild never runs during a check and nothing set either of them. - ArchiveChecker.check: stop passing drop_corrupt_tail. - tests: remove the 5 tests for drop_corrupt_tail, rewrite test_check_without_repair_does_not_drop_a_pack_tail as test_check_without_key_aborts_on_a_corrupt_pack_header.
c25f8d1 to
84a6388
Compare
…kup#8466 finish() validates the written packs against the shared index instead of rebuilding it from all packs.
84a6388 to
8903d7d
Compare
ThomasWaldmann
left a comment
ThomasWaldmann
left a comment
There was a problem hiding this comment.
Reviewed the new commit (8903d7d); the first one is #10378.
The approach is sound: check() already rebuilds the index from all packs at the start of --repair and nothing invalidates the shared index during the repair, so finish() only needs to re-read the packs the repair wrote. The tests are good, especially the ones asserting which packs finish() walks.
One real defect (inline, verify_written_packs), one docs item, the rest is small.
Behaviour change to be aware of: if a defect chunk deleted by --verify-data has another copy in a pack the repair did not write, the old finish() re-indexed that copy via the full rebuild. The new finish() leaves the chunk unindexed until the next check --repair. That matches what the archives check has just reported, so I consider it neutral or an improvement.
Docs still describe the rebuild this PR removes
After this PR finish() verifies the written packs and stores the index, it does not rebuild it. These places still say otherwise:
docs/internals/packs.rst, "Index Namespace" section: "borg check --repairwrites it before rebuilding the index after changing the packs"docs/internals/data-structures.rst,chunkindex-invalid: "beforeborg check --repairrebuilds the index after changing the packs"write_chunkindex_invalid()docstring incache.py: "and before rebuilding the index after pack changes the fragments do not record"
Also, the Repository.check() docstring lost its pointer to ArchiveChecker.finish() completely. finish() still stores the index again after the archives phase, so a (reworded) cross-reference is still useful there.
Nits
- "Re-reading the packs written by the repair: 3." reads like a label with a stray count, maybe "Re-reading 3 pack(s) written by the repair."
- "Writing the rebuilt repository chunks index." now also covers the path where
finish()rebuilt nothing. - An object at a wrong offset is counted twice ("not in the pack: 1" and "unindexed chunk id: 1"). OK as raw counts, just not obvious when reading the log.
Probe test for the inline finding (fails for [low], passes for [high] on 8903d7d)
import pytest
from ...archive import ArchiveChecker
from ...constants import ROBJ_FILE_STREAM
from ...manifest import Manifest
from ...repoobj import RepoObj
from ...repository import Repository
from . import cmd, create_test_files, RK_ENCRYPTION
@pytest.mark.parametrize("holder", ["low", "high"])
def test_probe_order_dependence(archiver, monkeypatch, holder):
"""Chunk X lives in one written pack, the index names the other written pack (bogus offset)."""
monkeypatch.setenv("BORG_PACK_MAX_COUNT", "1") # one object per pack
create_test_files(archiver.input_path)
cmd(archiver, "repo-create", RK_ENCRYPTION)
cmd(archiver, "create", "archive1", "input")
with Repository(archiver.repository_location, exclusive=True) as repository:
manifest = Manifest.load(repository)
ids = []
for data in [b"aaa", b"bbb"]:
cid = manifest.key.id_hash(data)
repository.put(cid, manifest.repo_objs.format(cid, {}, data, ro_type=ROBJ_FILE_STREAM))
ids.append(cid)
repository.flush()
entries = {cid: repository.chunks[cid] for cid in ids}
by_pack = sorted(ids, key=lambda c: entries[c].pack_id)
low_cid, high_cid = by_pack
x, other = (low_cid, high_cid) if holder == "low" else (high_cid, low_cid)
checker = ArchiveChecker()
checker.repair = True
checker.repository = repository
checker.key = checker.make_key(repository)
checker.repo_objs = RepoObj(checker.key)
checker.chunks = repository.chunks
checker.written_packs = {entries[low_cid].pack_id, entries[high_cid].pack_id}
checker.chunks[x] = entries[x]._replace(pack_id=entries[other].pack_id, obj_offset=1)
checker.verify_written_packs()
assert checker.error_found
assert x in checker.chunks, "chunk lost from the index although a written pack holds it"
fixed = checker.chunks[x]
assert (fixed.pack_id, fixed.obj_offset, fixed.obj_size) == (
entries[x].pack_id,
entries[x].obj_offset,
entries[x].obj_size,
)Two-pass variant of verify_written_packs() I tried (both probe directions pass, all of check_cmd_test.py still passes)
diff --git a/src/borg/archive.py b/src/borg/archive.py
index cfbc02352..8146e686f 100644
--- a/src/borg/archive.py
+++ b/src/borg/archive.py
@@ -2816,23 +2816,37 @@ def verify_written_packs(self):
if entries is not None:
entries.add((chunk_id, entry.obj_offset, entry.obj_size))
validate = object_validator(self.repo_objs)
+ # pass 1: read the packs and remove the index entries that name an object a pack does not hold.
+ # All removals come before pass 2 indexes anything, so the result does not depend on the pack order.
+ found_in = {} # pack_id -> list of (chunk_id, obj_offset, obj_size), None for a missing pack
+ not_found_in = {}
for pack_id in pack_ids:
# PackReader reads from the store, which does not refresh the repository lock.
self.repository._lock_refresh()
- pack_hex = bin_to_hex(pack_id)
- expected = indexed.pop(pack_id)
+ expected = indexed[pack_id]
reader = PackReader(self.repository.store, pack_id)
# iter_headers() yields nothing for a missing pack: the store reports size 0 for it.
if not self.repository.store.info(reader.key).exists:
self.error_found = True
- logger.error(f"pack {pack_hex}: written by the repair, but it is missing. Removing its index entries.")
- for chunk_id, _, _ in expected:
- del self.chunks[chunk_id]
- continue
- found = list(reader.iter_headers(validate=validate, on_drop=self.note_dropped_objects))
- not_found = sorted(expected.difference(found))
+ logger.error(
+ f"pack {bin_to_hex(pack_id)}: written by the repair, but it is missing. Removing its index entries."
+ )
+ found_in[pack_id] = None
+ not_found = sorted(expected)
+ else:
+ found_in[pack_id] = list(reader.iter_headers(validate=validate, on_drop=self.note_dropped_objects))
+ not_found = sorted(expected.difference(found_in[pack_id]))
+ not_found_in[pack_id] = not_found
for chunk_id, _, _ in not_found:
del self.chunks[chunk_id]
+ # pass 2: index the objects whose chunk id is not indexed.
+ for pack_id in pack_ids:
+ found = found_in[pack_id]
+ if found is None:
+ continue
+ pack_hex = bin_to_hex(pack_id)
+ expected = indexed[pack_id]
+ not_found = not_found_in[pack_id]
# the loop indexes each unindexed object, so of several unindexed copies of a chunk, the first
# is indexed and the others are superseded duplicates.
unindexed = []| for pack_id in pack_ids: | ||
| # PackReader reads from the store, which does not refresh the repository lock. | ||
| self.repository._lock_refresh() | ||
| pack_hex = bin_to_hex(pack_id) | ||
| expected = indexed.pop(pack_id) | ||
| reader = PackReader(self.repository.store, pack_id) | ||
| # iter_headers() yields nothing for a missing pack: the store reports size 0 for it. | ||
| if not self.repository.store.info(reader.key).exists: | ||
| self.error_found = True | ||
| logger.error(f"pack {pack_hex}: written by the repair, but it is missing. Removing its index entries.") | ||
| for chunk_id, _, _ in expected: | ||
| del self.chunks[chunk_id] | ||
| continue | ||
| found = list(reader.iter_headers(validate=validate, on_drop=self.note_dropped_objects)) | ||
| not_found = sorted(expected.difference(found)) | ||
| for chunk_id, _, _ in not_found: | ||
| del self.chunks[chunk_id] | ||
| # the loop indexes each unindexed object, so of several unindexed copies of a chunk, the first | ||
| # is indexed and the others are superseded duplicates. | ||
| unindexed = [] | ||
| for obj in found: | ||
| chunk_id, obj_offset, obj_size = obj | ||
| if obj in expected: | ||
| continue | ||
| if chunk_id in self.chunks: | ||
| logger.debug( | ||
| f"pack {pack_hex}: {bin_to_hex(chunk_id)} at offset {obj_offset}, {obj_size} bytes: " | ||
| "superseded duplicate" | ||
| ) | ||
| continue | ||
| unindexed.append(obj) | ||
| # size=0: the object header does not hold the plaintext size. | ||
| self.chunks[chunk_id] = ChunkIndexEntry( |
There was a problem hiding this comment.
The result depends on the pack order, and one order drops a chunk that a written pack holds.
Each pack gets its stale entries deleted and its unindexed objects indexed before the next pack is read, so the chunk_id in self.chunks test below runs against a half-fixed index.
Failing case: chunk X lives in written pack LOW, but the index names written pack HIGH.
LOWis read first: X is found, X is still in the index (pointing atHIGH), so it is skipped as a superseded duplicate.HIGHis read next: X is inexpected, not infound, so the entry is deleted.- X is gone from the index although
LOWholds it. With the pack ids the other way round the same corruption is repaired correctly.
This needs the index to be wrong about a repair-written pack already, i.e. a bug in put() / compact_pack, but that is exactly the case this function exists for, and rebuild_archives has already run, so nothing notices the archives referencing the lost chunk.
Fix: do all removals before any insertion. Pass 1 reads each pack once and deletes its not_found entries, pass 2 indexes the unindexed objects. Probe test and a diff I tried are in the review body.
| expected = indexed.pop(pack_id) | ||
| reader = PackReader(self.repository.store, pack_id) | ||
| # iter_headers() yields nothing for a missing pack: the store reports size 0 for it. | ||
| if not self.repository.store.info(reader.key).exists: |
There was a problem hiding this comment.
This is a second store.info() per pack: iter_headers() does one itself via reader.size(). One lookup would do (e.g. get the info once and hand the size to the reader), which matters for ssh:// and rest:// when the repair wrote many packs.
| self.repository.flush() | ||
| # store the pack writer buffer before the index is written (close() requires an empty buffer, #10055). | ||
| self.record_stored(self.repository.flush()) | ||
| if self.chunks_modified: |
There was a problem hiding this comment.
A non-empty written_packs implies chunks_modified today, but nothing enforces it, and the failure mode is a silently skipped verification. if self.chunks_modified or self.written_packs: (or an assert) would close that gap.
| def create_archive_entry(self, name, id, ts): | ||
| """Store the pack writer buffer, record the packs it wrote, create the archives directory entry.""" | ||
| self.record_stored(self.repository.flush()) | ||
| self.manifest.archives.create(name, id, ts) |
There was a problem hiding this comment.
Please say in the docstring why this wrapper exists: Archives.create() flushes the pack writer itself and drops the result, so without the flush here the pack ids would never reach written_packs. Otherwise someone will inline it again, and any future archives.create() call in the checker loses its packs silently.
| assert not repair or validate is not None # a repair validates every object it indexes | ||
| # store the chunks buffered in the pack writer, so the index below has their pack locations | ||
| # (pack id, offset and size in the pack). | ||
| self.repository.flush() |
There was a problem hiding this comment.
The result is dropped here while the two other flush() calls record theirs. That is correct (this runs before the index build, which reads that pack anyway), but a short comment saying so would prevent a wrong "fix".
2 participants
Description
Refs #8466, item 2 of #10318. Based on #10378, only the last commit is new.
With #10368 the checker and the repository share one chunk index, and
put()/delete()update it for every pack they write.ArchiveChecker.finish()stores that index and re-reads only the packs the repair wrote, to confirm the index matches them.ArchiveCheckerrecords the packs repair writes inwritten_packs: from theput()andflush()results, and fromdelete(), which now returnscompact_pack()'s(new_pack_id, dropped_bytes). A pack rewritten again is replaced by its new pack.create_archive_entry()stores the pack writer buffer and records its packs, then creates the archives directory entry.verify_written_packs()reads the object headers of each written pack with a validator and compares them with the index entries that name the pack:compact_packcopies one into the new pack when it lies in a byte range no index entry coversfinish()stores the index, then drops the in-memory one (invalidate_chunk_index()andself.chunks = None), becauseclose()would persist it over the index just stored.check()asserts that a repair has a validator, so every object it indexes is checked.Repository.flush()returns the objects of the packs it stored,Noneif it stored none.Tests (
check_cmd_test.py), each asserting which packsfinish()walks:delete()wrote, with more than ten packs in the repositorydelete()drops the pack and writes noneput()wrotefinish()'s ownflush()storestest_check_holds_a_single_chunk_index: the--repaircase builds one index in the checker, not twotest_check_repair_stopped_in_the_index_store_marks_the_index_invalid: renamed,finish()stores the index instead of rebuilding itTests (
repository_test.py):flush()returns the stored objects,Nonewith nothing buffered or no pack writer.Checklist
master(or maintenance branch if only applicable there)toxor the relevant test subset)