Re: [ovs-dev] [PATCH v3] ovn-controller: Support multiple gateway port on a distributed router

[Guoshuai Li]

On Fri, Mar 2, 2018 at 5:01 AM, Numan Siddique > wrote:




On Feb 28, 2018 6:12 PM, "Guru Shetty" mailto:g...@ovn.org>> wrote:

This is a little more complicated for a quick review. I think
it will need
a round from me to understand the implications for gateway
routers and one
round from Numan (or someone from openstack) that use
distributed gateway
routers.



Ack. I will take a look at the patch as well.

Thanks
Numan


On 14 February 2018 at 16:42, Ben Pfaff mailto:b...@ovn.org>> wrote:

> Hi Guru.  Are you willing to take a look at this patch?
>
> Thanks,
>
> Ben.
>
> On Fri, Feb 09, 2018 at 03:34:55PM +0800, Guoshuai Li wrote:
> > The main application scenario of this patch is that the
user flow wants
> to
> > different destination addresses through different external
networks.
> > This scenario requires a distributed route to be
associated with
> > multiple external network logical switches.
> >
> > Change l3dgw_port to l3dgw_ports in ovn_datapath,and change
> > l3redirect_port to ovn_port. Then in a distributed router,
the NAT
> > logical flow table is generated based on the external IP
lookup
> > distributed router port, otherwise not generated. And LB
is the same.
> >
> > When the destination address of the packet is an external
IP of the NAT
> rule,
> > and the ingress port is not a gateway, it is necessary to
route to the
> actual
> > outgoing port.
> >
> > Signed-off-by: Guoshuai Li mailto:l...@dtdream.com>>




Hi Guoshuai Li,

I am getting the below compilation errors with "-Werror" option

**
../ovn/northd/ovn-northd.c: In function ‘build_lrouter_flows’:
../ovn/northd/ovn-northd.c:5394:29: error: declaration of ‘i’ shadows 
a previous local [-Werror=shadow]

                 for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
                             ^
../ovn/northd/ovn-northd.c:5139:18: note: shadowed declaration is here
         for (int i = 0; i < od->nbr->n_nat; i++) {
                  ^
../ovn/northd/ovn-northd.c:5395:38: error: declaration of ‘l3dgw_port’ 
shadows a previous local [-Werror=shadow]

                     struct ovn_port *l3dgw_port = od->l3dgw_ports[i];
      ^~
../ovn/northd/ovn-northd.c:5197:30: note: shadowed declaration is here
             struct ovn_port *l3dgw_port = find_l3dgw_port(od,
^~
../ovn/northd/ovn-northd.c:5413:29: error: declaration of ‘i’ shadows 
a previous local [-Werror=shadow]

                 for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
                             ^
../ovn/northd/ovn-northd.c:5139:18: note: shadowed declaration is here
         for (int i = 0; i < od->nbr->n_nat; i++) {
                  ^
../ovn/northd/ovn-northd.c:5414:38: error: declaration of ‘l3dgw_port’ 
shadows a previous local [-Werror=shadow]

                     struct ovn_port *l3dgw_port = od->l3dgw_ports[i];
      ^~
../ovn/northd/ovn-northd.c:5197:30: note: shadowed declaration is here
             struct ovn_port *l3dgw_port = find_l3dgw_port(od,
*

Thanks
Numan



Thanks for review, Numan. I send v4.

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

Re: [ovs-dev] [PATCH v3] ovn-controller: Support multiple gateway port on a distributed router

[Numan Siddique]

On Fri, Mar 2, 2018 at 5:01 AM, Numan Siddique  wrote:

>
>
> On Feb 28, 2018 6:12 PM, "Guru Shetty"  wrote:
>
> This is a little more complicated for a quick review. I think it will need
> a round from me to understand the implications for gateway routers and one
> round from Numan (or someone from openstack) that use distributed gateway
> routers.
>
>
>
> Ack. I will take a look at the patch as well.
>
> Thanks
> Numan
>
>
> On 14 February 2018 at 16:42, Ben Pfaff  wrote:
>
> > Hi Guru.  Are you willing to take a look at this patch?
> >
> > Thanks,
> >
> > Ben.
> >
> > On Fri, Feb 09, 2018 at 03:34:55PM +0800, Guoshuai Li wrote:
> > > The main application scenario of this patch is that the user flow wants
> > to
> > > different destination addresses through different external networks.
> > > This scenario requires a distributed route to be associated with
> > > multiple external network logical switches.
> > >
> > > Change l3dgw_port to l3dgw_ports in ovn_datapath,and change
> > > l3redirect_port to ovn_port. Then in a distributed router, the NAT
> > > logical flow table is generated based on the external IP lookup
> > > distributed router port, otherwise not generated. And LB is the same.
> > >
> > > When the destination address of the packet is an external IP of the NAT
> > rule,
> > > and the ingress port is not a gateway, it is necessary to route to the
> > actual
> > > outgoing port.
> > >
> > > Signed-off-by: Guoshuai Li 
>
>


Hi Guoshuai Li,

I am getting the below compilation errors with "-Werror" option

**
../ovn/northd/ovn-northd.c: In function ‘build_lrouter_flows’:
../ovn/northd/ovn-northd.c:5394:29: error: declaration of ‘i’ shadows a
previous local [-Werror=shadow]
 for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
 ^
../ovn/northd/ovn-northd.c:5139:18: note: shadowed declaration is here
 for (int i = 0; i < od->nbr->n_nat; i++) {
  ^
../ovn/northd/ovn-northd.c:5395:38: error: declaration of ‘l3dgw_port’
shadows a previous local [-Werror=shadow]
 struct ovn_port *l3dgw_port = od->l3dgw_ports[i];
  ^~
../ovn/northd/ovn-northd.c:5197:30: note: shadowed declaration is here
 struct ovn_port *l3dgw_port = find_l3dgw_port(od,
  ^~
../ovn/northd/ovn-northd.c:5413:29: error: declaration of ‘i’ shadows a
previous local [-Werror=shadow]
 for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
 ^
../ovn/northd/ovn-northd.c:5139:18: note: shadowed declaration is here
 for (int i = 0; i < od->nbr->n_nat; i++) {
  ^
../ovn/northd/ovn-northd.c:5414:38: error: declaration of ‘l3dgw_port’
shadows a previous local [-Werror=shadow]
 struct ovn_port *l3dgw_port = od->l3dgw_ports[i];
  ^~
../ovn/northd/ovn-northd.c:5197:30: note: shadowed declaration is here
 struct ovn_port *l3dgw_port = find_l3dgw_port(od,
*

Thanks
Numan



> > > ---
> > >  ovn/northd/ovn-northd.8.xml |  22 +---
> > >  ovn/northd/ovn-northd.c | 273 +-
> > --
> > >  ovn/ovn-nb.xml  |  12 +-
> > >  tests/system-ovn.at | 162 +++---
> > >  4 files changed, 315 insertions(+), 154 deletions(-)
> > >
> > > ---
> > >
> > > I submitted this patch long ago, I think it might be useful,
> > > so resend it for more comments, thanks.
> > >
> > > v1 -> v2:
> > >   1. rebase from master.
> > >   2. add test case.
> > > v2 -> v3:
> > >   fixed build failed.
> > >
> > > ---
> > >
> > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > > index 6bc2dd6af..ff523614a 100644
> > > --- a/ovn/northd/ovn-northd.8.xml
> > > +++ b/ovn/northd/ovn-northd.8.xml
> > > @@ -1732,16 +1732,6 @@ output;
> > >  
> > >
> > >  
> > > -  For distributed logical routers where one of the logical
> > router
> > > -  ports specifies a redirect-chassis, a
> > priority-300
> > > -  logical flow with match REGBIT_NAT_REDIRECT ==
> 1
> > has
> > > -  actions ip.ttl--; next;.  The
> > outport
> > > -  will be set later in the Gateway Redirect table.
> > > -
> > > -  
> > > -
> > > -  
> > > -
> > >IPv4 routing table.  For each route to IPv4 network
> > N with
> > >netmask M, on router port P with IP
> > address
> > >A and Ethernet
> > > @@ -1827,12 +1817,12 @@ next;
> > >  
> > >
> > >  
> > > -  For distributed logical routers where one of the logical
> > router
> > > -  ports specifies a redirect-chassis, a
> > priority-200
> > > -  logical flow with match REGBIT_NAT_REDIRECT ==
> 1
> > has
> > > -  actions eth.dst = E; next;, where
> > > -  E is the ethernet address of the rou

Re: [ovs-dev] [PATCH v3] ovn-controller: Support multiple gateway port on a distributed router

[Numan Siddique]

On Feb 28, 2018 6:12 PM, "Guru Shetty"  wrote:

This is a little more complicated for a quick review. I think it will need
a round from me to understand the implications for gateway routers and one
round from Numan (or someone from openstack) that use distributed gateway
routers.



Ack. I will take a look at the patch as well.

Thanks
Numan


On 14 February 2018 at 16:42, Ben Pfaff  wrote:

> Hi Guru.  Are you willing to take a look at this patch?
>
> Thanks,
>
> Ben.
>
> On Fri, Feb 09, 2018 at 03:34:55PM +0800, Guoshuai Li wrote:
> > The main application scenario of this patch is that the user flow wants
> to
> > different destination addresses through different external networks.
> > This scenario requires a distributed route to be associated with
> > multiple external network logical switches.
> >
> > Change l3dgw_port to l3dgw_ports in ovn_datapath,and change
> > l3redirect_port to ovn_port. Then in a distributed router, the NAT
> > logical flow table is generated based on the external IP lookup
> > distributed router port, otherwise not generated. And LB is the same.
> >
> > When the destination address of the packet is an external IP of the NAT
> rule,
> > and the ingress port is not a gateway, it is necessary to route to the
> actual
> > outgoing port.
> >
> > Signed-off-by: Guoshuai Li 
> > ---
> >  ovn/northd/ovn-northd.8.xml |  22 +---
> >  ovn/northd/ovn-northd.c | 273 +-
> --
> >  ovn/ovn-nb.xml  |  12 +-
> >  tests/system-ovn.at | 162 +++---
> >  4 files changed, 315 insertions(+), 154 deletions(-)
> >
> > ---
> >
> > I submitted this patch long ago, I think it might be useful,
> > so resend it for more comments, thanks.
> >
> > v1 -> v2:
> >   1. rebase from master.
> >   2. add test case.
> > v2 -> v3:
> >   fixed build failed.
> >
> > ---
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 6bc2dd6af..ff523614a 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -1732,16 +1732,6 @@ output;
> >  
> >
> >  
> > -  For distributed logical routers where one of the logical
> router
> > -  ports specifies a redirect-chassis, a
> priority-300
> > -  logical flow with match REGBIT_NAT_REDIRECT == 1
> has
> > -  actions ip.ttl--; next;.  The
> outport
> > -  will be set later in the Gateway Redirect table.
> > -
> > -  
> > -
> > -  
> > -
> >IPv4 routing table.  For each route to IPv4 network
> N with
> >netmask M, on router port P with IP
> address
> >A and Ethernet
> > @@ -1827,12 +1817,12 @@ next;
> >  
> >
> >  
> > -  For distributed logical routers where one of the logical
> router
> > -  ports specifies a redirect-chassis, a
> priority-200
> > -  logical flow with match REGBIT_NAT_REDIRECT == 1
> has
> > -  actions eth.dst = E; next;, where
> > -  E is the ethernet address of the router's
> distributed
> > -  gateway port.
> > +  For distributed logical routers where router port
P
> > +  specifies a redirect-chassis, a priority-200
> > +  logical flow with match REGBIT_NAT_REDIRECT == 1
> > +  and outport == P has actions
> > +  eth.dst = E; next;, where
E
> > +  is the ethernet address of the router's distributed gateway
> port.
> >  
> >
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 4d95a3d9d..d38efcbed 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -418,12 +418,10 @@ struct ovn_datapath {
> >
> >  /* OVN northd only needs to know about the logical router gateway
> port for
> >   * NAT on a distributed router.  This "distributed gateway port" is
> > - * populated only when there is a "redirect-chassis" specified for
> one of
> > - * the ports on the logical router.  Otherwise this will be NULL.
*/
> > -struct ovn_port *l3dgw_port;
> > -/* The "derived" OVN port representing the instance of l3dgw_port
on
> > - * the "redirect-chassis". */
> > -struct ovn_port *l3redirect_port;
> > + * populated only when there is a "redirect-chassis" specified for
> the
> > + * ports on the logical router.  Otherwise this will be NULL. */
> > +struct ovn_port **l3dgw_ports;
> > +size_t n_l3dgw_ports;
> >  struct ovn_port *localnet_port;
> >  };
> >
> > @@ -472,6 +470,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> >  free(od->ipam_info);
> >  }
> >  free(od->router_ports);
> > +free(od->l3dgw_ports);
> >  free(od);
> >  }
> >  }
> > @@ -796,6 +795,10 @@ struct ovn_port {
> >  bool derived; /* Indicates whether this is an additional port
> > * derived from nbsp or nbrp. */
> >
> > + 

Re: [ovs-dev] [PATCH v3] ovn-controller: Support multiple gateway port on a distributed router

[Guru Shetty]

This is a little more complicated for a quick review. I think it will need
a round from me to understand the implications for gateway routers and one
round from Numan (or someone from openstack) that use distributed gateway
routers.

On 14 February 2018 at 16:42, Ben Pfaff  wrote:

> Hi Guru.  Are you willing to take a look at this patch?
>
> Thanks,
>
> Ben.
>
> On Fri, Feb 09, 2018 at 03:34:55PM +0800, Guoshuai Li wrote:
> > The main application scenario of this patch is that the user flow wants
> to
> > different destination addresses through different external networks.
> > This scenario requires a distributed route to be associated with
> > multiple external network logical switches.
> >
> > Change l3dgw_port to l3dgw_ports in ovn_datapath,and change
> > l3redirect_port to ovn_port. Then in a distributed router, the NAT
> > logical flow table is generated based on the external IP lookup
> > distributed router port, otherwise not generated. And LB is the same.
> >
> > When the destination address of the packet is an external IP of the NAT
> rule,
> > and the ingress port is not a gateway, it is necessary to route to the
> actual
> > outgoing port.
> >
> > Signed-off-by: Guoshuai Li 
> > ---
> >  ovn/northd/ovn-northd.8.xml |  22 +---
> >  ovn/northd/ovn-northd.c | 273 +-
> --
> >  ovn/ovn-nb.xml  |  12 +-
> >  tests/system-ovn.at | 162 +++---
> >  4 files changed, 315 insertions(+), 154 deletions(-)
> >
> > ---
> >
> > I submitted this patch long ago, I think it might be useful,
> > so resend it for more comments, thanks.
> >
> > v1 -> v2:
> >   1. rebase from master.
> >   2. add test case.
> > v2 -> v3:
> >   fixed build failed.
> >
> > ---
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 6bc2dd6af..ff523614a 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -1732,16 +1732,6 @@ output;
> >  
> >
> >  
> > -  For distributed logical routers where one of the logical
> router
> > -  ports specifies a redirect-chassis, a
> priority-300
> > -  logical flow with match REGBIT_NAT_REDIRECT == 1
> has
> > -  actions ip.ttl--; next;.  The
> outport
> > -  will be set later in the Gateway Redirect table.
> > -
> > -  
> > -
> > -  
> > -
> >IPv4 routing table.  For each route to IPv4 network
> N with
> >netmask M, on router port P with IP
> address
> >A and Ethernet
> > @@ -1827,12 +1817,12 @@ next;
> >  
> >
> >  
> > -  For distributed logical routers where one of the logical
> router
> > -  ports specifies a redirect-chassis, a
> priority-200
> > -  logical flow with match REGBIT_NAT_REDIRECT == 1
> has
> > -  actions eth.dst = E; next;, where
> > -  E is the ethernet address of the router's
> distributed
> > -  gateway port.
> > +  For distributed logical routers where router port P
> > +  specifies a redirect-chassis, a priority-200
> > +  logical flow with match REGBIT_NAT_REDIRECT == 1
> > +  and outport == P has actions
> > +  eth.dst = E; next;, where E
> > +  is the ethernet address of the router's distributed gateway
> port.
> >  
> >
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index 4d95a3d9d..d38efcbed 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -418,12 +418,10 @@ struct ovn_datapath {
> >
> >  /* OVN northd only needs to know about the logical router gateway
> port for
> >   * NAT on a distributed router.  This "distributed gateway port" is
> > - * populated only when there is a "redirect-chassis" specified for
> one of
> > - * the ports on the logical router.  Otherwise this will be NULL. */
> > -struct ovn_port *l3dgw_port;
> > -/* The "derived" OVN port representing the instance of l3dgw_port on
> > - * the "redirect-chassis". */
> > -struct ovn_port *l3redirect_port;
> > + * populated only when there is a "redirect-chassis" specified for
> the
> > + * ports on the logical router.  Otherwise this will be NULL. */
> > +struct ovn_port **l3dgw_ports;
> > +size_t n_l3dgw_ports;
> >  struct ovn_port *localnet_port;
> >  };
> >
> > @@ -472,6 +470,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> >  free(od->ipam_info);
> >  }
> >  free(od->router_ports);
> > +free(od->l3dgw_ports);
> >  free(od);
> >  }
> >  }
> > @@ -796,6 +795,10 @@ struct ovn_port {
> >  bool derived; /* Indicates whether this is an additional port
> > * derived from nbsp or nbrp. */
> >
> > +/* The "derived" OVN port representing the instance of l3dgw_port on
> > + * the "redirect-chassis". Othe

Re: [ovs-dev] [PATCH v3] ovn-controller: Support multiple gateway port on a distributed router

[Ben Pfaff]

Hi Guru.  Are you willing to take a look at this patch?

Thanks,

Ben.

On Fri, Feb 09, 2018 at 03:34:55PM +0800, Guoshuai Li wrote:
> The main application scenario of this patch is that the user flow wants to
> different destination addresses through different external networks.
> This scenario requires a distributed route to be associated with
> multiple external network logical switches.
> 
> Change l3dgw_port to l3dgw_ports in ovn_datapath,and change
> l3redirect_port to ovn_port. Then in a distributed router, the NAT
> logical flow table is generated based on the external IP lookup
> distributed router port, otherwise not generated. And LB is the same.
> 
> When the destination address of the packet is an external IP of the NAT rule,
> and the ingress port is not a gateway, it is necessary to route to the actual
> outgoing port.
> 
> Signed-off-by: Guoshuai Li 
> ---
>  ovn/northd/ovn-northd.8.xml |  22 +---
>  ovn/northd/ovn-northd.c | 273 
> +---
>  ovn/ovn-nb.xml  |  12 +-
>  tests/system-ovn.at | 162 +++---
>  4 files changed, 315 insertions(+), 154 deletions(-)
> 
> ---
> 
> I submitted this patch long ago, I think it might be useful,
> so resend it for more comments, thanks.
> 
> v1 -> v2:
>   1. rebase from master.
>   2. add test case.
> v2 -> v3:
>   fixed build failed.
> 
> ---
> 
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 6bc2dd6af..ff523614a 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -1732,16 +1732,6 @@ output;
>  
>
>  
> -  For distributed logical routers where one of the logical router
> -  ports specifies a redirect-chassis, a priority-300
> -  logical flow with match REGBIT_NAT_REDIRECT == 1 has
> -  actions ip.ttl--; next;.  The outport
> -  will be set later in the Gateway Redirect table.
> -
> -  
> -
> -  
> -
>IPv4 routing table.  For each route to IPv4 network N 
> with
>netmask M, on router port P with IP address
>A and Ethernet
> @@ -1827,12 +1817,12 @@ next;
>  
>
>  
> -  For distributed logical routers where one of the logical router
> -  ports specifies a redirect-chassis, a priority-200
> -  logical flow with match REGBIT_NAT_REDIRECT == 1 has
> -  actions eth.dst = E; next;, where
> -  E is the ethernet address of the router's distributed
> -  gateway port.
> +  For distributed logical routers where router port P
> +  specifies a redirect-chassis, a priority-200
> +  logical flow with match REGBIT_NAT_REDIRECT == 1
> +  and outport == P has actions
> +  eth.dst = E; next;, where E
> +  is the ethernet address of the router's distributed gateway port.
>  
>
>  
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4d95a3d9d..d38efcbed 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -418,12 +418,10 @@ struct ovn_datapath {
>  
>  /* OVN northd only needs to know about the logical router gateway port 
> for
>   * NAT on a distributed router.  This "distributed gateway port" is
> - * populated only when there is a "redirect-chassis" specified for one of
> - * the ports on the logical router.  Otherwise this will be NULL. */
> -struct ovn_port *l3dgw_port;
> -/* The "derived" OVN port representing the instance of l3dgw_port on
> - * the "redirect-chassis". */
> -struct ovn_port *l3redirect_port;
> + * populated only when there is a "redirect-chassis" specified for the
> + * ports on the logical router.  Otherwise this will be NULL. */
> +struct ovn_port **l3dgw_ports;
> +size_t n_l3dgw_ports;
>  struct ovn_port *localnet_port;
>  };
>  
> @@ -472,6 +470,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct 
> ovn_datapath *od)
>  free(od->ipam_info);
>  }
>  free(od->router_ports);
> +free(od->l3dgw_ports);
>  free(od);
>  }
>  }
> @@ -796,6 +795,10 @@ struct ovn_port {
>  bool derived; /* Indicates whether this is an additional port
> * derived from nbsp or nbrp. */
>  
> +/* The "derived" OVN port representing the instance of l3dgw_port on
> + * the "redirect-chassis". Otherwise this will be NULL. */
> +struct ovn_port *l3redirect_port;
> +
>  /* The port's peer:
>   *
>   * - A switch port S of type "router" has a router port R as a peer,
> @@ -1479,7 +1482,7 @@ join_logical_ports(struct northd_context *ctx,
>   "on L3 gateway router", nbrp->name);
>  continue;
>  }
> -if (od->l3dgw_port || od->l3redirect_port) {
> +if (op->l3redirect_port) {
>