cloud_storage: Add randomized tests back #8810

Lazin · 2023-02-10T19:54:41Z

Add the tests that check tiered storage read path with presence of inconsistencies (like overlaps between segments).
Modify offset_translator_state so it could tolerate seeing gaps out of order.

Backports Required

UX Changes

Release Notes

Improvements

Make tiered-storage robust in presence of inconsistencies in data in the object store

src/v/storage/offset_translator_state.cc

ztlpn · 2023-02-13T13:40:20Z

src/v/storage/offset_translator_state.cc

        int64_t gap_length = delta - last_delta;
-
-        if (gap_length != 0) {
+        // The 'gap_length' can be negative


Same here - I think we should rewrite this function a bit if it allows the offset to be in the middle of the map so that consistency is checked in any case. It should compare offset with the last offset in the map and:

if offset is within the map range, check that delta is consistent

if not, check that it is non-decreasing

src/v/storage/tests/offset_translator_state_test.cc

ztlpn · 2023-02-14T21:17:28Z

src/v/storage/offset_translator_state.cc

-      base_offset,
-      rbegin->first);
+    if (base_offset <= rbegin->first) {
+        if (!_last_offset2batch.contains(last_offset)) {


We should add handling of two additional cases:

the gap is before the beginning of the map - then we can't really say anything, because corresponding batches have already been prefix truncated from the translator, just exit.

last offset is in the map, but base offset is different - that's inconsistency.

something like:

if (base_offset <= rbegin->first) { if (last_offset > _last_offset2batch.begin()->first) { // check that exactly the same the gap is already present in the map auto it = _last_offset2batch.find(last_offset); if (it == _last_offset2batch.end()) { throw; } if (it->second.base_offset != base_offset) { throw; } } return; }

ztlpn · 2023-02-14T21:21:28Z

src/v/storage/offset_translator_state.cc

@@ -144,7 +157,7 @@ void offset_translator_state::add_gap(
 bool offset_translator_state::add_absolute_delta(
  model::offset offset, int64_t delta) {
    auto prev = model::prev_offset(offset);
-
+    vlog(stlog.debug, "add_absolute_delta: {}, delta{}", offset, delta);


Suggested change

vlog(stlog.debug, "add_absolute_delta: {}, delta{}", offset, delta);

vlog(stlog.debug, "add_absolute_delta: {}, delta {}", offset, delta);

src/v/storage/offset_translator_state.cc

ztlpn · 2023-02-14T21:50:59Z

src/v/storage/offset_translator_state.cc

              last_delta);

+            if (gap_length < 0) {


let's also check that the gap is not too big. so the check is probably best expressed in terms of base_offset: last_offset < base_offset <= prev

ztlpn · 2023-02-15T00:22:32Z

src/v/cloud_storage/remote_segment.cc

@@ -219,6 +219,12 @@ remote_segment::offset_data_stream(
                  .file_pos = 0,
                });
    }
+    vlog(
+      _ctxlog.debug,
+      "Offset data stream start reading at {}, log offset {}, delta {}",


I was thinking about an alternative solution - can we for a single partition read save somewhere next rp offset and pass it to the segment reader. The segment reader will then just skip every batch before this offset. This way overlaps shouldn't be a problem.

There is no good place to save it. The higher level reader (the one that scans remote_partition) is not retained between fetch requests. It's recreated and uses its reader_config to find cached remote_segment reader.

The solution I was thinking about is to store offset_translator_state inside the remote_segment_batch_reader . Currently it's part of the partition_record_batch_reader_impl. The partition reader passes its _ot_state to segment reader on every read_some call. Because of that the states of the reader and offset translator may diverge. To avoid this we can move ot-state to the segment reader. When the partition reader scans to the end of the segment it will fetch ot_state from the old reader and pass it to the next one. When the reader is saved for later use it will truncate it to avoid storing information about scanned record batches. But this can be implemented a bit later since it's a larger change. I'd rather use simple fix now to merge this before release.

This will also require a deep copy of the ot_state to avoid interaction between different fetch requests.

ztlpn · 2023-02-15T00:42:57Z

src/v/storage/offset_translator_state.cc

            _last_offset2batch.emplace(
              prev,
              batch_info{.base_offset = base_offset, .next_delta = delta});
            return true;
        } else {
+            // The record batch is already added, the delta computed using
+            // previous calls to 'add_gap' and 'add_absolute_delta' should match
+            // the parameter of this call. There is no need to change the state


So I have a suspicion that we can still end up with inconsistent data in the offset translator even if the segments themselves are good. The problem is that add_absolute_delta adds a fake gap, not a real one and if we then go back and start adding real gaps, we'll see inconsistency.

Example (not sure if this is possible with the way remote_partition chooses the order of segments, but probably it is):

execute fetch from offset 300:

start reading segment [200;400]: do add_absolute_delta(200, d_200)

add some gaps from the segment [200;400]

segment ends, we request the next segment: it is overlapping segment [100;500]

start reading segment [100;500]: do add_absolute_delta(100, d_100), but the offset translator state doesn't change! Because offset 100 is before the OT map end.

start adding gaps from segment [100;500]. Uh-oh, the gaps are inconsistent, the OT state in the 100-200 range is bogus.

I see two solutions:

(outlined in another comment) just save the next rp offset and skip batches in the new segment until we reach it

make add_absolute_delta throw away all OT map entries greater than offset - this way if we read some batches twice, that's okay because we will just rebuild OT state. This way we can even leave code of add_gap as-is because it will only add batches to the end of the OT map.

The tests check two cases: - one overlapping batch - duplicate segments

ztlpn · 2023-02-15T18:40:50Z

src/v/cloud_storage/remote_segment.cc

+            _parent._cur_ot_state->get().add_gap(
+              header.base_offset, header.last_offset());
+            _parent._cur_delta = model::offset_delta(
+              _parent._cur_ot_state->get().last_delta());


But what if current offset is not the last offset that we've seen? Then it will diverge from the true delta.

I think it should just be unconditional += header.last_offset_delta + 1 because the segment reader always reads one segment in order without any duplicates.

ztlpn · 2023-02-16T00:21:04Z

src/v/storage/offset_translator_state.cc

+            auto end = _last_offset2batch.end();
+            if (it == end || it->second.next_delta < delta) {
+                vassert(
+                  it != _last_offset2batch.end(),


Nit: the code is a bit strange in that we check it == end in the if condition and then assert that it is not end. Probably should be just assert?

ztlpn · 2023-02-16T00:21:52Z

src/v/storage/offset_translator_state.cc

+            // The record batch is already added, the delta computed using
+            // previous calls to 'add_gap' and 'add_absolute_delta' should match
+            // the parameter of this call. There is no need to change the state
+            // of the offset_translator_state. The invariant: any gap above the


nit: we do change it in the end :) so comment should be altered

ztlpn · 2023-02-16T00:30:33Z

src/v/storage/offset_translator_state.cc

+                  last_base_offset,
+                  gap_length));
+            }
+
            _last_offset2batch.emplace(


What about the case when gap_length=0? Previously we didn't modify the map in this case

Lazin · 2023-02-16T18:57:50Z

CI failure is unrelated

Make offset_translator_state process gaps out of order with the condition that the out of order data is the same as previously seen by the offsset_translator_state. This is needed to handle inconsistencies in the manifests, e.g. overlapping segments. Fix problem in remote_segment that may cuase the state of the reader to go out of sync with offset translator state.

github-actions bot added the area/redpanda label Feb 10, 2023

Lazin requested review from VladLazar and ztlpn February 10, 2023 19:54

ztlpn reviewed Feb 13, 2023

View reviewed changes

Lazin force-pushed the pr/add-randomized-tests-back branch from d7cdfea to 26ad183 Compare February 14, 2023 20:02

Lazin requested a review from ztlpn February 14, 2023 20:03

ztlpn reviewed Feb 15, 2023

View reviewed changes

Lazin force-pushed the pr/add-randomized-tests-back branch 2 times, most recently from 7c1ca8e to bd0522f Compare February 15, 2023 16:38

Lazin added 3 commits February 15, 2023 12:59

cloud_storage: Fix typo in the log line

cd33a36

cloud_storage: Add tests for incosistent manifests

ad21b4e

The tests check two cases: - one overlapping batch - duplicate segments

storage: Add offset_translator_state unit-tests

0a621fb

ztlpn reviewed Feb 15, 2023

View reviewed changes

Lazin requested a review from ztlpn February 15, 2023 19:33

Lazin force-pushed the pr/add-randomized-tests-back branch from bd0522f to 9d0ba2f Compare February 15, 2023 19:33

ztlpn reviewed Feb 16, 2023

View reviewed changes

Lazin force-pushed the pr/add-randomized-tests-back branch from 9d0ba2f to a5f20c6 Compare February 16, 2023 14:27

Lazin requested a review from ztlpn February 16, 2023 14:27

Lazin force-pushed the pr/add-randomized-tests-back branch from a5f20c6 to 66d76b1 Compare February 17, 2023 13:19

ztlpn previously approved these changes Feb 17, 2023

View reviewed changes

Lazin dismissed ztlpn’s stale review via e98fac4 February 17, 2023 15:56

Lazin force-pushed the pr/add-randomized-tests-back branch from 66d76b1 to e98fac4 Compare February 17, 2023 15:56

jcsp mentioned this pull request Feb 17, 2023

CI Failure (on dedicated nodes): ShadowIndexingWhileBusyTest.test_create_or_delete_topics_while_busy.short_retention=True TimeoutError #8289

Closed

ztlpn approved these changes Feb 17, 2023

View reviewed changes

Lazin merged commit 81941c9 into redpanda-data:dev Feb 17, 2023

This was referenced Feb 20, 2023

Failure in test_cloud_storage_rpunit.test_remote_partition_scan_incrementally_random_with_overlaps #7973

Closed

cloud_storage: reinstate remote_partition_test.cc cases #7723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud_storage: Add randomized tests back #8810

cloud_storage: Add randomized tests back #8810

Lazin commented Feb 10, 2023

ztlpn Feb 13, 2023

Lazin Feb 14, 2023

ztlpn Feb 14, 2023

ztlpn Feb 14, 2023

ztlpn Feb 14, 2023

ztlpn Feb 15, 2023

Lazin Feb 15, 2023

Lazin Feb 15, 2023

ztlpn Feb 15, 2023

ztlpn Feb 15, 2023

ztlpn Feb 16, 2023

ztlpn Feb 16, 2023

ztlpn Feb 16, 2023

Lazin commented Feb 16, 2023

	vlog(stlog.debug, "add_absolute_delta: {}, delta{}", offset, delta);
	vlog(stlog.debug, "add_absolute_delta: {}, delta {}", offset, delta);

cloud_storage: Add randomized tests back #8810

cloud_storage: Add randomized tests back #8810

Conversation

Lazin commented Feb 10, 2023

Backports Required

UX Changes

Release Notes

Improvements

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

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

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

Choose a reason for hiding this comment

Lazin commented Feb 16, 2023