kvstore: limit keys attached to single lease, and react to expiration #25966
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test |
marseel
reviewed
Jun 7, 2023
86a5223 to
0bbdede
Compare
marseel
approved these changes
Jun 7, 2023
0bbdede to
92e835f
Compare
Currently, each etcd client is associated with a single session, and the corresponding lease is attached to all upserted keys (if the lease parameter is set). This approach, though, suffers from performance issues, because put requests in etcd take linear apply time depending on the number of keys already attached to the lease (etcd-io/etcd#15993). This performance penalty is planned to be fixed in etcd (at least for the common case in which the user which performs the request has root role). In the meanwhile, let's make sure that we attach a limited number of keys to a single lease. In particular, this commit introduces the etcd lease manager, which is responsible for managing the lease acquisitions, tracking the keys that are attached to each of them. Once the number of keys per lease exceeds the configured threshold, a new lease gets automatically acquired. The lease usage counter is decremented when a given key gets deleted. Finally, in case one of the leases fails to be renewed, the manager allows to emit a notification event for all the keys that were attached to it. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
This commit modifies the etcd client to use the newly introduced lease manager in place of the static session, adapting the different call sites. One side effect is that we now create only one lease (instead of two) in case the client is used in read-only mode (e.g., in the clustermesh case). Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Now that we track the keys attached to each leases, let's allow to register callbacks to be executed for each key attached to a lease that has expired or failed to be renewed. This allows possible subscribers to be notified and take care of inserting again the given keys. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
The SyncStore now registers an observer to get notified in case the lease attached to any managed key expires, so that the given key can be enqueued to be synchronized again in the kvstore. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
92e835f to
da50c81
Compare
|
Rebased onto main to pick the CI fixes. |
|
/test |
tklauser
approved these changes
Jun 8, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reporting the description of the first commit for convenience:
Currently, each etcd client is associated with a single session, and the
corresponding lease is attached to all upserted keys (if the lease
parameter is set). This approach, though, suffers from performance
issues, because put requests in etcd take linear apply time depending
on the number of keys already attached to the lease (etcd-io/etcd#15993).
This performance penalty is planned to be fixed in etcd (at least for the
common case in which the user which performs the request has root role).
In the meanwhile, let's make sure that we attach a limited number of
keys to a single lease. In particular, this commit introduces the etcd
lease manager, which is responsible for managing the lease acquisitions,
tracking the keys that are attached to each of them. Once the number of
keys per lease exceeds the configured threshold, a new lease gets
automatically acquired. The lease usage counter is decremented when a
given key gets deleted. Finally, in case one of the leases fails to be
renewed, the manager allows to emit a notification event for all the keys
that were attached to it.
Additionally, the sync store implementation is modified to register for the lease expiration events, appropriately triggering the re-creation of the given keys. This allows to mitigate the possibility that keys get deleted because the associated lease failed to be renewed.