Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suffixの明示的な削除コードを追加(issue18対策) #42

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

yuezato
Copy link
Member

@yuezato yuezato commented Jun 4, 2021

PRの概要

Termがlog中で昇順に並ばないことがあるissue #18 を解決するためのコード群です。

ノード中のlogが古すぎる・staleしていて、ある位置 x のエントリがcommitすることがないことが判明した段階で、
suffix中のx以降のエントリ [x..) を全て削除するように変更します。

このPR以前では、ストレージ側にあるsuffixの削除は行っていないのですが、メモリ上のキャッシュに相当するHistoryでは削除を行っていました。
これに一致するようにsuffixも削除するコードである、とも言えます。

別の説明としては、Raftのオリジナル論文において、
AppendEntriesRPCではstaleが判明した段階でエントリを削除するよう以下の指示があり、

Receiver implementation:

  1. Reply false if term < currentTerm (§5.1)
  2. Reply false if log doesn’t contain an entry at prevLogIndex whose term matches prevLogTerm (§5.3)
  3. If an existing entry conflicts with a new one (same index but different terms), delete the existing entry and all that follow it (§5.3)
    (Figure 2より抜粋)

論文と同じ振る舞いをするように修正する、とも言えます。

コードの新規追加による修正箇所

新規追加による修正は大きく分けて以下の4つです。

  1. Followerに新たなsubstate FollowerDelete追加しました
    • これは、上記概要に記載の通り、ログの一部分を削除するためのsubstateです。
  2. 以前のコードで History に関する削除を行っていた箇所を、FollowerDelete 内に移動しました。
  3. 削除処理をIOのレイヤーに任せたいので、そのためのインタフェイスを追加しています:
  4. Common構造体に、logに対する削除が実行中かどうかを表すフラグメンバを追加しました。
    • 具体的にはこの箇所です。
    • このフラグは、下で見ますが、logとhistoryにズレがあるかどうかに対応しています。

Issue18の解決に対応する修正内容・箇所について

Issue18はつまるところ「ストレージに永続化されているlogと、そのメモリ上のキャッシュに相当するhistoryに「ズレ」が生じている状態で計算を続けると、クラスタが不整合のある状態になる」と言えます。

このズレを解消する処理として今回新たにDeleteを導入します。

Deleteの導入の有効性について

これまでDelete処理を行っていないために「logとhistoryにズレが発生し」そのまま計算を続けると問題が発生することを明らかにするテストケース シナリオ1シナリオ2 が既に存在していました。

このPRでのDeleteの導入によりlogとhistoryのズレが修正でき、上のテストケースが期待通りに動くことが確認できました。このことをもって、Deleteは有効であると言えます。

  • 問題の解消に従い、正しく実行された際の期待挙動を実際にとることを確認するassertionに差し替えています。
    ここここ の周辺になります。
  • 元のバグのある挙動はissue18の問題点を明らかにしていることを加味して、コメントアウトの形で残しています。
    ここここ です。

既存のコードに関して注意するべき点

Delete処理の導入だけでは、logとhistoryのズレを完全には解消できません。
なぜなら、Delete処理の途中で状態遷移を行うと、結局ズレが解消していないまま計算が続行されるからです。
(削除処理中に状態遷移をしてズレたまま計算すると、実際に問題が起こることについては、次のセクションで説明しています。)

これを防ぐために、Delete処理が完了するまでは状態遷移をしないように、既存のコードも合わせて修正します。
修正箇所は以下の通りです:

  1. Delete処理中にタイムアウトが発生しても、Candidateに即座に移行せずに、Deleteが完了するまでは処理を継続します。該当する箇所はここになっていて、内容としては:
    • タイムアウトが発生した場合には、その事実を記録しておき、Delete終了後にCandidateに遷移します。該当する処理はこの部分です。
    • タイムアウトが発生しなかった場合には、通常の遷移と同じく、FollowerIdleに遷移します。この箇所です。
  2. Delete処理中には、受信メッセージの処理による状態遷移が発生しないようにしました。
  3. 手動でタイムアウトを強制的に発動させるメソッドの内部についても、遷移を引き起こす可能性があるので修正しました。
    • この箇所です。
    • 以前のコードはFollowerなら即座にCandidateに遷移させるという、メソッド名と一致しないコードでした。
      これをより正確な実装である、Followerならばタイムアウトさせるメソッドを呼び出す、という実装に変更しています。
    • この変更は、既存のFollowerのsubstateに対しては意味を変えず、Deleteの場合にはCandidateへの遷移を遅らせるというという安全な実装になっています。

削除中のズレた状態で遷移すると発生する問題について

削除処理中に遷移すると実際に問題が生じることを述べたテストコードの内容を説明しているセクションです。
修正ではなくテストコードの説明を行っているセクションなので、skip可能です。

このテストコードでは次のような考え方をしています:

  1. タイムアウト判定処理が、削除処理1stepの実行に先行する(Followerのsubstateの処理はタイムアウト処理の後のここから呼ばれます)ことに注目すると、
  2. もしタイムアウトによるCandidateへの遷移を許している場合は たとえ削除処理が完了していたとしても、この行に到達せずに historyに反映されず、ズレた状態のcandidateが誕生する。より正確には、ストレージ上に存在しないエントリがhistory上では存在しているようにみえる。
  3. 選挙に関してはhistoryの情報のみを参照して計算が進むので、このズレたCandidateがLeaderになりうる。

実際に、テストコードでは、Delete処理中にlogとhistoryが食い違った状況を作っています。
その後でDelete処理中のズレたままの状態でタイムアウトを引き起こします。

この時、本PRでも採用しているように、削除中のタイムアウトによるCandidateへの遷移を防いでいる場合には問題は起こりません。

一方で、(PRのコードを修正して)削除中のタイムアウトによるCandidateへの即座の遷移を許す場合、ズレたCandidateがLeaderになるようなコードになっています。テストコードでは、リーダー選出直後のNoopログエントリ書き込みを、History情報を用いてlogに追加しようとして、ストレージ視点でログの非連結状態を生み、エラーとなります。

  • 図示すると次のような感じです:
     History: x x x x x x x y ← y が リーダー選出直後のエントリ
     Storage: x x . . . . . y ← . の位置は実際には削除されているエントリ(このような構造はLoaderの仮定としては許されていない)
    

Copy link
Contributor

@koba-e964 koba-e964 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

まだ見ている最中です

StepAll(iter) => {
for _ in 0..iter {
for (_n, io) in service.iter_mut() {
io.poll().expect("never fails in test senarios");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (typo):

Suggested change
io.poll().expect("never fails in test senarios");
io.poll().expect("never fails in test scenarios");

assert!(rawlog.head.index <= from);
rawlog
.truncate(from)
.expect("should not make an error in test senarios");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (typo):

Suggested change
.expect("should not make an error in test senarios");
.expect("should not make an error in test scenarios");

@yuezato yuezato changed the title suffixの明示的な削除コードを追加(issue18対策) [説明文の修正中です] suffixの明示的な削除コードを追加(issue18対策) Sep 10, 2021
@yuezato yuezato changed the title [説明文の修正中です] suffixの明示的な削除コードを追加(issue18対策) suffixの明示的な削除コードを追加(issue18対策) Sep 10, 2021
@shinnya shinnya merged commit 25d663b into frugalos:master Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants