Efficient watch resumption #94364

wojtek-t · 2020-08-31T11:19:13Z

Ref kubernetes/enhancements#1904

/kind feature

NONE

fedebongio · 2020-09-01T20:08:06Z

/cc @lavalamp

jingyih · 2020-09-11T09:09:08Z

@lavalamp @deads2k - this is ready for the first pass of review - PTAL when you will have chance (there are some missing unit tests blocked on etcd changes to their integration framework use, but that doesn't block reviewing the main code changes)

I just sent #94711 to see if we can unblock this PR sooner.

wojtek-t · 2020-09-15T08:11:54Z

/retest

wojtek-t · 2020-09-15T10:48:39Z

@lavalamp @deads2k - this is ready for the next pass - all dependencies are also merged and the PR is complete. PTAL

lavalamp · 2020-09-23T18:51:28Z

pkg/controlplane/instance.go

@@ -258,7 +258,7 @@ func (c *Config) createLeaseReconciler() reconcilers.EndpointReconciler {
 	if err != nil {
 		klog.Fatalf("Error determining service IP ranges: %v", err)
 	}
-	leaseStorage, _, err := storagefactory.Create(*config)
+	leaseStorage, _, err := storagefactory.Create(*config, nil)


Instead of changing every call site, can the new parameter be a member of the config struct?

This is a good point - will fix tomorrow.

Actually - this is part of createLeaseResonciler() method - this is basically callled once in the kube-apiserver lifetime.
So I don't think it makes sense to change anything.

I meant to include all the "generic.NewRawStorage(config, nil)" calls, too. If you think it's not worth it, that's fine.

It still seems weird to me to have a config thingy, and an option that's not part of the config thingy. What is the rule for adding to the config thingy vs adding another parameter?

Technically it may be possible to make it a part of config.

That said I looked deeper into the code (this time again), and piping the necessary information down to storageConfig (if we do that, we shouldn't pipe newFunc, but rather groupKind and use ObjectCreator from runtime.Scheme to create object) and that would be at least couple hundreds line of code (and e.g. path for CRDs is different than for built-in resources there).

I'm happy to look at this, but given this commit is actually fairly simple and relatively small (and almost all changes are in tests), I would prefer to not block this PR on it, so that we can get more soaking time of the feature itself.

If you're fine with this, I'm happy to open an issue and assign to myself to fix that once this one is merged.

What you have sounds better than plumbing it everywhere. I guess I was expecting something like, this code makes a shallow copy of the config and sets the additional field, so it'd be about the same change as what you have here, just call sites that are happy with nil wouldn't need to change at all.

But I wouldn't insist on this or anything, it might not be an improvement.

lavalamp · 2020-09-23T18:58:23Z

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

 		client:      client,
 		codec:       codec,
+		newFunc:     newFunc,


I'm a little surprised you can't get this out of the codec or versioner already.

Unfortunately you can't.
Codec only supports encoding and decoding:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go#L94
Versioner is only about conversions:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go#L220

There is ObjectCreater interface:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/runtime/interfaces.go#L254
that our runtime.Scheme implements, but that doesn't really change much in terms of passing arguments.

lavalamp · 2020-09-23T19:07:13Z

I'd like one more set of eyes on this before I approve, @jingyih can you review? I see you're already in the comments.

/assign @jingyih

jingyih

/lgtm

lavalamp · 2020-09-25T18:05:20Z

/approve

k8s-ci-robot · 2020-09-25T18:08:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, 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~~ [lavalamp,wojtek-t]
~~pkg/registry/OWNERS~~ [lavalamp,wojtek-t]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp,wojtek-t]
~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [lavalamp,wojtek-t]
~~test/OWNERS~~ [lavalamp,wojtek-t]

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

fejta-bot · 2020-09-25T21:10:29Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

wojtek-t self-assigned this Aug 31, 2020

k8s-ci-robot requested review from lavalamp and timothysc August 31, 2020 11:20

wojtek-t added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 31, 2020

wojtek-t force-pushed the efficient_watch_resumption branch 4 times, most recently from 5a1a277 to 24140dc Compare September 1, 2020 07:20

This was referenced Sep 1, 2020

[WIP] [DO NOT REVIEW] Experimental progress notify #92472

Closed

[WIP] [DO NOT REVIEW] Stress test progress notify POC #86769

Closed

wojtek-t force-pushed the efficient_watch_resumption branch from 24140dc to 0c97c57 Compare September 1, 2020 09:32

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 1, 2020

wojtek-t force-pushed the efficient_watch_resumption branch from 0c97c57 to e2d96bf Compare September 1, 2020 11:17

wojtek-t removed request for timothysc and lavalamp September 1, 2020 11:53

wojtek-t closed this Sep 1, 2020

wojtek-t reopened this Sep 1, 2020

jingyih mentioned this pull request Sep 11, 2020

Bump etcd to dd1b699fc489 #94711

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2020

wojtek-t added 5 commits September 15, 2020 08:19

Pipe newFunc to etcd3 storage layer

fbd65a2

Implement etcd3 progress-notify feature in etcd3 layer

56e7284

Allow tracking resource version for reflector store

4af1328

Enable progress notify events in watchcache

a94fb53

Test watchcache being updated in multietcd setup

af61e8f

wojtek-t force-pushed the efficient_watch_resumption branch from 64f732e to af61e8f Compare September 15, 2020 06:19

k8s-ci-robot added sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 15, 2020

wojtek-t mentioned this pull request Sep 18, 2020

enable etcd progress notify #77584

Closed

lavalamp reviewed Sep 23, 2020

View reviewed changes

k8s-ci-robot assigned jingyih Sep 23, 2020

jingyih reviewed Sep 25, 2020

View reviewed changes

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

k8s-ci-robot merged commit 0f39af9 into kubernetes:master Sep 25, 2020

k8s-ci-robot added this to the v1.20 milestone Sep 25, 2020

wojtek-t mentioned this pull request Sep 29, 2020

Watch bookmarks may contain version of objects of other types #95145

Merged

github-actions bot mentioned this pull request Sep 29, 2020

Week Ending September 27, 2020 dev-obs/actus#234

Open

wojtek-t mentioned this pull request Oct 22, 2020

[Flaky Test] go_test: //staging/src/k8s.io/apiserver/pkg/storage/cacher/go_default_test:run_1_of_2 #95394

Closed

chenyw1990 mentioned this pull request Apr 7, 2021

when apiservers have different RV for infrequently updated resources, client-go will get too large resource version error and relist the source again and again #100745

Closed

lavacat mentioned this pull request May 16, 2023

ETCD as a kubernetes SIG etcd-io/etcd#15875

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Efficient watch resumption #94364

Efficient watch resumption #94364

wojtek-t commented Aug 31, 2020

fedebongio commented Sep 1, 2020

jingyih commented Sep 11, 2020

wojtek-t commented Sep 15, 2020

wojtek-t commented Sep 15, 2020

lavalamp Sep 23, 2020

wojtek-t Sep 23, 2020

wojtek-t Sep 24, 2020

lavalamp Sep 24, 2020

wojtek-t Sep 25, 2020

lavalamp Sep 25, 2020

lavalamp Sep 23, 2020

wojtek-t Sep 23, 2020

lavalamp commented Sep 23, 2020

jingyih left a comment

lavalamp commented Sep 25, 2020

k8s-ci-robot commented Sep 25, 2020

fejta-bot commented Sep 25, 2020

Efficient watch resumption #94364

Efficient watch resumption #94364

Conversation

wojtek-t commented Aug 31, 2020

fedebongio commented Sep 1, 2020

jingyih commented Sep 11, 2020

wojtek-t commented Sep 15, 2020

wojtek-t commented Sep 15, 2020

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

lavalamp commented Sep 23, 2020

jingyih left a comment

Choose a reason for hiding this comment

lavalamp commented Sep 25, 2020

k8s-ci-robot commented Sep 25, 2020

fejta-bot commented Sep 25, 2020