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

Discussion: A fork, a merge, or something different? #9

Open
AaronFriel opened this issue Sep 5, 2018 · 10 comments
Open

Discussion: A fork, a merge, or something different? #9

AaronFriel opened this issue Sep 5, 2018 · 10 comments

Comments

@AaronFriel
Copy link

Context: @Arnavion has created the k8s-openapi package, and @anguslees created the kubernetes package in this repository. The two should probably speak each other's language, if the goal is to support the ecosystem of Kubernetes without having to write custom serialization for every new release and API type.

I forked this project a couple weeks ago and started working on what that might look like, and re-wrote the client-watch.rs example using features in nightly to show how ergonomic (or not) that could be. I'd really appreciate some review of this branch. A comparison reveals the large amounts of code changes, so perhaps a PR isn't the right way to handle this. I renamed the project for local development (and reserved the name on crates.io with a publish) for that reason, but also in anticipation that the changes are too significant or not the direction the other folks want.

Anyway, that's enough said. I'd like to hear what the people I've tagged think about these changes, and what they would like to do with it.

@mhristache
Copy link

@AaronFriel I wanted to play a bit with your implementation but could not find the repo. Is it private?

@anguslees
Copy link
Owner

@mhristache: https://github.com/AaronFriel/kubernetes-rs

@AaronFriel: I took a good look at the repo when you opened this issue earlier, and meant to reply - but was unsure what was both useful and brief enough for a github comment thread.

First, thanks :) It's a great and useful exploration.

There's a bunch of conflated changes here, and I think it would be useful to tease them out separately:

  • Rust-2018 changes. kubernetes-rs will obviously need to be updated to include this eventually. I didn't see any advantage to being an early adopter here, so had been consciously holding off until that whole meta-project was more final and less likely to change. Patches welcome, however.

  • API objects and k8s-openapi interaction. K8s-openapi is useful and very complete thanks to its auto-generated nature. A requirement for kubernetes-rs was that it must be possible to add external API types (through Rust traits) and have them "feel" like first class citizens. This is required for CRDs and custom API servers, but should also work for entire re-implementations of the core types too (like k8s-openapi).
    Currently I require Metadata + Serialize + DeserializeOwned + Send + 'static, of which the only controversial one is kubernetes::Metadata. If k8s-openapi can auto-generate those trivial Metadata trait implementations too, then they should "just work" with kubernetes-rs.
    I expect long-term, that I should split my API objects into a separate crate from kubernetes-rs for the same release-cycle reasons this has happened with client-go/apimachinery too. I am holding off on this for now until I have more confidence with the overall API relationships - in particular resources (eg the thing that has some sort of pods::getlogs() method, not the v1/Pod type) need a home somewhere.

  • Other changes. You've included a list-watcher implementation, which is great. I look forward to exploring all sorts of actually-useful abstractions on top of a basic API client. Given the potential design space here, I think these are best maintained as 3rd-party crates rather than incorporating them into the core client crate - but I'm open to suggestions, particularly where it's simply a better way of representing the underlying API call in Rust.

More discussion welcome.

@Arnavion
Copy link

Arnavion commented Oct 9, 2018

If k8s-openapi can auto-generate those trivial Metadata trait implementations too, then they should "just work" with kubernetes-rs.

Yes, I don't foresee any problems for that. The values are in the spec already (x-kubernetes-group-version-kind).

Arnavion pushed a commit to Arnavion/k8s-openapi that referenced this issue Oct 10, 2018
This change adds `TypeMeta` and `ValueMeta` traits that are implemented for
all resource types. The traits define the API version and kind of each resource
automatically, so the corresponding fields have been removed from the
resource types.

The traits are derived from https://github.com/anguslees/kubernetes-rs
based on the discussion in anguslees/kubernetes-rs#9
@AaronFriel
Copy link
Author

Thanks for replying! I haven't been checking this regularly, and I should have. I'm really happy to see this cooperation.

Rust-2018 changes...

I found it helpful to use a more ergonomic API around the metadata accessing (try { } blocks are great) but this is totally understandable. I think I used these more in the updated example than the library, and it's easy enough to refactor them out to a function call.

API objects and k8s-openapi interaction.

I already raised the topic with @Arnavion and I'll let their comment and WIP code speak for itself. I think this collaboration is great.

Other changes. You've included a list-watcher implementation, which is great. I look forward to exploring all sorts of actually-useful abstractions on top of a basic API client.

My original motivation for looking for a Rust crate implementation (and why I implemented a generic list watcher) was because I wanted to do a presentation for my company on Kubernetes, and wanted to demo what I think is its greatest strength: controllers/operators and custom resources.

Sadly, the Go docs require lots of manual codegen (ugh, lack of generics), and those were the ones I found the most material on how to write a controller/operator. So I started doing the research on what these operations entail, and I think there is a significant opening for a great Kubernetes library to provide a few of these primitive operations (list watching, reflectors/informers in the Go CRD controller docs). My argument here would be: these are essential building blocks. They're also pay-for-what-you-use features, which fits with the Rust philosophy here.

@AaronFriel
Copy link
Author

Just to elaborate on what I meant by opening. I think there is an opportunity to make Rust the best language for writing reliable abstractions on Kubernetes, even better than Go, because the generics, zero cost abstractions, and explicit error handling make it trivial to write a list watcher that never fails. The Go examples screw that up. There are many bug reports about controllers in use in production systems failing because controllers are written to rely on default timeout values, don't resume from failures, sporadically stop listening to events. I've experienced this multiple times even for controllers like nfs-server-provisioner!

This library with open-k8s however allowed writing a fully generic resource watcher that handled all a manner of failure conditions I could simulate - randomly killing pods, network partitions, etc. The stream of pod events produced by the example doesn't lag, time out, or fail because it's written not to. That was great, and it's surprising to me that the Go controller sample is not nearly as resilient.

@anguslees
Copy link
Owner

+1 to all that :)
My goals with kubernetes-rs are similarly to build a library that we can use for controllers, etc with significantly less boilerplate and explicitly-generated code thanks to parameterised types. I also want to promote the "Unstructured" object type from the beginning, and avoid the entire "REST Mapper" infrastructure that fills much of the client-go apimachinery library. I also hope this will make Rust-kubernetes binaries significantly smaller, since they should only include the k8s API types that they actually use (typically ~3 -- pay for what you use).

The challenge is time and competition with other priorities :/

@mhristache
Copy link

There is a new kid on the block and it's using k8s-openapi :)

Arnavion pushed a commit to Arnavion/k8s-openapi that referenced this issue Nov 11, 2018
This change adds a `Resource` trait that's implemented for all resource types.
The trait defines the API version and kind of each resource automatically,
so the corresponding fields have been removed from the resource types.

The trait is derived from https://github.com/anguslees/kubernetes-rs
based on the discussion in anguslees/kubernetes-rs#9
@ibotty
Copy link

ibotty commented Nov 22, 2018

Now that @Arnavion added a Metadata like trait. Should kubernetes-rs depend on k8s-openapi and replace its Metadata trait?

@AaronFriel, do you plan on maintaining your fork?

@Arnavion
Copy link

Arnavion commented Nov 22, 2018

This crate's trait has one more function that the trait in k8s-openapi doesn't have. The reason is that list resources like PodList have a ListMeta instead of an ObjectMeta, and I've not decided how to expose that. Two traits like trait ObjectResource: Resource and trait ListResource: Resource? Make Resource generic like trait Resource<T> { fn metadata(&self) -> Option<&T> } ? Something else?

Update: This is now done, with trait Metadata: Resource { type Ty; fn metadata(&self) -> Option<&<Self as Metadata>::Ty>; }

@AaronFriel
Copy link
Author

@ibotty I don't intend to maintain my fork unless @anguslees believes this project should not have a dependency on k8s-openapi.

anguslees added a commit that referenced this issue Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants