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. 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 > > -- Regards, Daniel Ding _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
