fix: use informer for istio gateways #4522
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @woehrl01. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
woehrl01
force-pushed
the
informer-gateways
branch
from
d477f90 to
b5f145f
Compare
Signed-off-by: Lukas Wöhrl <[email protected]>
woehrl01
force-pushed
the
informer-gateways
branch
from
b5f145f to
25eac69
Compare
|
/assign szuecs |
| gatewayInformer.Informer().AddEventHandler( | ||
| cache.ResourceEventHandlerFuncs{ | ||
| AddFunc: func(obj interface{}) { | ||
| log.Debug("gateway added") | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
I suggest to either remove this log or provide a more detailed debug log.
There was a problem hiding this comment.
Sure, should I also change the other informers? I just replicated the other ones. I suggest removing all of them. Wdyt?
There was a problem hiding this comment.
@mloiseleur the eventhandler is needed because of the following behaviour on the go-client: kubernetes/client-go#382, see also the comment above the first informer.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@mloiseleur do you have any updates on this? It seems like the "approved" label is missing here. Anything I can do about this? |
|
A maintainer will review it when he has time for final approval. The istio provider will move away from the tree, anyway. We have started the process to move the providers out of tree. |
|
@mloiseleur Why should Istio support be moved out of tree? It's about discovering DNS entries inside the cluster (its a configurable source). It's not a DNS provider like the list you have linked. |
|
Ah yes you're right. |
|
Any progress here? It this something which will be considered at all? |
|
@Raffo can you please have a look at this PR? Because of this not having fixed, external-dns is by far the most talking service to the api server if you have lots of virtual services. Thank you! |
|
@ivanfilippov @Dadeos-Menlo @AndrewCharlesHay Do you think one of you can review this PR ? |
|
/lgtm |
|
@Dadeos-Menlo: changing LGTM is restricted to collaborators In response to this:
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-sigs/prow repository. |
|
@Dadeos-Menlo In order to lgtm, following official guide, you'll need to Open an issue against the kubernetes/org repo. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mloiseleur 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Description
This PR changes to use an informer instead of direct api calls to read the gateway config. This solves a API ratelimit error on our setup and removes stress on the Kubernetes API
Following this change the tests needed to be updated:
Checklist