pkg/ansible/proxy; Integrate caching to AO proxy #760

dymurray · 2018-11-19T16:24:07Z

Description of the change:
This change passes in the manager's cache into the proxy to prevent spamming APIserver with GET requests.

Motivation for the change:
Performance enhancements

Metrics
1000 sequential requests against a resource.
Before changes:

1m1.615s

After changes:

0m15.547s

Overall this is a ~75% reduction in the time it takes to GET resources from the cluster.

TODO

Add prometheus metrics
Update proxy tests once https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/controller/controllertest/util.go#L44 is implemented to allow us to use a fake cache

openshift-ci-robot · 2018-11-19T16:24:21Z

Hi @dymurray. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

dymurray · 2018-11-19T17:29:05Z

I think it makes sense to rename the handler function... thoughts on changing InjectOwnerReferenceHandler to RequestHandler or something more generic?

pkg/ansible/proxy/proxy.go

dymurray · 2018-11-19T20:51:48Z

TODO: Working on adding tests to the proxy

joelanford · 2018-11-20T16:45:07Z

I think it makes sense to rename the handler function... thoughts on changing InjectOwnerReferenceHandler to RequestHandler or something more generic?

Another option could be to put the caching logic into a separate handler, since it doesn't look like it's used for the owner reference injection. Then we can add both to the HandlerChain

@shawn-hurley mentioned adding another handler at some point that automatically registers new watches for resources created by the ansible run, so that could be a third handler that gets added to the chain.

dymurray · 2018-11-20T20:35:23Z

@joelanford Makes sense to me. If others agree I'm happy to make that change.

shawn-hurley · 2018-11-20T21:37:59Z

I do like the idea of a handler chain. This would make turning off one or the other of the handlers easier as well.

if you want to do this as a follow-on I would be ok, if it is going to take minimal time and you can still use the tests that you wrote then if you could that would be awesome!

@joelanford sound good to you?

shawn-hurley · 2018-11-21T14:02:01Z

pkg/ansible/proxy/proxy_test.go

+	"sigs.k8s.io/controller-runtime/pkg/manager"
+)
+
+func TestHandler(t *testing.T) {


I think that instead of running the proxy, I think using this package would be the correct approach: https://golang.org/pkg/net/http/httptest/ to test the server and request

I would also like to see some tests using a fake cache and testing the correct logic.

https://godoc.org/k8s.io/client-go/tools/cache/testing#FakeControllerSource
and
https://godoc.org/sigs.k8s.io/controller-runtime/pkg/cache/informertest#FakeInformers

Lastly, if the test cases could look like the table driven tests

I'm having a lot of trouble integrating this. Will speak with you tomorrow about it.

shawn-hurley · 2018-11-21T14:02:48Z

pkg/ansible/proxy/requestfactory/requestinfo.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
+
+	"k8s.io/klog"


we should change this to use logr (edit to state the correct logger)

I used logf from controller-runtime since that is what the proxy is using, is there anything wrong with that approach? Wasn't sure the benefits of using logr.

mhrivnak · 2018-11-21T14:40:32Z

pkg/ansible/proxy/proxy.go

+
+			// Set X-Cache header to signal that response is served from Cache
+			w.Header().Set("X-Cache", "HIT")
+			// Pretty printing when hitting apiserver from CLI


Should there be a user agent check here to determine if the client is a human at a CLI?

We could, is there a benefit to not indenting the JSON if this isn't a human from a CLI? My comment didn't intend to imply we only want to indent for pretty printing to a CLI, I was simply trying to mimic the response that the APIserver responds with. I can look to see if the apiserver checks the user agent before writing the response.

Gotcha. The comment implied to me that you only wanted to pretty-print when the CLI is the client. If you want to always pretty-print, I suggest just removing the comment.

pkg/ansible/proxy/proxy.go

pkg/ansible/proxy/requestfactory/requestinfo.go

dymurray · 2018-11-27T20:51:27Z

Adding a TODO on refactoring the proxy tests. It appears we have things we need to update in controller-runtime before we can use a fake cache.

shawn-hurley · 2018-11-28T13:41:03Z

@dymurray in the TODO can you mentation that the implementation fo this: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/controller/controllertest/util.go#L44 needs to be done before we can truly use this as a fake.

shawn-hurley

LGTM

shawn-hurley · 2018-12-04T13:39:51Z

@dymurray This looks like the Gopkg.toml is not correct, can you update?

@mhrivnak can you please re-review

dymurray force-pushed the cache branch from 73e0001 to d1a825c Compare November 19, 2018 17:02

shawn-hurley requested changes Nov 19, 2018

View reviewed changes

pkg/ansible/proxy/proxy.go Outdated Show resolved Hide resolved

pkg/ansible/proxy/proxy.go Outdated Show resolved Hide resolved

shawn-hurley added the language/ansible Issue is related to an Ansible operator project label Nov 19, 2018

dymurray changed the title ~~[WIP] pkg/ansible/proxy; Integrate caching to AO proxy~~ pkg/ansible/proxy; Integrate caching to AO proxy Nov 20, 2018

openshift-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 20, 2018

shawn-hurley reviewed Nov 21, 2018

View reviewed changes

mhrivnak suggested changes Nov 21, 2018

View reviewed changes

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2018

dymurray force-pushed the cache branch from 2951701 to 4039295 Compare November 27, 2018 20:25

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 29, 2018

dymurray force-pushed the cache branch from 116d8a7 to a0d691c Compare November 29, 2018 18:24

shawn-hurley approved these changes Nov 29, 2018

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018

dymurray added 13 commits December 3, 2018 16:22

First attempt at adding a cache

a2bd8bb

Remove apiserver dep

ce0513a

Fix case of errors

23efdb8

Set GVK on unstructured object

072a136

Add unit tests for proxy

a51e534

Updates to unit test

c1ff067

Fix unit tests

40857c9

Updates based on feedback

cce0050

Add cache handler to properly use handler chain

381ebf6

Replace klog with logf

8e793c5

Add http.Error calls

f431c60

Remove pretty print comment

0b8cda1

Update Gopkg.lock

1170cd2

dymurray force-pushed the cache branch from 27b900f to 1170cd2 Compare December 3, 2018 21:27

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2018

Update gopkg.lock with proper dep version

af8ccaf

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 4, 2018

Fix ansible tests

d122070

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 4, 2018

Initialize default restmapper if nil

8cc1a3f

dymurray force-pushed the cache branch from 1b45655 to 8cc1a3f Compare December 4, 2018 15:54

mhrivnak approved these changes Dec 4, 2018

View reviewed changes

shawn-hurley merged commit 061d185 into operator-framework:master Dec 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/ansible/proxy; Integrate caching to AO proxy #760

pkg/ansible/proxy; Integrate caching to AO proxy #760

dymurray commented Nov 19, 2018 •

edited

Loading

openshift-ci-robot commented Nov 19, 2018

dymurray commented Nov 19, 2018

dymurray commented Nov 19, 2018

joelanford commented Nov 20, 2018

dymurray commented Nov 20, 2018

shawn-hurley commented Nov 20, 2018

shawn-hurley Nov 21, 2018

dymurray Nov 26, 2018

shawn-hurley Nov 21, 2018 •

edited

Loading

dymurray Nov 29, 2018

mhrivnak Nov 21, 2018

dymurray Nov 26, 2018

mhrivnak Nov 30, 2018

dymurray commented Nov 27, 2018

shawn-hurley commented Nov 28, 2018

shawn-hurley left a comment

shawn-hurley commented Dec 4, 2018

pkg/ansible/proxy; Integrate caching to AO proxy #760

pkg/ansible/proxy; Integrate caching to AO proxy #760

Conversation

dymurray commented Nov 19, 2018 • edited Loading

openshift-ci-robot commented Nov 19, 2018

dymurray commented Nov 19, 2018

dymurray commented Nov 19, 2018

joelanford commented Nov 20, 2018

dymurray commented Nov 20, 2018

shawn-hurley commented Nov 20, 2018

shawn-hurley Nov 21, 2018

Choose a reason for hiding this comment

dymurray Nov 26, 2018

Choose a reason for hiding this comment

shawn-hurley Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

dymurray Nov 29, 2018

Choose a reason for hiding this comment

mhrivnak Nov 21, 2018

Choose a reason for hiding this comment

dymurray Nov 26, 2018

Choose a reason for hiding this comment

mhrivnak Nov 30, 2018

Choose a reason for hiding this comment

dymurray commented Nov 27, 2018

shawn-hurley commented Nov 28, 2018

shawn-hurley left a comment

Choose a reason for hiding this comment

shawn-hurley commented Dec 4, 2018

dymurray commented Nov 19, 2018 •

edited

Loading

shawn-hurley Nov 21, 2018 •

edited

Loading