Support specifying `MaxBytes` in range request by linxiulei · Pull Request #14810 · etcd-io/etcd

linxiulei · 2022-11-19T18:54:14Z

fixes: #14809

server/storage/mvcc/kvstore_txn.go

ahrtr · 2022-11-19T22:39:05Z

The new feature makes sense to me, only limiting the max number of entries isn't enough if each entry is too big.

When client specifies both limit and maxBytes, then server side truncates the result when whichever is satisfied first.

It should have no impact on Kubernetes, because the maxByte defaults to 0, which means no limit.

ahrtr · 2022-11-19T22:41:49Z

Please also add some common e2e/integration test. Since this PR is already too big, so I'm OK to add it in a separate PR.

ahrtr · 2022-11-19T22:47:56Z

When this PR gets merged, probably someone could enhance K8s side to support maxBytes as well. cc @dims @liggitt @deads2k @lavalamp

FYI. #14810 (comment)

server/storage/mvcc/kv_test.go

ahrtr

LGTM

Thank yo @linxiulei

cc @ptabor @serathius @spzala

ahrtr · 2022-11-20T22:38:50Z

@linxiulei Please also add a changelog in https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.6.md, of course, can be in a separate PR.

TODO list:

add e2e/integration test. Please see Support specifying MaxBytes in range request #14810 (comment);
add changelog item.

linxiulei · 2022-11-20T22:46:33Z

Will do. Thanks @ahrtr !

server/etcdserver/txn/txn.go

serathius · 2022-11-21T10:23:45Z

server/storage/mvcc/kvstore_txn.go


 	limit := int(ro.Limit)
-	if limit <= 0 || limit > len(revpairs) {
+	if limit <= 0 || limit >= len(revpairs) {


This is no-op

server/storage/mvcc/kvstore_txn.go

server/etcdserver/txn/txn.go

serathius · 2022-11-21T12:59:23Z

I would like to see more tests before we merge this PR. This code is on a pretty important code path and there were already multiple edge cases not handled properly in this PR (for example maxBytes not working when sorting is enabled).

linxiulei · 2022-11-21T13:40:31Z

I manually tested it when making changes, which I can put into e2e/integration in later PR.

./bin/etcdctl get a z --max-bytes 14 --sort-by CREATE --order DESCEND --limit 2 -w json | python -m json.tool
{
    "header": {
        "cluster_id": 14841639068965178418,
        "member_id": 10276657743932975437,
        "revision": 4,
        "raft_term": 15
    },
    "kvs": [
        {
            "key": "Yw==",
            "create_revision": 4,
            "mod_revision": 4,
            "version": 1,
            "value": "YmFy"
        }
    ],
    "more": true,
    "count": 3
}

What tests else do we need here?

server/storage/mvcc/kvstore_txn.go

fuweid

LGTM(non-binding)

It seems that it reduces that grpc unary API memory usage. The grpc http2 transport only releases the memory after last byte has been sent. I was thinking that if the Range can be streaming type, the List-All from kube-apiserver won't cause memory spike in ETCD side because the pending bytes in memory is limit by http2 stream/connection windows size. However, it might be hard to integrate.

This patch can chunk the data. Thanks! @linxiulei Help a lot.

serathius · 2022-11-21T19:22:29Z

Would also want to get some feedback from K8s scalability people who have some experience with issue that this feature tries to fix. cc @mborsz

want more feedback from K8s people

mborsz · 2022-11-22T11:26:33Z

I think it would be worth discussing this with authors/reviewers of https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3157-watch-list which currently tries to solve similar (the same?) problem.

Probably it makes sense to get buy-in from sig-apimachinery that we will be able to use this in kube-apiserver before we make changes to etcd.

linxiulei · 2022-11-22T11:39:00Z

I agree to get more buy-ins from k8s side to make this feature more appealing. But I do not think it should be a blocker for etcd as this feature can benefit other use cases other than k8s. Also getting buy-ins from k8s side involves more discussions and takes time.

Nonetheless, I will try to initiate some discussion within k8s.

ptabor · 2022-11-25T10:38:47Z

server/etcdserver/txn/txn.go

+			totalBytes += int64(kv.Size())
+			if totalBytes > r.MaxBytes {
+				resp.More = true
+				rr.KVs = rr.KVs[:i]


If I understand the code correctly, it means we might be sending '0' items, it is, we might stuck completely,
giving empty responses and retries.

I think we should prioritize giving at least 1 item in such situation. And be very explicit that in this situation the maxBytes contract can be exceeded.
But this is the exact semantic discussion we should have with k8s api machinery team @mborsz @lavalamp

We also need to have tests for this.

Yes, you understood correctly. The initial version was to return the result with size just exceeding the maxBytes so at least one item is returned. But this made the maxBytes a bit ambiguous.

Given the max object size is 1.5MB, we can warm users the maxBytes should be bigger than 1.5MB to avoid returning 0 items. WDYT?

lavalamp · 2022-11-28T23:28:49Z

When this PR gets merged, probably someone could enhance K8s side to support maxBytes as well

k8s would not directly expose this to end users, to be clear. (but thanks for the heads up!)

Signed-off-by: Eric Lin <exlin@google.com>

Run 1. ./script/genproto.sh 2. ./scripts/update_proto_annotations.sh Signed-off-by: Eric Lin <exlin@google.com>

Signed-off-by: Eric Lin <exlin@google.com>

linxiulei · 2022-12-30T10:24:18Z

Please also add some common e2e/integration test. Since this PR is already too big, so I'm OK to add it in a separate PR.

I added a commit of e2e tests in the previous push

codecov-commenter · 2022-12-30T11:15:21Z

Codecov Report

Merging #14810 (97d4b6c) into main (4ae4d9f) will decrease coverage by 0.16%.
The diff coverage is 53.12%.

@@            Coverage Diff             @@
##             main   #14810      +/-   ##
==========================================
- Coverage   74.74%   74.57%   -0.17%     
==========================================
  Files         415      415              
  Lines       34287    34311      +24     
==========================================
- Hits        25629    25589      -40     
- Misses       7018     7071      +53     
- Partials     1640     1651      +11

Flag	Coverage Δ
all	`74.57% <53.12%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/storage/mvcc/kv.go	`40.00% <ø> (ø)`
client/v3/op.go	`73.97% <12.50%> (-1.89%)`	⬇️
server/etcdserver/txn/txn.go	`89.19% <46.66%> (-1.65%)`	⬇️
etcdctl/ctlv3/command/get_command.go	`69.15% <100.00%> (+0.58%)`	⬆️
server/storage/mvcc/kvstore_txn.go	`76.80% <100.00%> (+0.61%)`	⬆️
server/storage/mvcc/watchable_store.go	`85.14% <0.00%> (-7.61%)`	⬇️
client/v3/concurrency/mutex.go	`61.03% <0.00%> (-5.20%)`	⬇️
client/v3/leasing/txn.go	`88.09% <0.00%> (-3.18%)`	⬇️
server/etcdserver/cluster_util.go	`70.35% <0.00%> (-3.17%)`	⬇️
server/proxy/grpcproxy/watch.go	`92.48% <0.00%> (-2.90%)`	⬇️
... and 18 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

stale · 2023-04-02T05:28:07Z

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.

linxiulei · 2023-05-21T16:53:30Z

/reopen

linxiulei · 2023-07-25T09:55:40Z

How can I re-open it? I am still active on this PR if maintainers are able to evaluate it

ahrtr · 2023-07-25T09:58:05Z

I am not able to reopen either. Please raise a new PR, thx

fuweid · 2023-07-25T10:01:04Z

@linxiulei maybe it will be active after you repush?

linxiulei · 2023-07-25T10:20:34Z

repush didn't work either. Let me raise a new PR.

ahrtr reviewed Nov 19, 2022

View reviewed changes

server/storage/mvcc/kvstore_txn.go Outdated Show resolved Hide resolved

server/storage/mvcc/kvstore_txn.go Outdated Show resolved Hide resolved

server/storage/mvcc/kvstore_txn.go Outdated Show resolved Hide resolved

ahrtr changed the title ~~Support paginating by size~~ Support specifying MaxBytes in range request Nov 19, 2022

linxiulei force-pushed the size_paging branch from 2880545 to e7df7d4 Compare November 20, 2022 11:25

ahrtr reviewed Nov 20, 2022

View reviewed changes

server/storage/mvcc/kv_test.go Outdated Show resolved Hide resolved

linxiulei force-pushed the size_paging branch from e7df7d4 to b4c6693 Compare November 20, 2022 22:05

ahrtr approved these changes Nov 20, 2022

View reviewed changes

serathius reviewed Nov 21, 2022

View reviewed changes

server/etcdserver/txn/txn.go Show resolved Hide resolved

serathius reviewed Nov 21, 2022

View reviewed changes

server/storage/mvcc/kvstore_txn.go Outdated Show resolved Hide resolved

serathius reviewed Nov 21, 2022

View reviewed changes

server/storage/mvcc/kvstore_txn.go Outdated Show resolved Hide resolved

linxiulei force-pushed the size_paging branch from b4c6693 to f1f4dd1 Compare November 21, 2022 11:22

serathius reviewed Nov 21, 2022

View reviewed changes

server/etcdserver/txn/txn.go Show resolved Hide resolved

linxiulei force-pushed the size_paging branch from f1f4dd1 to 70a8cd5 Compare November 21, 2022 13:55

serathius previously approved these changes Nov 21, 2022

View reviewed changes

fuweid reviewed Nov 21, 2022

View reviewed changes

server/storage/mvcc/kvstore_txn.go Show resolved Hide resolved

fuweid approved these changes Nov 21, 2022

View reviewed changes

ptabor added the area/performance label Nov 25, 2022

ptabor reviewed Nov 25, 2022

View reviewed changes

ptabor added the stage/aliging-design label Nov 25, 2022

linxiulei mentioned this pull request Nov 29, 2022

Yxjetcd rangestream #12343

Closed

linxiulei mentioned this pull request Dec 28, 2022

KEP-3158: Optional maximum etcd page size on every list requests kubernetes/enhancements#3252

Closed

linxiulei added 6 commits December 30, 2022 10:17

server: add a new field More into RangeResult

3ccba17

Signed-off-by: Eric Lin <exlin@google.com>

rpc.proto: add a new field max_bytes into RangeRequest

55d9c12

Signed-off-by: Eric Lin <exlin@google.com>

rpc.proto: regenerate rpc.proto related files

1c0986a

Run 1. ./script/genproto.sh 2. ./scripts/update_proto_annotations.sh Signed-off-by: Eric Lin <exlin@google.com>

server: Implement MaxBytes option for RangeRequest

25b2a28

Signed-off-by: Eric Lin <exlin@google.com>

etcdctl: add max-bytes flag for get command

0c3aef1

Signed-off-by: Eric Lin <exlin@google.com>

tests: add tests with specifying MaxBytes option

97d4b6c

Signed-off-by: Eric Lin <exlin@google.com>

linxiulei force-pushed the size_paging branch 2 times, most recently from e97726b to 97d4b6c Compare December 30, 2022 10:23

stale bot added the stale label Apr 2, 2023

stale bot closed this May 21, 2023

fuweid mentioned this pull request Jul 24, 2023

Documentation: add roadmap #16279

Merged

ahrtr removed the stale label Jul 24, 2023

This comment was marked as off-topic.

Sign in to view

linxiulei mentioned this pull request Jul 25, 2023

Support specifying MaxBytes in range request #16300

Closed

Conversation

linxiulei commented Nov 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahrtr commented Nov 19, 2022

Uh oh!

ahrtr commented Nov 19, 2022

Uh oh!

ahrtr commented Nov 19, 2022

Uh oh!

Uh oh!

ahrtr left a comment

Choose a reason for hiding this comment

Uh oh!

ahrtr commented Nov 20, 2022

Uh oh!

linxiulei commented Nov 20, 2022

Uh oh!

Uh oh!

serathius Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serathius commented Nov 21, 2022

Uh oh!

linxiulei commented Nov 21, 2022

Uh oh!

Uh oh!

fuweid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serathius commented Nov 21, 2022

Uh oh!

mborsz commented Nov 22, 2022

Uh oh!

linxiulei commented Nov 22, 2022

Uh oh!

ptabor Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

linxiulei Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

lavalamp commented Nov 28, 2022

Uh oh!

linxiulei commented Dec 30, 2022

Uh oh!

codecov-commenter commented Dec 30, 2022

Codecov Report

Uh oh!

stale bot commented Apr 2, 2023

Uh oh!

linxiulei commented May 21, 2023

Uh oh!

linxiulei commented Jul 25, 2023

Uh oh!

ahrtr commented Jul 25, 2023

Uh oh!

fuweid commented Jul 25, 2023

Uh oh!

This comment was marked as off-topic.

linxiulei commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

8 participants

fuweid left a comment •

edited

Loading