Replaced event queue based watching resources in router with shared informers #16315

pravisankar · 2017-09-13T01:01:21Z

Custom shared informer is used to leverage namespace, label and field filtering.
(Auto generated shared informer does not allow this)
Listing resources by shared informers doesn't order by resource version/creation time.
So custom lister for routes is used to order the route list by creation time and this
will allow oldest route to be processed before new route to claim the host name.
Synchronization with the informer queue and cache is a bit difficult as the cache could
have newer changes than what was pushed on to the queue. Luckily We only care about the
first sync to avoid 503 status code for routes.
Handling first sync:
- Informers are started with no registered event handlers
- Wait for all informers to be synced
- Block router reload
- Get list of items from informers store and process manually
- Perform router reload
- Register router event handlers
  This guarantees first router sync is performed after processing all existing items.
Subsequent router syncs rely on informer syncing sate and uses rate limiter to coalesce changes.
Deleted eventQueue, no longer used

Trello card: https://trello.com/c/y6SFvOA7

pravisankar · 2017-09-13T01:01:28Z

[test]

pravisankar · 2017-09-13T01:02:29Z

@openshift/networking @knobunc @rajatchopra PTAL

dcbw · 2017-09-13T13:27:01Z

/test integration issue #16312

knobunc · 2017-09-19T15:49:19Z

/lgtm Thanks @pravisankar.

@smarterclayton does this look sane to you? Obviously we still need to look into some of the test case failures (especially the one around the router reload).

smarterclayton · 2017-09-19T23:51:38Z

I don't like the live calls. Let's do this the proper way and use the cache correctly. Wait for sync, then flip a bool and force a refresh on the cache.

Also, don't use the resource name switch, just embed five informer inits. Strong typing is better

smarterclayton · 2017-09-19T23:54:42Z

Note that I'm really glad the event queue is gone, just want to get the last extra mile to make this "normal". Live calls are bad because they won't take advantage of API chunking when we turn that on in 3.8

smarterclayton · 2017-09-20T17:01:08Z

@deads2k re: how to safely have "only do this after sync" with the informer I think using an index on hostname and then doing a sort is the right thing to do, and if the "ready boolean" is unset simply exit the loop and come back around. Or we can just delay the initial sync step of writing out the config until sync is safe. I do think we don't want to write back to the route API until we've fully synced, so we probably are going to have to: 1. complete the full sync and populate the index (writing nothing to route api for the hostname overlap) 2. wait for that 3. trigger a refresh that will then do the exact same work over again but will trigger hostname writes to route api 4. let the first write happen

…

On Tue, Sep 19, 2017 at 11:53 AM, Ben Bennett ***@***.***> wrote: /lgtm Thanks @pravisankar <https://github.com/pravisankar>. @smarterclayton <https://github.com/smarterclayton> does this look sane to you? Obviously we still need to look into some of the test case failures (especially the one around the router reload). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_2Y7RH4IhJpVR1bdJ18AvaoRqmhks5sj-KCgaJpZM4PVcfY> .

deads2k · 2017-09-20T17:18:31Z

@deads2k re: how to safely have "only do this after sync" with the informer

In general, "do this after sync" is expressed by filling a work queue while the cache is priming, but not starting any workers. After all caches have sync, you can synchronously do some work (fill a secondary cache perhaps?) and then start a single worker.

Doing it like that would ensure let you trigger based off of a shared informer, do some work before you consume and process any update, and process resources in order. If you must process individual watch notifications (not resources), then you could fill your own queue (or super deep channel).

smarterclayton · 2017-09-23T19:26:52Z

If you need additional feedback this week to get this closed out please don't hesitate to ask. I'd like this in 3.7.

…

On Sat, Sep 23, 2017 at 4:09 AM, OpenShift Merge Robot < ***@***.***> wrote: @pravisankar <https://github.com/pravisankar> PR needs rebase — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pz7r521BiHrAZhK9hhBFPFZa2haEks5slLy5gaJpZM4PVcfY> .

pravisankar · 2017-09-25T23:23:38Z

[test]

pravisankar · 2017-09-25T23:24:50Z

@smarterclayton @knobunc @rajatchopra Updated, PTAL

smarterclayton · 2017-09-26T01:43:35Z

pkg/router/controller/factory/factory.go

-	routes, err := lw.client.Routes(lw.namespace).List(opts)
-	if err != nil {
-		return nil, err
+	rc.FirstSyncDone = func() bool {


Once has synced has returned true, this is unnecessary.

Maybe we should talk in person, I still think this is much more complicated than it needs to be.

Set "syncing" boolean true (never commit while this is true)

have your config loops update internal structs

wait for all informers synced

set syncing to be true

trigger a refresh in informers

3-5 should be able to be done in a single method.

So the issue here is that you have two caches:

in the informer

caches in the route plugin

The second cache is filled by the first cache. The code that fills the second cache is not done under the lock that maintains the first cache, and therefore while order of events can be guaranteed, there is no guarantee that the second cache is up to date with the first cache.

You need to determine when the second cache has observed all of the events from the first cache once it has synced once.

I think you can address this by starting the informers, waiting for synced (on all of them) and then call each cache as informer.GetStore().List() and send those to the route controller as adds. Once all of them are done, then call commit. Then register the route controller as a listener on the informer, and every update from then on is safe.

pravisankar · 2017-09-27T01:06:10Z

/test extended_conformance_gce

smarterclayton · 2017-09-27T01:28:34Z

Regarding the question today, if an informer index of routes by host is used then the index is up to date once synced is complete (index updates are synchronous). So we can replace our internal map with the index, and if we set the Boolean after true and resync any handlers are guaranteed to observe the other routes with the same host. Alternatively, we can simply avoid registering our handlers until sync is true, and then register and resync (may have to double check the ordering guarantees around adding a handler). On Sep 26, 2017, at 9:06 PM, Ravi Sankar Penta <[email protected]> wrote: /test extended_conformance_gce — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p7cTBCV5gcTM86m4Jhl6Jsrm1R_9ks5smZ-EgaJpZM4PVcfY> .

smarterclayton · 2017-09-27T20:28:53Z

Because this is so critical it can be delivered post dcut, although i don't want to slip too much.

…

On Wed, Sep 27, 2017 at 12:42 AM, OpenShift CI Robot < ***@***.***> wrote: @pravisankar <https://github.com/pravisankar>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_gce ba301d2 <ba301d2> link <https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/16315/test_pull_request_origin_extended_conformance_gce/8687/> /test extended_conformance_gce Full PR test history <https://openshift-gce-devel.appspot.com/pr/16315>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/pravisankar>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_py719jojn0U-oyfiJf6h2t-DRq22ks5smdIagaJpZM4PVcfY> .

smarterclayton · 2017-09-27T21:19:29Z

pkg/router/controller/router_controller.go

-	// event handlers have the same view of sync state.
-	c.endpointsListConsumed = c.EndpointsListConsumed()
-	c.commit()
+	c.updateConsumedCount(endpoints)


Counting isn't enough to tell you whether you've got everything. If an endpoint is deleted while you're doing the sync you will never reach your target number. You can't synchronize with your handlers like this unfortunately.

pravisankar · 2017-09-28T00:40:58Z

@smarterclayton @knobunc Updated, PTAL

smarterclayton · 2017-09-28T01:46:28Z

pkg/router/controller/router_controller.go

-		}
-		time.Sleep(50 * time.Millisecond)
-	}
+	c.StartInformers(utilwait.NeverStop)


This doesn't seem to belong here, but in the factory. Why should this be a concern of the route controller?

Registering functions into another type is a code smell. Just have a higher level method that calls public methods on the controller from the factory

smarterclayton · 2017-09-28T01:47:15Z

pkg/router/controller/router_controller.go

+		glog.Fatalf("Failed to sync router informer cache: %v", err)
+	}
+	c.processExistingItems()
+	c.firstSyncDone = true


The only part of this method that really belongs here is setting this Boolean (which needs to be under a lock).

smarterclayton · 2017-09-28T01:47:41Z

pkg/router/controller/router_controller.go

+		c.HandleEndpoints(watch.Added, item.(*kapi.Endpoints))
+	}
+
+	for _, item := range c.InformerCacheList(&routeapi.Route{}) {


This abstraction is unnecessary. Do this from the factory and just call the store directly.

pravisankar · 2017-09-28T18:01:58Z

@smarterclayton @knobunc rearranged the code as suggested, please review

smarterclayton · 2017-09-28T20:29:04Z

pkg/router/controller/factory/factory.go

-	field     fields.Selector
-	namespace string
+func (f *RouterControllerFactory) initCallbacks(rc *routercontroller.RouterController) {
+	rc.HasSyncedInformers = func() bool {


Do you still need this?

When re-sync is in progress, we want to reduce the number of reloads. We could fully rely on router coalescing and can get rid of this informer synced check. @knobunc @rajatchopra what do you think?

Tested with 1000 routes to check whether router coalescing is sufficient or informer synced check is necessary to reduce reloads. Router coalescing without sync check worked fine, so removed this unnecessary check.

smarterclayton · 2017-09-28T20:29:47Z

pkg/router/controller/factory/factory.go

-	}
-	if lw.field != nil {
-		field = lw.field.String()
+func (f *RouterControllerFactory) processExistingItems(rc *routercontroller.RouterController) {


godoc on this function and how it is used (and reason)

…nformers - Custom shared informer is used to leverage namespace, label and field filtering. (Auto generated shared informer does not allow this) - Listing resources by shared informers doesn't order by resource version/creation time. So custom lister for routes is used to order the route list by creation time and this will allow oldest route to be processed before new route to claim the host name. - Synchronization with the informer queue and cache is a bit difficult as the cache could have newer changes than what was pushed on to the queue. Luckily We only care about the first sync to avoid 503 status code for routes. - Handling first sync: * Informers are started with no registered event handlers * Wait for all informers to be synced * Block router reload * Get list of items from informers store and process manually * Perform router reload * Register router event handlers This guarantees first router sync is performed after processing all existing items. - Subsequent router syncs rely on informer syncing sate and uses rate limiter to coalesce changes.

pravisankar · 2017-09-29T19:30:54Z

@smarterclayton @knobunc @rajatchopra can you please take another look?

smarterclayton · 2017-09-29T19:36:44Z

Looks phenomenal, thanks for cleaning up. Very easy to read now.

/lgtm

openshift-merge-robot · 2017-09-29T19:36:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [smarterclayton]
~~pkg/router/OWNERS~~ [knobunc,smarterclayton]
~~test/integration/OWNERS~~ [knobunc,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

pravisankar · 2017-09-29T20:11:17Z

/retest

smarterclayton · 2017-09-29T22:45:37Z

Was preapproved due to importance to getting this fixed

openshift-merge-robot · 2017-09-30T01:22:01Z

Automatic merge from submit-queue.

Automatic merge from submit-queue. Sharded router based on namespace labels should notice routes immediately - Currently, sharded router based on namespace labels could take 2 resync intervals (10 to 15 mins) to notice new routes which may not be acceptable to some customers. This change allows routes to work immediately just like the non-sharded router behavior. - Watching project resource may not guarantee the order of the events, so there is no behavior change to shared router based on project labels. Trello card: https://trello.com/c/Q0puUQOT Rebased on top of #16315

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 13, 2017

openshift-merge-robot assigned rajatchopra and knobunc Sep 13, 2017

pravisankar added the component/routing label Sep 13, 2017

pravisankar mentioned this pull request Sep 13, 2017

[WIP] switch router to watch using shared informer #15645

Closed

3 tasks

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2017

pravisankar force-pushed the router-change-to-informer branch 2 times, most recently from ff000e8 to 3dc6842 Compare September 25, 2017 23:20

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2017

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 25, 2017

smarterclayton reviewed Sep 26, 2017

View reviewed changes

pravisankar force-pushed the router-change-to-informer branch from 3dc6842 to ba301d2 Compare September 26, 2017 23:23

pravisankar mentioned this pull request Sep 26, 2017

Sharded router based on namespace labels should notice routes immediately #16039

Merged

smarterclayton reviewed Sep 27, 2017

View reviewed changes

pravisankar force-pushed the router-change-to-informer branch from ba301d2 to d86352a Compare September 28, 2017 00:39

smarterclayton reviewed Sep 28, 2017

View reviewed changes

pravisankar force-pushed the router-change-to-informer branch from d86352a to 168938a Compare September 28, 2017 17:58

pravisankar force-pushed the router-change-to-informer branch 2 times, most recently from ed94e3d to f5ddc5e Compare September 28, 2017 18:15

smarterclayton reviewed Sep 28, 2017

View reviewed changes

pravisankar force-pushed the router-change-to-informer branch from f5ddc5e to fba2402 Compare September 29, 2017 19:03

openshift-merge-robot added the needs-api-review label Sep 29, 2017

Ravi Sankar Penta added 2 commits September 29, 2017 12:21

Deleted eventQueue, no longer used

74560f7

pravisankar force-pushed the router-change-to-informer branch from fba2402 to 74560f7 Compare September 29, 2017 19:21

openshift-merge-robot removed the needs-api-review label Sep 29, 2017

openshift-ci-robot assigned smarterclayton Sep 29, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2017

smarterclayton added the kind/bug Categorizes issue or PR as related to a bug. label Sep 29, 2017

openshift-merge-robot merged commit 2af8e92 into openshift:master Sep 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced event queue based watching resources in router with shared informers #16315

Replaced event queue based watching resources in router with shared informers #16315

pravisankar commented Sep 13, 2017 •

edited

Loading

pravisankar commented Sep 13, 2017

pravisankar commented Sep 13, 2017

dcbw commented Sep 13, 2017

knobunc commented Sep 19, 2017

smarterclayton commented Sep 19, 2017

smarterclayton commented Sep 19, 2017

smarterclayton commented Sep 20, 2017 via email

deads2k commented Sep 20, 2017

smarterclayton commented Sep 23, 2017 via email

pravisankar commented Sep 25, 2017

pravisankar commented Sep 25, 2017

smarterclayton Sep 26, 2017

smarterclayton Sep 27, 2017 •

edited

Loading

pravisankar commented Sep 27, 2017

smarterclayton commented Sep 27, 2017 via email

smarterclayton commented Sep 27, 2017 via email

smarterclayton Sep 27, 2017

pravisankar commented Sep 28, 2017

smarterclayton Sep 28, 2017

smarterclayton Sep 28, 2017

smarterclayton Sep 28, 2017

pravisankar commented Sep 28, 2017

smarterclayton Sep 28, 2017

pravisankar Sep 28, 2017

pravisankar Sep 29, 2017

smarterclayton Sep 28, 2017

pravisankar commented Sep 29, 2017

smarterclayton commented Sep 29, 2017

openshift-merge-robot commented Sep 29, 2017

pravisankar commented Sep 29, 2017

smarterclayton commented Sep 29, 2017

openshift-merge-robot commented Sep 30, 2017

Replaced event queue based watching resources in router with shared informers #16315

Replaced event queue based watching resources in router with shared informers #16315

Conversation

pravisankar commented Sep 13, 2017 • edited Loading

pravisankar commented Sep 13, 2017

pravisankar commented Sep 13, 2017

dcbw commented Sep 13, 2017

knobunc commented Sep 19, 2017

smarterclayton commented Sep 19, 2017

smarterclayton commented Sep 19, 2017

smarterclayton commented Sep 20, 2017 via email

deads2k commented Sep 20, 2017

smarterclayton commented Sep 23, 2017 via email

pravisankar commented Sep 25, 2017

pravisankar commented Sep 25, 2017

Choose a reason for hiding this comment

smarterclayton Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

pravisankar commented Sep 27, 2017

smarterclayton commented Sep 27, 2017 via email

smarterclayton commented Sep 27, 2017 via email

Choose a reason for hiding this comment

pravisankar commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Sep 28, 2017

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

pravisankar commented Sep 29, 2017

smarterclayton commented Sep 29, 2017

openshift-merge-robot commented Sep 29, 2017

pravisankar commented Sep 29, 2017

smarterclayton commented Sep 29, 2017

openshift-merge-robot commented Sep 30, 2017

pravisankar commented Sep 13, 2017 •

edited

Loading

smarterclayton Sep 27, 2017 •

edited

Loading