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.
>
> 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
