Ignore old leader's leases revoking request

[ahrtr]

Fix #15247.

• The first commit prevents an old leader from revoking any leases, similar to etcdserver: ignore old leader's request to revoke lease #12531.
• The second commit prevents the clients from getting stuck on an unhealthy member (i.e. an old leader)

Also refer to https://docs.google.com/document/d/1peLDjwebnSuNR69ombuUiJ5fkSw7Rd3Slv_mUpOLsmg/edit#heading=h.jbaw5ynuj9i7

[chaochn47]

2023-10-26T19:47:41.7815241Z     failover_test.go:161: 
2023-10-26T19:47:41.7816607Z         	Error Trace:	/home/runner/work/etcd/etcd/tests/e2e/failover_test.go:161
2023-10-26T19:47:41.7819776Z         	            				/home/runner/work/etcd/etcd/tests/e2e/failover_test.go:140
2023-10-26T19:47:41.7822841Z         	Error:      	Received unexpected error:
2023-10-26T19:47:41.7827565Z         	            	Put "http://127.0.0.1:12381/defragBeforeCopy": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
2023-10-26T19:47:41.7834278Z         	Test:       	TestFailoverOnDefrag/defrag_blocks_one-third_of_requests_with_stopGRPCServiceOnDefrag_set_to_false

Known flake.. We may want to auto retry setting up failpoint just like #16094

[ahrtr]

@mitake @serathius @chaochn47 @fuweid @tjungblu Please take a look at this PR and also https://docs.google.com/document/d/1peLDjwebnSuNR69ombuUiJ5fkSw7Rd3Slv_mUpOLsmg/edit

I will try to add e2e test cases after we get consensus.

[mitake]

Thanks @ahrtr , let me check this PR and the doc. Did you check this PR with #15247 (comment) ? I'll do it on my side tomorrow.

[ahrtr]

Did you check this PR with #15247 (comment) ?

Yes, confirmed that it works.

[mitake]

@ahrtr I could also check that this PR can work with the failpoint based test, great!

I'm still feeling that radical long term solution might be something like disabling proposal forwarding (or adding term information in proposal forwarding), e.g. #16285 IIUC, this PR might still have rare corner cases like: other processes on the server which runs etcd or other threads of etcd itself become very busy and execution stops between leader check and sending request (this line https://github.com/etcd-io/etcd/pull/16822/files#diff-164b95d19232df0c51cd511fe4ca61de738660fc11db6ddcdafa0ec5f5c80a35R405)

However I think this solution can work for many practical cases. Is my understanding aligned with you (according to the doc I think it's aligned)? Anyway I agree with the approach of this PR.

[ahrtr]

@mitake thx for the feedback.

I'm still feeling that radical long term solution might be something like disabling proposal forwarding (or adding term information in proposal forwarding), e.g. #16285

Please see etcd-io/raft#88 (comment). Of course, the long-term solution is still open to discussion. There is no rush to make any decision for now.

this PR might still have rare corner cases like: other processes on the server which runs etcd or other threads of etcd itself become very busy and execution stops between leader check and sending request (this line https://github.com/etcd-io/etcd/pull/16822/files#diff-164b95d19232df0c51cd511fe4ca61de738660fc11db6ddcdafa0ec5f5c80a35R405)

Even it happens (It's unlikely in practice), it shouldn't be a big problem. The lease depends on physical wall clock, an error of tens of or even hundreds of milliseconds should be acceptable for lease. Note that the default checkpoint interval is 5 minutes; it means that etcd does't run checkpoint at all for leases <= 5minutes.

Anyway I agree with the approach of this PR.

thx

[serathius]

I agree with proposed approach, however to merge the PR we need proper e2e tests.

The change is also pretty risky for backport. Those problems persisted for so long because lease refresh is not as utilized as much as other parts of API. We should think about how we qualify this.

[ahrtr]

Just rebased this PR.

Proposed solution makes pretty non-trivial changes to lease API that are risky to be introduced immediately to old etcd versions without previous soak.

Actually the added source code are all protection code. It should be fine. The only minor concern is about the performance impact on leaseKeepAlive, but it shouldn't be a big problem,

• There is no any impact on K8s, because K8s doesn't call KeepAlive or KeepAliveOnce at all;
• As I mentioned in Ignore old leader's leases revoking request #16822 (comment), etcd client sends the LeaseKeepAliveRequest every TTL/3 seconds, so the lease keep alive's performance isn't that critical.

[ivanvc]

I'll attempt first to implement WaitLeader in 3.5 as suggested by @ahrtr (#17240 (comment))

[ivanvc]

Hi @ahrtr, after backporting ExecuteUntil and WaitLeader, what's missing to finalize backporting the rest of the PR?

[ahrtr]

Hi @ahrtr, after backporting ExecuteUntil and WaitLeader, what's missing to finalize backporting the rest of the PR?

Please feel free to backport the PR to 3.5 and 3.4 and fix whatever is missing, thx

[ivanvc]

@ahrtr, as I was working on backporting this into 3.5. I realized that another functionality needs to be added to the e2e tests. BinaryFailpoints receive a timeout in main, while it's not implemented on 3.5. Should I create a PR to implement this or add the implementation along with this backport?

[ahrtr]

as I was working on backporting this into 3.5. I realized that another functionality needs to be added to the e2e tests. BinaryFailpoints receive a timeout in main, while it's not implemented on 3.5. Should I create a PR to implement this or add the implementation along with this backport?

I would suggest the same principle as before: try to backport it firstly; if it needs huge change or effort, then try to implement it in 3.5 & 3.4 directly. Thanks.