[ovs-dev] northd: add support to make l3dgw ports fully distributed

This is used when traffic from HW VTEP goes to
routable networks and logical switch to which VTEP
logical port is attached also needs to support
distributed routing features such as NAT and others.

Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
---
 northd/northd.c |  8 +++++++-
 ovn-nb.xml      | 10 ++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

On Wed, Jul 20, 2022 at 12:15 AM Vladislav Odintsov <odivlad@gmail.com> wrote:
>
> This is used when traffic from HW VTEP goes to
> routable networks and logical switch to which VTEP
> logical port is attached also needs to support
> distributed routing features such as NAT and others.
>
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>

Thanks for the patch.

IMO,  the option "is_distributed" is very confusing.  By default, all
the logical routers (and router ports) are distributed in OVN unless
the logical router is a gateway router or if the router port has a
gateway chassis set.  And the default value of "is_distributed" is
false,
which makes it even more confusing.

I'd suggest renaming the option to something more relevant - which
indicates that this option is applicable only for gateway ports and
if the user/CMS wants to allow admission on all chassis.

But before that, my question is why does the CMS need to set the
gateway chassis for the router ports ? Can't it just not set it ?

Thanks
Numan

> ---
>  northd/northd.c |  8 +++++++-
>  ovn-nb.xml      | 10 ++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index d31cb1688..0be45e22a 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -1674,6 +1674,12 @@ is_cr_port(const struct ovn_port *op)
>      return op->l3dgw_port;
>  }
>
> +static bool
> +is_distributed(const struct ovn_port *op)
> +{
> +    return smap_get_bool(&op->nbrp->options, "distributed", false);
> +}
> +
>  static void
>  destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>  {
> @@ -10920,7 +10926,7 @@ build_adm_ctrl_flows_for_lrouter_port(
>          ds_clear(match);
>          ds_put_format(match, "eth.dst == %s && inport == %s",
>                        op->lrp_networks.ea_s, op->json_key);
> -        if (is_l3dgw_port(op)) {
> +        if (is_l3dgw_port(op) && !is_distributed(op)) {
>              /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>               * should only be received on the gateway chassis. */
>              ds_put_format(match, " && is_chassis_resident(%s)",
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index e26afd83c..d4b454791 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2976,6 +2976,16 @@
>            <ref column="options" key="gateway_mtu"/> option.
>          </p>
>        </column>
> +
> +      <column name="options" key="distributed" type='{"type": "boolean"}'>
> +        If <var>true</var>, indicates, that flow for current LRP in
> +        lr_in_admission must be installed on all chassis, even if it has
> +        associated chassisredirect port.
> +        Usable when traffic from HW VTEP goes to routable networks and logical
> +        switch to which VTEP logical port is attached also needs to support
> +        distributed routing features such as NAT, Load Balancing and others.
> +        Empty value (default) means false.
> +      </column>
>      </group>
>
>      <group title="Attachment">
> --
> 2.26.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Hi Numan,

thanks for the review.
My comments below.

Regards,
Vladislav Odintsov

> On 8 Aug 2022, at 03:24, Numan Siddique <numans@ovn.org> wrote:
> 
> On Wed, Jul 20, 2022 at 12:15 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote:
>> 
>> This is used when traffic from HW VTEP goes to
>> routable networks and logical switch to which VTEP
>> logical port is attached also needs to support
>> distributed routing features such as NAT and others.
>> 
>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>>
> 
> Thanks for the patch.
> 
> IMO,  the option "is_distributed" is very confusing.  By default, all
> the logical routers (and router ports) are distributed in OVN unless
> the logical router is a gateway router or if the router port has a
> gateway chassis set.  And the default value of "is_distributed" is
> false,
> which makes it even more confusing.
> 
> I'd suggest renaming the option to something more relevant - which
> indicates that this option is applicable only for gateway ports and
> if the user/CMS wants to allow admission on all chassis.
> 

I agree with your point about naming. Will think about better one.

> But before that, my question is why does the CMS need to set the
> gateway chassis for the router ports ? Can't it just not set it ?
> 

As I mentioned in ovn-nb manpage this is useful for GW LRPs, which are used in routed
HW VTEP-attached setups (LR with LS with LSP type==vtep).

Generally this option must be set for all LRPs, which are connected to LS with vtep LSP.
I didn’t want to grow up pipeline adding new flow to lr_in_admission, which has same
content as original flow plus match on reg0[14] and without is_chassis_redirect() part.

The initial problem is that user may want to create simple topology:

<vm1> - <ls1 192.168.0.0/24> - <LR> - <ls2 192.168.1.0/24> - <vm2>

And add VTEP LSP to ls2 (setting GW chassis to LRP in ls2).
If user wants to add GW services to this setup, VMs from ls2 will not be able to access
external network due to lr_in_admission rules.

Let me know if my explanation is clear or I should give more details.

So the proposed change allows a CMS to indicate that we such LRP may be not only limited
to its GW chassis, where CR port resides.
Technically this can be done automatically somehow in another way. Maybe add some helper
which checks if specified LRP’s LS has attached VTEP LSP and is_chassis_resident(%s) part
should be omitted.

What do you think?


> Thanks
> Numan
> 
>> ---
>> northd/northd.c |  8 +++++++-
>> ovn-nb.xml      | 10 ++++++++++
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/northd/northd.c b/northd/northd.c
>> index d31cb1688..0be45e22a 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -1674,6 +1674,12 @@ is_cr_port(const struct ovn_port *op)
>>     return op->l3dgw_port;
>> }
>> 
>> +static bool
>> +is_distributed(const struct ovn_port *op)
>> +{
>> +    return smap_get_bool(&op->nbrp->options, "distributed", false);
>> +}
>> +
>> static void
>> destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>> {
>> @@ -10920,7 +10926,7 @@ build_adm_ctrl_flows_for_lrouter_port(
>>         ds_clear(match);
>>         ds_put_format(match, "eth.dst == %s && inport == %s",
>>                       op->lrp_networks.ea_s, op->json_key);
>> -        if (is_l3dgw_port(op)) {
>> +        if (is_l3dgw_port(op) && !is_distributed(op)) {
>>             /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>>              * should only be received on the gateway chassis. */
>>             ds_put_format(match, " && is_chassis_resident(%s)",
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index e26afd83c..d4b454791 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -2976,6 +2976,16 @@
>>           <ref column="options" key="gateway_mtu"/> option.
>>         </p>
>>       </column>
>> +
>> +      <column name="options" key="distributed" type='{"type": "boolean"}'>
>> +        If <var>true</var>, indicates, that flow for current LRP in
>> +        lr_in_admission must be installed on all chassis, even if it has
>> +        associated chassisredirect port.
>> +        Usable when traffic from HW VTEP goes to routable networks and logical
>> +        switch to which VTEP logical port is attached also needs to support
>> +        distributed routing features such as NAT, Load Balancing and others.
>> +        Empty value (default) means false.
>> +      </column>
>>     </group>
>> 
>>     <group title="Attachment">
>> --
>> 2.26.3
>> 
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org <mailto:dev@openvswitch.org>
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>

On Mon, Aug 8, 2022 at 11:29 PM Vladislav Odintsov <odivlad@gmail.com> wrote:
>
> Hi Numan,
>
> thanks for the review.
> My comments below.
>
> Regards,
> Vladislav Odintsov
>
> > On 8 Aug 2022, at 03:24, Numan Siddique <numans@ovn.org> wrote:
> >
> > On Wed, Jul 20, 2022 at 12:15 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote:
> >>
> >> This is used when traffic from HW VTEP goes to
> >> routable networks and logical switch to which VTEP
> >> logical port is attached also needs to support
> >> distributed routing features such as NAT and others.
> >>
> >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>>
> >
> > Thanks for the patch.
> >
> > IMO,  the option "is_distributed" is very confusing.  By default, all
> > the logical routers (and router ports) are distributed in OVN unless
> > the logical router is a gateway router or if the router port has a
> > gateway chassis set.  And the default value of "is_distributed" is
> > false,
> > which makes it even more confusing.
> >
> > I'd suggest renaming the option to something more relevant - which
> > indicates that this option is applicable only for gateway ports and
> > if the user/CMS wants to allow admission on all chassis.
> >
>
> I agree with your point about naming. Will think about better one.

>
> > But before that, my question is why does the CMS need to set the
> > gateway chassis for the router ports ? Can't it just not set it ?
> >
>
> As I mentioned in ovn-nb manpage this is useful for GW LRPs, which are used in routed
> HW VTEP-attached setups (LR with LS with LSP type==vtep).
>
> Generally this option must be set for all LRPs, which are connected to LS with vtep LSP.
> I didn’t want to grow up pipeline adding new flow to lr_in_admission, which has same
> content as original flow plus match on reg0[14] and without is_chassis_redirect() part.
>
> The initial problem is that user may want to create simple topology:
>
> <vm1> - <ls1 192.168.0.0/24> - <LR> - <ls2 192.168.1.0/24> - <vm2>
>
> And add VTEP LSP to ls2 (setting GW chassis to LRP in ls2).
> If user wants to add GW services to this setup, VMs from ls2 will not be able to access
> external network due to lr_in_admission rules.
>
> Let me know if my explanation is clear or I should give more details.
>
> So the proposed change allows a CMS to indicate that we such LRP may be not only limited
> to its GW chassis, where CR port resides.
> Technically this can be done automatically somehow in another way. Maybe add some helper
> which checks if specified LRP’s LS has attached VTEP LSP and is_chassis_resident(%s) part
> should be omitted.
>
> What do you think?


Thanks for the explanation.  Seems like it's better to do this
automatically by checking if the
logical switch has vtep ports.  Maybe you can add "has_vtep_ports"
bool to the ovn_datapath struct ?
You need  to make sure that it is synced properly when a vtep port is
deleted.  If you find this to be
hard or if it is inefficient to do this, then an option seems ok to me.

Thanks
Numan


>
>
> > Thanks
> > Numan
> >
> >> ---
> >> northd/northd.c |  8 +++++++-
> >> ovn-nb.xml      | 10 ++++++++++
> >> 2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index d31cb1688..0be45e22a 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -1674,6 +1674,12 @@ is_cr_port(const struct ovn_port *op)
> >>     return op->l3dgw_port;
> >> }
> >>
> >> +static bool
> >> +is_distributed(const struct ovn_port *op)
> >> +{
> >> +    return smap_get_bool(&op->nbrp->options, "distributed", false);
> >> +}
> >> +
> >> static void
> >> destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
> >> {
> >> @@ -10920,7 +10926,7 @@ build_adm_ctrl_flows_for_lrouter_port(
> >>         ds_clear(match);
> >>         ds_put_format(match, "eth.dst == %s && inport == %s",
> >>                       op->lrp_networks.ea_s, op->json_key);
> >> -        if (is_l3dgw_port(op)) {
> >> +        if (is_l3dgw_port(op) && !is_distributed(op)) {
> >>             /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> >>              * should only be received on the gateway chassis. */
> >>             ds_put_format(match, " && is_chassis_resident(%s)",
> >> diff --git a/ovn-nb.xml b/ovn-nb.xml
> >> index e26afd83c..d4b454791 100644
> >> --- a/ovn-nb.xml
> >> +++ b/ovn-nb.xml
> >> @@ -2976,6 +2976,16 @@
> >>           <ref column="options" key="gateway_mtu"/> option.
> >>         </p>
> >>       </column>
> >> +
> >> +      <column name="options" key="distributed" type='{"type": "boolean"}'>
> >> +        If <var>true</var>, indicates, that flow for current LRP in
> >> +        lr_in_admission must be installed on all chassis, even if it has
> >> +        associated chassisredirect port.
> >> +        Usable when traffic from HW VTEP goes to routable networks and logical
> >> +        switch to which VTEP logical port is attached also needs to support
> >> +        distributed routing features such as NAT, Load Balancing and others.
> >> +        Empty value (default) means false.
> >> +      </column>
> >>     </group>
> >>
> >>     <group title="Attachment">
> >> --
> >> 2.26.3
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org <mailto:dev@openvswitch.org>
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hi Numan,

I’ve submitted a new version of this patch. It has a new subject as I’ve reworked its changes as you requested.
Also, I’ve added tests to check if has_vtep_lports property correctly set after vtep lport is removed from LS.
Let me know if this makes sense.

As I’ve posted an initial version of the patch before the soft freeze, is it still possible for this patch to be considered
for 22.09.0?

https://patchwork.ozlabs.org/project/ovn/patch/20220815141851.78904-1-odivlad@gmail.com/

Regards,
Vladislav Odintsov

> On 9 Aug 2022, at 04:54, Numan Siddique <numans@ovn.org> wrote:
> 
> On Mon, Aug 8, 2022 at 11:29 PM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote:
>> 
>> Hi Numan,
>> 
>> thanks for the review.
>> My comments below.
>> 
>> Regards,
>> Vladislav Odintsov
>> 
>>> On 8 Aug 2022, at 03:24, Numan Siddique <numans@ovn.org> wrote:
>>> 
>>> On Wed, Jul 20, 2022 at 12:15 AM Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>> wrote:
>>>> 
>>>> This is used when traffic from HW VTEP goes to
>>>> routable networks and logical switch to which VTEP
>>>> logical port is attached also needs to support
>>>> distributed routing features such as NAT and others.
>>>> 
>>>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com <mailto:odivlad@gmail.com>>
>>> 
>>> Thanks for the patch.
>>> 
>>> IMO,  the option "is_distributed" is very confusing.  By default, all
>>> the logical routers (and router ports) are distributed in OVN unless
>>> the logical router is a gateway router or if the router port has a
>>> gateway chassis set.  And the default value of "is_distributed" is
>>> false,
>>> which makes it even more confusing.
>>> 
>>> I'd suggest renaming the option to something more relevant - which
>>> indicates that this option is applicable only for gateway ports and
>>> if the user/CMS wants to allow admission on all chassis.
>>> 
>> 
>> I agree with your point about naming. Will think about better one.
> 
>> 
>>> But before that, my question is why does the CMS need to set the
>>> gateway chassis for the router ports ? Can't it just not set it ?
>>> 
>> 
>> As I mentioned in ovn-nb manpage this is useful for GW LRPs, which are used in routed
>> HW VTEP-attached setups (LR with LS with LSP type==vtep).
>> 
>> Generally this option must be set for all LRPs, which are connected to LS with vtep LSP.
>> I didn’t want to grow up pipeline adding new flow to lr_in_admission, which has same
>> content as original flow plus match on reg0[14] and without is_chassis_redirect() part.
>> 
>> The initial problem is that user may want to create simple topology:
>> 
>> <vm1> - <ls1 192.168.0.0/24> - <LR> - <ls2 192.168.1.0/24> - <vm2>
>> 
>> And add VTEP LSP to ls2 (setting GW chassis to LRP in ls2).
>> If user wants to add GW services to this setup, VMs from ls2 will not be able to access
>> external network due to lr_in_admission rules.
>> 
>> Let me know if my explanation is clear or I should give more details.
>> 
>> So the proposed change allows a CMS to indicate that we such LRP may be not only limited
>> to its GW chassis, where CR port resides.
>> Technically this can be done automatically somehow in another way. Maybe add some helper
>> which checks if specified LRP’s LS has attached VTEP LSP and is_chassis_resident(%s) part
>> should be omitted.
>> 
>> What do you think?
> 
> 
> Thanks for the explanation.  Seems like it's better to do this
> automatically by checking if the
> logical switch has vtep ports.  Maybe you can add "has_vtep_ports"
> bool to the ovn_datapath struct ?
> You need  to make sure that it is synced properly when a vtep port is
> deleted.  If you find this to be
> hard or if it is inefficient to do this, then an option seems ok to me.
> 
> Thanks
> Numan
> 
> 
>> 
>> 
>>> Thanks
>>> Numan
>>> 
>>>> ---
>>>> northd/northd.c |  8 +++++++-
>>>> ovn-nb.xml      | 10 ++++++++++
>>>> 2 files changed, 17 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/northd/northd.c b/northd/northd.c
>>>> index d31cb1688..0be45e22a 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -1674,6 +1674,12 @@ is_cr_port(const struct ovn_port *op)
>>>>    return op->l3dgw_port;
>>>> }
>>>> 
>>>> +static bool
>>>> +is_distributed(const struct ovn_port *op)
>>>> +{
>>>> +    return smap_get_bool(&op->nbrp->options, "distributed", false);
>>>> +}
>>>> +
>>>> static void
>>>> destroy_routable_addresses(struct ovn_port_routable_addresses *ra)
>>>> {
>>>> @@ -10920,7 +10926,7 @@ build_adm_ctrl_flows_for_lrouter_port(
>>>>        ds_clear(match);
>>>>        ds_put_format(match, "eth.dst == %s && inport == %s",
>>>>                      op->lrp_networks.ea_s, op->json_key);
>>>> -        if (is_l3dgw_port(op)) {
>>>> +        if (is_l3dgw_port(op) && !is_distributed(op)) {
>>>>            /* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>>>>             * should only be received on the gateway chassis. */
>>>>            ds_put_format(match, " && is_chassis_resident(%s)",
>>>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>>>> index e26afd83c..d4b454791 100644
>>>> --- a/ovn-nb.xml
>>>> +++ b/ovn-nb.xml
>>>> @@ -2976,6 +2976,16 @@
>>>>          <ref column="options" key="gateway_mtu"/> option.
>>>>        </p>
>>>>      </column>
>>>> +
>>>> +      <column name="options" key="distributed" type='{"type": "boolean"}'>
>>>> +        If <var>true</var>, indicates, that flow for current LRP in
>>>> +        lr_in_admission must be installed on all chassis, even if it has
>>>> +        associated chassisredirect port.
>>>> +        Usable when traffic from HW VTEP goes to routable networks and logical
>>>> +        switch to which VTEP logical port is attached also needs to support
>>>> +        distributed routing features such as NAT, Load Balancing and others.
>>>> +        Empty value (default) means false.
>>>> +      </column>
>>>>    </group>
>>>> 
>>>>    <group title="Attachment">
>>>> --
>>>> 2.26.3
>>>> 
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev@openvswitch.org <mailto:dev@openvswitch.org>
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org <mailto:dev@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>