On 12/5/23 13:58, Daniel Ding wrote:
>
>
> On Tue, 5 Dec 2023 at 18:58, Dumitru Ceara <[email protected]
> <mailto:[email protected]>> 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 <[email protected]
> <mailto:[email protected]>>
> > Acked-by: Dumitru Ceara <[email protected] <mailto:[email protected]>>
>
> 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.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
