Decouple Meta from k8s-openapi #385

MikailBag · 2021-01-15T13:56:42Z

This PR changes kube::api::Meta so that it no longer depends on k8s-openapi.

TODO:
debug generic_watcher not working (upd: fixed)

clux

There's a good idea at the core of this that can make DynamicResource usable. Left some comments on the new Meta trait, there is some work needed there IMO. That said, thanks a lot for this. You've found a huge lack in DynamicResource, which is worth raising a bug about as well.

clux · 2021-01-17T17:51:26Z

kube/src/api/metadata.rs

+    /// Types that know their metadata at compile time should select `Family = ()`.
+    /// Types that require some information at runtime should select `Family`
+    /// as type of this information.
+    type Family: Send + Sync + 'static;


This is the crux of the PR. We are changing the Meta trait to basically be our own, more practical, Resource trait. I think that overall is positive; DynamicResource, as you pointed out, just could not work in more than trivial situations without this change, and a blanket implementation of our trait of k8s_openapi::Resource + Metadata objects is - in theory - easy to do.

A rename of the trait is probably in order to reflect what the trait now does. Not sure to what though.

The Family type parameter here is definitely the most awkward implementation detail of the trait though. Ideally, I would like to avoid this, as it's making breaking changes to both kube::Resource and kube::Api (as well as making them more awkward).

Regarding naming, Do you think the following makes sense?

rename Meta to Resource

rename Resource to RequestBuilder/QueryBuilder

rename Family to RuntimeTypeInformation (long name, but anyway users will rarely interact with it)

I tried to make sure that kube::Api public API was not modified (if K has all necessary information at compile-time, Api::all and Api::namepsaced are still provided). Did I miss something? I will also add similar backward-compatible constructors to the Resource.

I think this has a lot more potential now that it's shown that we don't need to break Api or Resource, although I will leave some comments on the implementation inlined elsewhere.

Let's not worry about the names of those three yet (it'll make it super confusing to review, so we can change them later). Although I think in theory the first two are fine.

clux

some high level comments / questions

clux · 2021-01-28T23:18:12Z

kube/src/api/dynamic.rs

+    Static(&'static str),
+    /// Should be used when string is only known at runtime
+    Dynamic(Arc<str>),
+}


This string wrapper does feel like a pretty complicated thing to expose in an interface. Could you explain what it's trying to optimise for? The Arc in there feels particularly confusing.

It's purely an optimization to make cloning less expensive, especially when GVK is known at compile-time.

Do you think it should be replaced by a plain String?

If it's only a handful of strings that gets cloned once, then I would worry more about the prettiness of the api more, if it's happening all the time, then it's definitely a reasonable optimization.

I think it's probably a smart thing to do internally (now that GVK has a simpler constructor), but I don't think we should expose StringRef to the world.

Also, curious to the tradeoff of Arc<str> vs just String inside Dynamic. It feels like this would just be a ref to the apps' owned String, with atomic because it's used in threads? Maybe we can use Into<String> in the constructor and turn it to a String in the enum instead?

I'll change StringRef to be private (as well as GVK fields) and provide two constructors (static and dynamic) instead.

kube/src/api/dynamic.rs

clux · 2021-01-28T23:30:28Z

examples/generic_watcher.rs

+    };
+
+    let client = Client::try_default().await?;
+    let api = Api::<DynamicObject>::all_with(client, &erased_kind);


Not sure if this is achievable, but ideally, I feel like this could look like:

let gvk = GroupVersionKind::group(group).version(version).kind(kind); let api: Api<DynamicObject> = gvk.into_api(client);

which looks very similar to the current DynamicResource, so maybe you don't need the separate ErasedKind (or GroupVersionKind as i suggested calling it).
Don't worry about breaking DynamicResource; it already doesn't work.

clux · 2021-01-28T23:35:49Z

This is now touching on a lot of the questions posed in #292 so maybe @teozkr has some thoughts on this as well.

MikailBag · 2021-03-05T16:25:02Z

By the way, it seems that CI does not check that kube is able to compile without warnings.

MikailBag · 2021-03-05T16:27:23Z

kube-runtime/src/controller/mod.rs

 {
    stream
        .map_ok(move |obj| stream::iter(mapper(obj).into_iter().map(Ok)))
        .try_flatten()
 }

 /// Enqueues the object itself for reconciliation
-pub fn trigger_self<S>(stream: S) -> impl Stream<Item = Result<ObjectRef<S::Ok>, S::Error>>
+pub fn trigger_self<S, F>(stream: S, family: F) -> impl Stream<Item = Result<ObjectRef<S::Ok>, S::Error>>


This is slight breakage in a low-level API. Do you think it's OK not to provide separate trigger_self and trigger_self_with functions?

Personally, yes. I think this is ok.

MikailBag · 2021-03-15T08:30:09Z

Seems that the automatic merge has broken things a bit. I will rebase on the master.

clux

Left only a few tiny nits. It's a complex one, and sorry it's taken me this long to get to it, but it is really appreciated.

clux · 2021-03-14T21:49:07Z

kube-runtime/src/controller/mod.rs

 {
    stream
        .map_ok(move |obj| stream::iter(mapper(obj).into_iter().map(Ok)))
        .try_flatten()
 }

 /// Enqueues the object itself for reconciliation
-pub fn trigger_self<S>(stream: S) -> impl Stream<Item = Result<ObjectRef<S::Ok>, S::Error>>
+pub fn trigger_self<S, F>(stream: S, family: F) -> impl Stream<Item = Result<ObjectRef<S::Ok>, S::Error>>


Personally, yes. I think this is ok.

examples/generic_watcher.rs

kube-runtime/src/controller/mod.rs

kube/src/api/dynamic.rs

clux · 2021-03-15T10:54:42Z

Seems that the automatic merge has broken things a bit. I will rebase on the master.

Sorry, I tried to resolve conflicts last nights, thought I did it. The conflicts were all around type parameters where I had added Debug requirements in the interim.

clux · 2021-03-17T22:41:46Z

Finally merged! Thanks a lot for all the work here :D
Have documented the huge change in the changelog

I did discover one small p'tietoau quirk in #464. so will hold off on the release for a little bit while thinking about it, but it's pretty minor, and non-blocking. However, if you have time to look it'd be really appreciated.

kazk · 2021-03-17T23:19:30Z

Before releasing, I think we should discuss renaming the associated type Family and traits as suggested above.

For the changelog, I think we should briefly mention what needs to be changed to update when it's not obvious. This also helps users to understand the scope (if they're affected or not). If I'm understanding correctly, this change doesn't affect many, but the current changelog looks intimidating because it's unclear. I think most users are unfamiliar with Meta and anything listed under that change, but it sounds too important to skip. Maybe add a note like, "you're not affected by this change unless ..."?

clux · 2021-03-18T09:43:54Z

Yeah, I agree with that. I mostly wrote out the things as I saw them for myself. Wasn't even sure if many people have read this PR, as it's been silently sitting in the background for 3 months :-)

Naming. I'll open a new issue on this.

clux · 2021-03-31T20:33:28Z

Released in 0.52.0 😅

MikailBag changed the title ~~Meta~~ Decouple Meta from k8s-openapi Jan 15, 2021

clux requested changes Jan 17, 2021

View reviewed changes

MikailBag force-pushed the meta branch from 3de7ad5 to d605302 Compare January 27, 2021 19:10

clux reviewed Jan 28, 2021

View reviewed changes

clux mentioned this pull request Feb 7, 2021

Remove version from ObjectRef? #327

Closed

This was linked to issues Feb 7, 2021

Remove version from ObjectRef? #327

Closed

Confusing overlap between different resource metadata types #292

Closed

Resource unnecessarily clones #233

Closed

clux removed a link to an issue Feb 8, 2021

Resource unnecessarily clones #233

Closed

MikailBag force-pushed the meta branch from 206f463 to 2024d46 Compare March 5, 2021 16:17

MikailBag commented Mar 5, 2021

View reviewed changes

clux reviewed Mar 15, 2021

View reviewed changes

Decouple Meta from k8s-openapi

6b95f20

MikailBag force-pushed the meta branch from a25aeb5 to 6b95f20 Compare March 15, 2021 14:26

update example and debug impls

eb29dad

clux approved these changes Mar 15, 2021

View reviewed changes

clux merged commit 813da82 into kube-rs:master Mar 17, 2021

clux mentioned this pull request Mar 18, 2021

family/meta/resource names #465

Closed

clux added a commit that referenced this pull request Mar 18, 2021

less scary breaking warnings - for #385

0720a3d

clux mentioned this pull request Mar 18, 2021

protobuf encoding exploration #371

Closed

nightkr mentioned this pull request Mar 18, 2021

Clean up type bounds after Metapocalypse #469

Merged

clux mentioned this pull request Mar 29, 2021

Meta trait revamp #274

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple Meta from k8s-openapi #385

Decouple Meta from k8s-openapi #385

MikailBag commented Jan 15, 2021 •

edited

Loading

clux left a comment •

edited

Loading

clux Jan 17, 2021

MikailBag Jan 27, 2021

clux Jan 28, 2021

clux left a comment

clux Jan 28, 2021

MikailBag Jan 29, 2021

clux Jan 30, 2021

MikailBag Mar 5, 2021

clux Jan 28, 2021 •

edited

Loading

clux commented Jan 28, 2021

MikailBag commented Mar 5, 2021

MikailBag Mar 5, 2021

clux Mar 14, 2021

MikailBag commented Mar 15, 2021

clux left a comment

clux Mar 14, 2021

clux commented Mar 15, 2021

clux commented Mar 17, 2021 •

edited

Loading

kazk commented Mar 17, 2021

clux commented Mar 18, 2021

clux commented Mar 31, 2021

Decouple Meta from k8s-openapi #385

Decouple Meta from k8s-openapi #385

Conversation

MikailBag commented Jan 15, 2021 • edited Loading

clux left a comment • 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

clux left a 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

clux Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

clux commented Jan 28, 2021

MikailBag commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikailBag commented Mar 15, 2021

clux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clux commented Mar 15, 2021

clux commented Mar 17, 2021 • edited Loading

kazk commented Mar 17, 2021

clux commented Mar 18, 2021

clux commented Mar 31, 2021

MikailBag commented Jan 15, 2021 •

edited

Loading

clux left a comment •

edited

Loading

clux Jan 28, 2021 •

edited

Loading

clux commented Mar 17, 2021 •

edited

Loading