Detach batcher and direct feature #996
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
1fc2a69 to
e11fb52
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
e11fb52 to
8c1bfb0
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
8c1bfb0 to
0a21893
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
0a21893 to
1d716f8
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
1d716f8 to
2706f0a
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
2706f0a to
77232d6
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
77232d6 to
208e05a
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
208e05a to
b64cc85
Compare
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
b64cc85 to
3d172c1
Compare
LukasPukenis
commented
Dec 4, 2024
dfetti
reviewed
Dec 10, 2024
jjanowsk
reviewed
Dec 12, 2024
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
3d172c1 to
f62848b
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
f62848b to
10ae6c7
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
10ae6c7 to
a339eb2
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
a339eb2 to
9228b5b
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
9228b5b to
ed95223
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
ed95223 to
03de9c2
Compare
LukasPukenis
had a problem deploying
to
Internal
GitHub Actions
Failure
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
03de9c2 to
e2ecc6b
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
jjanowsk
reviewed
Dec 16, 2024
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
e2ecc6b to
b0e5359
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
jjanowsk
approved these changes
Dec 17, 2024
Batcher feature works by employing SessionKeeper however since it's under a direct feature flag it means that without enabling it as well, there's a failure. This commit introduces a testcase that highlights that and at the moment fails to pass. Signed-off-by: Lukas Pukenis <[email protected]>
SessionKeeper is solely used for Direct connections and thus only enabled when Direct feature is enabled. However since it is very minimal, it makes sense to augment it for use in batching more things. This commit moves out SessionKeeper out from Direct struct to the parent structure and is always present from now on. Signed-off-by: Lukas Pukenis <[email protected]>
SessionKeeper log refactored to be easier to follow. Also introduces calls to SessionKeeper if batching is enabled when upgrading from Proxying to Direct and vice-versa. Current solution because the key is the same skips that and misaligns the keepalives which is an easy win. A bunch of testcases are augmented with batching parameter Signed-off-by: Lukas Pukenis <[email protected]>
Signed-off-by: Lukas Pukenis <[email protected]>
SessionKeeper might fail as indicated by a failing test on MacOS without this commit. When failing the previous behaviour was to set direct entities to None and disable the functionality. This commit reintroduces behaviour and also toggles batching feature off since keepalives will rely on it. Signed-off-by: Lukas Pukenis <[email protected]>
Proxying persistent keepalives are being done by batcher when batching is used. Batcher uses ICMP echo requests(pings) which is a different mechanism from wireguard backend's keepalives. To ensure those are being emitted the testcase uses link detection and stops the peer. Given that there's no other generated traffic it should trigger link detection pretty soon and prove that batcher works correctly at least to some degree. Signed-off-by: Lukas Pukenis <[email protected]>
Signed-off-by: Lukas Pukenis <[email protected]>
LukasPukenis
force-pushed
the
LLT-5795_detach_session_keeper_and_direct_feature
branch
from
16efa59 to
aee6ac6
Compare
LukasPukenis
temporarily deployed
to
Internal
GitHub Actions
Inactive
LukasPukenis
deleted the
LLT-5795_detach_session_keeper_and_direct_feature
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Batching relies on Sessionkeeper(is baked into it) which is not present when direct feature is disabled or if SessionKeeper::start fails.
This breaks cases where batching is enabled but direct feature is not for VPN reestablishment for example(added testcases fail on
main). This is a limitation of the decision to keep SessionKeeper instead of introducing a separate entity for batching.The fix is to override the batching feature when either SessionKeeper or Direct are None which is done here.
However I did an extra step and moved SessionKeeper outside the Direct entities as I think it doesn't belong there anymore due to being repurposed for more stuff(vpn, proxy, stun keepalives when batching, derp keepalives in progress). This moving out provided very minimal changes, though the diff looks huge.
Please see individual commits for more details
☑️ Definition of Done checklist