Discovery: Add ApiGroup::resources_by_stability

[imuxin]

Motivation

Assume that there are custom resources: {resource1}.example.org/v1alpha1 and {resource2}.example.org/v1alpha2. Use the kubectl in the examples to exec kubectl get resource1 -A would result in resource1 not found. more details in pub fn versioned_resources, in the example only {resource2} returned.

Solution

So, I add a fn resources to return all the resource in the api group.

[clux]

This is kind of by design. Even kubectl just picks the recommended version to show when getting resources, because there is only ever one stored version of each resource (other versions get converted).

[clux]

Oh, wait. This is not what you are saying, you are saying that two different api resources (in the same api group) uses different versioning within the group and thus you only see the ones from the latest version. 🤔

[imuxin]

Yeah, I mean two different api resources (in the same api group). And I see the data has been sorted, right? So, the recommended resource will be preferred in the returned resources.

[imuxin]

Assume all resources are [(resource1, v1alpha1), (resource2, v1alpha1), (resource2, v1alpha2)].
Maybe, you want to return [(resource1, v1alpha1), (resource2, v1alpha2)] ? (resource2, v1alpha1) filtered.

[nightkr]

Assume all resources are [(resource1, v1alpha1), (resource2, v1alpha1), (resource2, v1alpha2)]. Maybe, you want to return [(resource1, v1alpha1), (resource2, v1alpha2)] ? (resource2, v1alpha1) filtered.

Yeah, this sounds like the correct behaviour.

Maybe at some point [(resource1, [v1alpha1]), (resource2, [v1alpha1, v1alpha2])] would make sense too, but happy to punt on that for another time.

[clux]

The annoying thing here is that Kubernetes effectively version their APIGroups by giving you one preferred version for all the resources in the group, which kind of implies that resources should follow the same version conventions within an api group (and I think in practice we see this a lot even for CRDs). That said, is not a universal requirement as the user ultimately specifies versions inside each CRD.

This PR is fine for catching cases where versioning differ within a group, but I think the sorting (in the example) is now broken as a result.
We are just doing a min_by_key on group.name() so I don't think we have any guarantee that we are picking the most stable version for the requested kind anymore.

[imuxin]

We are just doing a min_by_key on group.name() so I don't think we have any guarantee that we are picking the most stable version for the requested kind anymore.

I think we have no way to get the stable version by request format!("/apis/{}", apiversion), but latest version instead.

I made a test, there was a resource (virtualservice, [v1alpha3, v1beta1]) and v1alpha3 was the storedVersion. Then exec kubectl get virtualservice -A -v=9, the result showed that v1beta1 was chosen by kubectl.

here are more details:

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: virtualservices.networking.istio.io
  resourceVersion: "85434"
  uid: 382d9d63-0db4-4b27-9d1e-b3da72bb242b
spec:
  conversion:
    strategy: None
  group: networking.istio.io
  names:
    categories:
    - istio-io
    - networking-istio-io
    kind: VirtualService
    listKind: VirtualServiceList
    plural: virtualservices
    shortNames:
    - vs
    singular: virtualservice
  scope: Namespaced
  versions:
...
status:
  acceptedNames:
    categories:
    - istio-io
    - networking-istio-io
    kind: VirtualService
    listKind: VirtualServiceList
    plural: virtualservices
    shortNames:
    - vs
    singular: virtualservice
  conditions:
  - lastTransitionTime: "2022-09-16T05:45:55Z"
    message: no conflicts found
    reason: NoConflicts
    status: "True"
    type: NamesAccepted
  - lastTransitionTime: "2022-09-16T05:45:55Z"
    message: the initial names have been accepted
    reason: InitialNamesAccepted
    status: "True"
    type: Established
  storedVersions:
  - v1alpha3

kubectl get virtualservice -A -v=9
I0928 11:36:27.155774   38913 loader.go:372] Config loaded from file:  /Users/chengqinglin/.kube/config
I0928 11:36:27.176216   38913 round_trippers.go:435] curl -v -XGET  -H "Accept: application/json;as=Table;v=v1;g=meta.k8s.io,application/json;as=Table;v=v1beta1;g=meta.k8s.io,application/json" -H "User-Agent: kubectl/v1.22.4 (darwin/amd64) kubernetes/b695d79" 'https://192.168.189.136:6443/apis/networking.istio.io/v1beta1/virtualservices?limit=500'
I0928 11:36:27.448220   38913 round_trippers.go:454] GET https://192.168.189.136:6443/apis/networking.istio.io/v1beta1/virtualservices?limit=500 200 OK in 271 milliseconds
I0928 11:36:27.448245   38913 round_trippers.go:460] Response Headers:
I0928 11:36:27.448252   38913 round_trippers.go:463]     Cache-Control: no-cache, private
I0928 11:36:27.448257   38913 round_trippers.go:463]     Content-Type: application/json
I0928 11:36:27.448261   38913 round_trippers.go:463]     X-Kubernetes-Pf-Flowschema-Uid: 7d9c9ded-9205-47ff-82e0-bc626b11a468
I0928 11:36:27.448266   38913 round_trippers.go:463]     X-Kubernetes-Pf-Prioritylevel-Uid: 8378e0f6-65a5-4111-bcec-77f9ec138449
I0928 11:36:27.448270   38913 round_trippers.go:463]     Date: Wed, 28 Sep 2022 03:36:27 GMT
I0928 11:36:27.534416   38913 request.go:1181] Response Body: ***

[clux]

Hm, I don't think the stored version is relevant. A test on mv-crd will show that kubectl will pick the most stable version as long from the ones that have served: true (regardless of storage). I.e. it will pick v2alpha1 if and only if v1 is not served. Thus we should in theory also be able to use Version::priority to pick the best one.

[imuxin]

@clux, I just improve the recommended_resources function to return all resources(including the lost in the lower group version) and the resource version is the most stable.

[clux]

I like the idea of this, but I think it should be its own method. Currently recommended_resources has a semantic meaning of using the preferred_version set by the APIGroup and this code would change that behaviour.

However, would be open to having a method like resources_by_stability() doing what you are doing here provided it is documented and tested. It should also not be using itertools (since we don't use it anywhere else, and we are aiming for consistency). That said, I do really like this idea of maximizing the stability metric within the ApiGroup 👍

[imuxin]

@clux, I think the pr is ready now.

[clux]

It looks great! Thanks a lot. I have only added some minor nits here and there.

[imuxin]

The failed ci tests seem that the new added test derived_resources_by_stability_discoverable block the derived_resources_discoverable test. they have the same resource testcrs.kube.rs.

[clux]

Ah, it is also using testcrs.kube.rs specifically. A quick fix would be to just use a different group in your integration test.
The tests reuse the test environment so it's hard to prevent this type of thing.

[clux]

An alternative approach to battling the slow discovery server for testing is to private construct an ApiGroup with the data you want and just unit test that the new function sorts it correctly.

[imuxin]

An alternative approach to battling the slow discovery server for testing is to private construct an ApiGroup with the data you want and just unit test that the new function sorts it correctly.

Yeah, to private construct an ApiGroup is a simple way. But in the failed integration(latest) test, I have no idea why it failed.

[imuxin]

@clux. It's hard to solve the integration test problem. So I decide to take your advice to private construct an ApiGroup . Thanks a lot for helping me so much.

[clux]

Appreciate it! Unit tests are easier for us to maintain in the long run as well :-)