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

Remove dyntype from ObjectRef #1053

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Remove dyntype from ObjectRef #1053

wants to merge 18 commits into from

Conversation

clux
Copy link
Member

@clux clux commented Oct 17, 2022

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
Copy link
Member Author

clux commented Nov 2, 2022

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.

@clux clux added the changelog-change changelog change category for prs label Nov 2, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2022

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
Copy link
Member Author

clux commented Nov 2, 2022

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

@clux clux marked this pull request as ready for review November 2, 2022 22:16
@clux clux added this to the 0.77.0 milestone Nov 2, 2022
kube-core/src/metadata.rs Outdated Show resolved Hide resolved
/// );
/// ```
#[non_exhaustive]
pub struct ObjectRef<K: Resource> {
pub dyntype: K::DynamicType,
pub struct ObjectRef {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :/

Copy link
Member Author

@clux clux Dec 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Self {
name: meta.name.clone().unwrap_or_default(),
namespace: meta.namespace.clone(),
types: obj.types(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@clux clux Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

namespace: None,
extra: Extra::default(),
types: None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OwnerReference carries type metadata.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kube-runtime/src/reflector/object_ref.rs Outdated Show resolved Hide resolved
kube-runtime/src/reflector/object_ref.rs Outdated Show resolved Hide resolved
Comment on lines +30 to +36
/// assert_eq!(
/// ObjectRef::from_obj(&pod).within("ns"),
/// ObjectRef::from_owner(&oref).within("ns"),
/// );
/// assert_eq!(
/// ObjectRef::from_obj(&pod).within("ns"),
/// ObjectRef::from_resource::<Pod>("foo").within("ns")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

clux commented Dec 12, 2022

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.

@clux clux modified the milestones: 0.77.0, 0.78.0 Dec 12, 2022
more accurately explains what the trait is doing

Signed-off-by: clux <[email protected]>
Signed-off-by: clux <[email protected]>
@clux clux removed this from the 0.78.0 milestone Jan 5, 2023
@clux clux added this to the 0.79.0 milestone Jan 5, 2023
@clux clux modified the milestones: 0.79.0, 0.80.0 Feb 23, 2023
@clux clux removed this from the 0.80.0 milestone Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants