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/[email protected]/ Regards, Vladislav Odintsov > On 9 Aug 2022, at 04:54, Numan Siddique <[email protected]> wrote: > > On Mon, Aug 8, 2022 at 11:29 PM Vladislav Odintsov <[email protected] > <mailto:[email protected]>> wrote: >> >> Hi Numan, >> >> thanks for the review. >> My comments below. >> >> Regards, >> Vladislav Odintsov >> >>> On 8 Aug 2022, at 03:24, Numan Siddique <[email protected]> wrote: >>> >>> On Wed, Jul 20, 2022 at 12:15 AM Vladislav Odintsov <[email protected] >>> <mailto:[email protected]>> 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 <[email protected] >>>> <mailto:[email protected]>> >>> >>> 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 >>>> [email protected] <mailto:[email protected]> >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
