Refactor batcher and inline into SessionKeeper #1036

LukasPukenis · 2024-12-18T09:28:20Z

Refactor batcher and inline into SessionKeeper

This commit does these things:
- Inlines batching flag to SessionKeeper, removing batcher entirely
as a separate entity with all of its logic(threshold, traffic trigger).
- Emits all actions from SessionKeeper if batching is enabled when
an action is added. This works nicely if we can guarantee that actions
are gonna be a multiple of some T which is the case as currently we
target T=70s for proxy, direct, vpn, stun keepalives. This also makes
thresholds and the whole logic around them irrelevant.

Traffic triggered batching implementation was doing more harm than good
since it triggers on _any_ traffic, meaning even when alignment was
achieved, it might misalign since any traffic triggers it. This needs to
be done via separate channel which is out of scope for this PR

Feature flags are not removed in this commit so not to push for
version update of libtelio, so now they are no-op.

Problem

--describe problem being solved--

Solution

--describe selected solution--

☑️ Definition of Done checklist

Commit history is clean (requirements)
README.md is updated
Functionality is covered by unit or integration tests

jjanowsk

❤️ ❤️ ❤️

Only some minor nits from my side

jjanowsk · 2024-12-20T11:00:18Z

crates/telio-traversal/src/session_keeper.rs

+        .await
+        .map_err(Error::Task)?
+        .map_err(Error::RepeatedActionError)
+        .map(|_| ())?;


.map(|_| ())?; I think this is a no-op and can be removed

You're right, removed it. Probably previously it was inlined directly into Ok() so it made sense

jjanowsk · 2024-12-20T11:14:31Z

crates/telio-utils/src/repeated_actions.rs

+            .unwrap();
+        ctx.actions.select_action().await.unwrap().1(&mut ctx)
+            .await
+            .unwrap();


Why special case for ts_10 and ts_13? You can just add them to the vector and loop 6 times instead of 4

You're right about these testcases. I tried to be very explicit but now reading it it gives me impression that it's totally unecessary so I went along with your suggestions and inlined it effectively converting the case to just a vector and a loop :)

jjanowsk · 2024-12-20T11:19:53Z

crates/telio-utils/src/repeated_actions.rs

+                    .unwrap();
+
+                assert_eq!(ctx.get(), values_to_expect.pop().unwrap());
+            }


Maybe just:

let values_to_expect = vec!["ts_10", "ts_13", "ts_20", "ts_23", "ts_30", "ts_33"]; for time in values_to_expect { ctx.actions.select_action().await.unwrap().1(&mut ctx) .await .unwrap(); assert_eq!(ctx.get(), time); }

did so 👍

jjanowsk · 2024-12-20T11:20:08Z

crates/telio-utils/src/repeated_actions.rs

+                    .unwrap();
+
+                assert_eq!(ctx.get(), values_to_expect.pop().unwrap());
+            }


jjanowsk · 2024-12-20T11:23:01Z

src/device/wg_controller.rs

+        "actual_keys: {}, requested_keys: {}",
+        actual_peers.keys().len(),
+        requested_peers.keys().len(),
+    );


Leftover from debugging?

Good catch, thanks!

jjanowsk · 2024-12-20T11:23:33Z

src/device/wg_controller.rs

+                    telio_log_debug!(
+                        "Will insert with keepalive of {:?}",
+                        persistent_keepalive_interval
+                    );


Also look like some leftover?

Removed it, it was a leftover

This commit does these things: - Inlines batching flag to SessionKeeper, removing batcher entirely as a separate entity with all of its logic(threshold, traffic trigger). - Emits all actions from SessionKeeper if batching is enabled when an action is added. This works nicely if we can guarantee that actions are gonna be a multiple of some T which is the case as currently we target T=70s for proxy, direct, vpn, stun keepalives. This also makes thresholds and the whole logic around them irrelevant. Traffic triggered batching implementation was doing more harm than good since it triggers on _any_ traffic, meaning even when alignment was achieved, it might misalign since any traffic triggers it. Feature flags are not removed in this commit so not to push for version update of libtelio, so now they are no-op. Signed-off-by: Lukas Pukenis <[email protected]> Signed-off-by: Lukas Pukenis <[email protected]>

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 3678fc7 to 002eaa4 Compare December 18, 2024 09:39

LukasPukenis temporarily deployed to Internal December 18, 2024 09:39 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 002eaa4 to b3979cf Compare December 18, 2024 09:45

LukasPukenis temporarily deployed to Internal December 18, 2024 09:45 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from b3979cf to 5e49bb5 Compare December 18, 2024 10:02

LukasPukenis temporarily deployed to Internal December 18, 2024 10:02 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 5e49bb5 to 2d34b68 Compare December 18, 2024 10:06

LukasPukenis temporarily deployed to Internal December 18, 2024 10:06 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 2d34b68 to b73a4ea Compare December 18, 2024 10:24

LukasPukenis temporarily deployed to Internal December 18, 2024 10:24 — with GitHub Actions Inactive

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from b73a4ea to c61628a Compare December 18, 2024 11:40

LukasPukenis temporarily deployed to Internal December 18, 2024 11:40 — with GitHub Actions Inactive

LukasPukenis marked this pull request as ready for review December 18, 2024 11:41

LukasPukenis requested a review from a team as a code owner December 18, 2024 11:41

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from c61628a to 03ce6e3 Compare December 18, 2024 11:41

LukasPukenis temporarily deployed to Internal December 18, 2024 11:41 — with GitHub Actions Inactive

jjanowsk reviewed Dec 20, 2024

View reviewed changes

LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 03ce6e3 to 613ca6a Compare December 20, 2024 17:05

LukasPukenis temporarily deployed to Internal December 20, 2024 17:06 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor batcher and inline into SessionKeeper #1036

Refactor batcher and inline into SessionKeeper #1036

LukasPukenis commented Dec 18, 2024 •

edited

Loading

jjanowsk left a comment

jjanowsk Dec 20, 2024

LukasPukenis Dec 20, 2024

jjanowsk Dec 20, 2024

LukasPukenis Dec 20, 2024

jjanowsk Dec 20, 2024

LukasPukenis Dec 20, 2024

jjanowsk Dec 20, 2024

jjanowsk Dec 20, 2024

LukasPukenis Dec 20, 2024

jjanowsk Dec 20, 2024

LukasPukenis Dec 20, 2024

Refactor batcher and inline into SessionKeeper #1036

Are you sure you want to change the base?

Refactor batcher and inline into SessionKeeper #1036

Conversation

LukasPukenis commented Dec 18, 2024 • edited Loading

Problem

Solution

☑️ Definition of Done checklist

jjanowsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPukenis commented Dec 18, 2024 •

edited

Loading