On 12/8/23 14:51, Daniel Ding wrote: > On Thu, 7 Dec 2023 at 21:26, Dumitru Ceara <dce...@redhat.com> wrote: > >> On 12/6/23 02:56, Daniel Ding wrote: >>> Hi Dumitru! >>> >>> On Tue, 5 Dec 2023 at 23:59, Dumitru Ceara <dce...@redhat.com> wrote: >>> >>>> On 12/5/23 13:58, Daniel Ding wrote: >>>>> >>>>> >>>>> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara <dce...@redhat.com >>>>> <mailto:dce...@redhat.com>> wrote: >>>>> >>>>> Hi Daniel, >>>>> >>>>> Thanks for this new revision but why is it v3? I don't think I saw >>>> v2 >>>>> posted anywhere but maybe I missed it. >>>>> >>>>> >>>>> Sorry, that is my mistake. >>>>> >>>> >>>> No problem. >>>> >>>>> >>>>> On 12/5/23 08:33, Daniel Ding wrote: >>>>> > If the router has a snat rule and it's external ip isn't lrp >>>> address, >>>>> > when the arp request from other router for this external ip, will >>>>> > be drop, because of this external ip use same mac address as lrp, >>>> so >>>>> > can not forward to MC_FLOOD. >>>>> > >>>>> > Fixes: 32f5ebb06226 ("ovn-northd: Limit ARP/ND broadcast domain >>>>> whenever possible.") >>>>> > Reported-at: https://github.com/ovn-org/ovn/issues/209 >>>>> <https://github.com/ovn-org/ovn/issues/209> >>>>> > >>>>> > Signed-off-by: Daniel Ding <danieldin...@gmail.com >>>>> <mailto:danieldin...@gmail.com>> >>>>> > Acked-by: Dumitru Ceara <dce...@redhat.com <mailto: >>>> dce...@redhat.com>> >>>>> >>>>> Please don't add an "Acked-by: ... " if the person never explicitly >>>>> replied with "Acked-by: ... " on the previous version of the patch >> or >>>>> if you didn't have explicit agreement from that person to do so. >>>>> >>>>> Quoting from my previous reply to your v1, I said: >>>>> >>>>> "I think it makes sense to do what you're suggesting." >>>>> >>>>> >>>> >> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934 >>>> < >>>> >> https://patchwork.ozlabs.org/project/ovn/patch/callnitdhfcetzapkhjtddyx1cx_2cys2otqscrhpv6jse0m...@mail.gmail.com/#3214934 >>>>> >>>>> >>>>> That doesn't mean I acked the patch. >>>>> >>>>> >>>>> Got it. Thx! >>>>> >>>> >>>> No worries. >>>> >>>>> >>>>> > --- >>>>> > northd/northd.c | 18 +++++++++++++++++- >>>>> > tests/ovn-northd.at <http://ovn-northd.at> | 12 ++++++++++++ >>>>> > 2 files changed, 29 insertions(+), 1 deletion(-) >>>>> > >>>>> > diff --git a/northd/northd.c b/northd/northd.c >>>>> > index e9cb906e2..99fb30f46 100644 >>>>> > --- a/northd/northd.c >>>>> > +++ b/northd/northd.c >>>>> > @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct >>>>> ovn_port *op, >>>>> > } >>>>> > } >>>>> > >>>>> > + struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4); >>>>> > + struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6); >>>>> > + >>>>> > for (size_t i = 0; i < op->od->nbr->n_nat; i++) { >>>>> > struct ovn_nat *nat_entry = &op->od->nat_entries[i]; >>>>> > const struct nbrec_nat *nat = nat_entry->nb; >>>>> > @@ -8983,7 +8986,17 @@ build_lswitch_rport_arp_req_flows(struct >>>>> ovn_port *op, >>>>> > } >>>>> > >>>>> > if (!strcmp(nat->type, "snat")) { >>>>> > - continue; >>>>> > + if (nat_entry_is_v6(nat_entry)) { >>>>> > + if (sset_contains(&snat_ips_v6, >>>> nat->external_ip)) { >>>>> > + continue; >>>>> > + } >>>>> > + sset_add(&snat_ips_v6, nat->external_ip); >>>>> > + } else { >>>>> > + if (sset_contains(&snat_ips_v4, >>>> nat->external_ip)) { >>>>> > + continue; >>>>> > + } >>>>> > + sset_add(&snat_ips_v4, nat->external_ip); >>>>> > + } >>>>> >>>>> Essentially this just makes sure we don't skip NAT entries and that >>>> we >>>>> consider unique external_ips. I'm fine with relaxing the second >>>> part of >>>>> the condition in which case, as mentioned on v1, I think we can >> just >>>>> remove the whole "if (!strcmp(nat->type, "snat")) {" block. >>>>> >>>>> With the following incremental change applied (it removes the >> block) >>>>> your test still passes: >>>>> >>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>> index df7d2d60a5..20efd3b74c 100644 >>>>> --- a/northd/northd.c >>>>> +++ b/northd/northd.c >>>>> @@ -9372,9 +9372,6 @@ build_lswitch_rport_arp_req_flows(struct >>>>> ovn_port *op, >>>>> } >>>>> } >>>>> >>>>> - struct sset snat_ips_v4 = SSET_INITIALIZER(&snat_ips_v4); >>>>> - struct sset snat_ips_v6 = SSET_INITIALIZER(&snat_ips_v6); >>>>> - >>>>> for (size_t i = 0; i < op->od->nbr->n_nat; i++) { >>>>> struct ovn_nat *nat_entry = &op->od->nat_entries[i]; >>>>> const struct nbrec_nat *nat = nat_entry->nb; >>>>> @@ -9383,20 +9380,6 @@ build_lswitch_rport_arp_req_flows(struct >>>>> ovn_port *op, >>>>> continue; >>>>> } >>>>> >>>>> - if (!strcmp(nat->type, "snat")) { >>>>> - if (nat_entry_is_v6(nat_entry)) { >>>>> - if (sset_contains(&snat_ips_v6, >> nat->external_ip)) { >>>>> - continue; >>>>> - } >>>>> - sset_add(&snat_ips_v6, nat->external_ip); >>>>> - } else { >>>>> - if (sset_contains(&snat_ips_v4, >> nat->external_ip)) { >>>>> - continue; >>>>> - } >>>>> - sset_add(&snat_ips_v4, nat->external_ip); >>>>> - } >>>>> - } >>>>> - >>>>> /* Check if the ovn port has a network configured on which >>>>> we could >>>>> * expect ARP requests/NS for the DNAT external_ip. >>>>> */ >>>>> @@ -9436,9 +9419,6 @@ build_lswitch_rport_arp_req_flows(struct >>>>> ovn_port *op, >>>>> if (sw_od->n_router_ports != sw_od->nbs->n_ports) { >>>>> build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, >>>>> lflows); >>>>> } >>>>> - >>>>> - sset_destroy(&snat_ips_v4); >>>>> - sset_destroy(&snat_ips_v6); >>>>> } >>>>> >>>>> >>>>> If the nat_entries has multiple snats with the same external_ip, I >> think >>>>> it should check exernal_ip whether in a string hash. In addition, the >>>>> snat_and_dnat entry is working normally, so exclude it. >>>>> >>>> >>>> I'm not sure I understand, we weren't skipping dnat_and_snat before and >>>> we wouldn't be skipping it with this either. Regarding the duplicates, >>>> it really depends how common it is to have duplicates, I guess. >>>> >>>> >>> If I understand correctly, we just remove "if (!strcmp(nat->type, >> "snat")) >>> {" block. And keep external checking because we are not sure about too >> many >>> snats in a lr. >> >> If we're worried about duplicate SNAT IPs, we should probably just use >> the precomputed op->od->snat_ips shash. It already consists only of >> unique external IPs used for SNAT on the router. >> >> So no need for the additional ssets you're using. We can just walk the >> elements of the "nat_ips" shash directly. >> > > Hi, Dumitru! I'm using od->snat_ips shash to ensure unique external IPs for > SNAT. And please help to review again. Thank you! > > diff --git a/northd/northd.c b/northd/northd.c > index 65328434a..08b09da02 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7170,6 +7170,37 @@ build_lswitch_rport_arp_req_flows(struct ovn_port > *op, > } > } > > + struct shash_node *snat_snode; > + SHASH_FOR_EACH (snat_snode, &op->od->snat_ips) { > + struct ovn_snat_ip *snat_ip = snat_snode->data; > + > + if (ovs_list_is_empty(&snat_ip->snat_entries)) { > + continue; > + } > + > + struct ovn_nat *nat_entry = > + CONTAINER_OF(ovs_list_front(&snat_ip->snat_entries), > + struct ovn_nat, ext_addr_list_node); > + const struct nbrec_nat *nat = nat_entry->nb; > + > + /* Check if the ovn port has a network configured on which we could > + * expect ARP requests/NS for the DNAT external_ip. > + */ > + if (nat_entry_is_v6(nat_entry)) { > + if (!sset_contains(&op->od->lb_ips_v6, nat->external_ip)) { > + build_lswitch_rport_arp_req_flow( > + nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows, > + stage_hint); > + } > + } else { > + if (!sset_contains(&op->od->lb_ips_v4, nat->external_ip)) { > + build_lswitch_rport_arp_req_flow( > + nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows, > + stage_hint); > + } > + } > + } > +
I think this looks good. Do you mind posting it as v4 please? Like that it will also trigger 0-day bot to push it to its fork and run CI. Thanks, Dumitru > for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > build_lswitch_rport_arp_req_flow( > op->lrp_networks.ipv4_addrs[i].addr_s, AF_INET, sw_op, sw_od, > 80, > > >> >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index e9cb906e2..d93870e25 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -8974,6 +8974,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >> *op, >>> } >>> } >>> >>> + struct sset nat_ips_v4 = SSET_INITIALIZER(&nat_ips_v4); >>> + struct sset nat_ips_v6 = SSET_INITIALIZER(&nat_ips_v6); >>> + >>> for (size_t i = 0; i < op->od->nbr->n_nat; i++) { >>> struct ovn_nat *nat_entry = &op->od->nat_entries[i]; >>> const struct nbrec_nat *nat = nat_entry->nb; >>> @@ -8982,8 +8985,16 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >>> *op, >>> continue; >>> } >>> >>> - if (!strcmp(nat->type, "snat")) { >>> - continue; >>> + if (nat_entry_is_v6(nat_entry)) { >>> + if (sset_contains(&nat_ips_v6, nat->external_ip)) { >>> + continue; >>> + } >>> + sset_add(&nat_ips_v6, nat->external_ip); >>> + } else { >>> + if (sset_contains(&nat_ips_v4, nat->external_ip)) { >>> + continue; >>> + } >>> + sset_add(&nat_ips_v4, nat->external_ip); >>> } >>> >>> /* Check if the ovn port has a network configured on which we >> could >>> @@ -9025,6 +9036,9 @@ build_lswitch_rport_arp_req_flows(struct ovn_port >> *op, >>> if (sw_od->n_router_ports != sw_od->nbs->n_ports) { >>> build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, >> lflows); >>> } >>> + >>> + sset_destroy(&nat_ips_v4); >>> + sset_destroy(&nat_ips_v6); >>> } >>> >> >> Regards, >> Dumitru >> >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev