On 4/25/23 16:35, Mark Michelson wrote: > On 4/18/23 10:32, Dumitru Ceara wrote: >> On 4/14/23 15:10, Mark Michelson wrote: >>> Current code always skips conntrack for traffic that ingresses or >>> egresses on a localnet port. However, this makes it impossible for >>> traffic to be load-balanced when it arrives on a localnet port. >>> >>> This patch allows for traffic to be load balanced on localnet ports by >>> making two changes: >>> * Localnet ports now have a conntrack zone assigned. >>> * When a load balancer is configured on a logical switch containing a >>> localnet port, then conntack is no longer skipped for traffic >> >> typo: 'conntack' >> >>> involving the localnet port. >> >> Will this change behavior when both ACLs and LBs are applied to a switch >> with a localnet port? > > The biggest change is that localnet traffic will be sent to conntrack in > the PRE_STATEFUL ingress/egress stages, with the intent of defragmenting > the traffic. This also will happen if LBs are applied but *no* ACLs are > defined on the logical switch. >
But what about the case when "allow-related" ACLs are applied on the logical switch but no LB is? Shouldn't we send traffic to conntrack in that case too? > As far as behavior changes are concerned, this will result in more > conntrack entries being created, but it should not affect how traffic is > handled when localnet ports are involved. > I'm assuming this might also (to some extent) affect latency in the datapath due to the additional conntrack. But again, that's probably fine if users configured load balancers on that switch. >> >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2164652 >>> >>> Co-authored-by: Dumitru Ceara <dce...@redhat.com> >>> Signed-off-by: Mark Michelson <mmich...@redhat.com> >>> --- >>> This patch is based on an inlined patch provided by Dumitru Ceara on the >>> referenced bugzilla issue. I modified it to only skip conntrack when a >>> load balancer is present on the logical switch, and I added a simple >>> test case to verify the logical flows are populated as expected. >>> >>> Dumitru, if you want more than the "Co-authored-by" citation on this >>> issue, please let me know and I'll adjust it. >> >> I'm ok as co-author and, to keep the robot happy, if we decide to apply >> this to main: >> >> Signed-off-by: Dumitru Ceara <dce...@redhat.com> >> >>> --- >>> controller/ovn-controller.c | 13 ++++---- >>> northd/northd.c | 25 +++++++++++----- >>> tests/ovn-northd.at | 60 +++++++++++++++++++++++++++++++++++++ >> >> Would it be possible to add a system test too? > > Can do! > Thanks! >> >> Thanks, >> Dumitru >> >>> 3 files changed, 84 insertions(+), 14 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index e170e9262..3a89e386f 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -723,7 +723,7 @@ get_snat_ct_zone(const struct >>> sbrec_datapath_binding *dp) >>> } >>> static void >>> -update_ct_zones(const struct shash *binding_lports, >>> +update_ct_zones(const struct sset *local_lports, >>> const struct hmap *local_datapaths, >>> struct simap *ct_zones, unsigned long *ct_zone_bitmap, >>> struct shash *pending_ct_zones) >>> @@ -736,9 +736,9 @@ update_ct_zones(const struct shash *binding_lports, >>> unsigned long unreq_snat_zones_map[BITMAP_N_LONGS(MAX_CT_ZONES)]; >>> struct simap unreq_snat_zones = >>> SIMAP_INITIALIZER(&unreq_snat_zones); >>> - struct shash_node *shash_node; >>> - SHASH_FOR_EACH (shash_node, binding_lports) { >>> - sset_add(&all_users, shash_node->name); >>> + const char *local_lport; >>> + SSET_FOR_EACH (local_lport, local_lports) { >>> + sset_add(&all_users, local_lport); >>> } >>> /* Local patched datapath (gateway routers) need zones >>> assigned. */ >>> @@ -2392,7 +2392,7 @@ en_ct_zones_run(struct engine_node *node, void >>> *data) >>> EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); >>> restore_ct_zones(bridge_table, ovs_table, ct_zones_data); >>> - update_ct_zones(&rt_data->lbinding_data.lports, >>> &rt_data->local_datapaths, >>> + update_ct_zones(&rt_data->local_lports, &rt_data->local_datapaths, >>> &ct_zones_data->current, ct_zones_data->bitmap, >>> &ct_zones_data->pending); >>> @@ -2482,7 +2482,8 @@ ct_zones_runtime_data_handler(struct >>> engine_node *node, void *data) >>> SHASH_FOR_EACH (shash_node, &tdp->lports) { >>> struct tracked_lport *t_lport = shash_node->data; >>> if (strcmp(t_lport->pb->type, "") >>> - && strcmp(t_lport->pb->type, "localport")) { >>> + && strcmp(t_lport->pb->type, "localport") >>> + && strcmp(t_lport->pb->type, "localnet")) { >>> /* We allocate zone-id's only to VIF and localport >>> lports. */ >>> continue; >>> } >>> diff --git a/northd/northd.c b/northd/northd.c >>> index da3c8cf77..8439446aa 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -5959,10 +5959,12 @@ build_pre_acls(struct ovn_datapath *od, const >>> struct hmap *port_groups, >>> S_SWITCH_IN_PRE_ACL, >>> S_SWITCH_OUT_PRE_ACL, >>> 110, lflows); >>> } >>> - for (size_t i = 0; i < od->n_localnet_ports; i++) { >>> - skip_port_from_conntrack(od, od->localnet_ports[i], >>> - S_SWITCH_IN_PRE_ACL, >>> S_SWITCH_OUT_PRE_ACL, >>> - 110, lflows); >>> + if (!od->has_lb_vip) { >>> + for (size_t i = 0; i < od->n_localnet_ports; i++) { >>> + skip_port_from_conntrack(od, od->localnet_ports[i], >>> + S_SWITCH_IN_PRE_ACL, >>> S_SWITCH_OUT_PRE_ACL, >>> + 110, lflows); >>> + } >>> } >>> /* stateless filters always take precedence over stateful >>> ACLs. */ >>> @@ -6130,10 +6132,17 @@ build_pre_lb(struct ovn_datapath *od, const >>> struct shash *meter_groups, >>> S_SWITCH_IN_PRE_LB, >>> S_SWITCH_OUT_PRE_LB, >>> 110, lflows); >>> } >>> - for (size_t i = 0; i < od->n_localnet_ports; i++) { >>> - skip_port_from_conntrack(od, od->localnet_ports[i], >>> - S_SWITCH_IN_PRE_LB, >>> S_SWITCH_OUT_PRE_LB, >>> - 110, lflows); >>> + /* Localnet ports have no need for going through conntrack, unless >>> + * the logical switch has a load balancer. Then, conntrack is >>> necessary >>> + * so that traffic arriving via the localnet port can be load >>> + * balanced. >>> + */ >>> + if (!od->has_lb_vip) { >>> + for (size_t i = 0; i < od->n_localnet_ports; i++) { >>> + skip_port_from_conntrack(od, od->localnet_ports[i], >>> + S_SWITCH_IN_PRE_LB, >>> S_SWITCH_OUT_PRE_LB, >>> + 110, lflows); >>> + } >>> } >>> /* Do not sent statless flows via conntrack */ >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 9fee00094..909b06719 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -8830,3 +8830,63 @@ mac_binding_timestamp: true >>> AT_CLEANUP >>> ]) >>> + >>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>> +AT_SETUP([Localnet ports on LS with LB]) >>> +ovn_start >>> +# In the past, traffic arriving on localnet ports has skipped >>> conntrack. >>> +# This test ensures that we still skip conntrack for localnet ports, >>> +# *except* for the case where the logical switch has a load balancer >>> +# configured. In this case, the localnet port will not skip conntrack, >>> +# allowing for traffic to be load balanced on the localnet port. >>> + >>> +check ovn-nbctl ls-add sw >>> +check ovn-nbctl lsp-add sw sw-ln >>> +check ovn-nbctl lsp-set-type sw-ln localnet >>> +check ovn-nbctl lsp-set-addresses sw-ln unknown >>> +check ovn-nbctl --wait=sb sync >>> + >>> +# Since this test is only concerned with logical flows, we don't >>> need to >>> +# configure anything else that we normally would with regards to >>> localnet >>> +# ports >>> + >>> + >>> +# First, ensure that conntrack is skipped for the localnet port >>> since there >>> +# isn't a load balancer configured. >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> + table=??(ls_in_pre_lb ), priority=110 , match=(ip && inport >>> == "sw-ln"), action=(next;) >>> +]) >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> + table=??(ls_out_pre_lb ), priority=110 , match=(ip && >>> outport == "sw-ln"), action=(ct_clear; next;) >>> +]) >>> + >>> +# Now add a load balancer and ensure that we no longer are skipping >>> conntrack >>> +# for the localnet port >>> + >>> +check ovn-nbctl lb-add lb 10.0.0.1:80 10.0.0.100:8080 tcp >>> +check ovn-nbctl ls-lb-add sw lb >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> +]) >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> +]) >>> + >>> +# And ensure that removing the load balancer from the switch results >>> in skipping >>> +# conntrack again >>> +check ovn-nbctl ls-lb-del sw lb >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> + table=??(ls_in_pre_lb ), priority=110 , match=(ip && inport >>> == "sw-ln"), action=(next;) >>> +]) >>> + >>> +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_pre_lb | grep >>> priority=110 | grep sw-ln | sed 's/table=../table=??/'], [0], [dnl >>> + table=??(ls_out_pre_lb ), priority=110 , match=(ip && >>> outport == "sw-ln"), action=(ct_clear; next;) >>> +]) >>> + >>> +AT_CLEANUP >>> +]) >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
