storage: can a Snapshot remove the Raft HardState permanently?

[nvanbenschoten]

replica.applySnapshot claims that the HardState parameter it takes:

may be empty, as Raft may apply some snapshots which don't require an update to the HardState

If this is truly the case then I'm having trouble convincing myself that we don't have a bug. applySnapshot first deletes all replicated and unreplicated keys. It then writes the HardState, but only if the HardState provided by Raft is not empty. How does this work if the HardState provided by Raft is empty? I can't see (and my own testing hasn't found) how a HardState would be written, so it looks like we'll end up with a range without a HardState. This seems really bad, and basic testing has shown that it can lead to this assertion firing (applied(%d) is out of range, not tocommit(%d) is out of range).

The solution to this, if I'm not missing anything, would be to save the HardState before we clear it and write this old HardState again after the clearRange if Raft doesn't provide a new one. However, I'm not even convinced it's possible to raft to provide an empty HardState. This is because this check look like it should prevent a snapshot that doesn't also increase the commit index. I added in an assertion to test this and our entire test suite passes without it firing.

I ran into this question while performing some nearby refactoring. I began digging into the git history before finding that there's been a long history around this so I'd like to get input from those who are more knowledgeable about it before trying to make any changes myself.

cc. @bdarnell (who I already spoke to) @tschottdorf @petermattis

[tbg]

Looks like we dodged a bullet? I agree that it looks like we're handling the empty HardState incorrectly and that Raft wouldn't ask us to apply such a snapshot. I'm pretty sure that when I wrote this comment I was only afraid that Raft might emit such a HardState, but not sure that it could.

What do you suggest we do? Just reject snapshots with an empty HardState as they shouldn't exist?

[nvanbenschoten]

What do you suggest we do? Just reject snapshots with an empty HardState as they shouldn't exist?

Yes, that's what I was thinking. Adding in logic that won't be exercised seems like a generally bad idea.

[tbg]

👍