fixed tracking expected last offset of a follower #13495

mmaslankaprv · 2023-09-18T15:08:30Z

In Redpanda Raft implementation there may more than one
append_entries_request dispatched to the follower at the same time.
Leader tracks follower expected end offset to coordinate recovery_stm
and append_entries_stm and prevent delivering the same batches twice.
In classic raft implementation there is always only one append entries
request pending to the follower hence it is enough to update follower
state when processing append entries reply. We must track the expected
follower end before receiving response as the requests may already be in
flight.

Fixes: https://github.com/redpanda-data/core-internal/issues/752

Backports Required

Release Notes

Bug Fixes

fixed rare situation in which follower recovery stuck as the follower state was incorrectly updated

src/v/cluster/cluster_utils.cc

src/v/raft/recovery_stm.cc

src/v/raft/replicate_entries_stm.cc

Signed-off-by: Michal Maslanka <michal@redpanda.com>

src/v/raft/recovery_stm.cc

ztlpn · 2023-09-19T10:26:08Z

src/v/raft/consensus.cc

+     * requests that were not yet replied by the follower.
+     */
+    idx.expected_log_end_offset = std::max(
+      idx.last_dirty_log_index, idx.expected_log_end_offset);


I find it a bit suspicious that we are using the follower-side offset here without any term check (whereas in other places this is updated with a leader-side offset).

E.g. if both the follower and the leader have dirty_offset = 10 and term at offset 10 is different, but at offset 9 matches, and we send an append_entries with 9, the reply will be successful and expected_log_end_offset will be set to 10, but we are not really ready to send regular replicate append_entries yet.

this is done only for successful response here, in this case follower and leader logs up to the point indicated by append_entries_response match perfectly

In Redpanda Raft implementation there may more than one `append_entries_request` dispatched to the follower at the same time. Leader tracks follower expected end offset to coordinate `recovery_stm` and `append_entries_stm` and prevent delivering the same batches twice. In classic raft implementation there is always only one append entries request pending to the follower hence it is enough to update follower state when processing append entries reply. We must track the expected follower end before receiving response as the requests may already be in flight. Signed-off-by: Michal Maslanka <michal@redpanda.com>

When replying to stale append entries request a request that was already delivered to the follower we must clamp returned dirty offset not to allow Raft group leader to reason about offsets which are not yet know to be matching between leader and followers. This fixes situation in which follower `match_index` may updated before its log actually matches leader. Example: (term,offset) - represent a single entry Leader log: ``` (1,0),(1,1),(1,2),(3,3),(3,4),(3,5) committed_offset: 2 ``` Follower log: ``` (1,0),(1,1),(1,2),(2,3),(2,4) committed_offset: 2 ``` There is a term inconsistency starting at offset `3` If follower would receive an append entries request with prev_log_index=1 prev_log_term=1 The request would result in a successful reply as `prev_log_term` and matches the entry at offset 1, however follower log can not be truncated so the follower will reply with success. The success reply will 'lie' to the leader that the follower log matches leader log. Signed-off-by: Michal Maslanka <michal@redpanda.com>

ztlpn

confirmed that the patch fixes the original issue

mmaslankaprv · 2023-09-20T07:16:31Z

ci failure: #13491

vbotbuildovich · 2023-09-20T07:16:51Z

/backport v23.2.x

vbotbuildovich · 2023-09-20T07:16:52Z

/backport v23.1.x

vbotbuildovich · 2023-09-20T07:16:53Z

/backport v22.3.x

vbotbuildovich · 2023-09-20T07:17:49Z

Failed to create a backport PR to v23.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-13495-v23.1.x-197 remotes/upstream/v23.1.x
git cherry-pick -x fbf71007041db876fdd3f16096caf5a9ce2f6e76 299c32163da657d94fd48cb2050501fd2b370f52 d0e45c0baf88e2f0c3121a919d2051ef4e12fd8e

Workflow run logs.

vbotbuildovich · 2023-09-20T07:17:50Z

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-13495-v23.2.x-379 remotes/upstream/v23.2.x
git cherry-pick -x fbf71007041db876fdd3f16096caf5a9ce2f6e76 299c32163da657d94fd48cb2050501fd2b370f52 d0e45c0baf88e2f0c3121a919d2051ef4e12fd8e

Workflow run logs.

vbotbuildovich · 2023-09-20T07:17:53Z

Failed to create a backport PR to v22.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-13495-v22.3.x-107 remotes/upstream/v22.3.x
git cherry-pick -x fbf71007041db876fdd3f16096caf5a9ce2f6e76 299c32163da657d94fd48cb2050501fd2b370f52 d0e45c0baf88e2f0c3121a919d2051ef4e12fd8e

Workflow run logs.

[v23.2.x] Backport of #13495

github-actions bot added the area/redpanda label Sep 18, 2023

mmaslankaprv requested a review from ztlpn September 18, 2023 18:06

mmaslankaprv marked this pull request as ready for review September 18, 2023 18:06

ztlpn reviewed Sep 18, 2023

View reviewed changes

src/v/cluster/cluster_utils.cc Outdated Show resolved Hide resolved

src/v/raft/recovery_stm.cc Outdated Show resolved Hide resolved

src/v/raft/recovery_stm.cc Outdated Show resolved Hide resolved

src/v/raft/replicate_entries_stm.cc Outdated Show resolved Hide resolved

r/recovery_stm: removed unused state_changed method

fbf7100

Signed-off-by: Michal Maslanka <michal@redpanda.com>

mmaslankaprv force-pushed the fix-internal-752 branch from 3838046 to b3d8ba8 Compare September 19, 2023 08:08

mmaslankaprv changed the title ~~r/consensus: replace last_sent_offset with inflight_offset~~ fixed tracking expected last offset of a follower Sep 19, 2023

mmaslankaprv force-pushed the fix-internal-752 branch from b3d8ba8 to b80bb45 Compare September 19, 2023 08:12

mmaslankaprv requested a review from ztlpn September 19, 2023 08:12

mmaslankaprv force-pushed the fix-internal-752 branch from b80bb45 to 13d6188 Compare September 19, 2023 09:58

ztlpn reviewed Sep 19, 2023

View reviewed changes

mmaslankaprv added 2 commits September 19, 2023 12:44

mmaslankaprv force-pushed the fix-internal-752 branch from 13d6188 to d0e45c0 Compare September 19, 2023 11:51

ztlpn approved these changes Sep 19, 2023

View reviewed changes

mmaslankaprv merged commit 54c0d86 into redpanda-data:dev Sep 20, 2023
23 of 25 checks passed

mmaslankaprv deleted the fix-internal-752 branch September 20, 2023 07:16

vbotbuildovich mentioned this pull request Sep 20, 2023

[v23.1.x] fixed tracking expected last offset of a follower #13558

Closed

This was referenced Sep 20, 2023

[v23.2.x] fixed tracking expected last offset of a follower #13559

Closed

[v22.3.x] fixed tracking expected last offset of a follower #13560

Closed

mmaslankaprv mentioned this pull request Sep 20, 2023

[v23.2.x] Backport of #13495 #13561

Merged

7 tasks

piyushredpanda added a commit that referenced this pull request Sep 20, 2023

Merge pull request #13561 from mmaslankaprv/v23.2.x

0ad82db

[v23.2.x] Backport of #13495

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed tracking expected last offset of a follower #13495

fixed tracking expected last offset of a follower #13495

mmaslankaprv commented Sep 18, 2023 •

edited

ztlpn Sep 19, 2023

mmaslankaprv Sep 19, 2023

ztlpn left a comment

mmaslankaprv commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

fixed tracking expected last offset of a follower #13495

fixed tracking expected last offset of a follower #13495

Conversation

mmaslankaprv commented Sep 18, 2023 • edited

Backports Required

Release Notes

Bug Fixes

ztlpn Sep 19, 2023

Choose a reason for hiding this comment

mmaslankaprv Sep 19, 2023

Choose a reason for hiding this comment

ztlpn left a comment

Choose a reason for hiding this comment

mmaslankaprv commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

vbotbuildovich commented Sep 20, 2023

mmaslankaprv commented Sep 18, 2023 •

edited