Introduce Kubernetes interface to etcd client

[serathius]

Matching ticket on Kubernetes side https://github.com/kubernetes/kubernetes/pull/125258 cc @ahrtr @logicalhan

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

Part of https://github.com/etcd-io/etcd/issues/15820

[gritukan]

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]

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]

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]

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[bot]]

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]

Doing a second iteration based on https://github.com/kubernetes/kubernetes/pull/125258

ping @ahrtr for feedback

[ahrtr]

• The Kubernetes interface is just a higher level interface on top of the existing KV interface for application (apiserver). It makes more sense to get it included in Kubernetes/apiserver instead of etcd. Keep the Dependency inversion principle in mind, usually framework/upper layer owns the abstract/interface.
• We should always think about how to simplify etcd instead of complicate it.

[serathius]

Please comment on https://docs.google.com/document/d/1-nIpoW87qqQ9FxINOzXPkhu7CD_FWW1piYqHZDr39nU/edit?usp=sharing, the document provides the motivation why SIG-etcd and SIG-api-machinery should collaborate on this.

If you don't remember, this was agreed upon a long time ago https://github.com/kubernetes/community/blob/master/sig-etcd/vision.md#etcd-is-a-standalone-solution-for-managing-infrastructure-configuration

[serathius]

The interface was approved by @wojtek-t and @deads2k, we can start looking how we bring it in etcd. Part of https://github.com/etcd-io/etcd/issues/15820

cc @MadhavJivrajani @siyuanfoundation @fuweid

[ahrtr]

The interface was approved by @wojtek-t and @deads2k, we can start looking how we bring it in etcd.

Being approved by anyone != adding it into etcd. You can add it into K8s as well.

[serathius]

"we can start looking how we bring it in etcd" means we can start looking. It doesn't mean that we decided how to do that, nor that it was approved by etcd.

[serathius]

With https://github.com/kubernetes/enhancements/pull/4744 almost approved we can move forward with the PR.

Still need to add tests, however the code is ready to be reviewed.

[serathius]

/cc @wojtek-t @dims @ahrtr @jpbetz

[k8s-ci-robot]

@serathius: GitHub didn't allow me to request PR reviews from the following users: wojtek-t, dims, jpbetz.

Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @wojtek-t @dims @ahrtr @jpbetz

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.

[serathius]

ping @ahrtr

[serathius]

/retest

[ahrtr]

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

[serathius]

/hold

[serathius]

/retest

[serathius]

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 https://github.com/kubernetes/kubernetes/pull/125258. Would prefer to defer unit test to separate PR to ensure we are compatible with K8s first.

[k8s-ci-robot]

[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]

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

[codecov-commenter]

:warning: 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.

:exclamation: Current head cd3536e differs from pull request most recent head a26d984

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

:exclamation: 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.

[serathius]

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

[serathius]

/retest

[serathius]

/retest