etcdserver: improve linearizable renew lease

[aaronjzhang]

This change fixes the issue where excessive "apply request took too long" warnings can potentially block all lease keep-alive operations which is discussed in #18100 . The changes mitigates this impact while preserving system correctness:

1. Lease Not Found Handling: When a lease is not found, it might be undergoing application processing. We wait for the applied index to advance to confirm whether the lease truly doesn't exist, preventing false negatives.

2. Revoking Lease Handling: When a lease is found but in revoking state, the revoke request has been committed but not yet applied. Allowing the current renewal to proceed doesn't compromise correctness since the lease will still be properly revoked once the revoke request is applied.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaronjzhang
Once this PR has been reviewed and has the lgtm label, please assign siyuanfoundation for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @aaronjzhang. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[aaronjzhang]

@ahrtr would you please help to review this PR? thanks.

[ahrtr]

I am afraid it won't change anything, because you still need to wait for applied index to catch up with the commit index.

etcd/server/lease/lessor.go

Line 435 in 963a72e

if err := le.cp(context.Background(), &pb.LeaseCheckpointRequest{Checkpoints: []*pb.LeaseCheckpoint{{ID: int64(l.ID), Remaining_TTL: 0}}}); err != nil {

BTW, It seems there is a bug during renew process, refer to #20590

[ahrtr]

I am afraid it won't change anything, because you still need to wait for applied index to catch up with the commit index.

It might be improving a little bit. Assuming there are already N entries pending to be applied, during the waitAppliedIndex, there might be more/new entries being added to the pending list...

Have you confirmed that there is any improvement in your production or test environment?

[aaronjzhang]

I am afraid it won't change anything, because you still need to wait for applied index to catch up with the commit index.

It might be improving a little bit. Assuming there are already N entries pending to be applied, during the waitAppliedIndex, there might be more/new entries being added to the pending list...

Have you confirmed that there is any improvement in your production or test environment?

Yes, this has been confirmed in our production environment and can be easily reproduced in a test environment using the following steps:

1. Build etcd with gofail enabled.
2. Run etcd with gofail.
3. Grant a lease with a TTL of 5 seconds and perform lease keep-alive:
   lease=$(etcdctl lease grant 5 | awk '{print $2}'); etcdctl lease keep-alive $lease
4. Apply a delay via gofail:
   curl http://127.0.0.1:2001/beforeApplyOneEntryNormal -XPUT -d 'sleep("5s")'
5. Execute a put command:
   etcdctl put test v1

Without this improvement, the lease keep-alive fails; with the improvement, it functions correctly.

Based on my understanding, a lease is only checkpointed when its TTL is greater than the checkpoint interval. Since we are using the default checkpoint interval of 5 minutes, the code below would not be executed when renewing a lease with a short TTL.
if err := le.cp(context.Background(), &pb.LeaseCheckpointRequest{Checkpoints: []*pb.LeaseCheckpoint{{ID: int64(l.ID), Remaining_TTL: 0}}}); err != nil {

[ahrtr]

Yes, this has been confirmed in our production environment

Could you be more explicit on this? What's the difference on lease renew's behaviour before and after the change in this PR when etcdserver is under load?

[aaronjzhang]

Yes, this has been confirmed in our production environment

Could you be more explicit on this? What's the difference on lease renew's behaviour before and after the change in this PR when etcdserver is under load?

Before the change, each lease keep-alive operation had to wait for the current apply index to be processed—even if the ongoing applying process was unrelated to the lease. If the waitApplyIndex operation blocked for too long, it could delay the keep-alive and potentially cause the lease to expire.

After the change, we check whether the lease already exists. If it exists, it implies that none of the current applying operations should be related to that lease. Therefore, we can renew the lease without waiting for the current apply index.

This decouples the lease renewal process from unrelated applying operations, preventing their delays from causing leases to expire.

[ahrtr]

Yes, this has been confirmed in our production environment

Could you be more explicit on this? What's the difference on lease renew's behaviour before and after the change in this PR when etcdserver is under load?

Before the change, each lease keep-alive operation had to wait for the current apply index to be processed—even if the ongoing applying process was unrelated to the lease. If the waitApplyIndex operation blocked for too long, it could delay the keep-alive and potentially cause the lease to expire.

After the change, we check whether the lease already exists. If it exists, it implies that none of the current applying operations should be related to that lease. Therefore, we can renew the lease without waiting for the current apply index.

This decouples the lease renewal process from unrelated applying operations, preventing their delays from causing leases to expire.

Sorry, I should be clearer.

I wasn't asking you to explain this PR. Instead I was asking the outcome from user perspective, for example how many leases were wrongly revoked when etcdserver under heavy load in a certain period (i.e. every minute), and how much this PR improved that.

[aaronjzhang]

Yes, this has been confirmed in our production environment

Could you be more explicit on this? What's the difference on lease renew's behaviour before and after the change in this PR when etcdserver is under load?

Before the change, each lease keep-alive operation had to wait for the current apply index to be processed—even if the ongoing applying process was unrelated to the lease. If the waitApplyIndex operation blocked for too long, it could delay the keep-alive and potentially cause the lease to expire.
After the change, we check whether the lease already exists. If it exists, it implies that none of the current applying operations should be related to that lease. Therefore, we can renew the lease without waiting for the current apply index.
This decouples the lease renewal process from unrelated applying operations, preventing their delays from causing leases to expire.

Sorry, I should be clearer.

I wasn't asking you to explain this PR. Instead I was asking the outcome from user perspective, for example how many leases were wrongly revoked when etcdserver under heavy load in a certain period (i.e. every minute), and how much this PR improved that.

All leases were wrongly revoked without the change. With this PR, no leases are revoked wrongly.

[ahrtr]

OK, adding a feature gate might be a more prudent choice.

[silentred]

In the second scenario, there is a small chance that users might get confused when the LeaseKeepAlive responds no error even after they just successfuly called LeaseRevoke.
The LeaseRevoke will eventually be applied, KeepAlive will quit, so it does not break correctness.
A suprising renewal failure seems a bigger threat to a metadata system.

[ahrtr]

See #20590

[aaronjzhang]

OK, adding a feature gate might be a more prudent choice.

Added a feature gate.