kubewharf / kubeadmiral

Multi-Cluster Kubernetes Orchestration
https://kubeadmiral.io
Apache License 2.0
802 stars 95 forks source link

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

Open gary-lgy opened 10 months ago

gary-lgy commented 10 months ago

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?

SOF3 commented 10 months ago

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 commented 10 months ago

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.

SOF3 commented 10 months ago

Let's review the GetTemplateAsUnstructured usages one by one:

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.