Introduce Kubernetes interface to etcd client #16333

serathius · 2023-07-31T09:10:46Z

Matching ticket on Kubernetes side kubernetes/kubernetes#125258
cc @ahrtr @logicalhan

Design: https://docs.google.com/document/d/1-nIpoW87qqQ9FxINOzXPkhu7CD_FWW1piYqHZDr39nU/edit?usp=sharing

gritukan · 2023-08-11T09:01:13Z

Hello!

I'm not a developer of etcd or Kubernetes, asking just out of curiosity. From what I can see in the API you've provided, it appears that Kubernates never modifies multiple keys within a transaction. Is it true?

logicalhan · 2023-08-11T15:41:40Z

Hello!

I'm not a developer of etcd or Kubernetes, asking just out of curiosity. From what I can see in the API you've provided, it appears that Kubernates never modifies multiple keys within a transaction. Is it true?

It's not supposed to, it breaks the watch protocol. See this doc for more details.

gritukan · 2023-08-11T23:25:10Z

It's not supposed to, it breaks the watch protocol. See this doc for more details.

I see, thank you!

I'm a little bit confused about it, since I've read multiple times that etcd is a bottleneck for Kubernates scalability. If there are no multi-keys transactions why isn't is possible to just partition all keys by hash for example, run multiple instances of etcd with each instance handling requests for a given subset of keys and get scalable Kubernates?

serathius · 2023-08-17T11:26:31Z

I'm a little bit confused about it, since I've read multiple times that etcd is a bottleneck for Kubernates scalability. If there are no multi-keys transactions why isn't is possible to just partition all keys by hash for example, run multiple instances of etcd with each instance handling requests for a given subset of keys and get scalable Kubernates?

It's not, K8s scalability limits are mostly hit by badly written operators that do not use proper API machinery. People are sharding etcd under Kubernetes (for example events are frequently separated due their different nature event vs state).

Note; This is an PR and not a discussion about the K8s API design. Please leave any follow up questions on this document. Any further questions will be removed to avoid cluttering the PR comments

stale · 2024-03-17T12:56:50Z

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.

serathius · 2024-06-19T11:27:54Z

Doing a second iteration based on kubernetes/kubernetes#125258

ping @ahrtr for feedback

serathius · 2024-07-05T10:11:22Z

/hold

serathius · 2024-07-05T11:21:47Z

/retest

serathius · 2024-07-05T11:30:04Z

Also please add an unit test file (kubernetes_test.go), which can be done in a followup PR if you want.

I'm testing this PR by running K8s tests on it in kubernetes/kubernetes#125258. Would prefer to defer unit test to separate PR to ensure we are compatible with K8s first.

client/v3/kubernetes/client.go

ahrtr · 2024-07-11T11:16:35Z

client/v3/kubernetes/client.go

+	return resp, nil
+}
+
+func (k Client) optimisticTxn(ctx context.Context, key string, expectRevision int64, onSuccess, onFailure *pb.RequestOp) (*pb.TxnResponse, error) {


Suggested change

func (k Client) optimisticTxn(ctx context.Context, key string, expectRevision int64, onSuccess, onFailure *pb.RequestOp) (*pb.TxnResponse, error) {

func (k Client) optimisticTxn(ctx context.Context, key string, expectedRevision int64, onSuccess, onFailure *pb.RequestOp) (*pb.TxnResponse, error) {

ahrtr

LGTM with a minor comment.

cc @fuweid @ivanvc and @jmhbnz to take a look as well.

k8s-ci-robot · 2024-07-11T11:47:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, serathius

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,serathius]

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

serathius · 2024-07-12T08:26:48Z

Would like to get review from SIG-api-machinery too.
cc @jpbetz @wojtek-t @logicalhan

client/v3/kubernetes/interface.go

client/v3/kubernetes/client.go

client/v3/kubernetes/interface.go

jpbetz · 2024-07-12T14:28:17Z

client/v3/kubernetes/interface.go

+	//
+	// If opts.GetOnFailure is true, the modified key-value pair will be returned if the put operation fails due to a revision mismatch.
+	// If opts.LeaseID is provided, it overrides the lease associated with the key. If not provided, the existing lease is cleared.
+	OptimisticPut(ctx context.Context, key string, value []byte, expectedRevision int64, opts PutOptions) (PutResponse, error)


Potential alternative designs:

Just use Put for this, but accept expectedRevision as an optional option in Put, maybe in a precondition section.

Put expectedRevision in an OptimisticPutOptions here. (OptimisticPutOptions could embed PutOptions and extend it...)

Kubernetes doesn't do Put without condition, and don't think it ever will. Don't understand why we would separate it.
Considered having expectedRevision in options, however I consider options, being optional while expectedRevision needs to be always provided because even zero has a semantic meaning.

client/v3/kubernetes/interface.go

codecov-commenter · 2024-07-13T18:59:17Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 21.33333% with 118 lines in your changes missing coverage. Please review.

Project coverage is 68.71%. Comparing base (c9fdc60) to head (cd3536e).
Report is 19 commits behind head on main.

❗ Current head cd3536e differs from pull request most recent head a26d984

Please upload reports for the commit a26d984 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
client/v3/client.go	`84.89% <100.00%> (ø)`
client/v3/kv.go	`94.44% <100.00%> (ø)`
client/v3/txn.go	`100.00% <100.00%> (ø)`
client/v3/watch.go	`93.83% <100.00%> (ø)`
client/v3/cluster.go	`96.00% <60.00%> (ø)`
client/v3/auth.go	`68.49% <66.66%> (ø)`
client/v3/lease.go	`90.87% <40.00%> (ø)`
client/v3/maintenance.go	`58.55% <13.33%> (ø)`
client/v3/kubernetes/client.go	`0.00% <0.00%> (ø)`

... and 409 files with indirect coverage changes

@@            Coverage Diff            @@
##           main   #16333       +/-   ##
=========================================
+ Coverage      0   68.71%   +68.71%     
=========================================
  Files         0      418      +418     
  Lines         0    35423    +35423     
=========================================
+ Hits          0    24341    +24341     
- Misses        0     9665     +9665     
- Partials      0     1417     +1417

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9fdc60...a26d984. Read the comment docs.

client/v3/kubernetes/interface.go

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius · 2024-07-23T12:54:35Z

Fixed the comments, please take another look @jpbetz @deads2k @wojtek-t

serathius · 2024-07-23T13:22:38Z

/retest

serathius · 2024-07-23T14:01:54Z

/retest

serathius marked this pull request as draft July 31, 2023 09:11

stale bot added stale and removed stale labels Mar 17, 2024

serathius force-pushed the kubernetes branch from eca4676 to d198d8b Compare June 19, 2024 11:26

k8s-ci-robot added area/clientv3 size/L do-not-merge/work-in-progress labels Jun 19, 2024

serathius force-pushed the kubernetes branch from d198d8b to 10a3e4c Compare June 20, 2024 10:04

k8s-ci-robot added area/documentation area/etcdctl area/etcdutl area/observability area/testing area/tooling needs-rebase github_actions Pull requests that update GitHub Actions code size/XXL and removed size/L labels Jun 20, 2024

serathius force-pushed the kubernetes branch from 10a3e4c to 8e4eaee Compare June 20, 2024 10:05

k8s-ci-robot added size/L and removed needs-rebase size/XXL labels Jun 20, 2024

serathius force-pushed the kubernetes branch 2 times, most recently from 98c5b1c to 61005d1 Compare June 20, 2024 10:42

serathius force-pushed the kubernetes branch from afd8f15 to 1d52e0b Compare July 5, 2024 10:10

k8s-ci-robot added the approved label Jul 5, 2024

serathius force-pushed the kubernetes branch 3 times, most recently from 8d42d04 to 8fa70e1 Compare July 5, 2024 10:25

ahrtr reviewed Jul 8, 2024

View reviewed changes

client/v3/kubernetes/client.go Show resolved Hide resolved

ahrtr mentioned this pull request Jul 8, 2024

Etcd kubernetes interface kubernetes/kubernetes#125258

Merged

ahrtr reviewed Jul 11, 2024

View reviewed changes

ahrtr approved these changes Jul 11, 2024

View reviewed changes

ivanvc removed the github_actions Pull requests that update GitHub Actions code label Jul 12, 2024

jpbetz reviewed Jul 12, 2024

View reviewed changes

siyuanfoundation reviewed Jul 12, 2024

View reviewed changes

client/v3/kubernetes/interface.go Show resolved Hide resolved

client/v3/kubernetes/interface.go Outdated Show resolved Hide resolved

client/v3/kubernetes/interface.go Outdated Show resolved Hide resolved

serathius force-pushed the kubernetes branch 2 times, most recently from ad516c3 to a26d984 Compare July 13, 2024 18:41

fuweid reviewed Jul 18, 2024

View reviewed changes

client/v3/kubernetes/interface.go Show resolved Hide resolved

serathius force-pushed the kubernetes branch from a26d984 to c10377d Compare July 23, 2024 12:51

Introduce Kubernetes KV interface to etcd client

6c98a96

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius force-pushed the kubernetes branch from c10377d to 6c98a96 Compare July 23, 2024 12:53

serathius added the stage/merge-when-tests-green label Jul 23, 2024

serathius merged commit 24ff469 into etcd-io:main Jul 23, 2024
43 checks passed

serathius mentioned this pull request Jul 23, 2024

[3.5] Introduce Kubernetes KV interface to etcd client #18358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Kubernetes interface to etcd client #16333

Introduce Kubernetes interface to etcd client #16333

serathius commented Jul 31, 2023 •

edited

Loading

gritukan commented Aug 11, 2023

logicalhan commented Aug 11, 2023

gritukan commented Aug 11, 2023

serathius commented Aug 17, 2023

stale bot commented Mar 17, 2024

serathius commented Jun 19, 2024

serathius commented Jul 5, 2024

serathius commented Jul 5, 2024

serathius commented Jul 5, 2024

ahrtr Jul 11, 2024

serathius Jul 23, 2024

ahrtr left a comment

k8s-ci-robot commented Jul 11, 2024

serathius commented Jul 12, 2024

jpbetz Jul 12, 2024

serathius Jul 13, 2024

codecov-commenter commented Jul 13, 2024

serathius commented Jul 23, 2024 •

edited

Loading

serathius commented Jul 23, 2024

serathius commented Jul 23, 2024

	func (k Client) optimisticTxn(ctx context.Context, key string, expectRevision int64, onSuccess, onFailure pb.RequestOp) (pb.TxnResponse, error) {
	func (k Client) optimisticTxn(ctx context.Context, key string, expectedRevision int64, onSuccess, onFailure pb.RequestOp) (pb.TxnResponse, error) {

Introduce Kubernetes interface to etcd client #16333

Introduce Kubernetes interface to etcd client #16333

Conversation

serathius commented Jul 31, 2023 • edited Loading

gritukan commented Aug 11, 2023

logicalhan commented Aug 11, 2023

gritukan commented Aug 11, 2023

serathius commented Aug 17, 2023

stale bot commented Mar 17, 2024

serathius commented Jun 19, 2024

serathius commented Jul 5, 2024

serathius commented Jul 5, 2024

serathius commented Jul 5, 2024

ahrtr Jul 11, 2024

Choose a reason for hiding this comment

serathius Jul 23, 2024

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 11, 2024

serathius commented Jul 12, 2024

jpbetz Jul 12, 2024

Choose a reason for hiding this comment

serathius Jul 13, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jul 13, 2024

Codecov Report

serathius commented Jul 23, 2024 • edited Loading

serathius commented Jul 23, 2024

serathius commented Jul 23, 2024

serathius commented Jul 31, 2023 •

edited

Loading

serathius commented Jul 23, 2024 •

edited

Loading