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

Add default pagination to watcher #1249

Merged
merged 12 commits into from Jul 13, 2023
Merged

Add default pagination to watcher #1249

merged 12 commits into from Jul 13, 2023

Conversation

clux
Copy link
Member

@clux clux commented Jul 11, 2023

Carrying on #1211 for Nat.

Main change is setting a default limit to 500 to match upstream in client-go.

NB: This is changing behaviour, but not an api break, and it fixing a dangerous default; you would likely exceed your expected memory usage at some point if you do not use limits.

It is possible to opt-out of this and do old style unlimited page sizes by manually setting the page_size to None, but because we do not recommend doing this, there is no builder for this:

let wc = watcher::Config {
    page_size: None,
    ..watcher::Config::default()
}

Other minor changes from existing PR:

  • renamed controlling parameter from page_size_limit to page_size (also matches upstream and is shorter)
  • recent doc link breaks/clippy fixes from unrelated stuff (with a few related)

Can be cherry-picked into nat's PR or merged directly. At your preference.

nightkr and others added 8 commits April 28, 2023 17:48
See #1209, relatively untested prototype and doesn't include any proposed API changes
Users can do this anyway using for_stream
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #1249 (fac6bd9) into main (e99d0dc) will increase coverage by 0.31%.
The diff coverage is 83.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1249      +/-   ##
==========================================
+ Coverage   72.80%   73.11%   +0.31%     
==========================================
  Files          73       74       +1     
  Lines        5924     5989      +65     
==========================================
+ Hits         4313     4379      +66     
+ Misses       1611     1610       -1     
Impacted Files Coverage Δ
kube-runtime/src/controller/mod.rs 35.15% <ø> (ø)
kube-runtime/src/controller/runner.rs 94.36% <ø> (ø)
kube-runtime/src/reflector/store.rs 98.94% <ø> (ø)
kube-runtime/src/utils/delayed_init.rs 97.40% <ø> (ø)
kube-runtime/src/utils/event_modify.rs 95.83% <ø> (ø)
kube/src/lib.rs 89.04% <ø> (ø)
kube-runtime/src/watcher.rs 48.46% <62.50%> (+4.32%) ⬆️
kube/src/mock_tests.rs 97.87% <97.87%> (ø)

... and 3 files with indirect coverage changes

clux added 2 commits July 11, 2023 21:01
Signed-off-by: clux <sszynrae@gmail.com>
name matches what it's called upstream + it's shorter

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Paginated watcher continuation Add default pagination to watcher Jul 11, 2023
@clux clux added the changelog-add changelog added category for prs label Jul 11, 2023
@clux clux added this to the 0.84.0 milestone Jul 11, 2023
@clux
Copy link
Member Author

clux commented Jul 11, 2023

Did some quick sanity checking of this and logs are good, the limits and continuation works well:

diff --git a/examples/pod_watcher.rs b/examples/pod_watcher.rs
index e4e79fc4..3eb3805e 100644
--- a/examples/pod_watcher.rs
+++ b/examples/pod_watcher.rs
@@ -13,7 +13,7 @@ async fn main() -> anyhow::Result<()> {
     let client = Client::try_default().await?;
     let api = Api::<Pod>::default_namespaced(client);
 
-    watcher(api, watcher::Config::default())
+    watcher(api, watcher::Config::default().page_size(1))
         .applied_objects()
         .default_backoff()
         .try_for_each(|p| async move {

in a cluster with 3 pods:

$ RUST_LOG=info,kube=debug cargo run --example pod_watcher
DEBUG HTTP{http.method=GET http.url=https://0.0.0.0:45613/api/v1/pods?&limit=1 otel.name="list" otel.kind="client"}: kube_client::client::builder: requesting
DEBUG HTTP{http.method=GET http.url=https://0.0.0.0:45613/api/v1/pods?&limit=1&continue=eyJ2IjoibWV0YS5rOHMuaW8vdjEiLCJydiI6MzM1MDcsInN0YXJ0Ijoia3ViZS1zeXN0ZW0vbG9jYWwtcGF0aC1wcm92aXNpb25lci01ZDU2ODQ3OTk2LXoydnFrXHUwMDAwIn0 otel.name="list" otel.kind="client"}: kube_client::client::builder: requesting
DEBUG HTTP{http.method=GET http.url=https://0.0.0.0:45613/api/v1/pods?&limit=1&continue=eyJ2IjoibWV0YS5rOHMuaW8vdjEiLCJydiI6MzM1MDcsInN0YXJ0Ijoia3ViZS1zeXN0ZW0vY29yZWRucy01YzZiNmM1NDc2LXh6cDViXHUwMDAwIn0 otel.name="list" otel.kind="client"}: kube_client::client::builder: requesting
INFO pod_watcher: saw local-path-provisioner-5d56847996-z2vqk
INFO pod_watcher: saw coredns-5c6b6c5476-xzp5b
INFO pod_watcher: saw busybox1
DEBUG HTTP{http.method=GET http.url=https://0.0.0.0:45613/api/v1/pods?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=33507 otel.name="watch" otel.kind="client"}: kube_client::client::builder: requesting

I think this is one is really worth including in the next release, so will try to prioritise this before doing a release. If anyone have additional pointers, concerns, nits please give it a look. This already had some preliminary go-over in #1211.

@clux clux marked this pull request as ready for review July 11, 2023 20:37
@clux clux requested review from Dav1dde and nightkr July 11, 2023 20:37
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Jul 11, 2023

Have also incorporated some mock style testing strategies with tower_mock in the facade crate (mostly because this needs a resource and wanted this as small as possible) for this in e1e11ed. This is optional, but thought that it would be nice to have some testing for this even though the safety we get from the new mocks.rs is relatively light. Also not sure how easy it will be to do more advanced testing of the streams with tower_mock.

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Re Tests: Maybe those should be moved out into tests directory, they look and feel more like integration tests. I think this will especially help if the amount of tests grows and we need multiple files and modules to organize mocks and tests.

Unrelated: the paginated list seems pretty useful, maybe we should have a list which returns a stream of objects which internally lazily pages through all items?

kube-runtime/src/watcher.rs Outdated Show resolved Hide resolved
@nightkr
Copy link
Member

nightkr commented Jul 12, 2023

I wouldn't be against extracting that at some point.

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Jul 12, 2023

Re Tests: Maybe those should be moved out into tests directory, they look and feel more like integration tests. I think this will especially help if the amount of tests grows and we need multiple files and modules to organize mocks and tests.

Tried a little bit to move tests around, but ended up backing out of what felt like the start of a bigger test restructure now. Was getting compile errors i didn't understand, that i suspect is because i don't know how the test folder works in a multi-workspace scenario. Currently some of our integration tests work on a smaller dependency subset (which tbf is probably not particularly beneficial).

Happy to explore this again later though, as it might indeed make sense for readibility to have integration tests or mock based unit tests somewhere central, but it feels like a pretty big restructure now. especially as they live all over the place (kube/src/lib + kube-client/src/lib + entry + api/utils + events) with a documented searchable distinction (#[ignore"] marks).

@clux clux merged commit fa1e92e into main Jul 13, 2023
17 checks passed
@clux clux deleted the clux/paginated-watcher branch July 13, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watcher resync causes memory usage burst Support pagination for watcher
3 participants