When fetching the pod data for a known core Kind, use a typed client request instead of Unstructured #1457
Conversation
…request instead of Unstructured. This allows those requests to hit the informer cache while Unstructured gets cannot. If controller-runtime adds support for reading Unstructured gets from the cache, this could be reverted in the future. This also adds the permissions required for setting up watches. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
|
Might be good to restrict the additional permissions to only list+watch on deployments and statefulsets since I think those are the only two core objects where it makes sense for these features to activate? |
|
Also if doing that, kind of might as well limit the code to just those and drop the weird reflect stuff. Thoughts? |
There was a problem hiding this comment.
Generally looking good (the weird reflection stuff is cool 😄), but you are right, Deployment and StatefulSet are the only core resources, that make sense to scale.
I am thinking whether we shouldn't put these exceptions (types that we know that are Scalable and/or Podspecable) at one place in the code and reuse this across the codebase, for example in this PR as well: #1458. This way, if we potentially add other "known" types (and not neccessary core types), we can do that addition ideally in one place in the code.
|
#1458 is different in that what it wants is the scaling data rather than the podspec, but maybe a utils library with calls like |
|
Yeah, messing around with it I don't really see any utility function that allows for reuse. So I think the main choice here is reflect vs hardwiring it for Deployment and StatefulSet. I think I favor the latter as much less likely to break at runtime. |
|
ACK, agree with you, hardwiring for Deployment and StatefulSet is a better solution. |
… StatefulSet. These are the only two core types which can be scaled anyway and this code is much less likely to break at runtime in unexpected ways. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
These should be the only two types that are needed with it, so minimal permissions are nice. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
|
Baked it down to just the two types. Bit more verbose but more compile-time safe so I think it's a win. |
…request instead of Unstructured (kedacore#1457) * When fetching the pod data for a known core Kind, use a typed client request instead of Unstructured. This allows those requests to hit the informer cache while Unstructured gets cannot. If controller-runtime adds support for reading Unstructured gets from the cache, this could be reverted in the future. This also adds the permissions required for setting up watches. Signed-off-by: Noah Kantrowitz <noah@coderanger.net> * Fix controller-gen syntax. Signed-off-by: Noah Kantrowitz <noah@coderanger.net> * Update changelog. Signed-off-by: Noah Kantrowitz <noah@coderanger.net> * Replace reflect magic with a direct implementation for Deployment and StatefulSet. These are the only two core types which can be scaled anyway and this code is much less likely to break at runtime in unexpected ways. Signed-off-by: Noah Kantrowitz <noah@coderanger.net> * Only request cache permissions for Deployments and StatefulSets. These should be the only two types that are needed with it, so minimal permissions are nice. Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
This allows those requests to hit the informer cache while Unstructured gets cannot.
If controller-runtime adds support for reading Unstructured gets from the cache, this could be reverted in the future. This also adds the permissions required for setting up watches.
Checklist
Refs #1449