Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-14 Thread Numan Siddique
On Thu, Jun 13, 2019 at 3:51 AM Ankur Sharma 
wrote:

> Hi,
>
> Just adding a minor clarification inline.
>
> Regards,
> Ankur
>
>
>
> *From:* Numan Siddique 
> *Sent:* Wednesday, June 12, 2019 12:29 AM
> *To:* Han Zhou 
> *Cc:* ovs dev ; Ankur Sharma <
> ankur.sha...@nutanix.com>
> *Subject:* Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports
> connected to localnet switches
>
>
>
>
>
>
>
> On Tue, Jun 11, 2019 at 9:37 PM Han Zhou  wrote:
>
>
>
>
>
> On Mon, Jun 10, 2019 at 10:27 AM Numan Siddique 
> wrote:
>
>
>
>
>
> On Fri, Jun 7, 2019 at 5:18 AM Han Zhou  wrote:
>
>
>
> On Wed, May 1, 2019 at 9:04 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > With the commit [1], the routing for the provider logical switches
> > connected to a router is centralized on the master gateway chassis
> > (if the option - reside-on-redirect-chassis) is set. When the
> > failover happens and a standby gateway chassis becomes master,
> > it should send GARPs for the router port macs. Without this, the
> > physical switch doesn't learn the new location of the router port macs
> > immediately and this could result in traffic disruption.
> >
> > This patch addresses this issue so that the ovn-controller which claims
> the
> > distributed gatweway router port sends out the GARPs.
> >
> > [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to
> a gateway chassis")
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  ovn/northd/ovn-northd.c | 21 +++
> >  tests/ovn.at [ovn.at]
> <https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=NiZlnfKLpFZTne6nLvW2LY-mMfT1ROA_XO3vDUxswGA=UbZ9hOu17Ha6b2AEfkP9hmScErxLKfSpkmLB33spkiw=>
>| 58 +++--
> >  2 files changed, 77 insertions(+), 2 deletions(-)
> >
> Hi Numan,
>
> Thanks for the fix. I have 2 comments:
>
> 1. The title is general which seems to address the problem for all "router
> ports connected to localnet switches". However, the commit message body and
> the code seems only handling the "reside-on-redirect-chassis" scenario,
> without taking care of the more common scenario - the gateway port GARP.
>
>
>
> Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e
> sending the GAR for the gateway ports.
>
>
>
>
>
>
>
> 2. The fix seems all related to "nat_addresses" handling, but this problem
> is not directly related to "NAT". After reading more context of the code, I
> realized that it is the right place to fix, but it is really not
> straightforward to understand. Of course this is not a problem introduced
> by current patch. It would be better if we had named the option
> "garp_addresses" instead of "nat_addresses", but I think it may be hard to
> rename at this stage because it will introduce compatibility problem. So
> probably we can add some more comments just to make the context more clear
> for readers.
>
>
>
> How about adding a new column "garp_addresses" ? This column can be used
> both for the gw port GARP and for the "reside-on-redirect-chassis" ?
>
>
>
> Hi Numan, I am not sure if adding a new column is better than clarifying
> with some documentation. If we add a new column, we should deprecate the
> old "nat_addresses" column (and kept there only for ovsdb upgrading). I
> don't think there is a need to distinguish in the schema if we are sending
> GARP for NAT or for GW port.
>
>
>
>
>
> That's what even I thought. Ankur has some other requirements for sending
> the GARPs for the gateway router port IPs. He wants the GARPs to be sent
> periodically. I think where as now, the GARPs are sent as a burst whenever
> a chassis claims a
>
> gw router port.
>
>
>
> Do you think it's better to change the GARP interval as per this patch - 
> https://patchwork.ozlabs.org/patch/1107466/
> [patchwork.ozlabs.org]
> <https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1107466_=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=NiZlnfKLpFZTne6nLvW2LY-mMfT1ROA_XO3vDUxswGA=cGB3-m1D9uO-iDu65mItknPBQwEMNe7noJBtRKNaoBY=>
>
> which Ankur has proposed for all the addresses - router addresses and NAT
> addresses ? Please grep for "add_garp" in
>
> the above patch for more details.
>
>
I have submitted the v2 here -
https://patchwork.ozlabs.org/project/openvswitch/list/?series=113

Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-12 Thread Ankur Sharma
Hi,

Just adding a minor clarification inline.

Regards,
Ankur

From: Numan Siddique 
Sent: Wednesday, June 12, 2019 12:29 AM
To: Han Zhou 
Cc: ovs dev ; Ankur Sharma 
Subject: Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to 
localnet switches



On Tue, Jun 11, 2019 at 9:37 PM Han Zhou 
mailto:zhou...@gmail.com>> wrote:


On Mon, Jun 10, 2019 at 10:27 AM Numan Siddique 
mailto:nusid...@redhat.com>> wrote:


On Fri, Jun 7, 2019 at 5:18 AM Han Zhou 
mailto:zhou...@gmail.com>> wrote:


On Wed, May 1, 2019 at 9:04 AM 
mailto:nusid...@redhat.com>> wrote:
>
> From: Numan Siddique mailto:nusid...@redhat.com>>
>
> With the commit [1], the routing for the provider logical switches
> connected to a router is centralized on the master gateway chassis
> (if the option - reside-on-redirect-chassis) is set. When the
> failover happens and a standby gateway chassis becomes master,
> it should send GARPs for the router port macs. Without this, the
> physical switch doesn't learn the new location of the router port macs
> immediately and this could result in traffic disruption.
>
> This patch addresses this issue so that the ovn-controller which claims the
> distributed gatweway router port sends out the GARPs.
>
> [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to a 
> gateway chassis")
>
> Signed-off-by: Numan Siddique 
> mailto:nusid...@redhat.com>>
> ---
>  ovn/northd/ovn-northd.c | 21 +++
>  tests/ovn.at 
> [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=NiZlnfKLpFZTne6nLvW2LY-mMfT1ROA_XO3vDUxswGA=UbZ9hOu17Ha6b2AEfkP9hmScErxLKfSpkmLB33spkiw=>
> | 58 +++--
>  2 files changed, 77 insertions(+), 2 deletions(-)
>
Hi Numan,

Thanks for the fix. I have 2 comments:
1. The title is general which seems to address the problem for all "router 
ports connected to localnet switches". However, the commit message body and the 
code seems only handling the "reside-on-redirect-chassis" scenario, without 
taking care of the more common scenario - the gateway port GARP.

Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e sending 
the GAR for the gateway ports.



2. The fix seems all related to "nat_addresses" handling, but this problem is 
not directly related to "NAT". After reading more context of the code, I 
realized that it is the right place to fix, but it is really not 
straightforward to understand. Of course this is not a problem introduced by 
current patch. It would be better if we had named the option "garp_addresses" 
instead of "nat_addresses", but I think it may be hard to rename at this stage 
because it will introduce compatibility problem. So probably we can add some 
more comments just to make the context more clear for readers.

How about adding a new column "garp_addresses" ? This column can be used both 
for the gw port GARP and for the "reside-on-redirect-chassis" ?

Hi Numan, I am not sure if adding a new column is better than clarifying with 
some documentation. If we add a new column, we should deprecate the old 
"nat_addresses" column (and kept there only for ovsdb upgrading). I don't think 
there is a need to distinguish in the schema if we are sending GARP for NAT or 
for GW port.


That's what even I thought. Ankur has some other requirements for sending the 
GARPs for the gateway router port IPs. He wants the GARPs to be sent 
periodically. I think where as now, the GARPs are sent as a burst whenever a 
chassis claims a
gw router port.

Do you think it's better to change the GARP interval as per this patch - 
https://patchwork.ozlabs.org/patch/1107466/ 
[patchwork.ozlabs.org]<https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_1107466_=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=NiZlnfKLpFZTne6nLvW2LY-mMfT1ROA_XO3vDUxswGA=cGB3-m1D9uO-iDu65mItknPBQwEMNe7noJBtRKNaoBY=>
which Ankur has proposed for all the addresses - router addresses and NAT 
addresses ? Please grep for "add_garp" in
the above patch for more details.

[ANKUR]:
a. We should still be sending burst and by burst I mean sending multiple GARP 
packets with increasing inter packet interval.
i.e existing behavior is correct.

b. In addition to above I wanted to add 2 more changes.

c. Send the GARP for a gateway chassis attached router port, even if NAT is not 
configured on it.
For vlan backed networks, NAT is not mandatory to do North South 
communication.

d. For c. above do a. periodically. This is more being safe solution. Reason 
being that, in the absence of encapsulation,
redirection to gateway chassis will happen over 

Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-12 Thread Numan Siddique
On Tue, Jun 11, 2019 at 9:37 PM Han Zhou  wrote:

>
>
> On Mon, Jun 10, 2019 at 10:27 AM Numan Siddique 
> wrote:
>
>>
>>
>> On Fri, Jun 7, 2019 at 5:18 AM Han Zhou  wrote:
>>
>>>
>>>
>>> On Wed, May 1, 2019 at 9:04 AM  wrote:
>>> >
>>> > From: Numan Siddique 
>>> >
>>> > With the commit [1], the routing for the provider logical switches
>>> > connected to a router is centralized on the master gateway chassis
>>> > (if the option - reside-on-redirect-chassis) is set. When the
>>> > failover happens and a standby gateway chassis becomes master,
>>> > it should send GARPs for the router port macs. Without this, the
>>> > physical switch doesn't learn the new location of the router port macs
>>> > immediately and this could result in traffic disruption.
>>> >
>>> > This patch addresses this issue so that the ovn-controller which
>>> claims the
>>> > distributed gatweway router port sends out the GARPs.
>>> >
>>> > [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected
>>> to a gateway chassis")
>>> >
>>> > Signed-off-by: Numan Siddique 
>>> > ---
>>> >  ovn/northd/ovn-northd.c | 21 +++
>>> >  tests/ovn.at| 58
>>> +++--
>>> >  2 files changed, 77 insertions(+), 2 deletions(-)
>>> >
>>> Hi Numan,
>>>
>>> Thanks for the fix. I have 2 comments:
>>>
>>> 1. The title is general which seems to address the problem for all
>>> "router ports connected to localnet switches". However, the commit message
>>> body and the code seems only handling the "reside-on-redirect-chassis"
>>> scenario, without taking care of the more common scenario - the gateway
>>> port GARP.
>>>
>>
>> Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e
>> sending the GAR for the gateway ports.
>>
>>
>>
>>>
>>> 2. The fix seems all related to "nat_addresses" handling, but this
>>> problem is not directly related to "NAT". After reading more context of the
>>> code, I realized that it is the right place to fix, but it is really not
>>> straightforward to understand. Of course this is not a problem introduced
>>> by current patch. It would be better if we had named the option
>>> "garp_addresses" instead of "nat_addresses", but I think it may be hard to
>>> rename at this stage because it will introduce compatibility problem. So
>>> probably we can add some more comments just to make the context more clear
>>> for readers.
>>>
>>
>> How about adding a new column "garp_addresses" ? This column can be used
>> both for the gw port GARP and for the "reside-on-redirect-chassis" ?
>>
>
> Hi Numan, I am not sure if adding a new column is better than clarifying
> with some documentation. If we add a new column, we should deprecate the
> old "nat_addresses" column (and kept there only for ovsdb upgrading). I
> don't think there is a need to distinguish in the schema if we are sending
> GARP for NAT or for GW port.
>
>>
>>
That's what even I thought. Ankur has some other requirements for sending
the GARPs for the gateway router port IPs. He wants the GARPs to be sent
periodically. I think where as now, the GARPs are sent as a burst whenever
a chassis claims a
gw router port.

Do you think it's better to change the GARP interval as per this patch -
https://patchwork.ozlabs.org/patch/1107466/
which Ankur has proposed for all the addresses - router addresses and NAT
addresses ? Please grep for "add_garp" in
the above patch for more details.

Thanks
Numan




> If this makes sense, I will work on it.
>> @Ankur - I will submit a patch with a new column (which will send GARPs
>> for both the gw router ports and other router ports with the option -
>> reside-on-redirect-chassis ) and and then you can enhance it to send the
>> GARPs in periodic interval instead of initial bursts  (which the present
>> code does for NAT addresses ) ? Does this sound good ?
>>
>> Thanks
>> Numan
>>
>>
>>
>>> Thanks,
>>> Han
>>>
>>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-11 Thread Han Zhou
On Mon, Jun 10, 2019 at 10:27 AM Numan Siddique  wrote:

>
>
> On Fri, Jun 7, 2019 at 5:18 AM Han Zhou  wrote:
>
>>
>>
>> On Wed, May 1, 2019 at 9:04 AM  wrote:
>> >
>> > From: Numan Siddique 
>> >
>> > With the commit [1], the routing for the provider logical switches
>> > connected to a router is centralized on the master gateway chassis
>> > (if the option - reside-on-redirect-chassis) is set. When the
>> > failover happens and a standby gateway chassis becomes master,
>> > it should send GARPs for the router port macs. Without this, the
>> > physical switch doesn't learn the new location of the router port macs
>> > immediately and this could result in traffic disruption.
>> >
>> > This patch addresses this issue so that the ovn-controller which claims
>> the
>> > distributed gatweway router port sends out the GARPs.
>> >
>> > [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected
>> to a gateway chassis")
>> >
>> > Signed-off-by: Numan Siddique 
>> > ---
>> >  ovn/northd/ovn-northd.c | 21 +++
>> >  tests/ovn.at| 58 +++--
>> >  2 files changed, 77 insertions(+), 2 deletions(-)
>> >
>> Hi Numan,
>>
>> Thanks for the fix. I have 2 comments:
>>
>> 1. The title is general which seems to address the problem for all
>> "router ports connected to localnet switches". However, the commit message
>> body and the code seems only handling the "reside-on-redirect-chassis"
>> scenario, without taking care of the more common scenario - the gateway
>> port GARP.
>>
>
> Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e
> sending the GAR for the gateway ports.
>
>
>
>>
>> 2. The fix seems all related to "nat_addresses" handling, but this
>> problem is not directly related to "NAT". After reading more context of the
>> code, I realized that it is the right place to fix, but it is really not
>> straightforward to understand. Of course this is not a problem introduced
>> by current patch. It would be better if we had named the option
>> "garp_addresses" instead of "nat_addresses", but I think it may be hard to
>> rename at this stage because it will introduce compatibility problem. So
>> probably we can add some more comments just to make the context more clear
>> for readers.
>>
>
> How about adding a new column "garp_addresses" ? This column can be used
> both for the gw port GARP and for the "reside-on-redirect-chassis" ?
>

Hi Numan, I am not sure if adding a new column is better than clarifying
with some documentation. If we add a new column, we should deprecate the
old "nat_addresses" column (and kept there only for ovsdb upgrading). I
don't think there is a need to distinguish in the schema if we are sending
GARP for NAT or for GW port.

>
> If this makes sense, I will work on it.
> @Ankur - I will submit a patch with a new column (which will send GARPs
> for both the gw router ports and other router ports with the option -
> reside-on-redirect-chassis ) and and then you can enhance it to send the
> GARPs in periodic interval instead of initial bursts  (which the present
> code does for NAT addresses ) ? Does this sound good ?
>
> Thanks
> Numan
>
>
>
>> Thanks,
>> Han
>>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-10 Thread Ankur Sharma
Hi Numan,

Sure, we can wait for your “garp_addresses”  based changes to do both “gw ports 
and  “resident-on-redirect-chassis”.
I can provide periodic advertisement related changes after that.

Regards,
Ankur

From: Numan Siddique 
Sent: Monday, June 10, 2019 10:27 AM
To: Han Zhou 
Cc: ovs dev ; Ankur Sharma 
Subject: Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to 
localnet switches



On Fri, Jun 7, 2019 at 5:18 AM Han Zhou 
mailto:zhou...@gmail.com>> wrote:


On Wed, May 1, 2019 at 9:04 AM 
mailto:nusid...@redhat.com>> wrote:
>
> From: Numan Siddique mailto:nusid...@redhat.com>>
>
> With the commit [1], the routing for the provider logical switches
> connected to a router is centralized on the master gateway chassis
> (if the option - reside-on-redirect-chassis) is set. When the
> failover happens and a standby gateway chassis becomes master,
> it should send GARPs for the router port macs. Without this, the
> physical switch doesn't learn the new location of the router port macs
> immediately and this could result in traffic disruption.
>
> This patch addresses this issue so that the ovn-controller which claims the
> distributed gatweway router port sends out the GARPs.
>
> [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to a 
> gateway chassis")
>
> Signed-off-by: Numan Siddique 
> mailto:nusid...@redhat.com>>
> ---
>  ovn/northd/ovn-northd.c | 21 +++
>  tests/ovn.at 
> [ovn.at]<https://urldefense.proofpoint.com/v2/url?u=http-3A__ovn.at=DwMFaQ=s883GpUCOChKOHiocYtGcg=mZwX9gFQgeJHzTg-68aCJgsODyUEVsHGFOfL90J6MJY=9l_WOe1v0FLuY89KsBfxqhIU01WNzT5ZUjWrfxPfaCk=-UjbquhF3wtMvv-nrpNleIelQPt0yoB63gdfs13KZ1M=>
> | 58 +++--
>  2 files changed, 77 insertions(+), 2 deletions(-)
>
Hi Numan,

Thanks for the fix. I have 2 comments:
1. The title is general which seems to address the problem for all "router 
ports connected to localnet switches". However, the commit message body and the 
code seems only handling the "reside-on-redirect-chassis" scenario, without 
taking care of the more common scenario - the gateway port GARP.

Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e sending 
the GAR for the gateway ports.



2. The fix seems all related to "nat_addresses" handling, but this problem is 
not directly related to "NAT". After reading more context of the code, I 
realized that it is the right place to fix, but it is really not 
straightforward to understand. Of course this is not a problem introduced by 
current patch. It would be better if we had named the option "garp_addresses" 
instead of "nat_addresses", but I think it may be hard to rename at this stage 
because it will introduce compatibility problem. So probably we can add some 
more comments just to make the context more clear for readers.

How about adding a new column "garp_addresses" ? This column can be used both 
for the gw port GARP and for the "reside-on-redirect-chassis" ?

If this makes sense, I will work on it.
@Ankur - I will submit a patch with a new column (which will send GARPs for 
both the gw router ports and other router ports with the option - 
reside-on-redirect-chassis ) and and then you can enhance it to send the GARPs 
in periodic interval instead of initial bursts  (which the present code does 
for NAT addresses ) ? Does this sound good ?


Thanks
Numan



Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-10 Thread Numan Siddique
On Fri, Jun 7, 2019 at 5:18 AM Han Zhou  wrote:

>
>
> On Wed, May 1, 2019 at 9:04 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > With the commit [1], the routing for the provider logical switches
> > connected to a router is centralized on the master gateway chassis
> > (if the option - reside-on-redirect-chassis) is set. When the
> > failover happens and a standby gateway chassis becomes master,
> > it should send GARPs for the router port macs. Without this, the
> > physical switch doesn't learn the new location of the router port macs
> > immediately and this could result in traffic disruption.
> >
> > This patch addresses this issue so that the ovn-controller which claims
> the
> > distributed gatweway router port sends out the GARPs.
> >
> > [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to
> a gateway chassis")
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  ovn/northd/ovn-northd.c | 21 +++
> >  tests/ovn.at| 58 +++--
> >  2 files changed, 77 insertions(+), 2 deletions(-)
> >
> Hi Numan,
>
> Thanks for the fix. I have 2 comments:
>
> 1. The title is general which seems to address the problem for all "router
> ports connected to localnet switches". However, the commit message body and
> the code seems only handling the "reside-on-redirect-chassis" scenario,
> without taking care of the more common scenario - the gateway port GARP.
>

Agree. I think Ankur (CC'ed) is planning to handle this scenario - i.e
sending the GAR for the gateway ports.



>
> 2. The fix seems all related to "nat_addresses" handling, but this problem
> is not directly related to "NAT". After reading more context of the code, I
> realized that it is the right place to fix, but it is really not
> straightforward to understand. Of course this is not a problem introduced
> by current patch. It would be better if we had named the option
> "garp_addresses" instead of "nat_addresses", but I think it may be hard to
> rename at this stage because it will introduce compatibility problem. So
> probably we can add some more comments just to make the context more clear
> for readers.
>

How about adding a new column "garp_addresses" ? This column can be used
both for the gw port GARP and for the "reside-on-redirect-chassis" ?

If this makes sense, I will work on it.
@Ankur - I will submit a patch with a new column (which will send GARPs for
both the gw router ports and other router ports with the option -
reside-on-redirect-chassis ) and and then you can enhance it to send the
GARPs in periodic interval instead of initial bursts  (which the present
code does for NAT addresses ) ? Does this sound good ?

Thanks
Numan



> Thanks,
> Han
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-06-06 Thread Han Zhou
On Wed, May 1, 2019 at 9:04 AM  wrote:
>
> From: Numan Siddique 
>
> With the commit [1], the routing for the provider logical switches
> connected to a router is centralized on the master gateway chassis
> (if the option - reside-on-redirect-chassis) is set. When the
> failover happens and a standby gateway chassis becomes master,
> it should send GARPs for the router port macs. Without this, the
> physical switch doesn't learn the new location of the router port macs
> immediately and this could result in traffic disruption.
>
> This patch addresses this issue so that the ovn-controller which claims
the
> distributed gatweway router port sends out the GARPs.
>
> [1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to
a gateway chassis")
>
> Signed-off-by: Numan Siddique 
> ---
>  ovn/northd/ovn-northd.c | 21 +++
>  tests/ovn.at| 58 +++--
>  2 files changed, 77 insertions(+), 2 deletions(-)
>
Hi Numan,

Thanks for the fix. I have 2 comments:

1. The title is general which seems to address the problem for all "router
ports connected to localnet switches". However, the commit message body and
the code seems only handling the "reside-on-redirect-chassis" scenario,
without taking care of the more common scenario - the gateway port GARP.

2. The fix seems all related to "nat_addresses" handling, but this problem
is not directly related to "NAT". After reading more context of the code, I
realized that it is the right place to fix, but it is really not
straightforward to understand. Of course this is not a problem introduced
by current patch. It would be better if we had named the option
"garp_addresses" instead of "nat_addresses", but I think it may be hard to
rename at this stage because it will introduce compatibility problem. So
probably we can add some more comments just to make the context more clear
for readers.

Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH] ovn: Send GARP for the router ports connected to localnet switches

2019-05-01 Thread nusiddiq
From: Numan Siddique 

With the commit [1], the routing for the provider logical switches
connected to a router is centralized on the master gateway chassis
(if the option - reside-on-redirect-chassis) is set. When the
failover happens and a standby gateway chassis becomes master,
it should send GARPs for the router port macs. Without this, the
physical switch doesn't learn the new location of the router port macs
immediately and this could result in traffic disruption.

This patch addresses this issue so that the ovn-controller which claims the
distributed gatweway router port sends out the GARPs.

[1] - 85706c34d53d ("ovn: Avoid tunneling for VLAN packets redirected to a 
gateway chassis")

Signed-off-by: Numan Siddique 
---
 ovn/northd/ovn-northd.c | 21 +++
 tests/ovn.at| 58 +++--
 2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index de0c06d4b..4fb6c3a98 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -2525,6 +2525,27 @@ ovn_port_update_sbrec(struct northd_context *ctx,
  _addresses, 1);
 destroy_lport_addresses();
 }
+} else if (op->peer && op->peer->nbrp && op->peer->od->l3dgw_port
+   && op->peer->od->l3redirect_port
+   && smap_get_bool(>peer->nbrp->options,
+"reside-on-redirect-chassis", false)) {
+/* reside-on-gateway-chassis is set and the
+ * the logical router datapath has distributed router port. Add
+ * the router mac and IPv4 addresses so that GARP is sent for
+ * these IPs by the ovn-controller which handles the
+ * centralized routing. */
+struct ds nat_addr = DS_EMPTY_INITIALIZER;
+ds_put_format(_addr, "%s", op->peer->lrp_networks.ea_s);
+for (size_t i = 0; i < op->peer->lrp_networks.n_ipv4_addrs;
+ i++) {
+ds_put_format(_addr, " %s",
+  op->peer->lrp_networks.ipv4_addrs[i].addr_s);
+}
+ds_put_format(_addr, " is_chassis_resident(%s)",
+  op->peer->od->l3redirect_port->json_key);
+const char *s = ds_cstr(_addr);
+sbrec_port_binding_set_nat_addresses(op->sb, , 1);
+ds_destroy(_addr);
 } else {
 sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index 592f491fd..d0fe1f561 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -9395,9 +9395,10 @@ ovn_start
 # # (i.e 8.8.8.8 as destination ip).
 
 # Physical network:
-# # Three hypervisors hv[123].
+# # Four hypervisors hv[1234].
 # # hv1 hosts vif foo1.
 # # hv2 is the "redirect-chassis" that hosts the distributed router gateway 
port.
+# # Later to test GARPs for the router port - foo, hv2 and hv4 are added to 
the ha_chassis_group
 # # hv3 hosts nexthop port vif outside1.
 # # All other tests connect hypervisors to network n1 through br-phys for 
tunneling.
 # # But in this test, hv1 won't connect to n1(and no br-phys in hv1), and
@@ -9422,6 +9423,8 @@ ovs-vsctl \
 start_daemon ovn-controller
 ovs-vsctl -- add-port br-int hv1-vif1 -- \
 set interface hv1-vif1 external-ids:iface-id=foo1 \
+options:tx_pcap=hv1/vif1-tx.pcap \
+options:rxq_pcap=hv1/vif1-rx.pcap \
 ofport-request=1
 
 sim_add hv2
@@ -9441,6 +9444,12 @@ ovs-vsctl -- add-port br-int hv3-vif1 -- \
 ofport-request=1
 ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings="phys:br-phys"
 
+sim_add hv4
+as hv4
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.4
+ovs-vsctl set Open_vSwitch . 
external-ids:ovn-bridge-mappings="public:br-ex,phys:br-phys"
+
 # Create network n2 for vlan connectivity between hv1 and hv2
 net_add n2
 
@@ -9452,6 +9461,10 @@ as hv2
 ovs-vsctl add-br br-ex
 net_attach n2 br-ex
 
+as hv4
+ovs-vsctl add-br br-ex
+net_attach n2 br-ex
+
 OVN_POPULATE_ARP
 
 ovn-nbctl create Logical_Router name=R1
@@ -9634,7 +9647,48 @@ $PYTHON "$top_srcdir/utilities/ovs-pcap.in" 
hv3/vif1-tx.pcap | grep ${foo1_ip}${
 cat hv3-vif1.expected > expout
 AT_CHECK([sort hv3-vif1], [0], [expout])
 
-OVN_CLEANUP([hv1],[hv2],[hv3])
+# Test the GARP for the router port ip - 192.168.1.1
+ovn-nbctl --wait=sb ha-chassis-group-add hagrp1
+
+as hv1 reset_pcap_file hv1-vif1 hv1/vif1
+as hv2 reset_pcap_file br-ex_n2 hv2/br-ex_n2
+as hv4 reset_pcap_file br-ex_n2 hv4/br-ex_n2
+
+ovn-nbctl --wait=sb ha-chassis-group-add-chassis hagrp1 hv2 30
+ovn-nbctl --wait=sb ha-chassis-group-add-chassis hagrp1 hv4 20
+
+hagrp1_uuid=`ovn-nbctl --bare --columns _uuid find ha_chassis_group 
name=hagrp1`
+ovn-nbctl remove logical_router_port alice options redirect-chassis