Skip to content
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

FederatedObject.spec.template should have type map[string]interface{}? #284

Open
gary-lgy opened this issue Nov 20, 2023 · 3 comments
Open
Labels
enhancement New feature or request

Comments

@gary-lgy
Copy link
Member

gary-lgy commented Nov 20, 2023

FederatedObject.spec.template is currently of type k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1.JSON which stores the raw json bytes. This type was used because controller-gen refuses to implement support for map[string]interface{} fields for CRD generation as discussed in kubernetes-sigs/controller-tools#636. However, map[string]interface{} would be more efficient and ergonomic as it avoids the unmarshaling whenever we need to access the template. We'd like to use map[string]interface{} if we can find a way to hack controller-gen to generate CRDs nonetheless.

We could start by looking at post-gen patches (which is already done in config/crds/patches). Maybe we could ask controller-gen to ignore the template field and let the post-gen patches add it?

@gary-lgy gary-lgy added enhancement New feature or request good first issue Good for newcomers labels Nov 20, 2023
@SOF3
Copy link
Member

SOF3 commented Nov 20, 2023

How often do we need to unmarshal the template? It saves the cost of deserializing them every time federated object is updated (especially status-only updates), and it reduces memory footprint. Any benchmarks to support the argument that late deserializations are more frequent than informer deserializations?

@gary-lgy
Copy link
Member Author

How often do we need to unmarshal the template? It saves the cost of deserializing them every time federated object is updated (especially status-only updates), and it reduces memory footprint. Any benchmarks to support this argument?

No benchmarks, but you do have a valid point.
Currently template is accessed mainly via the GetTemplateAsUnstructured method, which does the deserialization. My concern is mainly when the method is invoked multiple times during the reconcile loop, e.g. in event handler's FilterFunc, UpdateFunc, then in reconcile, and potentially in downstream util methods. Ergonomics is an important factor too.

@gary-lgy gary-lgy removed the good first issue Good for newcomers label Nov 20, 2023
@gary-lgy gary-lgy changed the title FederatedObject.spec.template should have type map[string]interface{} for more efficient access FederatedObject.spec.template should have type map[string]interface{}? Nov 20, 2023
@SOF3
Copy link
Member

SOF3 commented Nov 20, 2023

Let's review the GetTemplateAsUnstructured usages one by one:

  • federate: It is only used to retrieve the GVK of the federated object. Could we just parse it as a metav1.GroupVersionKind there, or maybe just store the GVK in federated object labels? (Such a label would be useful for users anyway)
  • follower: Same as above, except we only need metav1.GroupKind
  • nsautoprop: Same as above, used to filter federated objects. (I even wanted to suggest using a filtered watch, but apparently it doesn't work well with shared informers)
  • override (image overrider): this is the scope of resource interpreters, see below
  • scheduler: We only need the metadata (labels/anotations). Allocating so many hashmaps for the nested objects is still wasteful.
  • status/statusaggregator: Same as federate controller, only need metav1.GroupVersionKind
  • sync: OK, this is where we actually need a map[string]any in order to execute JSONPatch properly
  • util/pod: See resource interpreter below
  • util/propagationstatus: This compares two JSON objects. Not sure if there are more efficient ways to do this. But this is only called from statusaggregator anyway.

For resource interpreter cases: If we support interpreter plugins written in Go, they'd better be deserialized into their native types. If we support interpreter plugins in some dynamic library or scripting language (WASM, Lua, etc, whatever), passing the object in JSON is probably the most efficient way to cross language boundaries. Allocating it into map[string]any in advance does not seem to provide any notable benefit.

So given these use cases, the only reasonable use cases would be sync and statusaggregator, which most likely reconcile much less frequently than informer updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants