Remove dyntype from ObjectRef

[clux]

An experiment based on things discussed with @nightkr .

Basically, #1038 is blocked because ObjectRef equality is problematic when we change the DynamicType because it affects how controllers check equality. Teo suggested to lift the dyntype concerns into the reflector and this is a WIP for that.

We have added a Inspect trait to look up TypeMeta at runtime (without the need for complicated associated types). This should always work except in the few pathological cases and it simplifies usage everywhere because we can blanket implement the trait for all normal cases (the two dynamic ones plus the static ones).

Things to note:

• Resource trait also extended with a ::typemeta method to aid the old-style erasure in ObjectRef
• ObjectRef now has a ::with_types field that plugs in typemeta (possibly via Resource::typemeta from a static type)
• conversion from ObjectRef to ObjectReference now a try_from (user might not always supply GVK data, even though we do)
• lots of _with suffixed and dynamic concerns like watches_with owns_with disappear in controller/mod
• lots of signatures simplified in reflector and store
• owner filtering is now done dynamically in trigger_owners against the owner dyntype (which is still required)
• formatting of ReconcileRequests and its relied upon ObjectRef display is mostly unchanged because the GVK data is now fetched via the typemeta getter

There are also possibly more places we can kill dyntype in controller, but this is is a minimal MVP.

[clux]

Ok, have done a lot of work here to help us get the type information into kube_runtime's ObjectRef while at the same time dropping dyntype from ObjectRef. It took a convoluted route of adding an entirely new TypeInfoInspect trait that does a lot of the same things to Resource (but can only be used to peek into an active object). This should let us drop most uses of dyntype inside runtime and thus kill all those _with helpers therein.

I think it's actually possible to make the signatures involving Resource significantly better by not having the associated type DynamicType be generic and instead hardcode ApiResource for dynamic lookups, but that's one for a later one.

[codecov-commenter]

Codecov Report

Merging #1053 (d031950) into main (43e6875) will decrease coverage by 0.61%.
The diff coverage is 34.71%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1053      +/-   ##
==========================================
- Coverage   72.76%   72.16%   -0.61%     
==========================================
  Files          66       66              
  Lines        5063     5090      +27     
==========================================
- Hits         3684     3673      -11     
- Misses       1379     1417      +38     

Impacted Files	Coverage Δ
kube-core/src/dynamic.rs	61.53% <0.00%> (-10.47%)	⬇️
kube-core/src/object.rs	77.41% <0.00%> (-2.59%)	⬇️
kube-derive/src/custom_resource.rs	13.75% <0.00%> (-0.09%)	⬇️
kube-runtime/src/reflector/mod.rs	100.00% <ø> (ø)
kube-core/src/metadata.rs	17.30% <17.94%> (+1.92%)	⬆️
kube-runtime/src/controller/mod.rs	38.46% <19.04%> (+0.34%)	⬆️
kube-runtime/src/reflector/object_ref.rs	60.00% <48.93%> (-7.08%)	⬇️
kube-runtime/src/reflector/store.rs	96.38% <80.00%> (-0.09%)	⬇️
kube-core/src/resource.rs	55.68% <100.00%> (+2.11%)	⬆️

[clux]

If desirable can split out the TypeInfoInspect trait to its own PR, but comments here would be welcome. Have edited description above.

[nightkr]

Not a fan of losing the type information here. Especially how this effectively makes it possible (and even the default!) to make a completely untyped ObjectRef, which is ~never valid.

Treating namespace: None as a wildcard has been problematic enough so far IMO. :/

[clux]

This is kind of the whole point of the PR; to pass the type information dynamically rather than statically.

I think it's a great guarantee to have it statically available, but it comes at a great cost:

• Controller::owns_with + Controller::watches_with becomes required ways to pass the dyn info (despite the TypeMeta always being available at runtime)
• our ObjectRef becomes very hard to use outside of kube in a generic setting (e.g. ObjectRef cannot even be Display'd without knowledge of root type)
• dyntype infects type signatures in Writer, Store, ReconcileRequest, trigger_with further complicating our api

This complexity can be hidden by doing runtime evaluation of the the TypeMeta. I think maybe we can make better constructors for this if the defaults are not that great for you, maybe some safer builders? I am happy to go down that avenue if this is better?

Note that I am also doing this because we need to type-erase K from ObjectRef to progress on the ApiResource work needed for the v2 client work (we need Scope on the Dyntype, and that requires refactoring the ApiResource, which requires refactoring the ObjectRef that uses the dyntype for equality). ..so down the line, this does aim to remove the namespace: None problem (via client-v2).

[nightkr]

Not super happy about each ObjectRef needing its own clone of all the type metadata when we know it all statically anyway.

[clux]

This is a lot less cloning in the dynamic case though. Before we had to clone the full dynamictype, which is a full ApiResource. I don't see a way to be able to get type information out of dynamic types without either:

• the current complicated dynamic type feeding (which is more clone heavy on dyn types, and has more difficult ctors)
• actually cloning the typemeta for each object

I don't think it's such a big hit. It's worth it to simplify the api so we can start moving away from _with suffixed ctors. Our reflectors are the real memory hog here that's worth optimising for imo.

[nightkr]

OwnerReference carries type metadata.

[clux]

Trying to clean up the constructors of ObjectRef now. Have grabbed the available information now in ObjectRef::from_owner via OwnerReference to provide less avenues for not putting valid information in there.

This ctor is used only by trigger_owners to create an ObjectRef, so using this data here means that it's possible for a user to orphan a controller owned resource by kubectl editing out the ownerref of a child. Still, that is probably the right behaviour.

[clux]

Have reworked the primary ctors of ObjectRef such that there are only these three exposed (manual construction still possible) and all of these have a way of injecting the typeinfo by default.

[clux]

While I want to get a resolution to this, will take this out of the planned 0.77 release (and push it to 0.78) given the complexity and how the benefit is not going to be apparent until the two downstream PRs #1038 and #1037 get done.