Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-19 Thread Vladislav Odintsov 


Regards,
Vladislav Odintsov

> On 19 Aug 2022, at 04:05, Han Zhou  wrote:
> 
> 
> 
> On Thu, Aug 18, 2022 at 12:50 PM Vladislav Odintsov  > wrote:
> 
> 
> Regards,
> Vladislav Odintsov
> 
>> On 18 Aug 2022, at 20:07, Han Zhou mailto:hz...@ovn.org>> 
>> wrote:
>> 
>> 
>> 
>> On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov > > wrote:
>> Hi Han,
>> 
>> As I could understand, admission control flows were introduced for 
>> "gateway/outside" LSs, with GW LRPs.
>> The scenario, which was covered with this change shouldn’t overlap with a 
>> "gateway/outside" LS scenario:
>> User creates an LS, adds normal VIF ports (VMs) to LS and additionally he 
>> may want to attach vtep lport to this LS to extend L2 domain outside of the 
>> OVN with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this 
>> could be the only one problem place.
>> 
>> Thanks for explaining, but I am still confused. Here is your original 
>> description of the problem from the earlier thread:
>> ---
>> The initial problem is that user may want to create simple topology:
>> 
>>  - http://192.168.0.0/24>> -  - > 192.168.1.0/24 > - 
>> 
>> 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.
>> ---
>> So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set, 
>> which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF 
>> (VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis, 
>> but also on any HVs, right? Maybe you are saying, you don't connect a 
>> localnet port to the LS2, and LS2 is an overlay network, instead of underlay?
> 
> Yes this is exactly what I mean.
> 
>> But if that's the case why would you set the gateway-chassis to it at all?
> 
> This is the only way how to enable routing with HW VTEP gateway (which is a 
> L2 service).
> You can check this out [1] to see more details about GW chassis for the VTEP 
> L3 scenario.
> 
> I see. Sorry that I didn't know the usage of gateway chassis in this way, but 
> for the patch [1], I wonder if the VTEP chassis (where the 
> ovn-controller-vtep is running) should be responsible for replying ARP 
> requests for the LRP IP from the LS's ARP responder pipeline, instead of 
> requiring a DGP to perform this? I didn't expect DGP to be required for VTEP 
> to work.

ovn-controller-vtep is only responsible for converting ovn-sb entities to 
hardware_vtep schema for futher processing by thirdparies like cumulus vtep, 
vtep emulator by ovs, arista vtep and so on. It doesn’t directly participate in 
datapath processing.

> 
> But anyway, this answers my question to the current patch. Thanks!
> 

Glad to help you!

> Han
> 
> 1: 
> https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307
>  
> 
>> 
>> Thanks,
>> han
>>  
>> And this is_chassis_redirect() check is not added to flow only if VTEP lport 
>> is in associated LS. All other cases left untouched.
>> 
>> Let me know if you have any questions.
>> 
>> Regards,
>> Vladislav Odintsov
>> 
>>> On 18 Aug 2022, at 18:45, Han Zhou mailto:hz...@ovn.org>> 
>>> wrote:
>>> 
>>> 
>>> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav >> > wrote:
>>> >
>>> > Thanks Numan.
>>> >
>>> > regards,
>>> > Vladislav Odintsov
>>> >
>>> > > On 16 Aug 2022, at 04:56, Numan Siddique >> > > > wrote:
>>> > >
>>> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov >> > > > wrote:
>>> > >>
>>> > >> If LRP's logical switch has attached LSP of vtep type, the
>>> > >> is_chassis_resident() part is not added to lflow to allow traffic
>>> > >> originated from logical switch to reach LR services (LBs, NAT).
>>> > >>
>>> 
>>> Sorry that I just noticed this and have a question here. I think we had 
>>> is_chassis_resident() for the admission control flows for a reason. I don't 
>>> remember all details, but probably one of the reasons is to prevent 
>>> underlay BUM packets to be handled by multiple chassises?
>>> If we disable that check, would it create problems? Is VTEP attached LS 
>>> immune to those problems?
>>> 
>>> Thanks,
>>> Han
>>>  
>>> >
>>> > >> Signed-off-by: Vladislav Odintsov >> > >> >
>>> > >
>>> > > Thanks for v2 and for the test case.
>>> > > Applied the patch to main.
>>> > >
>>> > > Numan
>>> > >
>>> > >
>>> > >> ---
>>> > >> This is a continuation from [1] as a v2 edition after Numan's review.
>>> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing 
>>> > >> option
>>> > >>  'is_distributed'
>>> > >> - added related tests
>>> > >> - updated ovn-northd flows docs
>>> > >>
>>> > >> 1: 
>>> > >>

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-18 Thread Han Zhou 
On Thu, Aug 18, 2022 at 12:50 PM Vladislav Odintsov 
wrote:

>
>
> Regards,
> Vladislav Odintsov
>
> On 18 Aug 2022, at 20:07, Han Zhou  wrote:
>
>
>
> On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov 
> wrote:
>
>> Hi Han,
>>
>> As I could understand, admission control flows were introduced for
>> "gateway/outside" LSs, with GW LRPs.
>> The scenario, which was covered with this change shouldn’t overlap with a
>> "gateway/outside" LS scenario:
>> User creates an LS, adds normal VIF ports (VMs) to LS and additionally he
>> may want to attach vtep lport to this LS to extend L2 domain outside of the
>> OVN with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this
>> could be the only one problem place.
>>
>
> Thanks for explaining, but I am still confused. Here is your original
> description of the problem from the earlier thread:
> ---
> The initial problem is that user may want to create simple topology:
>
>  -  -  -  - 
>
> 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.
> ---
> So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set,
> which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF
> (VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis,
> but also on any HVs, right? Maybe you are saying, you don't connect a
> localnet port to the LS2, and LS2 is an overlay network, instead of
> underlay?
>
>
> Yes this is exactly what I mean.
>
> But if that's the case why would you set the gateway-chassis to it at all?
>
>
> This is the only way how to enable routing with HW VTEP gateway (which is
> a L2 service).
> You can check this out [1] to see more details about GW chassis for the
> VTEP L3 scenario.
>

I see. Sorry that I didn't know the usage of gateway chassis in this way,
but for the patch [1], I wonder if the VTEP chassis (where the
ovn-controller-vtep is running) should be responsible for replying ARP
requests for the LRP IP from the LS's ARP responder pipeline, instead of
requiring a DGP to perform this? I didn't expect DGP to be required for
VTEP to work.

But anyway, this answers my question to the current patch. Thanks!

Han

>
> 1:
> https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307
>
>
> Thanks,
> han
>
>
>> And this is_chassis_redirect() check is not added to flow only if VTEP
>> lport is in associated LS. All other cases left untouched.
>>
>> Let me know if you have any questions.
>>
>> Regards,
>> Vladislav Odintsov
>>
>> On 18 Aug 2022, at 18:45, Han Zhou  wrote:
>>
>>
>> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav 
>> wrote:
>> >
>> > Thanks Numan.
>> >
>> > regards,
>> > Vladislav Odintsov
>> >
>> > > On 16 Aug 2022, at 04:56, Numan Siddique  wrote:
>> > >
>> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <
>> odiv...@gmail.com> wrote:
>> > >>
>> > >> If LRP's logical switch has attached LSP of vtep type, the
>> > >> is_chassis_resident() part is not added to lflow to allow traffic
>> > >> originated from logical switch to reach LR services (LBs, NAT).
>> > >>
>>
>> Sorry that I just noticed this and have a question here. I think we had
>> is_chassis_resident() for the admission control flows for a reason. I don't
>> remember all details, but probably one of the reasons is to prevent
>> underlay BUM packets to be handled by multiple chassises?
>> If we disable that check, would it create problems? Is VTEP attached LS
>> immune to those problems?
>>
>> Thanks,
>> Han
>>
>> >
>> > >> Signed-off-by: Vladislav Odintsov 
>> > >
>> > > Thanks for v2 and for the test case.
>> > > Applied the patch to main.
>> > >
>> > > Numan
>> > >
>> > >
>> > >> ---
>> > >> This is a continuation from [1] as a v2 edition after Numan's review.
>> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing
>> option
>> > >>  'is_distributed'
>> > >> - added related tests
>> > >> - updated ovn-northd flows docs
>> > >>
>> > >> 1:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html
>> > >> ---
>> > >> northd/northd.c | 33 +---
>> > >> northd/ovn-northd.8.xml |  4 +++
>> > >> tests/ovn-northd.at | 69
>> +
>> > >> 3 files changed, 102 insertions(+), 4 deletions(-)
>> > >>
>> > >> diff --git a/northd/northd.c b/northd/northd.c
>> > >> index facd41a59..b1e9ffc87 100644
>> > >> --- a/northd/northd.c
>> > >> +++ b/northd/northd.c
>> > >> @@ -637,6 +637,7 @@ struct ovn_datapath {
>> > >> bool has_lb_vip;
>> > >> bool has_unknown;
>> > >> bool has_acls;
>> > >> +bool has_vtep_lports;
>> > >>
>> > >> /* IPAM data. */
>> > >> struct ipam_info ipam_info;
>> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct
>> nbrec_logical_switch_port *nbsp)
>> > >> return !strcmp(nbsp->type, "localnet");
>> > >> }
>> > >>
>> > >> +static

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-18 Thread Vladislav Odintsov 


Regards,
Vladislav Odintsov

> On 18 Aug 2022, at 20:07, Han Zhou  wrote:
> 
> 
> 
> On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov  > wrote:
> Hi Han,
> 
> As I could understand, admission control flows were introduced for 
> "gateway/outside" LSs, with GW LRPs.
> The scenario, which was covered with this change shouldn’t overlap with a 
> "gateway/outside" LS scenario:
> User creates an LS, adds normal VIF ports (VMs) to LS and additionally he may 
> want to attach vtep lport to this LS to extend L2 domain outside of the OVN 
> with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this could 
> be the only one problem place.
> 
> Thanks for explaining, but I am still confused. Here is your original 
> description of the problem from the earlier thread:
> ---
> The initial problem is that user may want to create simple topology:
> 
>  - http://192.168.0.0/24>> -  -  192.168.1.0/24 > - 
> 
> 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.
> ---
> So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set, 
> which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF 
> (VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis, 
> but also on any HVs, right? Maybe you are saying, you don't connect a 
> localnet port to the LS2, and LS2 is an overlay network, instead of underlay?

Yes this is exactly what I mean.

> But if that's the case why would you set the gateway-chassis to it at all?

This is the only way how to enable routing with HW VTEP gateway (which is a L2 
service).
You can check this out [1] to see more details about GW chassis for the VTEP L3 
scenario.

1: 
https://github.com/ovn-org/ovn/commit/4e90bcf55c2ef1ab9940836455e4bfe36e57f307

> 
> Thanks,
> han
>  
> And this is_chassis_redirect() check is not added to flow only if VTEP lport 
> is in associated LS. All other cases left untouched.
> 
> Let me know if you have any questions.
> 
> Regards,
> Vladislav Odintsov
> 
>> On 18 Aug 2022, at 18:45, Han Zhou mailto:hz...@ovn.org>> 
>> wrote:
>> 
>> 
>> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav > > wrote:
>> >
>> > Thanks Numan.
>> >
>> > regards,
>> > Vladislav Odintsov
>> >
>> > > On 16 Aug 2022, at 04:56, Numan Siddique > > > > wrote:
>> > >
>> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov > > > > wrote:
>> > >>
>> > >> If LRP's logical switch has attached LSP of vtep type, the
>> > >> is_chassis_resident() part is not added to lflow to allow traffic
>> > >> originated from logical switch to reach LR services (LBs, NAT).
>> > >>
>> 
>> Sorry that I just noticed this and have a question here. I think we had 
>> is_chassis_resident() for the admission control flows for a reason. I don't 
>> remember all details, but probably one of the reasons is to prevent underlay 
>> BUM packets to be handled by multiple chassises?
>> If we disable that check, would it create problems? Is VTEP attached LS 
>> immune to those problems?
>> 
>> Thanks,
>> Han
>>  
>> >
>> > >> Signed-off-by: Vladislav Odintsov > > >> >
>> > >
>> > > Thanks for v2 and for the test case.
>> > > Applied the patch to main.
>> > >
>> > > Numan
>> > >
>> > >
>> > >> ---
>> > >> This is a continuation from [1] as a v2 edition after Numan's review.
>> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing option
>> > >>  'is_distributed'
>> > >> - added related tests
>> > >> - updated ovn-northd flows docs
>> > >>
>> > >> 1: 
>> > >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html 
>> > >> 
>> > >> ---
>> > >> northd/northd.c | 33 +---
>> > >> northd/ovn-northd.8.xml |  4 +++
>> > >> tests/ovn-northd.at  | 69 
>> > >> +
>> > >> 3 files changed, 102 insertions(+), 4 deletions(-)
>> > >>
>> > >> diff --git a/northd/northd.c b/northd/northd.c
>> > >> index facd41a59..b1e9ffc87 100644
>> > >> --- a/northd/northd.c
>> > >> +++ b/northd/northd.c
>> > >> @@ -637,6 +637,7 @@ struct ovn_datapath {
>> > >> bool has_lb_vip;
>> > >> bool has_unknown;
>> > >> bool has_acls;
>> > >> +bool has_vtep_lports;
>> > >>
>> > >> /* IPAM data. */
>> > >> struct ipam_info ipam_info;
>> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct 
>> > >> nbrec_logical_switch_port *nbsp)
>> > >> return !strcmp(nbsp->type, "localnet");
>> > >> }
>> > >>
>> > >> +static bool
>> > >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
>> > >> +{
>> > >> +return !strcmp(nbsp->type, "vtep");
>> > >> +}
>> > >> +
>> > >> static bool
>> > >>

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-18 Thread Han Zhou 
On Thu, Aug 18, 2022 at 8:54 AM Vladislav Odintsov 
wrote:

> Hi Han,
>
> As I could understand, admission control flows were introduced for
> "gateway/outside" LSs, with GW LRPs.
> The scenario, which was covered with this change shouldn’t overlap with a
> "gateway/outside" LS scenario:
> User creates an LS, adds normal VIF ports (VMs) to LS and additionally he
> may want to attach vtep lport to this LS to extend L2 domain outside of the
> OVN with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this
> could be the only one problem place.
>

Thanks for explaining, but I am still confused. Here is your original
description of the problem from the earlier thread:
---
The initial problem is that user may want to create simple topology:

 -  -  -  - 

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.
---
So, the LRP to LS2 (Let's call it LRP-LS2) is the one with GW chassis set,
which means it is L3 GW LRP (DGP), right? LS2 has a DGP, a VTEP, and a VIF
(VM2), and you want to allow traffic to LRP-LS2 not only on the GW chassis,
but also on any HVs, right? Maybe you are saying, you don't connect a
localnet port to the LS2, and LS2 is an overlay network, instead of
underlay? But if that's the case why would you set the gateway-chassis to
it at all?

Thanks,
han


> And this is_chassis_redirect() check is not added to flow only if VTEP
> lport is in associated LS. All other cases left untouched.
>
> Let me know if you have any questions.
>
> Regards,
> Vladislav Odintsov
>
> On 18 Aug 2022, at 18:45, Han Zhou  wrote:
>
>
> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav 
> wrote:
> >
> > Thanks Numan.
> >
> > regards,
> > Vladislav Odintsov
> >
> > > On 16 Aug 2022, at 04:56, Numan Siddique  wrote:
> > >
> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov <
> odiv...@gmail.com> wrote:
> > >>
> > >> If LRP's logical switch has attached LSP of vtep type, the
> > >> is_chassis_resident() part is not added to lflow to allow traffic
> > >> originated from logical switch to reach LR services (LBs, NAT).
> > >>
>
> Sorry that I just noticed this and have a question here. I think we had
> is_chassis_resident() for the admission control flows for a reason. I don't
> remember all details, but probably one of the reasons is to prevent
> underlay BUM packets to be handled by multiple chassises?
> If we disable that check, would it create problems? Is VTEP attached LS
> immune to those problems?
>
> Thanks,
> Han
>
> >
> > >> Signed-off-by: Vladislav Odintsov 
> > >
> > > Thanks for v2 and for the test case.
> > > Applied the patch to main.
> > >
> > > Numan
> > >
> > >
> > >> ---
> > >> This is a continuation from [1] as a v2 edition after Numan's review.
> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing
> option
> > >>  'is_distributed'
> > >> - added related tests
> > >> - updated ovn-northd flows docs
> > >>
> > >> 1:
> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html
> > >> ---
> > >> northd/northd.c | 33 +---
> > >> northd/ovn-northd.8.xml |  4 +++
> > >> tests/ovn-northd.at | 69
> +
> > >> 3 files changed, 102 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/northd/northd.c b/northd/northd.c
> > >> index facd41a59..b1e9ffc87 100644
> > >> --- a/northd/northd.c
> > >> +++ b/northd/northd.c
> > >> @@ -637,6 +637,7 @@ struct ovn_datapath {
> > >> bool has_lb_vip;
> > >> bool has_unknown;
> > >> bool has_acls;
> > >> +bool has_vtep_lports;
> > >>
> > >> /* IPAM data. */
> > >> struct ipam_info ipam_info;
> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct
> nbrec_logical_switch_port *nbsp)
> > >> return !strcmp(nbsp->type, "localnet");
> > >> }
> > >>
> > >> +static bool
> > >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
> > >> +{
> > >> +return !strcmp(nbsp->type, "vtep");
> > >> +}
> > >> +
> > >> static bool
> > >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
> > >> {
> > >> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input
> *input_data,
> > >>od->localnet_ports[od->n_localnet_ports++] = op;
> > >> }
> > >>
> > >> +if (lsp_is_vtep(nbsp)) {
> > >> +od->has_vtep_lports = true;
> > >> +}
> > >> +
> > >> op->lsp_addrs
> > >> = xmalloc(sizeof *op->lsp_addrs *
> nbsp->n_addresses);
> > >> for (size_t j = 0; j < nbsp->n_addresses; j++) {
> > >> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op,
> struct hmap *lflows,
> > >> ds_put_format(actions, "set_queue(%s); ", queue_id);
> > >> }
> > >>
> > >> -if (!strcmp(op->nbsp->type, "vtep")) {
> > >> +if (lsp_is_vtep(op->nbsp)) {
> >

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-18 Thread Vladislav Odintsov 
Hi Han,

As I could understand, admission control flows were introduced for 
"gateway/outside" LSs, with GW LRPs.
The scenario, which was covered with this change shouldn’t overlap with a 
"gateway/outside" LS scenario:
User creates an LS, adds normal VIF ports (VMs) to LS and additionally he may 
want to attach vtep lport to this LS to extend L2 domain outside of the OVN 
with VTEP. I can’t imaging user wants to have a L3 GW LRP here - this could be 
the only one problem place.
And this is_chassis_redirect() check is not added to flow only if VTEP lport is 
in associated LS. All other cases left untouched.

Let me know if you have any questions.

Regards,
Vladislav Odintsov

> On 18 Aug 2022, at 18:45, Han Zhou  wrote:
> 
> 
> On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav  > wrote:
> >
> > Thanks Numan.
> >
> > regards,
> > Vladislav Odintsov
> >
> > > On 16 Aug 2022, at 04:56, Numan Siddique  > > > wrote:
> > >
> > > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov  > > > wrote:
> > >>
> > >> If LRP's logical switch has attached LSP of vtep type, the
> > >> is_chassis_resident() part is not added to lflow to allow traffic
> > >> originated from logical switch to reach LR services (LBs, NAT).
> > >>
> 
> Sorry that I just noticed this and have a question here. I think we had 
> is_chassis_resident() for the admission control flows for a reason. I don't 
> remember all details, but probably one of the reasons is to prevent underlay 
> BUM packets to be handled by multiple chassises?
> If we disable that check, would it create problems? Is VTEP attached LS 
> immune to those problems?
> 
> Thanks,
> Han
>  
> >
> > >> Signed-off-by: Vladislav Odintsov  > >> >
> > >
> > > Thanks for v2 and for the test case.
> > > Applied the patch to main.
> > >
> > > Numan
> > >
> > >
> > >> ---
> > >> This is a continuation from [1] as a v2 edition after Numan's review.
> > >> - reworked to use od->has_vtep_lports and removed lrp's confusing option
> > >>  'is_distributed'
> > >> - added related tests
> > >> - updated ovn-northd flows docs
> > >>
> > >> 1: 
> > >> https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html 
> > >> 
> > >> ---
> > >> northd/northd.c | 33 +---
> > >> northd/ovn-northd.8.xml |  4 +++
> > >> tests/ovn-northd.at  | 69 
> > >> +
> > >> 3 files changed, 102 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/northd/northd.c b/northd/northd.c
> > >> index facd41a59..b1e9ffc87 100644
> > >> --- a/northd/northd.c
> > >> +++ b/northd/northd.c
> > >> @@ -637,6 +637,7 @@ struct ovn_datapath {
> > >> bool has_lb_vip;
> > >> bool has_unknown;
> > >> bool has_acls;
> > >> +bool has_vtep_lports;
> > >>
> > >> /* IPAM data. */
> > >> struct ipam_info ipam_info;
> > >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct 
> > >> nbrec_logical_switch_port *nbsp)
> > >> return !strcmp(nbsp->type, "localnet");
> > >> }
> > >>
> > >> +static bool
> > >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
> > >> +{
> > >> +return !strcmp(nbsp->type, "vtep");
> > >> +}
> > >> +
> > >> static bool
> > >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
> > >> {
> > >> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input 
> > >> *input_data,
> > >>od->localnet_ports[od->n_localnet_ports++] = op;
> > >> }
> > >>
> > >> +if (lsp_is_vtep(nbsp)) {
> > >> +od->has_vtep_lports = true;
> > >> +}
> > >> +
> > >> op->lsp_addrs
> > >> = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
> > >> for (size_t j = 0; j < nbsp->n_addresses; j++) {
> > >> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, 
> > >> struct hmap *lflows,
> > >> ds_put_format(actions, "set_queue(%s); ", queue_id);
> > >> }
> > >>
> > >> -if (!strcmp(op->nbsp->type, "vtep")) {
> > >> +if (lsp_is_vtep(op->nbsp)) {
> > >> ds_put_format(actions, REGBIT_FROM_RAMP" = 1; ");
> > >> ds_put_format(actions, "next(pipeline=ingress, table=%d);",
> > >>   ovn_stage_get_table(S_SWITCH_IN_HAIRPIN));
> > >> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows, 
> > >> struct ovn_port *op,
> > >> va_end(extra_actions_args);
> > >> }
> > >>
> > >> +static bool
> > >> +consider_l3dwg_port_is_centralized(struct ovn_port *op)
> > >> +{
> > >> +if (op->peer && op->peer->od->has_vtep_lports) {
> > >> +return false;
> > >> +}
> > >> +
> > >> +if (is_l3dgw_port(op)) {
> > >> +/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> > >> + * should only be

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-18 Thread Han Zhou 
On Mon, Aug 15, 2022 at 10:01 PM Odintsov Vladislav 
wrote:
>
> Thanks Numan.
>
> regards,
> Vladislav Odintsov
>
> > On 16 Aug 2022, at 04:56, Numan Siddique  wrote:
> >
> > On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov 
wrote:
> >>
> >> If LRP's logical switch has attached LSP of vtep type, the
> >> is_chassis_resident() part is not added to lflow to allow traffic
> >> originated from logical switch to reach LR services (LBs, NAT).
> >>

Sorry that I just noticed this and have a question here. I think we had
is_chassis_resident() for the admission control flows for a reason. I don't
remember all details, but probably one of the reasons is to prevent
underlay BUM packets to be handled by multiple chassises?
If we disable that check, would it create problems? Is VTEP attached LS
immune to those problems?

Thanks,
Han

>
> >> Signed-off-by: Vladislav Odintsov 
> >
> > Thanks for v2 and for the test case.
> > Applied the patch to main.
> >
> > Numan
> >
> >
> >> ---
> >> This is a continuation from [1] as a v2 edition after Numan's review.
> >> - reworked to use od->has_vtep_lports and removed lrp's confusing
option
> >>  'is_distributed'
> >> - added related tests
> >> - updated ovn-northd flows docs
> >>
> >> 1:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html
> >> ---
> >> northd/northd.c | 33 +---
> >> northd/ovn-northd.8.xml |  4 +++
> >> tests/ovn-northd.at | 69 +
> >> 3 files changed, 102 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/northd/northd.c b/northd/northd.c
> >> index facd41a59..b1e9ffc87 100644
> >> --- a/northd/northd.c
> >> +++ b/northd/northd.c
> >> @@ -637,6 +637,7 @@ struct ovn_datapath {
> >> bool has_lb_vip;
> >> bool has_unknown;
> >> bool has_acls;
> >> +bool has_vtep_lports;
> >>
> >> /* IPAM data. */
> >> struct ipam_info ipam_info;
> >> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct
nbrec_logical_switch_port *nbsp)
> >> return !strcmp(nbsp->type, "localnet");
> >> }
> >>
> >> +static bool
> >> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
> >> +{
> >> +return !strcmp(nbsp->type, "vtep");
> >> +}
> >> +
> >> static bool
> >> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
> >> {
> >> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input
*input_data,
> >>od->localnet_ports[od->n_localnet_ports++] = op;
> >> }
> >>
> >> +if (lsp_is_vtep(nbsp)) {
> >> +od->has_vtep_lports = true;
> >> +}
> >> +
> >> op->lsp_addrs
> >> = xmalloc(sizeof *op->lsp_addrs *
nbsp->n_addresses);
> >> for (size_t j = 0; j < nbsp->n_addresses; j++) {
> >> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op,
struct hmap *lflows,
> >> ds_put_format(actions, "set_queue(%s); ", queue_id);
> >> }
> >>
> >> -if (!strcmp(op->nbsp->type, "vtep")) {
> >> +if (lsp_is_vtep(op->nbsp)) {
> >> ds_put_format(actions, REGBIT_FROM_RAMP" = 1; ");
> >> ds_put_format(actions, "next(pipeline=ingress, table=%d);",
> >>   ovn_stage_get_table(S_SWITCH_IN_HAIRPIN));
> >> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows,
struct ovn_port *op,
> >> va_end(extra_actions_args);
> >> }
> >>
> >> +static bool
> >> +consider_l3dwg_port_is_centralized(struct ovn_port *op)
> >> +{
> >> +if (op->peer && op->peer->od->has_vtep_lports) {
> >> +return false;
> >> +}
> >> +
> >> +if (is_l3dgw_port(op)) {
> >> +/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> >> + * should only be received on the gateway chassis. */
> >> +return true;
> >> +}
> >> +
> >> +return false;
> >> +}
> >> +
> >> /* Logical router ingress Table 0: L2 Admission Control
> >>  * This table drops packets that the router shouldn’t see at all based
> >>  * on their Ethernet headers.
> >> @@ -10930,9 +10957,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)) {
> >> -/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> >> - * should only be received on the gateway chassis. */
> >> +if (consider_l3dwg_port_is_centralized(op)) {
> >> ds_put_format(match, " && is_chassis_resident(%s)",
> >>   op->cr_port->json_key);
> >> }
> >> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> >> index ff21c0737..9b6459d9e 100644
> >> --- a/northd/ovn-northd.8.xml
> >> +++ b/northd/ovn-northd.8.xml
> >> @@ -2114,6 +2114,10 @@ output;
> >>   gateway chassis), the above flow matching
> >>   eth.dst == E is only programmed on
> >>

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-15 Thread Odintsov Vladislav 
Thanks Numan.

regards,
Vladislav Odintsov

> On 16 Aug 2022, at 04:56, Numan Siddique  wrote:
> 
> On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov  
> wrote:
>> 
>> If LRP's logical switch has attached LSP of vtep type, the
>> is_chassis_resident() part is not added to lflow to allow traffic
>> originated from logical switch to reach LR services (LBs, NAT).
>> 
>> Signed-off-by: Vladislav Odintsov 
> 
> Thanks for v2 and for the test case.
> Applied the patch to main.
> 
> Numan
> 
> 
>> ---
>> This is a continuation from [1] as a v2 edition after Numan's review.
>> - reworked to use od->has_vtep_lports and removed lrp's confusing option
>>  'is_distributed'
>> - added related tests
>> - updated ovn-northd flows docs
>> 
>> 1: https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html
>> ---
>> northd/northd.c | 33 +---
>> northd/ovn-northd.8.xml |  4 +++
>> tests/ovn-northd.at | 69 +
>> 3 files changed, 102 insertions(+), 4 deletions(-)
>> 
>> diff --git a/northd/northd.c b/northd/northd.c
>> index facd41a59..b1e9ffc87 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -637,6 +637,7 @@ struct ovn_datapath {
>> bool has_lb_vip;
>> bool has_unknown;
>> bool has_acls;
>> +bool has_vtep_lports;
>> 
>> /* IPAM data. */
>> struct ipam_info ipam_info;
>> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct 
>> nbrec_logical_switch_port *nbsp)
>> return !strcmp(nbsp->type, "localnet");
>> }
>> 
>> +static bool
>> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
>> +{
>> +return !strcmp(nbsp->type, "vtep");
>> +}
>> +
>> static bool
>> localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
>> {
>> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input *input_data,
>>od->localnet_ports[od->n_localnet_ports++] = op;
>> }
>> 
>> +if (lsp_is_vtep(nbsp)) {
>> +od->has_vtep_lports = true;
>> +}
>> +
>> op->lsp_addrs
>> = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
>> for (size_t j = 0; j < nbsp->n_addresses; j++) {
>> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct 
>> hmap *lflows,
>> ds_put_format(actions, "set_queue(%s); ", queue_id);
>> }
>> 
>> -if (!strcmp(op->nbsp->type, "vtep")) {
>> +if (lsp_is_vtep(op->nbsp)) {
>> ds_put_format(actions, REGBIT_FROM_RAMP" = 1; ");
>> ds_put_format(actions, "next(pipeline=ingress, table=%d);",
>>   ovn_stage_get_table(S_SWITCH_IN_HAIRPIN));
>> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows, struct 
>> ovn_port *op,
>> va_end(extra_actions_args);
>> }
>> 
>> +static bool
>> +consider_l3dwg_port_is_centralized(struct ovn_port *op)
>> +{
>> +if (op->peer && op->peer->od->has_vtep_lports) {
>> +return false;
>> +}
>> +
>> +if (is_l3dgw_port(op)) {
>> +/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>> + * should only be received on the gateway chassis. */
>> +return true;
>> +}
>> +
>> +return false;
>> +}
>> +
>> /* Logical router ingress Table 0: L2 Admission Control
>>  * This table drops packets that the router shouldn’t see at all based
>>  * on their Ethernet headers.
>> @@ -10930,9 +10957,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)) {
>> -/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
>> - * should only be received on the gateway chassis. */
>> +if (consider_l3dwg_port_is_centralized(op)) {
>> ds_put_format(match, " && is_chassis_resident(%s)",
>>   op->cr_port->json_key);
>> }
>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>> index ff21c0737..9b6459d9e 100644
>> --- a/northd/ovn-northd.8.xml
>> +++ b/northd/ovn-northd.8.xml
>> @@ -2114,6 +2114,10 @@ output;
>>   gateway chassis), the above flow matching
>>   eth.dst == E is only programmed on
>>   the gateway port instance on the gateway chassis.
>> +  If LRP's logical switch has attached LSP of vtep 
>> type,
>> +  the is_chassis_resident() part is not added to lflow 
>> to
>> +  allow traffic originated from logical switch to reach LR services
>> +  (LBs, NAT).
>> 
>> 
>> 
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index 5b5eeb0ee..3ffa39419 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -5747,6 +5747,75 @@ AT_CHECK([grep lr_in_gw_redirect lrflows | grep cr-DR 
>> | sed 's/table=../table=??
>> AT_CLEANUP
>> ])
>> 
>> +OVN_FOR_EACH_NORTHD([
>>

Re: [ovs-dev] [PATCH ovn] northd: support vtep LSP-attached LS to use L3 services

2022-08-15 Thread Numan Siddique 
On Tue, Aug 16, 2022 at 12:19 AM Vladislav Odintsov  wrote:
>
> If LRP's logical switch has attached LSP of vtep type, the
> is_chassis_resident() part is not added to lflow to allow traffic
> originated from logical switch to reach LR services (LBs, NAT).
>
> Signed-off-by: Vladislav Odintsov 

Thanks for v2 and for the test case.
Applied the patch to main.

Numan


> ---
> This is a continuation from [1] as a v2 edition after Numan's review.
> - reworked to use od->has_vtep_lports and removed lrp's confusing option
>   'is_distributed'
> - added related tests
> - updated ovn-northd flows docs
>
> 1: https://mail.openvswitch.org/pipermail/ovs-dev/2022-August/396796.html
> ---
>  northd/northd.c | 33 +---
>  northd/ovn-northd.8.xml |  4 +++
>  tests/ovn-northd.at | 69 +
>  3 files changed, 102 insertions(+), 4 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index facd41a59..b1e9ffc87 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -637,6 +637,7 @@ struct ovn_datapath {
>  bool has_lb_vip;
>  bool has_unknown;
>  bool has_acls;
> +bool has_vtep_lports;
>
>  /* IPAM data. */
>  struct ipam_info ipam_info;
> @@ -1847,6 +1848,12 @@ lsp_is_localnet(const struct nbrec_logical_switch_port 
> *nbsp)
>  return !strcmp(nbsp->type, "localnet");
>  }
>
> +static bool
> +lsp_is_vtep(const struct nbrec_logical_switch_port *nbsp)
> +{
> +return !strcmp(nbsp->type, "vtep");
> +}
> +
>  static bool
>  localnet_can_learn_mac(const struct nbrec_logical_switch_port *nbsp)
>  {
> @@ -2655,6 +2662,10 @@ join_logical_ports(struct northd_input *input_data,
> od->localnet_ports[od->n_localnet_ports++] = op;
>  }
>
> +if (lsp_is_vtep(nbsp)) {
> +od->has_vtep_lports = true;
> +}
> +
>  op->lsp_addrs
>  = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
>  for (size_t j = 0; j < nbsp->n_addresses; j++) {
> @@ -5518,7 +5529,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct 
> hmap *lflows,
>  ds_put_format(actions, "set_queue(%s); ", queue_id);
>  }
>
> -if (!strcmp(op->nbsp->type, "vtep")) {
> +if (lsp_is_vtep(op->nbsp)) {
>  ds_put_format(actions, REGBIT_FROM_RAMP" = 1; ");
>  ds_put_format(actions, "next(pipeline=ingress, table=%d);",
>ovn_stage_get_table(S_SWITCH_IN_HAIRPIN));
> @@ -10894,6 +10905,22 @@ build_gateway_mtu_flow(struct hmap *lflows, struct 
> ovn_port *op,
>  va_end(extra_actions_args);
>  }
>
> +static bool
> +consider_l3dwg_port_is_centralized(struct ovn_port *op)
> +{
> +if (op->peer && op->peer->od->has_vtep_lports) {
> +return false;
> +}
> +
> +if (is_l3dgw_port(op)) {
> +/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> + * should only be received on the gateway chassis. */
> +return true;
> +}
> +
> +return false;
> +}
> +
>  /* Logical router ingress Table 0: L2 Admission Control
>   * This table drops packets that the router shouldn’t see at all based
>   * on their Ethernet headers.
> @@ -10930,9 +10957,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)) {
> -/* Traffic with eth.dst = l3dgw_port->lrp_networks.ea_s
> - * should only be received on the gateway chassis. */
> +if (consider_l3dwg_port_is_centralized(op)) {
>  ds_put_format(match, " && is_chassis_resident(%s)",
>op->cr_port->json_key);
>  }
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index ff21c0737..9b6459d9e 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2114,6 +2114,10 @@ output;
>gateway chassis), the above flow matching
>eth.dst == E is only programmed on
>the gateway port instance on the gateway chassis.
> +  If LRP's logical switch has attached LSP of vtep type,
> +  the is_chassis_resident() part is not added to lflow 
> to
> +  allow traffic originated from logical switch to reach LR services
> +  (LBs, NAT).
>  
>
>  
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 5b5eeb0ee..3ffa39419 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -5747,6 +5747,75 @@ AT_CHECK([grep lr_in_gw_redirect lrflows | grep cr-DR 
> | sed 's/table=../table=??
>  AT_CLEANUP
>  ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-northd -- lr admission with vtep lports])
> +AT_KEYWORDS([multiple-l3dgw-ports])
> +ovn_start NORTHD_TYPE
> +check ovn-sbctl chassis-add ch1 geneve 127.0.0.2
> +
> +check ovn-nbctl lr-add lr1
> +check ovn-nbctl