Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-26 Thread Numan Siddique 
On Tue, May 26, 2020 at 9:16 PM Dumitru Ceara  wrote:

> On 5/26/20 5:39 PM, Lorenzo Bianconi wrote:
> >> On 5/26/20 5:23 AM, Han Zhou wrote:
> >>>
> >>>
> >>> On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi
> >>> mailto:lorenzo.bianc...@redhat.com>>
> wrote:
> 
>  Introduce 150-priority logical flows for each NAT rule that can
>  be handled in a distributed manner to manage ARP request locally
>  for FIP traffic. In particular set reg1 and eth.src to NAT external
>  ip and NAT external mac respectivelly and do not distribute ARP
>  traffic using FIP
> 
>  Signed-off-by: Lorenzo Bianconi  >>> >
>  ---
>   northd/ovn-northd.8.xml | 12 
>   northd/ovn-northd.c | 18 +-
>   tests/ovn.at | 28
> +---
>   tests/system-ovn.at  | 28
> >>> 
>   4 files changed, 82 insertions(+), 4 deletions(-)
> 
>  diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>  index 9423cbfd1..5e37d9758 100644
>  --- a/northd/ovn-northd.8.xml
>  +++ b/northd/ovn-northd.8.xml
>  @@ -2875,6 +2875,18 @@ icmp4 {
>   
> 
>   
>  +  
>  +For each NAT rule in the OVN Northbound database that can
>  +be handled in a distributed manner, a priorirty-150 logical
>  +flow with match ip.src == A 
>  +is_chassis_resident(B), where A
>  +is NAT logical ip and B is NAT logical port.
>  +IP traffic matching the above rule will be managed locally
>  +setting reg1 to C and
> >>> eth.src
>  +to D, where C is NAT external ip and
>  +D is NAT external mac.
>  +  
>  +
> 
>   For each NAT rule in the OVN Northbound database that can
>   be handled in a distributed manner, a priority-100 logical
>  diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>  index 7ae5e45da..20f0ee2e4 100644
>  --- a/northd/ovn-northd.c
>  +++ b/northd/ovn-northd.c
>  @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths,
> >>> struct hmap *ports,
>   /* Ingress Gateway Redirect Table: For NAT on a
> distributed
>    * router, add flows that are specific to a NAT rule.
> These
>    * flows indicate the presence of an applicable NAT rule
> that
>  - * can be applied in a distributed manner. */
>  + * can be applied in a distributed manner.
>  + * Moreover add logical flows to avoid ARP distributing
> >>> when the
>  + * chassis has a direct connection to the underlay
> >>> network through
>  + * a localnet port
> >>>
> >>> I think the two flows may be combined, right? Firstly, the match
> >>> "outport == DGW" is missing from the new flow. Secondly, the
> >>> "is_chassis_resident()" seems to be missing from the original flow. If
> >>> this understanding is correct, the both flows just have same match
> >>> condition, so the actions should be combined as well.
> >>>
> >>
> >> Hi Lorenzo, Han,
> >>
> >> I agree that both flows should have the same match and that actions
> >> should be combined.
> >>
> >> I'm not sure however if we can ever hit these flows if
> >> is_chassis_resident(logical_port) == false. If I understand correctly we
> >> can't, but just to be safe it's probably fine to add the
> >> is_chassis_resident(logical_port) check.
> >>
> >>> In addition, I'd suggest to add more clear comment to state the reason
> >>> why setting reg1 and eth.src. For example: it is done to make sure when
> >>> ARP request is triggered (in next stage), the ARP generated can have
> the
> >>> eth.src = xxx and src IP as yyy, so that it can bypass the flow that
> >>> redirect the ARP request to GW node ...
> >>>
> >>> Thanks,
> >>> Han
> >>>
>  + */
>   if (distributed) {
>  +ds_clear();
>  +ds_clear();
>  +ds_put_format(,
>  +  "ip%s.src == %s &&
> >>> is_chassis_resident(\"%s\")",
>  +  is_v6 ? "6" : "4", nat->logical_ip,
>  +  nat->logical_port);
>  +ds_put_format(, "eth.src = %s; %sreg1 = %s;
> >>> next;",
>  +  nat->external_mac, is_v6 ? "xx" : "",
>  +  nat->external_ip);
>  +ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT,
> 150,
>  +  ds_cstr(), ds_cstr());
>  +
>   ds_clear();
>   ds_put_format(, "ip%s.src == %s && outport ==
> %s",
> is_v6 ? "6" : "4",
>  diff --git a/tests/ovn.at

Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-26 Thread Dumitru Ceara 
On 5/26/20 5:39 PM, Lorenzo Bianconi wrote:
>> On 5/26/20 5:23 AM, Han Zhou wrote:
>>>
>>>
>>> On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi
>>> mailto:lorenzo.bianc...@redhat.com>> wrote:

 Introduce 150-priority logical flows for each NAT rule that can
 be handled in a distributed manner to manage ARP request locally
 for FIP traffic. In particular set reg1 and eth.src to NAT external
 ip and NAT external mac respectivelly and do not distribute ARP
 traffic using FIP

 Signed-off-by: Lorenzo Bianconi >> >
 ---
  northd/ovn-northd.8.xml | 12 
  northd/ovn-northd.c     | 18 +-
  tests/ovn.at             | 28 
 +---
  tests/system-ovn.at      | 28
>>> 
  4 files changed, 82 insertions(+), 4 deletions(-)

 diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
 index 9423cbfd1..5e37d9758 100644
 --- a/northd/ovn-northd.8.xml
 +++ b/northd/ovn-northd.8.xml
 @@ -2875,6 +2875,18 @@ icmp4 {
      

      
 +      
 +        For each NAT rule in the OVN Northbound database that can
 +        be handled in a distributed manner, a priorirty-150 logical
 +        flow with match ip.src == A 
 +        is_chassis_resident(B), where A
 +        is NAT logical ip and B is NAT logical port.
 +        IP traffic matching the above rule will be managed locally
 +        setting reg1 to C and
>>> eth.src
 +        to D, where C is NAT external ip and
 +        D is NAT external mac.
 +      
 +
        
          For each NAT rule in the OVN Northbound database that can
          be handled in a distributed manner, a priority-100 logical
 diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
 index 7ae5e45da..20f0ee2e4 100644
 --- a/northd/ovn-northd.c
 +++ b/northd/ovn-northd.c
 @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths,
>>> struct hmap *ports,
              /* Ingress Gateway Redirect Table: For NAT on a 
 distributed
               * router, add flows that are specific to a NAT rule. 
  These
               * flows indicate the presence of an applicable NAT 
 rule that
 -             * can be applied in a distributed manner. */
 +             * can be applied in a distributed manner.
 +             * Moreover add logical flows to avoid ARP distributing
>>> when the
 +             * chassis has a direct connection to the underlay
>>> network through
 +             * a localnet port
>>>
>>> I think the two flows may be combined, right? Firstly, the match
>>> "outport == DGW" is missing from the new flow. Secondly, the
>>> "is_chassis_resident()" seems to be missing from the original flow. If
>>> this understanding is correct, the both flows just have same match
>>> condition, so the actions should be combined as well.
>>>
>>
>> Hi Lorenzo, Han,
>>
>> I agree that both flows should have the same match and that actions
>> should be combined.
>>
>> I'm not sure however if we can ever hit these flows if
>> is_chassis_resident(logical_port) == false. If I understand correctly we
>> can't, but just to be safe it's probably fine to add the
>> is_chassis_resident(logical_port) check.
>>
>>> In addition, I'd suggest to add more clear comment to state the reason
>>> why setting reg1 and eth.src. For example: it is done to make sure when
>>> ARP request is triggered (in next stage), the ARP generated can have the
>>> eth.src = xxx and src IP as yyy, so that it can bypass the flow that
>>> redirect the ARP request to GW node ...
>>>
>>> Thanks,
>>> Han
>>>
 +             */
              if (distributed) {
 +                ds_clear();
 +                ds_clear();
 +                ds_put_format(,
 +                              "ip%s.src == %s &&
>>> is_chassis_resident(\"%s\")",
 +                              is_v6 ? "6" : "4", 
 nat->logical_ip,
 +                              nat->logical_port);
 +                ds_put_format(, "eth.src = %s; %sreg1 = 
 %s;
>>> next;",
 +                              nat->external_mac, is_v6 ? 
 "xx" : "",
 +                              nat->external_ip);
 +                ovn_lflow_add(lflows, od, 
 S_ROUTER_IN_GW_REDIRECT, 150,
 +                              ds_cstr(), 
 ds_cstr());
 +
                  ds_clear();
                  ds_put_format(, "ip%s.src == %s && outport 
 == %s",
                  

Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-26 Thread Lorenzo Bianconi 
> On 5/26/20 5:23 AM, Han Zhou wrote:
> > 
> > 
> > On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi
> > mailto:lorenzo.bianc...@redhat.com>> wrote:
> >>
> >> Introduce 150-priority logical flows for each NAT rule that can
> >> be handled in a distributed manner to manage ARP request locally
> >> for FIP traffic. In particular set reg1 and eth.src to NAT external
> >> ip and NAT external mac respectivelly and do not distribute ARP
> >> traffic using FIP
> >>
> >> Signed-off-by: Lorenzo Bianconi  > >
> >> ---
> >>  northd/ovn-northd.8.xml | 12 
> >>  northd/ovn-northd.c     | 18 +-
> >>  tests/ovn.at             | 28 +---
> >>  tests/system-ovn.at      | 28
> > 
> >>  4 files changed, 82 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> >> index 9423cbfd1..5e37d9758 100644
> >> --- a/northd/ovn-northd.8.xml
> >> +++ b/northd/ovn-northd.8.xml
> >> @@ -2875,6 +2875,18 @@ icmp4 {
> >>      
> >>
> >>      
> >> +      
> >> +        For each NAT rule in the OVN Northbound database that can
> >> +        be handled in a distributed manner, a priorirty-150 logical
> >> +        flow with match ip.src == A 
> >> +        is_chassis_resident(B), where A
> >> +        is NAT logical ip and B is NAT logical port.
> >> +        IP traffic matching the above rule will be managed locally
> >> +        setting reg1 to C and
> > eth.src
> >> +        to D, where C is NAT external ip and
> >> +        D is NAT external mac.
> >> +      
> >> +
> >>        
> >>          For each NAT rule in the OVN Northbound database that can
> >>          be handled in a distributed manner, a priority-100 logical
> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >> index 7ae5e45da..20f0ee2e4 100644
> >> --- a/northd/ovn-northd.c
> >> +++ b/northd/ovn-northd.c
> >> @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths,
> > struct hmap *ports,
> >>              /* Ingress Gateway Redirect Table: For NAT on a distributed
> >>               * router, add flows that are specific to a NAT rule.  These
> >>               * flows indicate the presence of an applicable NAT rule that
> >> -             * can be applied in a distributed manner. */
> >> +             * can be applied in a distributed manner.
> >> +             * Moreover add logical flows to avoid ARP distributing
> > when the
> >> +             * chassis has a direct connection to the underlay
> > network through
> >> +             * a localnet port
> > 
> > I think the two flows may be combined, right? Firstly, the match
> > "outport == DGW" is missing from the new flow. Secondly, the
> > "is_chassis_resident()" seems to be missing from the original flow. If
> > this understanding is correct, the both flows just have same match
> > condition, so the actions should be combined as well.
> > 
> 
> Hi Lorenzo, Han,
> 
> I agree that both flows should have the same match and that actions
> should be combined.
> 
> I'm not sure however if we can ever hit these flows if
> is_chassis_resident(logical_port) == false. If I understand correctly we
> can't, but just to be safe it's probably fine to add the
> is_chassis_resident(logical_port) check.
> 
> > In addition, I'd suggest to add more clear comment to state the reason
> > why setting reg1 and eth.src. For example: it is done to make sure when
> > ARP request is triggered (in next stage), the ARP generated can have the
> > eth.src = xxx and src IP as yyy, so that it can bypass the flow that
> > redirect the ARP request to GW node ...
> > 
> > Thanks,
> > Han
> > 
> >> +             */
> >>              if (distributed) {
> >> +                ds_clear();
> >> +                ds_clear();
> >> +                ds_put_format(,
> >> +                              "ip%s.src == %s &&
> > is_chassis_resident(\"%s\")",
> >> +                              is_v6 ? "6" : "4", nat->logical_ip,
> >> +                              nat->logical_port);
> >> +                ds_put_format(, "eth.src = %s; %sreg1 = %s;
> > next;",
> >> +                              nat->external_mac, is_v6 ? "xx" : "",
> >> +                              nat->external_ip);
> >> +                ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 150,
> >> +                              ds_cstr(), ds_cstr());
> >> +
> >>                  ds_clear();
> >>                  ds_put_format(, "ip%s.src == %s && outport == %s",
> >>                                is_v6 ? "6" : "4",
> >> diff --git a/tests/ovn.at  b/tests/ovn.at 
> >> index 8fa1a7e1b..15b40ca1e 100644
> >> --- a/tests/ovn.at 
> >> +++ b/tests/ovn.at 
> >> @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
> >>      set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
> >>      

Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-26 Thread Lorenzo Bianconi 
> On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
> >
> > Introduce 150-priority logical flows for each NAT rule that can
> > be handled in a distributed manner to manage ARP request locally
> > for FIP traffic. In particular set reg1 and eth.src to NAT external
> > ip and NAT external mac respectivelly and do not distribute ARP
> > traffic using FIP
> >
> > Signed-off-by: Lorenzo Bianconi 
> > ---
> >  northd/ovn-northd.8.xml | 12 
> >  northd/ovn-northd.c | 18 +-
> >  tests/ovn.at| 28 +---
> >  tests/system-ovn.at | 28 
> >  4 files changed, 82 insertions(+), 4 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 9423cbfd1..5e37d9758 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -2875,6 +2875,18 @@ icmp4 {
> >  
> >
> >  
> > +  
> > +For each NAT rule in the OVN Northbound database that can
> > +be handled in a distributed manner, a priorirty-150 logical
> > +flow with match ip.src == A 
> > +is_chassis_resident(B), where A
> > +is NAT logical ip and B is NAT logical port.
> > +IP traffic matching the above rule will be managed locally
> > +setting reg1 to C and
> eth.src
> > +to D, where C is NAT external ip and
> > +D is NAT external mac.
> > +  
> > +
> >
> >  For each NAT rule in the OVN Northbound database that can
> >  be handled in a distributed manner, a priority-100 logical
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 7ae5e45da..20f0ee2e4 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> >  /* Ingress Gateway Redirect Table: For NAT on a distributed
> >   * router, add flows that are specific to a NAT rule.  These
> >   * flows indicate the presence of an applicable NAT rule that
> > - * can be applied in a distributed manner. */
> > + * can be applied in a distributed manner.
> > + * Moreover add logical flows to avoid ARP distributing when
> the
> > + * chassis has a direct connection to the underlay network
> through
> > + * a localnet port
> 
> I think the two flows may be combined, right? Firstly, the match "outport
> == DGW" is missing from the new flow. Secondly, the "is_chassis_resident()"
> seems to be missing from the original flow. If this understanding is
> correct, the both flows just have same match condition, so the actions
> should be combined as well.
> 
> In addition, I'd suggest to add more clear comment to state the reason why
> setting reg1 and eth.src. For example: it is done to make sure when ARP
> request is triggered (in next stage), the ARP generated can have the
> eth.src = xxx and src IP as yyy, so that it can bypass the flow that
> redirect the ARP request to GW node ...

Hi Han,

ack, thx for the review. I will fix it in v2.

Regards,
Lorenzo

> 
> Thanks,
> Han
> 
> > + */
> >  if (distributed) {
> > +ds_clear();
> > +ds_clear();
> > +ds_put_format(,
> > +  "ip%s.src == %s &&
> is_chassis_resident(\"%s\")",
> > +  is_v6 ? "6" : "4", nat->logical_ip,
> > +  nat->logical_port);
> > +ds_put_format(, "eth.src = %s; %sreg1 = %s;
> next;",
> > +  nat->external_mac, is_v6 ? "xx" : "",
> > +  nat->external_ip);
> > +ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 150,
> > +  ds_cstr(), ds_cstr());
> > +
> >  ds_clear();
> >  ds_put_format(, "ip%s.src == %s && outport == %s",
> >is_v6 ? "6" : "4",
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 8fa1a7e1b..15b40ca1e 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
> >  set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
> >  options:tx_pcap=hv2/vif1-tx.pcap \
> >  options:rxq_pcap=hv2/vif1-rx.pcap \
> > -ofport-request=1
> > +ofport-request=2
> > +ovs-vsctl -- add-port br-int hv2-vif2 -- \
> > +set interface hv2-vif2 external-ids:iface-id=sw0-p1 \
> > +options:tx_pcap=hv2/vif2-tx.pcap \
> > +options:rxq_pcap=hv2/vif2-rx.pcap \
> > +ofport-request=3
> >
> > -ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> > +ovn-nbctl create Logical_Router name=lr0
> >  ovn-nbctl ls-add sw0
> >  ovn-nbctl ls-add sw1
> >
> > @@ -14954,13 +14959,16 @@ ovn-nbctl lsp-add sw0 rp-sw0 -- set
> Logical_Switch_Port rp-sw0 \
> >

Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-26 Thread Dumitru Ceara 
On 5/26/20 5:23 AM, Han Zhou wrote:
> 
> 
> On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi
> mailto:lorenzo.bianc...@redhat.com>> wrote:
>>
>> Introduce 150-priority logical flows for each NAT rule that can
>> be handled in a distributed manner to manage ARP request locally
>> for FIP traffic. In particular set reg1 and eth.src to NAT external
>> ip and NAT external mac respectivelly and do not distribute ARP
>> traffic using FIP
>>
>> Signed-off-by: Lorenzo Bianconi  >
>> ---
>>  northd/ovn-northd.8.xml | 12 
>>  northd/ovn-northd.c     | 18 +-
>>  tests/ovn.at             | 28 +---
>>  tests/system-ovn.at      | 28
> 
>>  4 files changed, 82 insertions(+), 4 deletions(-)
>>
>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>> index 9423cbfd1..5e37d9758 100644
>> --- a/northd/ovn-northd.8.xml
>> +++ b/northd/ovn-northd.8.xml
>> @@ -2875,6 +2875,18 @@ icmp4 {
>>      
>>
>>      
>> +      
>> +        For each NAT rule in the OVN Northbound database that can
>> +        be handled in a distributed manner, a priorirty-150 logical
>> +        flow with match ip.src == A 
>> +        is_chassis_resident(B), where A
>> +        is NAT logical ip and B is NAT logical port.
>> +        IP traffic matching the above rule will be managed locally
>> +        setting reg1 to C and
> eth.src
>> +        to D, where C is NAT external ip and
>> +        D is NAT external mac.
>> +      
>> +
>>        
>>          For each NAT rule in the OVN Northbound database that can
>>          be handled in a distributed manner, a priority-100 logical
>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
>> index 7ae5e45da..20f0ee2e4 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths,
> struct hmap *ports,
>>              /* Ingress Gateway Redirect Table: For NAT on a distributed
>>               * router, add flows that are specific to a NAT rule.  These
>>               * flows indicate the presence of an applicable NAT rule that
>> -             * can be applied in a distributed manner. */
>> +             * can be applied in a distributed manner.
>> +             * Moreover add logical flows to avoid ARP distributing
> when the
>> +             * chassis has a direct connection to the underlay
> network through
>> +             * a localnet port
> 
> I think the two flows may be combined, right? Firstly, the match
> "outport == DGW" is missing from the new flow. Secondly, the
> "is_chassis_resident()" seems to be missing from the original flow. If
> this understanding is correct, the both flows just have same match
> condition, so the actions should be combined as well.
> 

Hi Lorenzo, Han,

I agree that both flows should have the same match and that actions
should be combined.

I'm not sure however if we can ever hit these flows if
is_chassis_resident(logical_port) == false. If I understand correctly we
can't, but just to be safe it's probably fine to add the
is_chassis_resident(logical_port) check.

> In addition, I'd suggest to add more clear comment to state the reason
> why setting reg1 and eth.src. For example: it is done to make sure when
> ARP request is triggered (in next stage), the ARP generated can have the
> eth.src = xxx and src IP as yyy, so that it can bypass the flow that
> redirect the ARP request to GW node ...
> 
> Thanks,
> Han
> 
>> +             */
>>              if (distributed) {
>> +                ds_clear();
>> +                ds_clear();
>> +                ds_put_format(,
>> +                              "ip%s.src == %s &&
> is_chassis_resident(\"%s\")",
>> +                              is_v6 ? "6" : "4", nat->logical_ip,
>> +                              nat->logical_port);
>> +                ds_put_format(, "eth.src = %s; %sreg1 = %s;
> next;",
>> +                              nat->external_mac, is_v6 ? "xx" : "",
>> +                              nat->external_ip);
>> +                ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 150,
>> +                              ds_cstr(), ds_cstr());
>> +
>>                  ds_clear();
>>                  ds_put_format(, "ip%s.src == %s && outport == %s",
>>                                is_v6 ? "6" : "4",
>> diff --git a/tests/ovn.at  b/tests/ovn.at 
>> index 8fa1a7e1b..15b40ca1e 100644
>> --- a/tests/ovn.at 
>> +++ b/tests/ovn.at 
>> @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
>>      set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
>>      options:tx_pcap=hv2/vif1-tx.pcap \
>>      options:rxq_pcap=hv2/vif1-rx.pcap \
>> -    ofport-request=1
>> +    ofport-request=2
>> +ovs-vsctl -- add-port br-int hv2-vif2 -- \
>> +    set interface hv2-vif2 external-ids:iface-id=sw0-p1 \
>> +    

Re: [ovs-dev] [PATCH ovn 2/2] northd: manage ARP request locally for FIP traffic

2020-05-25 Thread Han Zhou 
On Mon, May 25, 2020 at 2:55 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:
>
> Introduce 150-priority logical flows for each NAT rule that can
> be handled in a distributed manner to manage ARP request locally
> for FIP traffic. In particular set reg1 and eth.src to NAT external
> ip and NAT external mac respectivelly and do not distribute ARP
> traffic using FIP
>
> Signed-off-by: Lorenzo Bianconi 
> ---
>  northd/ovn-northd.8.xml | 12 
>  northd/ovn-northd.c | 18 +-
>  tests/ovn.at| 28 +---
>  tests/system-ovn.at | 28 
>  4 files changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 9423cbfd1..5e37d9758 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2875,6 +2875,18 @@ icmp4 {
>  
>
>  
> +  
> +For each NAT rule in the OVN Northbound database that can
> +be handled in a distributed manner, a priorirty-150 logical
> +flow with match ip.src == A 
> +is_chassis_resident(B), where A
> +is NAT logical ip and B is NAT logical port.
> +IP traffic matching the above rule will be managed locally
> +setting reg1 to C and
eth.src
> +to D, where C is NAT external ip and
> +D is NAT external mac.
> +  
> +
>
>  For each NAT rule in the OVN Northbound database that can
>  be handled in a distributed manner, a priority-100 logical
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 7ae5e45da..20f0ee2e4 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -9181,8 +9181,24 @@ build_lrouter_flows(struct hmap *datapaths, struct
hmap *ports,
>  /* Ingress Gateway Redirect Table: For NAT on a distributed
>   * router, add flows that are specific to a NAT rule.  These
>   * flows indicate the presence of an applicable NAT rule that
> - * can be applied in a distributed manner. */
> + * can be applied in a distributed manner.
> + * Moreover add logical flows to avoid ARP distributing when
the
> + * chassis has a direct connection to the underlay network
through
> + * a localnet port

I think the two flows may be combined, right? Firstly, the match "outport
== DGW" is missing from the new flow. Secondly, the "is_chassis_resident()"
seems to be missing from the original flow. If this understanding is
correct, the both flows just have same match condition, so the actions
should be combined as well.

In addition, I'd suggest to add more clear comment to state the reason why
setting reg1 and eth.src. For example: it is done to make sure when ARP
request is triggered (in next stage), the ARP generated can have the
eth.src = xxx and src IP as yyy, so that it can bypass the flow that
redirect the ARP request to GW node ...

Thanks,
Han

> + */
>  if (distributed) {
> +ds_clear();
> +ds_clear();
> +ds_put_format(,
> +  "ip%s.src == %s &&
is_chassis_resident(\"%s\")",
> +  is_v6 ? "6" : "4", nat->logical_ip,
> +  nat->logical_port);
> +ds_put_format(, "eth.src = %s; %sreg1 = %s;
next;",
> +  nat->external_mac, is_v6 ? "xx" : "",
> +  nat->external_ip);
> +ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 150,
> +  ds_cstr(), ds_cstr());
> +
>  ds_clear();
>  ds_put_format(, "ip%s.src == %s && outport == %s",
>is_v6 ? "6" : "4",
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 8fa1a7e1b..15b40ca1e 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14943,9 +14943,14 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
>  set interface hv2-vif1 external-ids:iface-id=sw1-p0 \
>  options:tx_pcap=hv2/vif1-tx.pcap \
>  options:rxq_pcap=hv2/vif1-rx.pcap \
> -ofport-request=1
> +ofport-request=2
> +ovs-vsctl -- add-port br-int hv2-vif2 -- \
> +set interface hv2-vif2 external-ids:iface-id=sw0-p1 \
> +options:tx_pcap=hv2/vif2-tx.pcap \
> +options:rxq_pcap=hv2/vif2-rx.pcap \
> +ofport-request=3
>
> -ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> +ovn-nbctl create Logical_Router name=lr0
>  ovn-nbctl ls-add sw0
>  ovn-nbctl ls-add sw1
>
> @@ -14954,13 +14959,16 @@ ovn-nbctl lsp-add sw0 rp-sw0 -- set
Logical_Switch_Port rp-sw0 \
>  type=router options:router-port=sw0 \
>  -- lsp-set-addresses rp-sw0 router
>
> -ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24
2002:0:0:0:0:0:0:1/64
> +ovn-nbctl lrp-add lr0 sw1 00:00:02:01:02:03 172.16.1.1/24
2002:0:0:0:0:0:0:1/64 \
> +-- set Logical_Router_Port sw1