Native multi reflector #1692

Danil-Grigorev · 2025-02-10T16:07:38Z

Motivation

Exploring alternatives for performing reflection on multiple watches, and storing them in a shared cache. Purely a POC at the moment, inspired by #1687

Solution

since the question has showed up a few times. ultimately this is awkward because the hard generic use in ObjectRef<K> and in Store<K> which ties them to a particular resource. It feels like it shouldn't have to be this way because the Store::get takes an ObjectRef, which feels dynamically typed, but actually isnt. Tried out a couple of potentials and had to just resort to the dumb thing instead. Maybe there's some other things we can do for dynamics. Maybe worth exploring in an issue? Signed-off-by: clux <[email protected]>

codecov · 2025-02-10T16:21:37Z

Codecov Report

Attention: Patch coverage is 44.59459% with 41 lines in your changes missing coverage. Please review.

Project coverage is 75.5%. Comparing base (0bcc625) to head (a4035ee).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
kube-core/src/metadata.rs	5.3%	36 Missing ⚠️
kube-core/src/resource.rs	0.0%	2 Missing ⚠️
kube-runtime/src/reflector/store.rs	85.8%	2 Missing ⚠️
kube-core/src/object.rs	94.5%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1692     +/-   ##
=======================================
- Coverage   75.8%   75.5%   -0.3%     
=======================================
  Files         84      84             
  Lines       7630    7683     +53     
=======================================
+ Hits        5779    5794     +15     
- Misses      1851    1889     +38

Files with missing lines	Coverage Δ
kube-core/src/gvk.rs	`75.5% <ø> (ø)`
kube-runtime/src/controller/mod.rs	`30.2% <ø> (ø)`
kube-runtime/src/reflector/mod.rs	`100.0% <ø> (ø)`
kube-runtime/src/utils/reflect.rs	`100.0% <100.0%> (ø)`
kube-runtime/src/utils/watch_ext.rs	`22.3% <ø> (ø)`
kube-core/src/object.rs	`87.2% <94.5%> (+1.2%)`	⬆️
kube-core/src/resource.rs	`58.9% <0.0%> (-1.3%)`	⬇️
kube-runtime/src/reflector/store.rs	`96.9% <85.8%> (ø)`
kube-core/src/metadata.rs	`34.8% <5.3%> (-32.9%)`	⬇️

... and 2 files with indirect coverage changes

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev · 2025-02-13T11:01:34Z

kube-core/src/object.rs

+        } = DynamicList::deserialize(d)?;
+        let mut resources = vec![];
+        for o in items.iter_mut() {
+            o.types = Some(types.clone().singular());


Sorry for the ping @clux, just wanted to ask about this change. DynamicObject lists are affected by kubernetes/kubernetes#80609, and this performs correction.

k8s-openapi already does this for all generated objects during individual object deserialization, so the issue appears only with unstructured resource list.

Is this something which can be done in this instance? Issue may not go away for a while.

..pooosssibly. It's hard to tell from this change alone; we could preserve the kind like this with unit tests and care, but in theory overriding it to fix a bug is possible like this. I'm struggling a bit to visualise the direct use case for doing this conversion. Don't we already have the type information of the kind we are doing dynamic api calls for as an ApiResource?

If it is passed directly, then largely it is available, but it is on dev to store it and pass it around. Internals don't do that, and returned objects in the list as Vec<DynamicObject> always have empty type meta as the ListMeta goes out of scope, and the only clue is stored there.

The story is different in static types, as it is custom serialization which fixes that: https://github.com/Arnavion/k8s-openapi/blob/30a268f669650450fd4391d3785a078dbbf1d1ee/src/v1_32/api/core/v1/pod.rs#L153-L154

clux

some quick comments

clux · 2025-02-20T15:46:18Z

kube-core/src/metadata.rs

+    /// Construct a new `TypeMeta` for the object from the list `TypeMeta`.
+    ///
+    /// ```
+    /// # use k8s_openapi::api::core::v1::Pod;
+    /// # use kube_core::TypeMeta;
+    ///
+    /// let mut type_meta = TypeMeta::resource::<Pod>();
+    /// type_meta.kind = "PodList".to_string();
+    /// assert_eq!(type_meta.clone().singular().kind, "Pod");
+    /// assert_eq!(type_meta.clone().singular().api_version, "v1");
+    /// ```
+    pub fn singular(self) -> Self {
+        Self {
+            kind: self.kind.strip_suffix("List").unwrap_or(&self.kind).to_string(),
+            ..self
+        }
+    }


i think this fn is nice by itself. but it relies on context that is not clear from the name;

it's not clear that it works on a List type

it's not clear it's a noop on a non-list type

maybe it's better to have this fn named as fn singularize_list to resolve this.
feel free to do this as it's own pr.

clux · 2025-02-20T15:48:01Z

kube-core/src/metadata.rs

+    fn gvk(res: &Self::Resource) -> GroupVersionKind {
+        res.type_meta().try_into().unwrap_or_default()


we have derived Default on GroupVersionKind for this afaikt, but this is not particularly useful. I would rather not derive this Default (because it's not useful in the default state), and have this fn return an Option

clux · 2025-02-20T15:50:41Z

kube-runtime/src/reflector/store.rs

+pub trait CacheWriter<K> {
+    /// Applies a single watcher event to the store
+    fn apply_watcher_event(&mut self, event: &watcher::Event<K>);
+}


I think this kind of makes sense to traitify at some point. But I am not sure we are ready to do this yet. There are uncertainties around how it operates (e.g. the delete event issue).

clux · 2025-02-20T15:53:46Z

kube-core/src/metadata.rs

@@ -175,6 +193,123 @@ impl<K: Resource> Resource for PartialObjectMeta<K> {
    }
 }

+///
+pub trait TypedResource: Resource + Sized {


How does this trait differ from one of our existing ones here? How does this help out with solving the issue? I see a lot of trait magic here, and it's non-trivial for me to try to decipher this.

There is a problem with anything else available. DynamicObject is a special case of resource, and unlike any typed resource it has types field we can use.

Unfortunately for anything generated by k8s-openapi or derive CustomResourceDefinition types, TypeMeta is available from constant kind, group and version. There is no type field we can use, so data, even if returned from API server, is lost.

This ✨ magic ✨ makes it possible to have GVK of a resource available as if in a blanket implementation, and it is used for identification of resources in dynamic watchers cache, for routing of events.

This dynamic cache can later be used as a source to establish dynamic watches on statically typed resources, if used as a stream which filters resources by GVK and serializes it to the expected type. This is not a part of this implementation yet, thought.

Such cache, if passed around one or multiple controllers, may serve as an up-to-date state of all watched objects for read operations.

clux · 2025-02-20T16:01:07Z

kube-core/src/object.rs

+        } = DynamicList::deserialize(d)?;
+        let mut resources = vec![];
+        for o in items.iter_mut() {
+            o.types = Some(types.clone().singular());


..pooosssibly. It's hard to tell from this change alone; we could preserve the kind like this with unit tests and care, but in theory overriding it to fix a bug is possible like this. I'm struggling a bit to visualise the direct use case for doing this conversion. Don't we already have the type information of the kind we are doing dynamic api calls for as an ApiResource?

clux · 2025-02-20T16:01:53Z

kube-core/src/metadata.rs

+impl<K> TypedResource for K
+where
+    K: Resource,
+    (K, K::DynamicType): TypedResourceImpl<Resource = K>,


lol. yeah, this is a bit too much magic imo.

Danil-Grigorev force-pushed the native-multi-reflector branch 3 times, most recently from 450df09 to d7906eb Compare February 10, 2025 16:15

Danil-Grigorev force-pushed the native-multi-reflector branch 5 times, most recently from cb67680 to f26a217 Compare February 13, 2025 01:03

Exploring shared cache based on multi reflectors

a4035ee

Signed-off-by: Danil-Grigorev <[email protected]>

Danil-Grigorev force-pushed the native-multi-reflector branch from f26a217 to a4035ee Compare February 13, 2025 01:07

Danil-Grigorev commented Feb 13, 2025

View reviewed changes

clux reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native multi reflector #1692

Native multi reflector #1692

Danil-Grigorev commented Feb 10, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

Danil-Grigorev Feb 13, 2025

clux Feb 20, 2025

Danil-Grigorev Feb 20, 2025

clux left a comment

clux Feb 20, 2025 •

edited

Loading

clux Feb 20, 2025

clux Feb 20, 2025

clux Feb 20, 2025

Danil-Grigorev Feb 20, 2025

clux Feb 20, 2025

clux Feb 20, 2025

		fn gvk(res: &Self::Resource) -> GroupVersionKind {
		res.type_meta().try_into().unwrap_or_default()

Native multi reflector #1692

Are you sure you want to change the base?

Native multi reflector #1692

Conversation

Danil-Grigorev commented Feb 10, 2025

Motivation

Solution

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux left a comment

Choose a reason for hiding this comment

clux Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2025 •

edited

Loading

clux Feb 20, 2025 •

edited

Loading