Cache serializations across watchers #81914

wojtek-t · 2019-08-25T22:19:55Z

Reuse serialization result across watchers - before this change, we were serializing objects independently for every single watcher. With this PR, when dispatching an event in watchcache:

wrap the object into one that is caching its encodings
reuse them if multiple watchers expect the same serialization format

This is significant performance improvement:

in 5k tests, we save ~15% of memory allocations
in 5k tests, we save ~5% of CPU
This is also addressing Creating large Endpoints objects in large clusters takes long #75294

Ref kubernetes/enhancements#1152

k8s-ci-robot · 2019-08-25T22:19:56Z

@wojtek-t: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

fejta-bot · 2019-08-25T23:14:36Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

wojtek-t · 2019-08-26T17:58:42Z

/retest

jpbetz

First pass, I'l try to get another pass out shortly

staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning_test.go

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

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

staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go

wojtek-t

The updates to first 5 commits were done in the parent PR: #81585

staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go

staging/src/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning_test.go

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

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

jpbetz

a couple more suggestions

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

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

staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go

staging/src/k8s.io/apimachinery/pkg/runtime/testing/cacheable_object.go

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

liggitt · 2019-09-10T15:23:22Z

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

+// The following functions implement metav1.Object interface:
+// - getters simply delegate for underlying object
+// - SetSelfLink stores the value passed in the first call
+// - all other setters ignore the passed value.


this is pretty confusing... I expected all getters/setters to pass through

This is a bit tricky. We can let them pass through and this will work, because noone is really using that.
But if we would e.g. create an encoder that is trying to overwrite one of the metadata, this can clearly break it for everyone. If course this is possible to fix - that encode instead of using CacheEncode should be calling GetObject() and operate on it. But that's relying on others.

I personally think that ignoring the values is safer - passing it through is easy to do, but I'm not sure it's what we really want.

spoke offline. will switch setters to lock, invalidate the cache on changes, and pass through. we don't actually expect these to get called frequently, so this is just for completeness and to fulfill the objectmeta interface

liggitt · 2019-09-10T15:25:02Z

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

+		defer o.lock.Unlock()
+		if o.selfLink != nil {
+			selfLink := *o.selfLink
+			result.selfLink = &selfLink


special-casing selfLink seems problematic. Can we generalize the object ownership principles so that the cached encoding can safely operate without this special-casing?

Once we will remove selflink, this special case will also be removed - this is sth that I had in the back of my head too.

can drop this as part of passing through setters

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

liggitt · 2019-09-10T15:52:54Z

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

 	case !curObjPasses && oldObjPasses:
 		// return a delete event with the previous object content, but with the event's resource version
-		oldObj := event.PrevObject.DeepCopyObject()
-		if err := c.versioner.UpdateObject(oldObj, event.ResourceVersion); err != nil {


hmm... this is fragile and potentially problematic (the same original object+resourceVersion can be encoded with different resourceVersions)... definitely need a test that exercises an object at a given resourceVersion getting encoded into delete events at different resourceVersions. For example:

start three watchers

unfiltered

labelSelector=foo=true

labelSelector=bar=true

add object with labels foo=true, bar=true at rv=1

watch events with rv=1 get sent to all watchers

update object to remove foo=true label at rv=2

update event with rv=2 and bar=true content gets sent to unfiltered and bar=true watchers

delete event with rv=2 and foo=true,bar=true content gets sent to foo=true watcher

update object to remove bar=true label at rv=3

update event with rv=3 and no label content gets sent to unfiltered watcher

delete event with rv=3 and bar=true content gets sent to bar=true watcher

delete object at rv=4

delete event with rv=4 and no label content gets sent to unfiltered watcher

Yeah - this is also the reason why I think the deep-copy is needed.
Will work on adding this test now.

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

k8s-ci-robot · 2019-09-30T08:57:38Z

@wojtek-t: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

wojtek-t · 2019-09-30T10:36:00Z

/retest

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

liggitt · 2019-10-01T13:56:19Z

/lgtm
/approve

thanks for all the work on this

k8s-ci-robot · 2019-10-01T13:56:59Z

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

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/client-go/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 · 2019-10-01T18:38:00Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	`25a728a`	link	`/test pull-kubernetes-e2e-gce-100-performance`

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 assigned liggitt Aug 25, 2019

wojtek-t mentioned this pull request Aug 25, 2019

Cachable object support #81585

Closed

k8s-ci-robot requested review from cjcullen and fgrzadkowski August 25, 2019 22:21

wojtek-t force-pushed the cache_serializations_across_watchers branch from 899779b to 3f20ed6 Compare August 26, 2019 02:33

wojtek-t assigned jpbetz Aug 26, 2019

jpbetz reviewed Aug 26, 2019

View reviewed changes

wojtek-t force-pushed the cache_serializations_across_watchers branch from 3f20ed6 to 7eb32f6 Compare August 26, 2019 20:30

wojtek-t commented Aug 26, 2019

View reviewed changes

jpbetz reviewed Aug 26, 2019

View reviewed changes

wojtek-t force-pushed the cache_serializations_across_watchers branch from 7eb32f6 to e876bce Compare August 27, 2019 00:03

liggitt reviewed Sep 10, 2019

View reviewed changes

wojtek-t force-pushed the cache_serializations_across_watchers branch from e876bce to 3ad2feb Compare September 11, 2019 09:16

k8s-ci-robot added the area/kubectl label Sep 11, 2019

liggitt reviewed Sep 27, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/cacher/caching_object.go Show resolved Hide resolved

liggitt reviewed Sep 27, 2019

View reviewed changes

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

liggitt reviewed Sep 27, 2019

View reviewed changes

wojtek-t added 8 commits September 30, 2019 08:58

Extend interfaces to support CacheableObject

248549b

Add Identifier method to GroupVersioner interface

36300c8

Implement Encoder.Identifier() method

cd4215a

CacheableObject test

1397b80

Fix transformObject to work with CacheableObject.

1dd4372

Implement support for CacheableObject

970f103

CachingObject

4cd8154

Cache serializations

25a728a

wojtek-t force-pushed the cache_serializations_across_watchers branch from f4d16a5 to 25a728a Compare September 30, 2019 08:49

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 30, 2019

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 30, 2019

liggitt reviewed Oct 1, 2019

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/cacher/caching_object.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2019

k8s-ci-robot merged commit 7878160 into kubernetes:master Oct 1, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 1, 2019

wojtek-t mentioned this pull request Oct 3, 2019

Unnecessary identifier() calls in versioning encoder #83449

Closed

wojtek-t mentioned this pull request Oct 22, 2019

Creating large Endpoints objects in large clusters takes long #75294

Closed

mrbobbytables mentioned this pull request Oct 22, 2019

Avoid serializing the same object independently for every watcher kubernetes/enhancements#1152

Closed

wojtek-t deleted the cache_serializations_across_watchers branch December 18, 2019 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache serializations across watchers #81914

Cache serializations across watchers #81914

wojtek-t commented Aug 25, 2019 •

edited

k8s-ci-robot commented Aug 25, 2019

fejta-bot commented Aug 25, 2019

wojtek-t commented Aug 26, 2019

jpbetz left a comment

wojtek-t left a comment

jpbetz left a comment

liggitt Sep 10, 2019

wojtek-t Sep 11, 2019

liggitt Sep 20, 2019 •

edited

wojtek-t Sep 24, 2019

liggitt Sep 10, 2019

wojtek-t Sep 11, 2019

liggitt Sep 20, 2019

wojtek-t Sep 24, 2019

liggitt Sep 10, 2019

wojtek-t Sep 11, 2019

wojtek-t Sep 11, 2019

k8s-ci-robot commented Sep 30, 2019

wojtek-t commented Sep 30, 2019

liggitt commented Oct 1, 2019

k8s-ci-robot commented Oct 1, 2019

k8s-ci-robot commented Oct 1, 2019

Cache serializations across watchers #81914

Cache serializations across watchers #81914

Conversation

wojtek-t commented Aug 25, 2019 • edited

k8s-ci-robot commented Aug 25, 2019

fejta-bot commented Aug 25, 2019

wojtek-t commented Aug 26, 2019

jpbetz left a comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

jpbetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Sep 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 30, 2019

wojtek-t commented Sep 30, 2019

liggitt commented Oct 1, 2019

k8s-ci-robot commented Oct 1, 2019

k8s-ci-robot commented Oct 1, 2019

wojtek-t commented Aug 25, 2019 •

edited

liggitt Sep 20, 2019 •

edited