| Message ID | 20230707055147.961462-1-numans@ovn.org |
|---|---|
| Headers | show |
| Series | northd: I-P for load balancer and lb groups | expand |
Hi Numan, I gave the series a look. I've looked at the code but haven't yet run any tests with it. The main reason for this is that the series does not apply cleanly to OVN main. Overall, I only have small notes. I've replied to patches 2 and 3 with the specifics. On 7/7/23 01:51, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > This patch series adds the support to handle load balancer and > load balancer group changes incrementally in the "northd" engine > node. "flow" engine node doesn't support I-P yet and falls back > to full recompute. > > Note: I still need to do some scale testing and measure the > improvements. I'll first attempt adding I-P in the "flow" > engine node before attempting some scale tests. Neverthless > this patch series is good enough for reviews. > > > v1 -> v2 > -------- > * Resolved the conflicts and rebased > * Fixed a bug in p6. > > Numan Siddique (8): > northd I-P: Sync SB load balancers in a separate engine node. > northd: Add a new engine node - northd_lb_data. > northd: Add initial I-P for load balancer and load balancer groups > northd: Refactor the 'northd' node code which handles logical switch > changes. > northd: Handle load balancer changes for a logical switch. > northd: Handle load balancer group changes for a logical switch. > northd: Sync SB Port bindings NAT column in a separate engine node. > northd: Handle load balancer changes for a logical router. > > lib/lb.c | 356 ++++++- > lib/lb.h | 113 ++- > northd/automake.mk | 2 + > northd/en-lflow.c | 8 +- > northd/en-northd-lb-data.c | 308 ++++++ > northd/en-northd-lb-data.h | 56 ++ > northd/en-northd.c | 57 +- > northd/en-northd.h | 2 + > northd/en-sync-sb.c | 60 ++ > northd/en-sync-sb.h | 9 + > northd/inc-proc-northd.c | 27 +- > northd/northd.c | 1858 +++++++++++++++++++++++++----------- > northd/northd.h | 44 +- > tests/ovn-northd.at | 131 +++ > 14 files changed, 2354 insertions(+), 677 deletions(-) > create mode 100644 northd/en-northd-lb-data.c > create mode 100644 northd/en-northd-lb-data.h >
On Sat, Jul 8, 2023 at 1:27 AM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Numan, > > I gave the series a look. I've looked at the code but haven't yet run > any tests with it. The main reason for this is that the series does not > apply cleanly to OVN main. > > Overall, I only have small notes. I've replied to patches 2 and 3 with > the specifics. Thanks for the review Mark. I'll address those comments and any other in v3. I'll wait for a bit before submitting v3 if you or others have more comments. I did rebase before submitting. Maybe some mistake from my side. But looks like ovsrobot was able to successfully apply the patches and run the tests. If you want to continue reviewing can you please pull it from here - https://github.com/numansiddique/ovn/tree/northd_ip_refactor_lb_ip_v2_p8 or - https://github.com/ovsrobot/ovn/tree/series_362800 Thanks Numan > > On 7/7/23 01:51, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > This patch series adds the support to handle load balancer and > > load balancer group changes incrementally in the "northd" engine > > node. "flow" engine node doesn't support I-P yet and falls back > > to full recompute. > > > > Note: I still need to do some scale testing and measure the > > improvements. I'll first attempt adding I-P in the "flow" > > engine node before attempting some scale tests. Neverthless > > this patch series is good enough for reviews. > > > > > > v1 -> v2 > > -------- > > * Resolved the conflicts and rebased > > * Fixed a bug in p6. > > > > Numan Siddique (8): > > northd I-P: Sync SB load balancers in a separate engine node. > > northd: Add a new engine node - northd_lb_data. > > northd: Add initial I-P for load balancer and load balancer groups > > northd: Refactor the 'northd' node code which handles logical switch > > changes. > > northd: Handle load balancer changes for a logical switch. > > northd: Handle load balancer group changes for a logical switch. > > northd: Sync SB Port bindings NAT column in a separate engine node. > > northd: Handle load balancer changes for a logical router. > > > > lib/lb.c | 356 ++++++- > > lib/lb.h | 113 ++- > > northd/automake.mk | 2 + > > northd/en-lflow.c | 8 +- > > northd/en-northd-lb-data.c | 308 ++++++ > > northd/en-northd-lb-data.h | 56 ++ > > northd/en-northd.c | 57 +- > > northd/en-northd.h | 2 + > > northd/en-sync-sb.c | 60 ++ > > northd/en-sync-sb.h | 9 + > > northd/inc-proc-northd.c | 27 +- > > northd/northd.c | 1858 +++++++++++++++++++++++++----------- > > northd/northd.h | 44 +- > > tests/ovn-northd.at | 131 +++ > > 14 files changed, 2354 insertions(+), 677 deletions(-) > > create mode 100644 northd/en-northd-lb-data.c > > create mode 100644 northd/en-northd-lb-data.h > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Jul 11, 2023 at 10:41 PM Numan Siddique <numans@ovn.org> wrote: > > On Sat, Jul 8, 2023 at 1:27 AM Mark Michelson <mmichels@redhat.com> wrote: > > > > Hi Numan, > > > > I gave the series a look. I've looked at the code but haven't yet run > > any tests with it. The main reason for this is that the series does not > > apply cleanly to OVN main. > > > > Overall, I only have small notes. I've replied to patches 2 and 3 with > > the specifics. > > Thanks for the review Mark. I'll address those comments and any other in v3. > I'll wait for a bit before submitting v3 if you or others have more comments. > Thanks Numan. I spent some time understanding the overall dependencies and the strategy, and I completed reviewing for the first 3 patches. Mostly look good except some nits. But I am still working on the rest of the patches. Sorry for my slowness! I am also curious if you have done any scale tests and if there is any performance data. Or is it going to be more meaningful when I-P in the lflow node is implemented for LB changes (to achieve end-to-end I-P for LB)? > I did rebase before submitting. Maybe some mistake from my side. > But looks like ovsrobot was able to successfully apply the patches and > run the tests. > I didn't have any problem applying patches to main. It was clean. Thanks, Han > If you want to continue reviewing can you please pull it from here > - https://github.com/numansiddique/ovn/tree/northd_ip_refactor_lb_ip_v2_p8 > or > - https://github.com/ovsrobot/ovn/tree/series_362800 > > Thanks > Numan > > > > > > > On 7/7/23 01:51, numans@ovn.org wrote: > > > From: Numan Siddique <numans@ovn.org> > > > > > > This patch series adds the support to handle load balancer and > > > load balancer group changes incrementally in the "northd" engine > > > node. "flow" engine node doesn't support I-P yet and falls back > > > to full recompute. > > > > > > Note: I still need to do some scale testing and measure the > > > improvements. I'll first attempt adding I-P in the "flow" > > > engine node before attempting some scale tests. Neverthless > > > this patch series is good enough for reviews. > > > > > > > > > v1 -> v2 > > > -------- > > > * Resolved the conflicts and rebased > > > * Fixed a bug in p6. > > > > > > Numan Siddique (8): > > > northd I-P: Sync SB load balancers in a separate engine node. > > > northd: Add a new engine node - northd_lb_data. > > > northd: Add initial I-P for load balancer and load balancer groups > > > northd: Refactor the 'northd' node code which handles logical switch > > > changes. > > > northd: Handle load balancer changes for a logical switch. > > > northd: Handle load balancer group changes for a logical switch. > > > northd: Sync SB Port bindings NAT column in a separate engine node. > > > northd: Handle load balancer changes for a logical router. > > > > > > lib/lb.c | 356 ++++++- > > > lib/lb.h | 113 ++- > > > northd/automake.mk | 2 + > > > northd/en-lflow.c | 8 +- > > > northd/en-northd-lb-data.c | 308 ++++++ > > > northd/en-northd-lb-data.h | 56 ++ > > > northd/en-northd.c | 57 +- > > > northd/en-northd.h | 2 + > > > northd/en-sync-sb.c | 60 ++ > > > northd/en-sync-sb.h | 9 + > > > northd/inc-proc-northd.c | 27 +- > > > northd/northd.c | 1858 +++++++++++++++++++++++++----------- > > > northd/northd.h | 44 +- > > > tests/ovn-northd.at | 131 +++ > > > 14 files changed, 2354 insertions(+), 677 deletions(-) > > > create mode 100644 northd/en-northd-lb-data.c > > > create mode 100644 northd/en-northd-lb-data.h > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
From: Numan Siddique <numans@ovn.org> This patch series adds the support to handle load balancer and load balancer group changes incrementally in the "northd" engine node. "flow" engine node doesn't support I-P yet and falls back to full recompute. Note: I still need to do some scale testing and measure the improvements. I'll first attempt adding I-P in the "flow" engine node before attempting some scale tests. Neverthless this patch series is good enough for reviews. v1 -> v2 -------- * Resolved the conflicts and rebased * Fixed a bug in p6. Numan Siddique (8): northd I-P: Sync SB load balancers in a separate engine node. northd: Add a new engine node - northd_lb_data. northd: Add initial I-P for load balancer and load balancer groups northd: Refactor the 'northd' node code which handles logical switch changes. northd: Handle load balancer changes for a logical switch. northd: Handle load balancer group changes for a logical switch. northd: Sync SB Port bindings NAT column in a separate engine node. northd: Handle load balancer changes for a logical router. lib/lb.c | 356 ++++++- lib/lb.h | 113 ++- northd/automake.mk | 2 + northd/en-lflow.c | 8 +- northd/en-northd-lb-data.c | 308 ++++++ northd/en-northd-lb-data.h | 56 ++ northd/en-northd.c | 57 +- northd/en-northd.h | 2 + northd/en-sync-sb.c | 60 ++ northd/en-sync-sb.h | 9 + northd/inc-proc-northd.c | 27 +- northd/northd.c | 1858 +++++++++++++++++++++++++----------- northd/northd.h | 44 +- tests/ovn-northd.at | 131 +++ 14 files changed, 2354 insertions(+), 677 deletions(-) create mode 100644 northd/en-northd-lb-data.c create mode 100644 northd/en-northd-lb-data.h