Try to avoid etcd.Get as part of Delete operation #89828

wojtek-t · 2020-04-03T16:09:46Z

This was done for GuaranteedUpdate before in #35415 but later there were multiple (direct or indirect) bug fixes to that logic:
#40664 : Allow values to be wrapped prior to serialization in etcd
#47703 : Do not persist SelfLink into etcd storage
#48394 : GuaranteedUpdate must write if stored data is not canonical
#43152 : etcd3 store: retry with live object on conflict if there was a suggestion
#54780 : partial fix crd patch failing
#58375 : Recheck if transformed data is stale when doing live lookup during update
#77619 : In GuaranteedUpdate, retry on any error if we are working with cached data
#78713 : Set expected in-memory version when decoding unstructured objects from etcd
#82303 : In GuaranteedUpdate, retry on a precondition check failure if we are working with cached data

The PR upfront tries to address it by extensive testing based on all the issues listed above

Depending on the run and scale, we've seen between 10% and 60% reduction of latency on 99th percentiled for Delete API calls.

NONE

staging/src/k8s.io/apiserver/pkg/storage/interfaces.go

wojtek-t · 2020-04-03T19:29:24Z

/retest

k8s-ci-robot · 2020-11-21T16:56:48Z

@wojtek-t: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	`f265b1b`	link	`/test pull-kubernetes-node-e2e-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

wojtek-t · 2020-11-22T10:47:19Z

Neither of the errors couldn't be caused by problems with delete.
/retest

wojtek-t · 2020-11-23T13:14:32Z

@liggitt - I have analyzed deeply everything that happened with GuaranteedUpdate, added some tests to upfronet ensure that issues that it caused are tested explicitly (for those that it made sense) and I think it's ready for a pass of review; PTAL when you will get out of your 1.20 work

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go

liggitt · 2020-12-15T15:12:01Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

@@ -227,8 +269,13 @@ func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.O
 			return err


add a unit test to make sure the right thing happens if the suggestion is stale and the key no longer exists (was already deleted). I think we would get a NotFound here and return, but verify the right thing happens

Added "TestDeleteWithSuggestionOfDeletedObject" test.

That said - it doesn't exercise this path. What happens is that the transation fails, we get into

if !txnResp.Succeeded {

branch and this is handled in getState (getResp contains empty KV field and that is handled as returning "not-found" error).

txnResp.Responses[0] exists for a NotFound response?

Yes - it exists and being GetResponse contains empty set of KV pairs.

wojtek-t

@liggitt - comments addressed PTAL

staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

wojtek-t · 2020-12-16T07:27:51Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

@@ -227,8 +269,13 @@ func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.O
 			return err


Added "TestDeleteWithSuggestionOfDeletedObject" test.

That said - it doesn't exercise this path. What happens is that the transation fails, we get into

if !txnResp.Succeeded {

branch and this is handled in getState (getResp contains empty KV field and that is handled as returning "not-found" error).

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

wojtek-t · 2020-12-16T15:18:45Z

/hold cancel

liggitt · 2020-12-16T21:35:20Z

ok, I'm satisfied this is functionally correct. I still didn't see the performance numbers demonstrating the benefit was worth the additional complexity:

Do you have evidence that the performance gains are significant? this makes the code quite a bit more complex, and it's not as obvious that deleting from cache is as much of a performance gain as updating from cache.

Sure - that's a good question. All debugging shows it will help a lot, but we need to run scale tests to provide numbers. Holding before we have them.

Did I miss where those were provided?

wojtek-t · 2020-12-17T07:05:58Z

Did I miss where those were provided?

Sorry - I forgot to add that in the meantime. I added the following to the PR description:
Depending on the run and scale, we've seen between 10% and 60% reduction of latency on 99th percentiled for Delete API calls.

@liggitt - PTAL

liggitt · 2020-12-17T20:03:46Z

/lgtm
/approve

k8s-ci-robot · 2020-12-17T20:04:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

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:

~~pkg/controlplane/OWNERS~~ [liggitt,wojtek-t]
~~pkg/registry/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt,wojtek-t]

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 requested review from deads2k and madhusudancs April 3, 2020 16:11

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 3, 2020

wojtek-t force-pushed the suggestions_for_delete branch 2 times, most recently from b8ed2cb to 3ef6954 Compare April 3, 2020 16:30

enj reviewed Apr 3, 2020

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/interfaces.go Outdated Show resolved Hide resolved

wojtek-t force-pushed the suggestions_for_delete branch 4 times, most recently from 107abf0 to f265b1b Compare April 3, 2020 18:51

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 3, 2020

wojtek-t added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 3, 2020

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Apr 3, 2020

wojtek-t added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 3, 2020

wojtek-t changed the title ~~[WIP] Try to avoid etcd.Get as part of Delete operation~~ Try to avoid etcd.Get as part of Delete operation Apr 3, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2020

wojtek-t assigned liggitt Apr 3, 2020

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2020

wojtek-t force-pushed the suggestions_for_delete branch 2 times, most recently from ef03de5 to dc551cc Compare November 3, 2020 13:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2020

wojtek-t unassigned smarterclayton Nov 20, 2020

wojtek-t force-pushed the suggestions_for_delete branch from 97585a3 to c50ea57 Compare November 21, 2020 16:08

wojtek-t changed the title ~~[WIP] Try to avoid etcd.Get as part of Delete operation~~ Try to avoid etcd.Get as part of Delete operation Nov 23, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2020

wojtek-t added this to the v1.21 milestone Nov 23, 2020

wojtek-t removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Nov 23, 2020

liggitt reviewed Dec 15, 2020

View reviewed changes

Add suggestion to storage interface Delete method

c2d6189

wojtek-t force-pushed the suggestions_for_delete branch from c50ea57 to 0f88803 Compare December 16, 2020 07:34

Use current state from watchcache to avoid etcd get for deletions

7bab6a9

wojtek-t force-pushed the suggestions_for_delete branch from 0f88803 to 7bab6a9 Compare December 16, 2020 07:53

wojtek-t commented Dec 16, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2020

k8s-ci-robot merged commit 974cf4c into kubernetes:master Dec 17, 2020

ChunyiLyu mentioned this pull request May 19, 2021

StorageError: invalid object, Code: 4...StorageError: invalid object, Code: 4 rabbitmq/cluster-operator#608

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to avoid etcd.Get as part of Delete operation #89828

Try to avoid etcd.Get as part of Delete operation #89828

wojtek-t commented Apr 3, 2020 •

edited

Loading

wojtek-t commented Apr 3, 2020

k8s-ci-robot commented Nov 21, 2020 •

edited

Loading

wojtek-t commented Nov 22, 2020

wojtek-t commented Nov 23, 2020

liggitt Dec 15, 2020

wojtek-t Dec 16, 2020

liggitt Dec 16, 2020

wojtek-t Dec 17, 2020

wojtek-t left a comment

wojtek-t Dec 16, 2020

wojtek-t commented Dec 16, 2020

liggitt commented Dec 16, 2020

wojtek-t commented Dec 17, 2020

liggitt commented Dec 17, 2020

k8s-ci-robot commented Dec 17, 2020

		@@ -227,8 +269,13 @@ func (s *store) conditionalDelete(ctx context.Context, key string, out runtime.O
		return err

Try to avoid etcd.Get as part of Delete operation #89828

Try to avoid etcd.Get as part of Delete operation #89828

Conversation

wojtek-t commented Apr 3, 2020 • edited Loading

wojtek-t commented Apr 3, 2020

k8s-ci-robot commented Nov 21, 2020 • edited Loading

wojtek-t commented Nov 22, 2020

wojtek-t commented Nov 23, 2020

liggitt Dec 15, 2020

Choose a reason for hiding this comment

wojtek-t Dec 16, 2020

Choose a reason for hiding this comment

liggitt Dec 16, 2020

Choose a reason for hiding this comment

wojtek-t Dec 17, 2020

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t Dec 16, 2020

Choose a reason for hiding this comment

wojtek-t commented Dec 16, 2020

liggitt commented Dec 16, 2020

wojtek-t commented Dec 17, 2020

liggitt commented Dec 17, 2020

k8s-ci-robot commented Dec 17, 2020

wojtek-t commented Apr 3, 2020 •

edited

Loading

k8s-ci-robot commented Nov 21, 2020 •

edited

Loading