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 > 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
