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

clean up api::discovery module - for #523 #538

Merged
merged 19 commits into from
May 31, 2021
Merged

clean up api::discovery module - for #523 #538

merged 19 commits into from
May 31, 2021

Conversation

clux
Copy link
Member

@clux clux commented May 23, 2021

  • moves the modules from kube::client::discovery to kube::api::discovery top level kube::discovery
  • introduces easier way to get recommended_resources and recommended_kind from an ApiGroup
  • restructures file for readability
  • internal Version sorting implementation documented and tested better (and moved to its own module under kube::discovery)
  • renamed ApiResourceExtras to ApiCapabilities
  • renamed ApiGroup::group to ApiGroup::get
  • added support for Discovery::single which avoids having to deal with the HashMap and returns a straight ApiGroup (making the recommended doc examples more compelling)
  • renamed ApiGroup::preferred_version_or_guess to ApiGroup::preferred_version_or_latest and referred to the version policy
  • separated querying to helpers on ApiGroup
  • moved sorting to ApiGroup construction part (as it's an operation internal to each group)

Fundamentally leaves the behaviour from @MikailBag 's original PRs unchanged, still a bit unsure about some of the algorithms used and the various converters from k8s_openapi types to our types, but that's generally internal.

@clux
Copy link
Member Author

clux commented May 24, 2021

A bit of a pattern is emerging here in terms of what people might want to discover:

  • Determining info from GVK -> Discovery::resolve_gvk (only deduces plural + caps, not helping you much)
  • Determining info from GK -> Discovery::single -> ApiGroup::recommended_kind (deduces version + plural, helpful)
  • Determining info from K? (helpful, but inconclusive?)

We should probably have a uniform interface for these if we want to support all 3. E.g. the second might be Discovery::resolve_gk (if we like potential typos).

The last one is interesting. kubectl tries to treat the list of available kinds as a flat vector - insofar as it is possible (ask for secret and you get corev1/Secret but there might be another crd with kind Secret).

It is possible we can do this so that you get a vector of options, or the first matching kind (with perhaps a preference for core?).
I assume kubectl has logic for this because you can do kubectl get deploys and it knows when to use apps::v1 and when to use extensions::v1beta1 etc. But have no idea what is considered correct here.

clux added 6 commits May 28, 2021 22:43
- moves the modules from kube::client to kube::api
- introduces easier way to get recommended_resources and
  recommended_kind from an `ApiGroup`
- restructures file for readability (internal Version sorting quirks
  documented and tested)
- renamed `ApiResourceExtras` to `ApiCapabilities`
- renamed `ApiGroup::group` to `ApiGroup::get`
- added support for `Discovery::single` which avoids having to deal with
  the `HashMap` and returns a straight `ApiGroup` (making the
recommended doc examples more compelling)
- documents the version sorting a bit better
- renamed `ApiGroup::preferred_version_or_guess` to
  `ApiGroup::preferred_version_or_latest` and referred to the version
policy
- separated querying to helpers on `ApiGroup`
- moved sorting to `ApiGroup` construction part
`resources_by_version` -> `versioned_resources`
to match `recommended_resources`.

few more doc links
@clux
Copy link
Member Author

clux commented May 28, 2021

Have cleaned up some more. Found out that the new module actually was less efficient w.r.t. api calls (needlessly calling list_api_groups even if people knew the group and version). This is now mitigated and there are currently two solutions:

  1. The old client lister:
let apps = client.list_api_group_resources("apiregistration.k8s.io").await?;
let apiservice = apps.iter().find(|ar| ar.kind == "APIService").unwrap();
let resource = ApiResource::from_apiresource(&apiservice, &apps.group_version);
let api: Api<DynamicObject> = Api::all_with(client.clone(), "default", &resource);

vs

  1. The new Discovery::gvk entry point.
let gvk = GroupVersionKind::gvk("apiregistration.k8s.io", "v1", "APIService"); 
let (ar, caps) = Discovery::gvk(&client, &gvk).await?.unwrap();
let api: Api<DynamicObject> = Api::all_with(client.clone(), &ar);

Think this should let me privatize ApiResource::from_apiresource and Client::list_ methods.

There are all sorts of efficient (to less efficient) ways we can discover now:

  • exactly one gvk (as above)
  • all kinds in a group via Discovery::single_pinned (efficient, no outer list_api_groups call)
  • all kinds in a group (but with all versions, and a preferred version), via Discovery::single (requires the outer client.list_api_groups to discover the available versions)
  • all kinds in all groups; sequentially doing what Discovery::single is doing to all listed api groups (can do the list only once)

The last one I can only see being useful in a kubectl get all --all type emulation, but the first two are probably the 99% use case. Possibly the 3rd is also good if devs have a good versioning policy.

At any rate, going to mull this over some more tomorrow, then probably make the old apis private, then merge.

clux added 2 commits May 29, 2021 00:43
actually many legit use cases here, so made it kind of nice.
finally, this allows us to deprecate old ApiResource from_apiresource
as well as old Client listers (which we use internally).

possibly some more cleanups incoming.
@clux
Copy link
Member Author

clux commented May 29, 2021

ok, this is turning into a pretty big yak... have:

  • introduced a discovery::Oneshot system that bypasses the cache inside Discovery
  • deprecated Client::list_* finally (since Oneshot/Discovery is now equivalent and smarter (handles core transparently)
  • added a GroupVersion struct inside kube_core::gvk module to house the apiVersion <-> Group/Version logic
  • added pub api_version getters on GroupVersion and GroupVersionKind
  • added a private DiscoveryMode to determine how the caching system works
  • made Discovery::run the starting point, and it now can be re-run (for refreshing_)

a few small things left to look over / think about:

  • possible that the Oneshot ctors should just be plain methods, but it gives them some hierarchy, maybe just a mod? mod now
  • possible that i can do with more re-use of the client listers, looks like i am using the same methods a lot atm cleaned up a bit
  • Oneshot naming: ::group, ::gv, and ::gvk is a bit awkward and mixes abbreviations and non-abbreviations (but don't like the full expanded ones, and don't like the idea of Oneshot::g)

@@ -19,19 +19,11 @@ async fn main() -> anyhow::Result<()> {

// Turn them into a GVK
let gvk = GroupVersionKind::gvk(&group, &version, &kind);
let mut api_resource = ApiResource::from_gvk(&gvk);
// Use API discovery to identify more information about the type (like its plural)
let (ar, _caps) = discovery::Oneshot::gvk(&client, &gvk).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: Stopped recommending people construct ApiResources manually when using DynamicObjects and recommended doing the basic oneshot discovery to find the plural.

@clux
Copy link
Member Author

clux commented May 30, 2021

Did put #[deprecated(since = "0.56.0", note = "Replaced by discovery module")] in front of all the Client::list_* methods at some point (and a corresponding #[allow(deprecated)] in deps), but realised that's probably a bit premature.

Have left them untouched for now (but with a note in docs). However, the changes made to some of the more confusion-inducing parsers (for ApiResource and ApiCapabilities) have resulted in some of the old api surface becoming private (after a major refactor also), so it's potentially a bit hard to use the old client api anyway. Maybe I should add some of the discovery::parse functions (particularly to parse an ApiResource from k8s-openapi primitives) public. I really rather not have that stuff depended on though (and it has only been public for the last one/two versions).

Think I am mostly done with this now (finally). Have only two outstanding questions to mull over:

  • naming consistency of oneshots: two options: [oneshot::group, oneshot::gv, oneshot::gvb] or [oneshot::group, oneshot::pinned_group, oneshot::pinned_kind] (gone for the latter)
  • whether to make discovery::parse constructor for ApiResource public (left it private for now, and will wait to see if people have good reasons for it)
  • see if it's possible to implement discoverers for GroupVersion and GroupVersionKind (gvk.discover(&client) to forward to oneshots) - looks difficult because the type is defined in kube_core not doing this, don't want too many ways of doing the same thing

@clux clux merged commit a2c7aef into master May 31, 2021
@clux clux deleted the discovery-cleanup branch May 31, 2021 16:43
@clux clux linked an issue May 31, 2021 that may be closed by this pull request
2 tasks
@clux clux mentioned this pull request May 31, 2021
2 tasks
kazk pushed a commit to kazk/kube-rs that referenced this pull request May 31, 2021
* clean up api::discovery module - for kube-rs#523

- moves the modules from kube::client to kube::api
- introduces easier way to get recommended_resources and
  recommended_kind from an `ApiGroup`
- restructures file for readability (internal Version sorting quirks
  documented and tested)
- renamed `ApiResourceExtras` to `ApiCapabilities`
- renamed `ApiGroup::group` to `ApiGroup::get`
- added support for `Discovery::single` which avoids having to deal with
  the `HashMap` and returns a straight `ApiGroup` (making the
recommended doc examples more compelling)
- documents the version sorting a bit better
- renamed `ApiGroup::preferred_version_or_guess` to
  `ApiGroup::preferred_version_or_latest` and referred to the version
policy
- separated querying to helpers on `ApiGroup`
- moved sorting to `ApiGroup` construction part

* clippy

* changelog

* promote discovery to its own module

* factor out version logic into own private module

* add some docs + small renames

`resources_by_version` -> `versioned_resources`
to match `recommended_resources`.

few more doc links

* remove stray todo and add back kube::client::Status

* add better entry point for the cheaper single case

* fix tests

* restructure completely again for cache/oneshot distinction

actually many legit use cases here, so made it kind of nice.
finally, this allows us to deprecate old ApiResource from_apiresource
as well as old Client listers (which we use internally).

possibly some more cleanups incoming.

* clippy

* split discovery into 3 files

* discovery error type

* fix tests

* rename openapi mod to parse mod

* revert premature deprecation

* document changes

* make DiscoveryError work on config only feat + fix rustfmt makefile

* resolve last questions
@clux
Copy link
Member Author

clux commented Jun 5, 2021

released in 0.56

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

Successfully merging this pull request may close these issues.

discovery module feedback discussions
1 participant