Reimplement watcher backoff as FSM by nightkr · Pull Request #720 · kube-rs/kube

nightkr · 2021-11-19T09:37:35Z

Part of #703

nightkr · 2021-11-19T09:40:00Z

-    //})
-    ws
+        let next_item = this.stream.try_poll_next(cx);
+        if let Poll::Ready(Some(Err(_))) = &next_item {


This still bubbles up the error, because otherwise the user has no way of logging what's going on. The "actual" indicator for a fatal error is that the Stream closes (returns None).

oh, right, so the it's still technically a TryStream, but the user can just drop the errors. That seems reasonable to me.

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

clux

You are a lifesaver. Thank you so much.

clux · 2021-11-19T18:26:46Z

+        match this.state.as_mut().project() {
+            StreamBackoffStateProj::BackingOff(mut backoff_sleep) => match backoff_sleep.as_mut().poll(cx) {
+                Poll::Ready(()) => {
+                    tracing::debug!(deadline = ?backoff_sleep.deadline(), "Backoff complete, waking up");
+                    this.state.set(State::Awake)
+                }
+                Poll::Pending => {
+                    let deadline = backoff_sleep.deadline();
+                    tracing::trace!(
+                        ?deadline,
+                        remaining_duration = ?deadline.saturating_duration_since(Instant::now()),
+                        "Still waiting for backoff sleep to complete"
+                    );
+                    return Poll::Pending;
+                }
+            },
+            StreamBackoffStateProj::GivenUp => {
+                tracing::debug!("Backoff has given up, stream is closed");
+                return Poll::Ready(None);
+            }
+            StreamBackoffStateProj::Awake => {}
+        }


This is great. Really appreciate the work here. I felt super stuck and frustrated last night, and it would not have occurred to me to try to do a manual Stream impl with this before the poll delegation.

clux · 2021-11-19T18:33:03Z

+        assert_eq!(poll!(rx.next()), Poll::Pending);
+        tokio::time::advance(Duration::from_secs(3)).await;
+        assert_eq!(poll!(rx.next()), Poll::Pending);
+        tx.unbounded_send(Err(3)).unwrap();
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Err(3))));
+        tx.unbounded_send(Ok(4)).unwrap();
+        assert_eq!(poll!(rx.next()), Poll::Pending);
+        tokio::time::advance(Duration::from_secs(3)).await;
+        assert_eq!(poll!(rx.next()), Poll::Pending);
+        tokio::time::advance(Duration::from_secs(2)).await;
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Ok(4))));
+        assert_eq!(poll!(rx.next()), Poll::Pending);


Maybe add a // pending because stream empty or // pending because backing off N seconds
on the assert_eq!(poll!(rx.next()), Poll::Pending); lines for readability.

i can add that.

clux · 2021-11-19T18:33:22Z

+        let rx = stream::iter([Ok(0), Ok(1), Err(2), Ok(3), Ok(4)]);
+        let rx = StreamBackoff::new(rx, backoff::backoff::Constant::new(tick));
+        pin_mut!(rx);
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Ok(0))));
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Ok(1))));
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Err(2))));
+        assert_eq!(poll!(rx.next()), Poll::Pending);
+        tokio::time::advance(tick * 2).await;
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Ok(3))));
+        assert_eq!(poll!(rx.next()), Poll::Ready(Some(Ok(4))));
+        assert_eq!(poll!(rx.next()), Poll::Ready(None));


These tests are great 👍

Based on kube-rs/kube#720, but with generalizations for runtime-agnosticity, and and pin-project-lite.

* implement backoff for watcher - for #577 Signed-off-by: clux <sszynrae@gmail.com> * move magic number into strategy Signed-off-by: clux <sszynrae@gmail.com> * expose backoff from watcher and semi-propagate into controller awkward. will write a comment Signed-off-by: clux <sszynrae@gmail.com> * potential abstraction Signed-off-by: clux <sszynrae@gmail.com> * another builder layer; allow eliding ListParams Signed-off-by: clux <sszynrae@gmail.com> * forgot to add file Signed-off-by: clux <sszynrae@gmail.com> * easy parts of code review Signed-off-by: clux <sszynrae@gmail.com> * rewrite as a helper (take N) jesus this stuff is hard. Signed-off-by: clux <sszynrae@gmail.com> * rename as suggested Signed-off-by: clux <sszynrae@gmail.com> * Reimplement watcher backoff as FSM (#720) * 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> * remove backoff pin, fix docs Signed-off-by: clux <sszynrae@gmail.com> * newline Signed-off-by: clux <sszynrae@gmail.com> * Add `Backoff` wrapper that implements client-go's reset timer behaviour (#729) Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * use new reset backoff and replicate client-go reflector values Signed-off-by: clux <sszynrae@gmail.com> * fix node watcher example Signed-off-by: clux <sszynrae@gmail.com> * Use released `backoff` Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Factor out default `Backoff` Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Add note to `watcher` about backoff Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Added backoff to Controller Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Changelog Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Revert `Observer` for now Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * The clippyman comes for us all, eventually And we must all pay our due respects, or pay the price. Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * Fix build warnings Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se> * remove backoff_watch Signed-off-by: clux <sszynrae@gmail.com> * doc tweaks Signed-off-by: clux <sszynrae@gmail.com> * sentence Signed-off-by: clux <sszynrae@gmail.com> * upgrading backoff is not actually breaking Signed-off-by: clux <sszynrae@gmail.com> Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de> Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>

nightkr commented Nov 19, 2021

View reviewed changes

nightkr force-pushed the backoff-watcher-/fsm branch 2 times, most recently from c202c95 to adc7c43 Compare November 19, 2021 09:58

nightkr added 5 commits November 19, 2021 10:59

Fix clippy warnings

0d1ca8a

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

Reimplement watch backoff as FSM

2199ebe

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

Remove useless lifetime bounds

0659d02

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

Silence clippy size warning

f6422a0

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

Silence clippy properly this time around

830204b

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

nightkr force-pushed the backoff-watcher-/fsm branch from adc7c43 to 830204b Compare November 19, 2021 09:59

nightkr requested a review from clux November 19, 2021 09:59

nightkr added 3 commits November 19, 2021 11:16

Split StreamBackoff into a separate utils module

b4663be

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

Backoff tests

6989096

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

Add stream close test

8ed788b

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

clux approved these changes Nov 19, 2021

View reviewed changes

clux merged commit b8407ab into kube-rs:backoff-watcher Nov 19, 2021

clux mentioned this pull request Nov 19, 2021

Add backoff handling for watcher and Controller #703

Merged

4 tasks

nightkr added a commit to nightkr/backoff that referenced this pull request Nov 22, 2021

Add a helper for pausing a Stream on errors

a8c0110

Based on kube-rs/kube#720, but with generalizations for runtime-agnosticity, and and pin-project-lite.

nightkr mentioned this pull request Nov 22, 2021

Add a helper for pausing a Stream on errors ihrwein/backoff#50

Open

Navigation Menu

Uh oh!

Uh oh!

Reimplement watcher backoff as FSM - #720

Reimplement watcher backoff as FSM#720
clux merged 8 commits into
kube-rs:backoff-watcherfrom
nightkr:backoff-watcher-/fsm

nightkr commented Nov 19, 2021

Uh oh!

nightkr Nov 19, 2021

Uh oh!

clux Nov 19, 2021

Uh oh!

Uh oh!

Uh oh!

clux left a comment

Uh oh!

clux Nov 19, 2021

Uh oh!

clux Nov 19, 2021

Uh oh!

clux Nov 19, 2021

Uh oh!

clux Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

nightkr commented Nov 19, 2021

Uh oh!

nightkr Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

clux Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clux left a comment

Choose a reason for hiding this comment

Uh oh!

clux Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

clux Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

clux Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

clux Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants