Align `watcher::Event` init/page variants #1504

clux · 2024-05-29T15:43:55Z

Merges - Event::InitPage into Event::InitApply

Follow-up to the unreleased #1494 as suggested in #1499.

This has several consequences;

buffering of individual pages done in watcher
no buffering needed in any flatteners (no need for the concept of flattening)
no need to eventually break the Event enum if we remove paging (when streaming lists is stabilised and everywhere)

Because of 1. this slightly changing the no-buffering PR in #1494 and as such it needs to be benchmarked, but it is a nice reduction in complexity within the runtime if we can justify it. EDIT: it performs the same.

The second point kind of hints at an even larger refactoring where we do not pass vector around internally at all and possibly killing/repurposing EventFlatten.

**Merges - `Event::InitPage` and `Event::InitApply`** This has several consequences; - buffering of pages done in watcher (need to benchmark this since it is likely worse memory wise) - no buffering needed in any flatteners (further simplification can be done later) Signed-off-by: clux <[email protected]>

Signed-off-by: clux <[email protected]>

codecov · 2024-05-29T16:26:13Z

Codecov Report

Attention: Patch coverage is 83.87097% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.1%. Comparing base (de3fe1e) to head (7706413).

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1504     +/-   ##
=======================================
+ Coverage   75.0%   75.1%   +0.2%     
=======================================
  Files         78      78             
  Lines       6854    6864     +10     
=======================================
+ Hits        5134    5150     +16     
+ Misses      1720    1714      -6

Files	Coverage Δ
kube-runtime/src/reflector/dispatcher.rs	`96.5% <100.0%> (+0.3%)`	⬆️
kube-runtime/src/reflector/mod.rs	`100.0% <100.0%> (ø)`
kube-runtime/src/reflector/store.rs	`96.9% <ø> (+3.0%)`	⬆️
kube-runtime/src/utils/event_flatten.rs	`91.2% <100.0%> (+1.5%)`	⬆️
kube-runtime/src/utils/event_modify.rs	`95.5% <100.0%> (ø)`
kube-runtime/src/utils/predicate.rs	`73.4% <100.0%> (ø)`
kube-runtime/src/utils/reflect.rs	`100.0% <100.0%> (ø)`
kube-runtime/src/utils/watch_ext.rs	`22.3% <50.0%> (ø)`
kube-runtime/src/watcher.rs	`41.8% <67.9%> (+1.7%)`	⬆️

Signed-off-by: clux <[email protected]>

clux · 2024-05-29T16:55:55Z

Quick controller deployment of gives me the exact same memory profile as the one with zero buffering, so this appears to be a pure ergonomic improvement.

Signed-off-by: clux <[email protected]>

clux · 2024-05-30T10:11:43Z

For clarity: my benchmarked (running in a real world controller):

last two uses new store buffering, and the last uses this PR. memory profile is always slightly growing in this controller (separate issue), but the starting baseline is exactly the same between this and the previous store buffering. All controllers doing the old internal buffering started at ~80+M (though with some variance), but the two new ones are right there at 46M which nothing has gotten close to.

clux · 2024-05-30T15:09:46Z

kube-runtime/src/watcher.rs

-            // We're filtering by object name, so getting more than one object means that either:
-            // 1. The apiserver is accepting multiple objects with the same name, or
-            // 2. The apiserver is ignoring our query
-            // In either case, the K8s apiserver is broken and our API will return invalid data, so
-            // we had better bail out ASAP.
-            Ok(Event::InitPage(objs)) if objs.len() > 1 => Some(Err(Error::TooManyObjects)),


NB: This comment was technically wrong because it can happen if users use a Api::all scoped Api against names that exist in multiple namespaces. Now we just pick the first consistently (which is also what we did for streaming lists).

Additionally because page events do not happen anymore, the TooManyObjects error goes away (this is the only real place it happened). Tests have used this error variant everywhere as a type of convenience though, so that's why there's many strange test changes touching this.

Ah, that explains the tests... is the ordering random or does the api server ever pre-filter the objects based on some other property like timestamp (i.e. creation time)?

i think on a watch against rv="0" it's actually alphabetical - which in this case doesn't help much if there's duplicate objects (in say different namespaces).

bad idea

it's possible we could change this fn to split depending on what scope the `Api` is used for, ala:
let fields = if let Some(ns) = api.namespace { format!("metadata.name={name},metadata.namespace={ns}") } else { format!("metadata.name={name}") };

but that requires exposing namespace as pub, so will do that as a follow-up
EDIT: it also doesn't help because if the Api (being passed in` is scoped to namespace, then the watch is also scoped so this suggestion is pointless.

minor docs follow-up for this: #1510

mateiidavid

Looks great! Thanks so much for the clean-up and really straightforward comments, it made reviewing much easier.

mateiidavid · 2024-06-03T21:34:33Z

kube-runtime/src/utils/event_flatten.rs

-        Self {
-            stream,
-            queue: vec![].into_iter(),
-            emit_deleted,
-        }


kind of cool that this got simplified as a result

yeah, think we can maybe deprecate / repurpose this thing entirely to some kind of filter module instead (since the concept of an "unflattened stream" kind of goes away externally (which is good, it was a big source of confusion).

kube-runtime/src/watcher.rs

mateiidavid · 2024-06-03T21:42:03Z

kube-runtime/src/watcher.rs

-            }
+            InitialListStrategy::ListWatch => (Some(Ok(Event::Init)), State::InitPage {
+                continue_token: None,
+                objects: VecDeque::default(),


If we know the page size in advance (since it's part of the strategy) would it make sense to allocate a ring buffer with a predetermined capacity equal to the page size? Bit of a micro optimisation I suppose 🤔

Edit: nvm, this wouldn't work since we always construct a new InitPage for each page we consume. Would overcomplicate things.

hm, yeah, maybe there is something we can do here, but not sure what. it's also going to become "the old way" of doing things once this kubernetes feature becomes stabilised and rolled out everywhere, so 🤷

it doesn't seem to perform any noticeably worse than with main (with the zero buffering in watcher setup) so it's probably fine. have run this for a week now.

mateiidavid · 2024-06-03T21:50:04Z

kube-runtime/src/watcher.rs

-            // We're filtering by object name, so getting more than one object means that either:
-            // 1. The apiserver is accepting multiple objects with the same name, or
-            // 2. The apiserver is ignoring our query
-            // In either case, the K8s apiserver is broken and our API will return invalid data, so
-            // we had better bail out ASAP.
-            Ok(Event::InitPage(objs)) if objs.len() > 1 => Some(Err(Error::TooManyObjects)),


Ah, that explains the tests... is the ordering random or does the api server ever pre-filter the objects based on some other property like timestamp (i.e. creation time)?

and two other minor ones Signed-off-by: clux <[email protected]>

clux added the changelog-change changelog change category for prs label May 29, 2024

clux changed the title ~~Align watcher::Event variants for initialisation~~ Align watcher::Event init/page variants May 29, 2024

clux added 2 commits May 29, 2024 17:15

get tests to compile

e4dcf14

Signed-off-by: clux <[email protected]>

fix tests

6ebfb5b

Signed-off-by: clux <[email protected]>

Event::into_iter helpers are now unnecessary (and unused)

16eadae

Signed-off-by: clux <[email protected]>

clux marked this pull request as ready for review May 29, 2024 16:55

clux added 2 commits May 29, 2024 18:13

avoid reversing event list from pages

f9b7216

Signed-off-by: clux <[email protected]>

can kill smallvec now

8a60bb3

Signed-off-by: clux <[email protected]>

clux requested review from nightkr and mateiidavid May 29, 2024 19:00

Merge branch 'main' into watcher-strategy-align

443f1e5

clux added this to the 0.92.0 milestone May 30, 2024

docs for the enum were bad, update after proofread

3787de8

Signed-off-by: clux <[email protected]>

clux commented May 30, 2024

View reviewed changes

clux mentioned this pull request May 30, 2024

Use a channel for watcher Restarted instead of buffering to vec #1506

Closed

mateiidavid approved these changes Jun 3, 2024

View reviewed changes

clux added 2 commits June 4, 2024 11:50

typing tweaks spotted from codereview

29d96a3

and two other minor ones Signed-off-by: clux <[email protected]>

Merge branch 'main' into watcher-strategy-align

7706413

clux merged commit 6ce3978 into main Jun 4, 2024
17 checks passed

clux deleted the watcher-strategy-align branch June 4, 2024 11:08

This was referenced Jun 10, 2024

Release 0.92 #1513

Closed

Remove relics of unflattened / flattened stream #1517

Closed

Create a memory benchmark for watcher #1505

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align `watcher::Event` init/page variants #1504

Align `watcher::Event` init/page variants #1504

clux commented May 29, 2024 •

edited

Loading

codecov bot commented May 29, 2024 •

edited

Loading

clux commented May 29, 2024 •

edited

Loading

clux commented May 30, 2024

clux May 30, 2024 •

edited

Loading

mateiidavid Jun 3, 2024

clux Jun 4, 2024 •

edited

Loading

clux Jun 4, 2024

mateiidavid left a comment

mateiidavid Jun 3, 2024

clux Jun 4, 2024

mateiidavid Jun 3, 2024

clux Jun 4, 2024 •

edited

Loading

mateiidavid Jun 3, 2024

Align watcher::Event init/page variants #1504

Align watcher::Event init/page variants #1504

Conversation

clux commented May 29, 2024 • edited Loading

codecov bot commented May 29, 2024 • edited Loading

Codecov Report

clux commented May 29, 2024 • edited Loading

clux commented May 30, 2024

clux May 30, 2024 • edited Loading

Choose a reason for hiding this comment

mateiidavid Jun 3, 2024

Choose a reason for hiding this comment

clux Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

clux Jun 4, 2024

Choose a reason for hiding this comment

mateiidavid left a comment

Choose a reason for hiding this comment

mateiidavid Jun 3, 2024

Choose a reason for hiding this comment

clux Jun 4, 2024

Choose a reason for hiding this comment

mateiidavid Jun 3, 2024

Choose a reason for hiding this comment

clux Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

mateiidavid Jun 3, 2024

Choose a reason for hiding this comment

Align `watcher::Event` init/page variants #1504

Align `watcher::Event` init/page variants #1504

clux commented May 29, 2024 •

edited

Loading

codecov bot commented May 29, 2024 •

edited

Loading

clux commented May 29, 2024 •

edited

Loading

clux May 30, 2024 •

edited

Loading

clux Jun 4, 2024 •

edited

Loading

clux Jun 4, 2024 •

edited

Loading