Split ListParams and WatchParams

[nabokihms]

Motivation

Split list and watch params

In client-go, some list options only work for watch requests, and some only for list requests. It is not convenient to validate such structures.

To add new field only for lists, it is required to add validations to watch calls to deny them.

Add resource version to ListParams

Speaking from experience, resource version options are helpful. My case is to reduce the Kubernetes API server load by providing the resourceVersion=0 to avoid etcd hits and getting data from the cache.

The full explanation of the resource version semantic can be found here.

Some issues to show the usefulness of the option
vectordotdev/vector#7943
kubernetes/kube-state-metrics#1548

Solution

1. Add resourceVersion and resourceVersionMatch options for ListParams (inspired by client-go)

   • The list is the most expensive type of request in Kubernetes because it loads all keys for the kind and namespace from etcd
   • Adding resourceVersion for get requests will be a breaking change, so I decided to go with just lists for now

2. Split list and watch params:

   • Split options
   • Add a new structure for watcher configuration
   • Convert watcher configuration to list params for list requests and watch params to watch requests

[codecov]

Codecov Report

Merging #1162 (6c331e6) into main (2e5e4de) will increase coverage by 0.05%.
The diff coverage is 72.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1162      +/-   ##
==========================================
+ Coverage   72.59%   72.65%   +0.05%     
==========================================
  Files          67       67              
  Lines        5123     5211      +88     
==========================================
+ Hits         3719     3786      +67     
- Misses       1404     1425      +21     

Impacted Files	Coverage Δ
kube-client/src/api/mod.rs	68.75% <ø> (ø)
kube-runtime/src/controller/mod.rs	36.95% <0.00%> (ø)
kube/src/lib.rs	89.04% <ø> (ø)
kube-client/src/api/core_methods.rs	71.83% <50.00%> (ø)
kube-runtime/src/watcher.rs	43.51% <52.94%> (+2.00%)	⬆️
kube-core/src/params.rs	77.01% <81.25%> (-1.52%)	⬇️
kube-core/src/request.rs	94.72% <89.18%> (+0.30%)	⬆️
kube-client/src/lib.rs	93.85% <100.00%> (-0.11%)	⬇️

[clux]

Hey, thanks a lot for doing this. Had missed this new feature, and now wondering how we should integrate it with the larger runtime setup!

I think the change as it stands from a kube-core/client POV is perfectly fine. However, infinite streams/controller flows via watcher is the real usage point for most users here, it might be worth seeing how to best integrate the "most desired" resource version passing setup into the statemachinery there. The upstream reports indicate that we are doing the wrong thing (e.g. not setting resource_version=0), if that's the case, maybe we should just do that by default on list-requests in watcher also. Kubernetes 1.19 (where the feature was introduced) is already below the minimally supported Kubernetes version.

Also made a separate point in there that it might be worth us splitting ListParams and create an alternate WatchParams ala WatchOptional to avoid a validation problem (which i think can also improve the watcher interface that does not care about all the list options anyway). But that is obviously a bit of a breaking change that requires some extra thought. Happy to not deal with that for now if we can integrate with watcher.

[clux]

Ignore the lint failure. Duplicate versions of rustix way down the tree (and one of them is a dev-dep only) that should be added to deny.toml.

[nabokihms]

@clux I've just realized that I made a huge mistake with these validations. I added a hack to make watchers work with resource version options (without e2e test for now). Could you please take a look and tell me whether you agree with the approach?

[nabokihms]

Missed some doc tests errors. It should be fixed now.

[clux]

minor comments - the changes look thorough and well done in general though!

[nabokihms]

@clux I fixed clippy errors and added a couple of tests. Please tell me if something else is required.

[clux]

this is looking great now. have only left a couple of minor nits in there.

[clux]

Looks great to me now. Thanks a lot!

[clux]

Hey, I was doing some sanity checking and doc fixups in main and noticed something we missed :(

• config::Matcher takes the VersionMatch enum for its list calls
• VersionMatch enum contains a resourceVersion now
• this resourceVersion trickles down into the Api::list call in watcher
• watcher re-uses the resource version for every call if using NotOlderThan or Exact

Thus this means it's effectively impossible for us to configure NotOlderThan or Exact semantics in the watcher.

This probably makes this unreleaseable in the current state and unless there's a smart way forward, we probably need to unroll this back into an unconnected enum :(

[nabokihms]

@clux got it. I will think about the possible fix meantime. It seems worth opening an issue.

[clux]

@nabokihms : i have started a little bit of work on this to try to change it around. The pattern I am trying is back to the tri-state enum, but being a little cleverer about how to expose helpers to compensate. Will have a draft PR up in a little bit.