8000 Add EventHandler that remaps APIManager-based Route events to APIManager events by miguelsorianod · Pull Request #561 · 3scale/3scale-operator · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add EventHandler that remaps APIManager-based Route events to APIManager events #561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 18, 2021
Merged

Add EventHandler that remaps APIManager-based Route events to APIManager events #561

merged 1 commit into from
Jan 18, 2021

Conversation

miguelsorianod
Copy link
Contributor
@miguelsorianod miguelsorianod commented Jan 12, 2021 •
edited
Loading

This PR contains a possible implementation on how to detect changes that happen on Route objects that are automatically created by Zync when an APIManager installation is performed. This needs additional specific logic because Zync-managed routes do not contain an OwnerReference to an APIManager, so the common EnqueueRequestForOwner handler cannot be used.
It also implements remapping directly owned APIManager Route events to APIManager events (like the backend route).

The approach that this PR follows is based on the K8s EventHandler mechanism (https://godoc.org/sigs.k8s.io/controller-runtime/pkg/handler#EventHandler):

  • A new mapper named APIManagerRoutesEventMapper has been implemented and it watches changes of Route objects in the namespace. When those Route object change it inspects the event and:
    • If the route contains an OwnerReference that is an APIManager it converts the event to an APIManager event
    • If the route contains an OwnerReference that is a DeploymentConfig named 'zync-que' it then recursively resolves the ownerReference and repeats the OwnerReference evaluation process. It does so even if the route event is a zync-managed route that is not one of the default routes created when deploying an APIManager
      • If the recursive OwnerReference evaluation fails we log it and ignore it and continue the processing of other possible OwnerReferences
    • If the route contains an OwnerReference that is not an APIManager nor the 'zync-que' DeploymentConfig then it continues with the processing of other possible OwnerReferences in the route. If there are no other OwnerReferences then the event is ignored(discarded)
    • If the route does not contain an OwnerReference then the event is ignored(discarded)
  • At the APIManager controller level a Watch on Route objects is set with the APIManagerRoutesEventMapper handler.

The contents of this PR are built on top of #549 PR.

If we consider this a suitable approach and maintainable over the long term we can merge to the other branch.

@miguelsorianod miguelsorianod requested a review from eguzki January 12, 2021 16:24
@miguelsorianod
Copy link
Contributor Author

Some example log messages output in the new handler on some scenarios:

When a zync-managed Route event happens:

{"level":"Level(-2)","ts":1610466829.4573402,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Processing meta object","Name":"zync-3scale-api-7fv8b","Namespace":"msoriano-test"}
{"level":"Level(-2)","ts":1610466829.4576368,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Evaluating OwnerReference","GroupVersion":"apps.openshift.io/v1","Kind":"DeploymentConfig","Name":"zync-que"}
{"level":"Level(-2)","ts":1610466829.4576936,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"OwnerReference to Zync-Que detected. Recursively looking for APIManager OwnerReferences..."}
{"level":"Level(-2)","ts":1610466829.4578168,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Processing meta object","Name":"zync-que","Namespace":"msoriano-test"}
{"level":"Level(-2)","ts":1610466829.4578867,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Evaluating OwnerReference","GroupVersion":"apps.3scale.net/v1alpha1","Kind":"APIManager","Name":"example-apimanager"}
{"level":"Level(-2)","ts":1610466829.4579232,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"APIManager OwnerReference detected. Reenqueuing as APIManager event","APIManager name":"example-apimanager","APIManager namespace":"msoriano-test"}

When backend route is found:

{"level":"Level(-2)","ts":1610467389.1055357,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Processing meta object","Name":"backend","Namespace":"msoriano-test"}
{"level":"Level(-2)","ts":1610467389.1056328,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Evaluating OwnerReference","GroupVersion":"apps.3scale.net/v1alpha1","Kind":"APIManager","Name":"example-apimanager"}
{"level":"Level(-2)","ts":1610467389.1056883,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"APIManager OwnerReference detected. Reenqueuing as APIManager event","APIManager name":"example-apimanager","APIManager namespace":"msoriano-test"}

When a zync-manasged route is found but its zync-que ownerreference is specified but it does not exist. This can happen for example when APIManager object is deleted:

{"level":"Level(-2)","ts":1610467822.594731,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Processing meta object","Name":"zync-3scale-master-k4q7k","Namespace":"msoriano-test"}
{"level":"Level(-2)","ts":1610467822.5947793,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Evaluating OwnerReference","GroupVersion":"apps.openshift.io/v1","Kind":"DeploymentConfig","Name":"zync-que"}
{"level":"Level(-2)","ts":1610467822.5948017,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"OwnerReference to Zync-Que detected. Recursively looking for APIManager OwnerReferences..."}
{"level":"error","ts":1610467822.5948298,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"Could not get object","Kind":"DeploymentConfig","APIVersion":"apps.openshift.io/v1","Name":"zync-que","Namespace":"msoriano-test","stacktrace":"github.com/go-logr/zapr.(*zapLogger).Error\n\t/home/msoriano/go/pkg/mod/github.com/go-logr/zapr@v0.1.1/zapr.go:128\ngithub.com/3scale/3scale-operator/controllers/apps.(*APIManagerRoutesHandler).getAPIManagerOwnerReconcileRequest\n\t/home/msoriano/go/src/github.com/3scale/3scale-operator/controllers/apps/apimanager_managed_routes_handler.go:134\ngithub.com/3scale/3scale-operator/controllers/apps.(*APIManagerRoutesHandler).Delete\n\t/home/msoriano/go/src/github.com/3scale/3scale-operator/controllers/apps/apimanager_managed_routes_handler.go:68\nsigs.k8s.io/controller-runtime/pkg/source/internal.EventHandler.OnDelete\n\t/home/msoriano/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.6.3/pkg/source/internal/eventsource.go:176\nk8s.io/client-go/tools/cache.(*processorListener).run.func1\n\t/home/msoriano/go/pkg/mod/k8s.io/client-go@v0.18.6/tools/cache/shared_informer.go:746\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1\n\t/home/msoriano/go/pkg/mod/k8s.io/apimachinery@v0.18.6/pkg/util/wait/wait.go:155\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil\n\t/home/msoriano/go/pkg/mod/k8s.io/apimachinery@v0.18.6/pkg/util/wait/wait.go:156\nk8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/home/msoriano/go/pkg/mod/k8s.io/apimachinery@v0.18.6/pkg/util/wait/wait.go:133\nk8s.io/apimachinery/pkg/util/wait.Until\n\t/home/msoriano/go/pkg/mod/k8s.io/apimachinery@v0.18.6/pkg/util/wait/wait.go:90\nk8s.io/client-go/tools/cache.(*processorListener).run\n\t/home/msoriano/go/pkg/mod/k8s.io/client-go@v0.18.6/tools/cache/shared_informer.go:738\nk8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1\n\t/home/msoriano/go/pkg/mod/k8s.io/apimachinery@v0.18.6/pkg/util/wait/wait.go:73"}
{"level":"Level(-3)","ts":1610467822.594939,"logger":"controllers.APIManager.APIManagerRoutesHandler","msg":"No APIManager OwnerReference detected"}

@miguelsorianod miguelsorianod changed the title Add EventHandler that remaps APIManager-based route events to APIManager events Add EventHandler that remaps APIManager-based Route events to APIManager events Jan 12, 2021
@eguzki
Copy link
Member
eguzki commented Jan 14, 2021 •
edited
Loading
  • I like the general approach. Basically it extends the "Directly Owned" watch to "IsAncestorOf". I think it deserves a try.
  • Instead of implementing a EventHandler, why not use EnqueueRequestsFromMapFunc? The mapping function would be basically the same with less glue code.
  • The current "mapping" function maps when the child is zync-que object. Not sure we should limit to that or implement generic approach of "walk route ancestors until an APIManager is found and then enqueue the request if such ancestor is found"
  • An alternative mapping function implementation would be remove the recursive approach and look for the direct owner reference of the zync-que. Less generic. More easy to undertand. Recursive algorithms are useful for walking graphs of any height. We already know it is only one level further. Just food for though.
  • The generic approach of "AnyAncestor" of any level (not just one level) is also interesting. It can be implemented using recursive approach, but also loop approach. Just food for though.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Jan 14, 2021
eguzki
eguzki reviewed Jan 14, 2021
View reviewed changes
@miguelsorianod
Copy link
Contributor Author

The current "mapping" function maps when the child is zync-que object. Not sure we should limit to that or implement generic approach of "walk route ancestors until an APIManager is found and then enqueue the request if such ancestor is found"

At the moment I don't see a need to implement it for all cases. If we need it in the future we can change it 👍 . Additionally I do it that way as a sort of optimization for our use case where we know we routes will either directly have an ownerreference or be coming from zync-que

Instead of implementing a EventHandler, why not use EnqueueRequestsFromMapFunc? The mapping function would be basically the same with less glue code.

I'll take a look at it to see how it fits 👍

eguzki
eguzki reviewed Jan 14, 2021
View reviewed changes
@miguelsorianod
Copy link
Contributor Author
miguelsorianod comment 8000 ed Jan 14, 2021 •
edited
Loading

Regarding EnqueueRequestsFromMapFunc, the linked documentation is not the appropriate one. We are using controller-runtime 0.6.3. The documentation from it is:
https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.6.3/pkg/handler#EnqueueRequestsFromMapFunc

It uses different types and method signatures.

We can implement using this. When operator-sdk upgrades to the newer version of controller-runtime we will have to migrate to the new types of EnqueueRequestsFromMapFunc. It shouldn't be too hard but it will be required.

@miguelsorianod
Copy link
Contributor Author

@eguzki I've refactored the apimanager routes handler to a handler.Mapper interface to be able to be used in a EnqueueRequestForOwner event handler. I've added it as a new commit. In case we are ok with the changes I'll squash them.

@miguelsorianod
Copy link
Contributor Author

An alternative mapping function implementation would be remove the recursive approach and look for the direct owner reference of the zync-que. Less generic. More easy to undertand. Recursive algorithms are useful for walking graphs of any height. We already know it is only one level further. Just food for though.

You are right. I'm doubting between simplifying to just look at the direct owner or leaving what we have right now to give it space in case we need to generalize it in the future.

@miguelsorianod miguelsorianod requested a review from eguzki January 14, 2021 16:28
eguzki
eguzki reviewed Jan 15, 2021
View reviewed changes
@eguzki eguzki assigned miguelsorianod and unassigned eguzki Jan 15, 2021
@miguelsorianod miguelsorianod force-pushed the add-status-apimanagerrouteshandler branch from a98793c to 50c3f6a Compare January 15, 2021 15:53
@miguelsorianod miguelsorianod force-pushed the add-status-apimanagerrouteshandler branch from 50c3f6a to 4922a31 Compare January 15, 2021 16:26
@miguelsorianod miguelsorianod requested a review from eguzki January 15, 2021 16:47
@miguelsorianod miguelsorianod force-pushed the add-status-apimanagerrouteshandler branch from 4922a31 to ccd6fce Compare January 15, 2021 16:49
@miguelsorianod miguelsorianod force-pushed the add-status-apimanagerrouteshandler branch from ccd6fce to 7021290 Compare January 15, 2021 16:50
@miguelsorianod miguelsorianod merged commit c18f516 into improve-apimanager-status Jan 18, 2021
@miguelsorianod miguelsorianod deleted the add-status-apimanagerrouteshandler branch January 18, 2021 13:04
@miguelsorianod miguelsorianod mentioned this pull request Jan 18, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eguzki eguzki eguzki approved these changes

Assignees

@eguzki eguzki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@miguelsorianod @eguzki
0