kubeadmiral icon indicating copy to clipboard operation
kubeadmiral copied to clipboard

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

Open gary-lgy opened this issue 1 year ago • 3 comments

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 https://github.com/kubernetes-sigs/controller-tools/issues/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 avatar Nov 20 '23 07:11 gary-lgy

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?

SOF3 avatar Nov 20 '23 07:11 SOF3

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 avatar Nov 20 '23 08:11 gary-lgy

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.

SOF3 avatar Nov 20 '23 08:11 SOF3