Add backoff handling for watcher and Controller #703

clux · 2021-11-11T10:43:44Z

backoff_watch helper from teo Reimplement watcher backoff as FSM #720
ResetTimerBackoff helper from teo Add Backoff wrapper that implements client-go's reset timer behaviour #729
~~Observer abstraction around watcher + backoff_watch + ResetTimerBackoff~~ ignored for now
default_backoff following client-go defaults
requires teo's pr of backoff: Implement Backoff for Box<B: Backoff> ihrwein/backoff#49 (cargo deny blocks git pins)

introduces a dependency on backoff 0.4.0 for parts of this.

old notes, no longer relevant not hiding errors

initial experiment with watcher + backoff. injecting a backoff into the main `stream::unfold` of `watcher` and sleeping between steps. It works with `configmapgen_controller` (in say the example where the crd has not been installed); it keeps sleeping until a max time hits (figured we'd set this to something like 10-30minutes) after which the stream ends.

Problems:

watcher bubbles up the error as it stands - so normal watcher users stop at the first error anyway
Controller ignores error events from watcher so at some point the Controller becomes dysfunctional
how to expose the parameter - imo, hide it behind a builder with sensible defaults - e.g. Cache: implement a builder wrapper for reflector #698 .

Solutions:
We could let the timeout grow almost indefinitely (to like an hour or more backoff) so that Controller can always deal with it.
We could also expose a new watcher error type that wraps the original - so that the user can ignore it easily, but not sure, what that interface would look like yet. Maybe this is something a builder can ignore by default in the way it presents the stream - i.e. ignore until the error has been going on for so lonng.

Also, should try to get the magic number baked into the backoff struct (it supports returning None). It didn't work with max_interval, but looks like it should work with https://docs.rs/backoff/0.3.0/backoff/exponential/struct.ExponentialBackoff.html
max_elapsed_time - yup works.

Anyway, will try more later.

nightkr · 2021-11-12T11:50:56Z

Sorry about the slow reviewing, I've been (and still am) home sick. I'll try to have a closer look when I'm back alive...

clux · 2021-11-12T16:12:02Z

Sorry about the slow reviewing, I've been (and still am) home sick. I'll try to have a closer look when I'm back alive...

No rush. Hope you get some rest and get to recover!

examples/configmapgen_controller.rs

kube-runtime/src/observer.rs

kube-runtime/src/watcher.rs

kube-runtime/src/observer.rs

kube-runtime/src/watcher.rs

Signed-off-by: clux <sszynrae@gmail.com>

awkward. will write a comment Signed-off-by: clux <sszynrae@gmail.com>

Signed-off-by: clux <sszynrae@gmail.com>

jesus this stuff is hard. Signed-off-by: clux <sszynrae@gmail.com>

Signed-off-by: clux <sszynrae@gmail.com>

* Fix clippy warnings Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Reimplement watch backoff as FSM Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Remove useless lifetime bounds Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Silence clippy size warning Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Silence clippy properly this time around Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Split StreamBackoff into a separate utils module Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Backoff tests Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Add stream close test Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Signed-off-by: clux <sszynrae@gmail.com>

…ur (#729) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Signed-off-by: clux <sszynrae@gmail.com>

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

And we must all pay our due respects, or pay the price. Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

…eanup Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Backoff watcher cleanup

codecov-commenter · 2021-12-21T14:45:34Z

Codecov Report

Merging #703 (d02b1ec) into master (155859b) will increase coverage by 0.31%.
The diff coverage is 77.09%.

@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
+ Coverage   70.83%   71.15%   +0.31%     
==========================================
  Files          52       54       +2     
  Lines        3405     3529     +124     
==========================================
+ Hits         2412     2511      +99     
- Misses        993     1018      +25

Impacted Files	Coverage Δ
kube-client/src/discovery/mod.rs	`89.18% <ø> (ø)`
kube-core/src/dynamic.rs	`66.66% <ø> (ø)`
kube-core/src/object.rs	`80.00% <ø> (ø)`
kube-core/src/params.rs	`66.35% <ø> (ø)`
kube-core/src/subresource.rs	`75.72% <ø> (ø)`
kube-runtime/src/controller/mod.rs	`0.00% <0.00%> (ø)`
kube-runtime/src/utils/mod.rs	`17.64% <ø> (ø)`
kube-runtime/src/watcher.rs	`54.92% <0.00%> (-4.17%)`	⬇️
kube/src/lib.rs	`87.80% <ø> (ø)`
kube-client/src/lib.rs	`92.80% <25.00%> (-0.06%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 155859b...d02b1ec. Read the comment docs.

clux · 2021-12-21T14:46:07Z

kube-runtime/src/utils/stream_backoff.rs

+impl<S: TryStream, B: Backoff> StreamBackoff<S, B> {
+    pub fn new(stream: S, backoff: B) -> Self {
+        Self {
+            stream,
+            backoff,
+            state: State::Awake,
+        }
+    }
+}


One thought on StreamBackoff: because this is effectively a stream analogue of https://docs.rs/backoff/0.4.0/backoff/fn.retry.html , maybe we should allow bailing at bad errors in a simiar way to how backoff.retry defines its error type: https://docs.rs/backoff/0.4.0/backoff/enum.Error.html ?

Hm, maybe. I'd lean towards KISS for now, and letting @ihrwein decide the future plan in the upstreaming PR (ihrwein/backoff#50).

clux · 2021-12-21T14:58:59Z

kube-runtime/src/watcher.rs

+/// Pauses the stream for a while (as defined by `backoff`) after each [`Err`].
+pub fn backoff_watch<K, B>(
+    stream: impl Stream<Item = Result<Event<K>>> + Send,
+    backoff: B,
+) -> impl Stream<Item = Result<Event<K>>> + Send
+where
+    K: Resource + Send,
+    B: Backoff + Send,
+{
+    StreamBackoff::new(stream, backoff)
+}


This appears unused now after the Observer cancel. I can remove this?

Good catch. This was an attempt to be somewhat more ergonomic and point people who were already working in the watcher module towards it (rather than focusing on the implementation details of StreamBackoff itself). We can either drop this, or revert StreamBackoff::new to pub(crate) and fixing the examples to use backoff_watch instead.

I'm fine with either.

I removed it for now. Want to focus on the future chain pipeline before teaching people too much new stuff.

Signed-off-by: clux <sszynrae@gmail.com>

CHANGELOG.md

This comment has been minimized.

Sign in to view

clux requested a review from nightkr November 11, 2021 21:22

clux mentioned this pull request Nov 11, 2021

Make SystemClock Clone to make ExponentialBackoff Clone ihrwein/backoff#46

Merged

clux linked an issue Nov 11, 2021 that may be closed by this pull request

Add backoff to watcher reconnection #577

Closed

nightkr reviewed Nov 16, 2021

View reviewed changes

clux added 7 commits November 16, 2021 19:47

implement backoff for watcher - for #577

c26e894

Signed-off-by: clux <sszynrae@gmail.com>

move magic number into strategy

19e6b5b

Signed-off-by: clux <sszynrae@gmail.com>

expose backoff from watcher and semi-propagate into controller

09ee2f3

awkward. will write a comment Signed-off-by: clux <sszynrae@gmail.com>

potential abstraction

dda1a7b

Signed-off-by: clux <sszynrae@gmail.com>

another builder layer; allow eliding ListParams

f6b034e

Signed-off-by: clux <sszynrae@gmail.com>

forgot to add file

f5063a7

Signed-off-by: clux <sszynrae@gmail.com>

easy parts of code review

ff2cfe4

Signed-off-by: clux <sszynrae@gmail.com>

clux force-pushed the backoff-watcher branch from 5a2bd69 to ff2cfe4 Compare November 16, 2021 19:49

clux added 2 commits November 19, 2021 00:16

rewrite as a helper (take N)

0fc7159

jesus this stuff is hard. Signed-off-by: clux <sszynrae@gmail.com>

rename as suggested

344cb92

Signed-off-by: clux <sszynrae@gmail.com>

nightkr mentioned this pull request Nov 19, 2021

Reimplement watcher backoff as FSM #720

Merged

nightkr and others added 4 commits November 19, 2021 18:34

remove backoff pin, fix docs

62679a9

Signed-off-by: clux <sszynrae@gmail.com>

newline

3144791

Signed-off-by: clux <sszynrae@gmail.com>

Merge branch 'master' into backoff-watcher

00e94f3

clux marked this pull request as ready for review November 19, 2021 23:53

Merge branch 'master' into backoff-watcher

dbccd9b

This was referenced Nov 20, 2021

builders for kube-runtime reflector #276

Closed

Rate Limiting Strategies #500

Closed

nightkr mentioned this pull request Nov 22, 2021

Add Backoff wrapper that implements client-go's reset timer behaviour #729

Merged

nightkr and others added 3 commits November 22, 2021 21:23

Add Backoff wrapper that implements client-go's reset timer behavio…

be2dc1c

…ur (#729) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Merge remote-tracking branch 'origin/master' into backoff-watcher

bc71e9e

use new reset backoff and replicate client-go reflector values

5c780f5

Signed-off-by: clux <sszynrae@gmail.com>

nightkr mentioned this pull request Dec 21, 2021

Backoff watcher cleanup #763

Merged

nightkr and others added 10 commits December 21, 2021 11:33

Use released backoff

927d630

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Factor out default Backoff

676f085

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Add note to watcher about backoff

c142ca1

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Added backoff to Controller

6aa0d79

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Changelog

2805d14

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Revert Observer for now

0402c0b

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

The clippyman comes for us all, eventually

426a30a

And we must all pay our due respects, or pay the price. Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Merge remote-tracking branch 'origin/master' into backoff-watcher-/cl…

33083f3

…eanup Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Fix build warnings

4fb49a8

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>

Merge pull request #763 from teozkr/backoff-watcher-/cleanup

d02b1ec

Backoff watcher cleanup

clux added the changelog-add changelog added category for prs label Dec 21, 2021

clux added this to the 0.66.0 milestone Dec 21, 2021

clux commented Dec 21, 2021

View reviewed changes

nightkr approved these changes Dec 21, 2021

View reviewed changes

clux changed the title ~~implement backoff for watcher - for #577~~ Add backoff handling for watcher and Controller - for #577 Dec 21, 2021

clux commented Dec 21, 2021

View reviewed changes

clux added 4 commits December 21, 2021 22:03

remove backoff_watch

ee8face

Signed-off-by: clux <sszynrae@gmail.com>

doc tweaks

c8179f4

Signed-off-by: clux <sszynrae@gmail.com>

sentence

c973255

Signed-off-by: clux <sszynrae@gmail.com>

upgrading backoff is not actually breaking

fd24a0f

Signed-off-by: clux <sszynrae@gmail.com>

clux commented Dec 21, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

nightkr approved these changes Dec 21, 2021

View reviewed changes

clux merged commit 276719c into master Dec 21, 2021

clux deleted the backoff-watcher branch December 21, 2021 16:16

clux added changelog-change changelog change category for prs and removed changelog-add changelog added category for prs labels Dec 22, 2021

clux changed the title ~~Add backoff handling for watcher and Controller - for #577~~ Add backoff handling for watcher and Controller Jan 15, 2022

soenkeliebau mentioned this pull request Jan 17, 2022

Update kube-rs from 0.65.0 to 0.66.0 stackabletech/operator-rs#295

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add backoff handling for watcher and Controller #703

Add backoff handling for watcher and Controller #703

clux commented Nov 11, 2021 •

edited

Loading

This comment has been minimized.

nightkr commented Nov 12, 2021

clux commented Nov 12, 2021

codecov-commenter commented Dec 21, 2021

clux Dec 21, 2021

nightkr Dec 21, 2021

clux Dec 21, 2021

nightkr Dec 21, 2021

clux Dec 21, 2021 •

edited

Loading

Add backoff handling for watcher and Controller #703

Add backoff handling for watcher and Controller #703

Conversation

clux commented Nov 11, 2021 • edited Loading

This comment has been minimized.

nightkr commented Nov 12, 2021

clux commented Nov 12, 2021

codecov-commenter commented Dec 21, 2021

Codecov Report

clux Dec 21, 2021

Choose a reason for hiding this comment

nightkr Dec 21, 2021

Choose a reason for hiding this comment

clux Dec 21, 2021

Choose a reason for hiding this comment

nightkr Dec 21, 2021

Choose a reason for hiding this comment

clux Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

clux commented Nov 11, 2021 •

edited

Loading

clux Dec 21, 2021 •

edited

Loading