Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow VM to talk to a co-locating server via a private load balancer #341

Merged
merged 8 commits into from
Aug 2, 2023

Conversation

byteocean
Copy link
Contributor

This PR deals with a special communication scenario as depicted in the following figure (thanks to @guvenc). More precisely, due to the complex flow pattern, the connection tracking was not correctly performed and matched for packets. This PR includes two major changes:

  1. move connection tracking related code out so as this tracking path can be reused;
  2. correctly mark lb traffic as part of its flow key;
  3. allow correct flow tracking and tcp state tracking for such scenario.

image

@github-actions github-actions bot added size/XL bug Something isn't working labels Jul 20, 2023
@guvenc
Copy link
Collaborator

guvenc commented Jul 20, 2023

@byteocean clang build fails.

Copy link
Collaborator

@guvenc guvenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes needed and some open questions. Thanks.

src/dp_cntrack.c Show resolved Hide resolved
src/nodes/conntrack_node.c Outdated Show resolved Hide resolved
src/dp_cntrack.c Outdated Show resolved Hide resolved
src/dp_cntrack.c Outdated Show resolved Hide resolved
src/dp_flow.c Outdated Show resolved Hide resolved
src/rte_flow/dp_rte_flow_traffic_forward.c Show resolved Hide resolved
src/nodes/tx_node.c Outdated Show resolved Hide resolved
@guvenc guvenc force-pushed the fix/lb_track_new branch 2 times, most recently from 573202e to 8e1a1c5 Compare July 31, 2023 12:17
@guvenc
Copy link
Collaborator

guvenc commented Jul 31, 2023

@byteocean I pushed an improved version of the solution in this PR. Conntrack refactoring is ok but the way the traffic is isolated changed a bit. (More similar to the "other" uni-directional flows we have) Please take a look.
Please also remove the commit https://github.com/onmetal/net-dpservice/pull/341/commits/7dc87c27dfb802cd4ce090125e4297d2e130e983 from this PR, as it has its own PR.
Thanks.

@byteocean byteocean force-pushed the fix/lb_track_new branch 3 times, most recently from 5db9b28 to 5a55108 Compare August 1, 2023 12:45
@byteocean
Copy link
Contributor Author

Some minor revisions on code comments have been added to be more clear in the code why it is written in this way, otherwise, it is a difficult special case that is hard to digest.

A improvement solution on the route instability issue (on-demand, which is different from #344 ) is also provided in this PR, otherwise, this PR cannot work properly.

@guvenc
Copy link
Collaborator

guvenc commented Aug 1, 2023

@byteocean What about age_ctx storage ? This still seems to be 4 ? Dont we need to increase it as discussed today ?

@byteocean
Copy link
Contributor Author

@guvenc it was put into another branch, but now also fixed in this one

@guvenc
Copy link
Collaborator

guvenc commented Aug 2, 2023

@byteocean
Please remove the commit https://github.com/onmetal/net-dpservice/pull/341/commits/aa9947f30ba2082569e403506515f60fbd2d3d58 from this PR, as we discussed yesterday. It resolves a separate issue and I want to discuss it further in its own PR #344 . Please update that PR #344 with this new commit and remove it here so we can merge this one. Thanks.

@byteocean
Copy link
Contributor Author

This new commit is removed. The point of having this commit is to make this PR a working one when route change is happening.

@guvenc guvenc merged commit 8073477 into main Aug 2, 2023
9 checks passed
@guvenc guvenc deleted the fix/lb_track_new branch August 2, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants