kubernetes: k8s.io/client-go/tools/cache/Reflector's paging is duplicated

What happened: I was studying the code in https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/client-go/tools/cache and noticed that the paginated list reflectors do against the API server to initially populate informer’s caches is doubly paginated. In fact, the reflector manually performs pagination: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/reflector.go#L206-L216 . Notice how the page function of the reflector’s paginator invokes the list function of the reflector’s ListerWatcher: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/reflector.go#L209 . That ListerWatcher is of type https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/listwatch.go#L58 ; its list function (the one invoked by the reflector’s paginator paging function) in turn uses paging: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/listwatch.go#L106 . The result is that reflectors use double pagination: they use a paging function that in turn uses paging. This seems to me very odd and undesirable.

What you expected to happen: I expected reflector’s list to be paginated only once.

How to reproduce it (as minimally and precisely as possible): I assume there’s a way to verify the issue by examining any running Kubernetes controller with no additional modifications, but I don’t know how to do that, so I took a different approach.

Here you can find a fork of https://github.com/kubernetes/sample-controller that has been modified as follows:

Dependencies are vendored; vendor is included in the github repo.
The vendored dependency k8s.io/client-go/tools/cache has been modified by inserting a print of the stack trace in the paging function of the pager created by the ListerWatcher (not the one created by the reflector).
A Dockerfile, a Makefile and a few YAML manifests have been included to build and deploy the sample controller modified with a print of the stack trace in the logs.

You can deploy the modified controller with the appropriate Makefile target in the repo https://github.com/matte21/sample-controller/tree/print-pager-stack-trace ; if you examine the controller logs you should see the stack trace print and notice that double-paging is performed. I did that, here’s the stack trace:

 1: goroutine 13 [running]:
 2: runtime/debug.Stack(0x0, 0x0, 0x0)
 3:		/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
 4: runtime/debug.PrintStack()
 5: 	/usr/local/go/src/runtime/debug/stack.go:16 +0x22
 6: k8s.io/client-go/tools/cache.(*ListWatch).List.func1(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x140efa7, ...)
 7:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/cache/listwatch.go:108 +0x40
 8: k8s.io/client-go/tools/pager.SimplePageFunc.func1(0x1626fa0, 0xc00009c210, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
 9:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/pager/pager.go:40 +0x64
10: k8s.io/client-go/tools/pager.(*ListPager).List(0xc0000a0bc0, 0x1626fa0, 0xc00009c210, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
11:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/pager/pager.go:88 +0x12d
12: k8s.io/client-go/tools/cache.(*ListWatch).List(0xc0002cbc80, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
13:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/cache/listwatch.go:111 +0x124
14: k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1.1.2(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x140efa7, ...)
15:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/cache/reflector.go:209 +0x72
16: k8s.io/client-go/tools/pager.SimplePageFunc.func1(0x1626fa0, 0xc00009c208, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
17:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/pager/pager.go:40 +0x64
18: k8s.io/client-go/tools/pager.(*ListPager).List(0xc0000a0f10, 0x1626fa0, 0xc00009c208, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
19:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/pager/pager.go:88 +0x12d
20: k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1.1(0xc00007c5a0, 0xc00003c630, 0xc00033a180, 0xc00003a160, 0xc00003a170, 0xc000334300)
21:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/cache/reflector.go:215 +0x190
22: created by k8s.io/client-go/tools/cache.(*Reflector).ListAndWatch.func1
23:		/Users/matteo/debug-double-paging/sample-controller/vendor/k8s.io/client-go/tools/cache/reflector.go:200 +0x29d

Notice how a paging function at https://github.com/kubernetes/client-go/blob/master/tools/pager/pager.go#L88 gets called first at line 19 (this is the reflector’s pager, the topmost one) and then again at line 11 (this is the ListerWatcher’s pager, the innermost one) .

Anything else we need to know?: I’ll open a PR to remove paging from the reflector and keep it only in the ListerWatcher; IMO whether paging is used is a detail of how the LIST is performed which fits better in the ListerWatcher than the reflector. Notice that currently actual pagination is not done because the reflector uses a watch cache and pagination from a watch cache is not supported. That fact, however, is orthogonal to this issue.

Edit: fixed a typo.

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 18 (15 by maintainers)

Most upvoted comments

The result is that reflectors use double pagination: they use a paging function that in turn uses paging. This seems to me very odd and undesirable.

This might not be as bad as it looks at first glance. Pagers do compose. What will happen here is that a page limit will be set on the options passed from the outer pager to inner pager. If the inner supports paging, the passed in limit will override the limit set on the inner pager. The inner pager will return a response without a continue, the outer pager will know it got a full response and that is does not need to do any paging itself. If the ListWatcher doesn’t do paging (e.g. DisableChunking is set), then it will return a continue and the outer pager will see this and perform the paging by calling List on the ListWatcher for each page. Either way a single pager does the actual paging.

If we did decide remove the pager in the reflector, we need to be very careful to understand the impact of all reflectors. For example, the watch cache configures it’s reflector with a large page size, and the lister watcher used there would need to be updated do to the paging with that page size instead. I expect it’s quite a bit of work to sort this all out.

(edit: expanded above to clarify behavior in more detail)

jpbetz on Oct 10, 2019