-
Notifications
You must be signed in to change notification settings - Fork 494
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.
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
Set watch option #2822
Set watch option #2822
Conversation
@@ -275,6 +276,9 @@ func main() { | |||
}), | |||
Cache: cache.Options{ | |||
DefaultNamespaces: namespaces, | |||
DefaultLabelSelector: labels.SelectorFromSet(map[string]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the docs from the operator-sdk but they're out of date after this release kubernetes-sigs/controller-runtime#2300. This is the best way going forward!
I believe the reason for e2e test failures is that in the course of adding sidecars or instrumentations, we sometimes ask the API Server for resources owning the Pod - ReplicaSets, Deployments, and the like. Those don't have the label, and hence we fail. We probably need a more nuanced policy here. Something like:
Incidentally, are we doing the right thing by setting |
@swiatekm-sumo i see... i think this makes sense. I'll make some changes and we can test it out. I think we could even put this behind a featuregate given its just an option on the manager config, that would let us know pretty clearly if this is helpful for performance as well. My worry is that there's some unknown unknown consequence of this |
main.go
Outdated
&v1.Job{}: {}, | ||
&appsv1.Deployment{}: {}, | ||
&appsv1.DaemonSet{}: {}, | ||
&appsv1.StatefulSet{}: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need ReplicaSet and CronJob here as well. Or maybe just Job, ReplicaSet and DaemonSet (the immediate Pod owners) are sufficient? Both are probably worth testing.
This may also be easier to test via envtest, as we're running into problems at the webhook level. In general, I think we should move our setup code out of |
Description: This is a performance improvement for us to only watch items we manage.
Link to tracking Issue(s):
Partially resolves #2808
Testing: Tested locally
Documentation: n/a