On Mon, Aug 8, 2022 at 11:29 PM Vladislav Odintsov <[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]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
