Add more gofailpoints to validate critical parts of the code

[serathius]

What would you like to be added?

Followup from #14733. Thanks to gofailpoint in defrag code we were able to find an issue with data inconsistency.

Based on this we should increase coverage of other processes similar to defrag. TODO:

• Create a list of processes to add gofailpoints
• Add the gofailpoint into the code. Example:
  

  etcd/server/storage/backend/backend.go

  Line 504 in c83b1ad

  // gofail: var defragBeforeCopy struct{}

• Add code to linearizability tests to trigger the failpoints. Example
  

  etcd/tests/linearizability/failpoints.go

  Lines 35 to 44 in 2f558ca

  	DefragBeforeCopyPanic Failpoint = goFailpoint{"backend/defragBeforeCopy", "panic", triggerDefrag}
  	DefragBeforeRenamePanic Failpoint = goFailpoint{"backend/defragBeforeRename", "panic", triggerDefrag}
  	BeforeCommitPanic Failpoint = goFailpoint{"backend/beforeCommit", "panic", nil}
  	AfterCommitPanic Failpoint = goFailpoint{"backend/afterCommit", "panic", nil}
  	RaftBeforeSavePanic Failpoint = goFailpoint{"etcdserver/raftBeforeSave", "panic", nil}
  	RaftAfterSavePanic Failpoint = goFailpoint{"etcdserver/raftAfterSave", "panic", nil}
  	RandomFailpoint Failpoint = randomFailpoint{[]Failpoint{
  	KillFailpoint, BeforeCommitPanic, AfterCommitPanic, RaftBeforeSavePanic,
  	RaftAfterSavePanic, DefragBeforeCopyPanic, DefragBeforeRenamePanic,
  	}}

Let's start from creating the list:

• Compact Linearizability compact #14765
• AuthEnable/Disable
• ConsistenIndex
• MoveLeader
• BackendHooks etcdserver: Add gofailpoints for pre commit hook #14746
• Downgrade
• Backend Transaction (etcdserver: add failpoints for backend #14757)
• Watch

Feel free to suggest other processes that require a gofailpoint. cc @ahrtr @ptabor

Why is this needed?

This should allow us to better detect issues with data inconsistency like #14733

[ahrtr]

I will provide a list next week.

[ahrtr]

Supplement:

• downgrade
• Moveleader
• backend (I will add some failpoints)

[serathius]

Would love to also get some suggestions from @tbg @pavelkalinnikov. Raft library already has couple of failpoints, still it might be worth to expand them, especially with #14627.

[lavacat]

@serathius I've checked ConsistenIndex and I think the most interesting use case for failpoint is UnsafeSave, but it's indirectly covered by PreCommitHook failpoints.
Do you have something else in mind for ConsistenIndex? If not, I think it can be crossed out.

[lavacat]

@ahrtr Checked Moveleader and it seams most of the work is happening inside raft lib. I think it will be useful to add MoveLeader call to DefaultTraffic. But I don't see a good place inside etcd repo for a new failpoint. Do you have something else in mind for MoveLeader failpoint?

[serathius]

@serathius I've checked ConsistenIndex and I think the most interesting use case for failpoint is UnsafeSave, but it's indirectly covered by PreCommitHook failpoints. Do you have something else in mind for ConsistenIndex? If not, I think it can be crossed out.

No, list is to ensure we give some thought to it. Thanks for confirming.

[serathius]

I think we should also cover watch with failpoints. First step would be to reproduce #15248 by adding a failpoint after sending a progress notify. cc @ahrtr

[ahrtr]

@ahrtr Checked Moveleader and it seams most of the work is happening inside raft lib. I think it will be useful to add MoveLeader call to DefaultTraffic. But I don't see a good place inside etcd repo for a new failpoint. Do you have something else in mind for MoveLeader failpoint?

Sorry for the late response. We can use newLeader to check whether the leader changed, and add failpoint accordingly.

I think we should also cover watch with failpoints. First step would be to reproduce #15248 by adding a failpoint after sending a progress notify. cc @ahrtr

This seems to be a good suggestion. But I still do not think it can reliably reproduce the duplicated event issue. Submitted a PR #15272.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.