Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On 5/3/23 22:13, Ihar Hrachyshka wrote:
> When a multichassis port belongs to a switch with a localnet port,
> packets originating or directed to the multichassis port are NOT sent
> thorugh the localnet port. Instead, tunneling is enforced in-cluster to
> guarantee delivery of all packets to all chassis of the port.
>
> This behavior has an unfortunate side effect, where - because of
> additional tunnel header added to each packet - the effective MTU of the
> path for multichassis ports changes from what's set as mtu_request. This
> effectively makes OVN to black hole all packets for the port that use
> full capacity of the interface MTU. This breaks usual TCP / UDP
> services, among other things (SSH, iperf sessions etc.)
>
> This patch adds flows so that
> - (in table 38) detect too-big packets (table 38), and then
> - (in table 39) icmp fragmentation needed / too big errors are sent
> back to offending port.
>
> Once the error is received, the sender is expected to adjust the route
> MTU accordingly, sending the next packets with the new path MTU. After a
> multichassis port is re-assigned to a single chassis, the effective path
> MTU is restored to "usual". Peers will eventually see their "learned"
> path MTU cache expire, which will make them switch back to the "usual"
> MTU.
>
> Among other scenarios, this patch helps to maintain existing services
> working during live migration of a VM, if multichassis ports are used.
> (E.g. in OpenStack Nueutron.)
I'm not necessarily rejecting this change. I just wanted to bring up an
alternative approach (I'm not sure if it's possible to implement it though):
The CMS (e.g., Neutron) probably knows before hand the reduced MTU value
it should impose on the localnet port. That means it might be possible
to just implement all this through logical flows that use
'check_pkt_larger()'.
In any case, the code in this patch looks OK to me overall. I have a
few minor comments below.
>
> Fixes: 7084cf437421 ("Always funnel multichassis port traffic through
> tunnels")
>
> Signed-off-by: Ihar Hrachyshka
> ---
> NEWS| 6 +
> controller/ovn-controller.c | 3 +
> controller/physical.c | 293 +++-
> controller/physical.h | 1 +
> lib/ovn-util.h | 11 ++
> tests/ovn.at| 263
> 6 files changed, 572 insertions(+), 5 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 60467581a..9d5eef268 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,12 @@ Post v23.03.0
> existing behaviour of flooding these arp requests to all attached Ports.
>- Always allow IPv6 Router Discovery, Neighbor Discovery, and Multicast
> Listener Discovery protocols, regardless of ACLs defined.
> + - Send ICMP Fragmentation Needed packets back to offending ports when
> +communicating with multichassis ports using frames that don't fit
> through a
> +tunnel. This is done only for logical switches that are attached to a
> +physical network via a localnet port, in which case multichassis ports
> may
> +have an effective MTU different from regular ports and hence may need
> this
> +mechanism to maintain connectivity with other peers in the network.
>
> OVN v23.03.0 - 03 Mar 2023
> --
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c094cb74d..9359925fa 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -4083,6 +4083,9 @@ static void init_physical_ctx(struct engine_node *node,
> p_ctx->patch_ofports = &non_vif_data->patch_ofports;
> p_ctx->chassis_tunnels = &non_vif_data->chassis_tunnels;
>
> +struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
> +p_ctx->if_mgr = ctrl_ctx->if_mgr;
> +
> pflow_output_get_debug(node, &p_ctx->debug);
> }
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 1b0482e3b..a2a25d067 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -41,6 +41,7 @@
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> #include "ovn/actions.h"
> +#include "if-status.h"
> #include "physical.h"
> #include "pinctrl.h"
> #include "openvswitch/shash.h"
> @@ -91,6 +92,7 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>
> ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_mtu);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
> ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
> }
> @@ -1104,6 +1106,273 @@ setup_activation_strategy(const struct
> sbrec_port_binding *binding,
> }
> }
>
> +static size_t
> +encode_start_controller_op(enum action_opcode opcode, bool pause,
> + uint32_t meter_id, struct of
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
Thank you Dumitru! See below.
On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote:
> I'm not necessarily rejecting this change. I just wanted to bring up an
> alternative approach (I'm not sure if it's possible to implement it though):
>
> The CMS (e.g., Neutron) probably knows before hand the reduced MTU value
> it should impose on the localnet port. That means it might be possible
> to just implement all this through logical flows that use
> 'check_pkt_larger()'.
>
I think I mentioned this alternative in my cover letter for the
series. (At least one of the variations.)
I don't understand the specifics of what you are describing here
though. Yes, CMS may do calculations for the "desired path MTU". But
what's the mechanism for CMS to pass this information down to the OVN
controller? ACL? I don't think the space of possible actions that
would allow CMS itself to define the behavior is available to CMS.
Perhaps you mean (as I think we discussed before in private) the
approach where OVN northd creates Logical_Flow objects that would
implement the behavior. If so, yes that could be possible. But note
that northd doesn't know the desired path MTU (because it's not CMS
and OVN northbound database API doesn't support MTU primitive for
ports.) So the best we could do in the alternative way is implement
half of the solution via Logical_Flow (the one that sends ICMP replies
for "too-big" frames), leaving the other half (tagging the "too-big"
frames based on interface MTU for processing by the Logical_Flows) to
OVN controller. I think I listed this variation of the approach in my
cover letter, but please let me know if it's not sufficient.
(The split could be avoided if OVN adds a MTU attribute to its ports.
But I of course am not trying to tackle this in this series, for
obvious reasons. But MTU for ports is - in my mind - a worthy feature
to have in OVN, if you ask me.)
> In any case, the code in this patch looks OK to me overall. I have a
> few minor comments below.
>
> >
> > Fixes: 7084cf437421 ("Always funnel multichassis port traffic through
> > tunnels")
> >
> > Signed-off-by: Ihar Hrachyshka
> > ---
> > NEWS| 6 +
> > controller/ovn-controller.c | 3 +
> > controller/physical.c | 293 +++-
> > controller/physical.h | 1 +
> > lib/ovn-util.h | 11 ++
> > tests/ovn.at| 263
> > 6 files changed, 572 insertions(+), 5 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 60467581a..9d5eef268 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -14,6 +14,12 @@ Post v23.03.0
> > existing behaviour of flooding these arp requests to all attached
> > Ports.
> >- Always allow IPv6 Router Discovery, Neighbor Discovery, and Multicast
> > Listener Discovery protocols, regardless of ACLs defined.
> > + - Send ICMP Fragmentation Needed packets back to offending ports when
> > +communicating with multichassis ports using frames that don't fit
> > through a
> > +tunnel. This is done only for logical switches that are attached to a
> > +physical network via a localnet port, in which case multichassis ports
> > may
> > +have an effective MTU different from regular ports and hence may need
> > this
> > +mechanism to maintain connectivity with other peers in the network.
> >
> > OVN v23.03.0 - 03 Mar 2023
> > --
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c094cb74d..9359925fa 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -4083,6 +4083,9 @@ static void init_physical_ctx(struct engine_node
> > *node,
> > p_ctx->patch_ofports = &non_vif_data->patch_ofports;
> > p_ctx->chassis_tunnels = &non_vif_data->chassis_tunnels;
> >
> > +struct controller_engine_ctx *ctrl_ctx =
> > engine_get_context()->client_ctx;
> > +p_ctx->if_mgr = ctrl_ctx->if_mgr;
> > +
> > pflow_output_get_debug(node, &p_ctx->debug);
> > }
> >
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 1b0482e3b..a2a25d067 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -41,6 +41,7 @@
> > #include "lib/ovn-sb-idl.h"
> > #include "lib/ovn-util.h"
> > #include "ovn/actions.h"
> > +#include "if-status.h"
> > #include "physical.h"
> > #include "pinctrl.h"
> > #include "openvswitch/shash.h"
> > @@ -91,6 +92,7 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> >
> > ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
> > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> > +ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_mtu);
> > ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_ofport);
> > ovsdb_idl_track_add_column(ovs_idl,
> > &ovsrec_interface_col_external_ids);
> > }
> > @@ -1104,6 +1106,273 @@ setup_activation_strategy(const struct
> > sbr
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On 5/17/23 18:46, Ihar Hrachyshka wrote: > Thank you Dumitru! See below. > > On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote: >> I'm not necessarily rejecting this change. I just wanted to bring up an >> alternative approach (I'm not sure if it's possible to implement it though): >> >> The CMS (e.g., Neutron) probably knows before hand the reduced MTU value >> it should impose on the localnet port. That means it might be possible >> to just implement all this through logical flows that use >> 'check_pkt_larger()'. >> > > I think I mentioned this alternative in my cover letter for the > series. (At least one of the variations.) > > I don't understand the specifics of what you are describing here > though. Yes, CMS may do calculations for the "desired path MTU". But > what's the mechanism for CMS to pass this information down to the OVN > controller? ACL? I don't think the space of possible actions that > would allow CMS itself to define the behavior is available to CMS. > > Perhaps you mean (as I think we discussed before in private) the > approach where OVN northd creates Logical_Flow objects that would > implement the behavior. If so, yes that could be possible. But note > that northd doesn't know the desired path MTU (because it's not CMS > and OVN northbound database API doesn't support MTU primitive for > ports.) So the best we could do in the alternative way is implement > half of the solution via Logical_Flow (the one that sends ICMP replies > for "too-big" frames), leaving the other half (tagging the "too-big" > frames based on interface MTU for processing by the Logical_Flows) to > OVN controller. I think I listed this variation of the approach in my > cover letter, but please let me know if it's not sufficient. > > (The split could be avoided if OVN adds a MTU attribute to its ports. > But I of course am not trying to tackle this in this series, for > obvious reasons. But MTU for ports is - in my mind - a worthy feature > to have in OVN, if you ask me.) I was referring to this last part: adding MTU for ports and ovn-northd translating those to logical flows that use check_pkt_larger() and set REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate the ICMP error packets. But there's indeed no obvious way of doing that because we need to act only on traffic that's destined to remote hypervisors and that's not really an ovn-northd concept but more of an ovn-controller one. If we go for your current approach and later add MTU for OVN ports, do we need to be careful now in order to avoid complicated upgrades later? Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On Mon, May 22, 2023 at 7:55 AM Dumitru Ceara wrote: > > On 5/17/23 18:46, Ihar Hrachyshka wrote: > > Thank you Dumitru! See below. > > > > On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote: > >> I'm not necessarily rejecting this change. I just wanted to bring up an > >> alternative approach (I'm not sure if it's possible to implement it > >> though): > >> > >> The CMS (e.g., Neutron) probably knows before hand the reduced MTU value > >> it should impose on the localnet port. That means it might be possible > >> to just implement all this through logical flows that use > >> 'check_pkt_larger()'. > >> > > > > I think I mentioned this alternative in my cover letter for the > > series. (At least one of the variations.) > > > > I don't understand the specifics of what you are describing here > > though. Yes, CMS may do calculations for the "desired path MTU". But > > what's the mechanism for CMS to pass this information down to the OVN > > controller? ACL? I don't think the space of possible actions that > > would allow CMS itself to define the behavior is available to CMS. > > > > Perhaps you mean (as I think we discussed before in private) the > > approach where OVN northd creates Logical_Flow objects that would > > implement the behavior. If so, yes that could be possible. But note > > that northd doesn't know the desired path MTU (because it's not CMS > > and OVN northbound database API doesn't support MTU primitive for > > ports.) So the best we could do in the alternative way is implement > > half of the solution via Logical_Flow (the one that sends ICMP replies > > for "too-big" frames), leaving the other half (tagging the "too-big" > > frames based on interface MTU for processing by the Logical_Flows) to > > OVN controller. I think I listed this variation of the approach in my > > cover letter, but please let me know if it's not sufficient. > > > > (The split could be avoided if OVN adds a MTU attribute to its ports. > > But I of course am not trying to tackle this in this series, for > > obvious reasons. But MTU for ports is - in my mind - a worthy feature > > to have in OVN, if you ask me.) > > I was referring to this last part: adding MTU for ports and ovn-northd > translating those to logical flows that use check_pkt_larger() and set > REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate > the ICMP error packets. But there's indeed no obvious way of doing that > because we need to act only on traffic that's destined to remote > hypervisors and that's not really an ovn-northd concept but more of an > ovn-controller one. > > If we go for your current approach and later add MTU for OVN ports, do > we need to be careful now in order to avoid complicated upgrades later? > I'm trying to think what complications could arise. Isn't it just generating effectively the same flows, now via Logical_Flow path? The flows act on local frames generated by regular VIF ports, so it shouldn't affect the rest of cluster that e.g. some of them are generated by Logical_Flow path and some through the proposed solution. Or am I talking about something else? > Regards, > Dumitru > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On 5/22/23 15:42, Ihar Hrachyshka wrote: > On Mon, May 22, 2023 at 7:55 AM Dumitru Ceara wrote: >> >> On 5/17/23 18:46, Ihar Hrachyshka wrote: >>> Thank you Dumitru! See below. >>> >>> On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote: I'm not necessarily rejecting this change. I just wanted to bring up an alternative approach (I'm not sure if it's possible to implement it though): The CMS (e.g., Neutron) probably knows before hand the reduced MTU value it should impose on the localnet port. That means it might be possible to just implement all this through logical flows that use 'check_pkt_larger()'. >>> >>> I think I mentioned this alternative in my cover letter for the >>> series. (At least one of the variations.) >>> >>> I don't understand the specifics of what you are describing here >>> though. Yes, CMS may do calculations for the "desired path MTU". But >>> what's the mechanism for CMS to pass this information down to the OVN >>> controller? ACL? I don't think the space of possible actions that >>> would allow CMS itself to define the behavior is available to CMS. >>> >>> Perhaps you mean (as I think we discussed before in private) the >>> approach where OVN northd creates Logical_Flow objects that would >>> implement the behavior. If so, yes that could be possible. But note >>> that northd doesn't know the desired path MTU (because it's not CMS >>> and OVN northbound database API doesn't support MTU primitive for >>> ports.) So the best we could do in the alternative way is implement >>> half of the solution via Logical_Flow (the one that sends ICMP replies >>> for "too-big" frames), leaving the other half (tagging the "too-big" >>> frames based on interface MTU for processing by the Logical_Flows) to >>> OVN controller. I think I listed this variation of the approach in my >>> cover letter, but please let me know if it's not sufficient. >>> >>> (The split could be avoided if OVN adds a MTU attribute to its ports. >>> But I of course am not trying to tackle this in this series, for >>> obvious reasons. But MTU for ports is - in my mind - a worthy feature >>> to have in OVN, if you ask me.) >> >> I was referring to this last part: adding MTU for ports and ovn-northd >> translating those to logical flows that use check_pkt_larger() and set >> REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate >> the ICMP error packets. But there's indeed no obvious way of doing that >> because we need to act only on traffic that's destined to remote >> hypervisors and that's not really an ovn-northd concept but more of an >> ovn-controller one. >> >> If we go for your current approach and later add MTU for OVN ports, do >> we need to be careful now in order to avoid complicated upgrades later? >> > > I'm trying to think what complications could arise. Isn't it just > generating effectively the same flows, now via Logical_Flow path? The > flows act on local frames generated by regular VIF ports, so it > shouldn't affect the rest of cluster that e.g. some of them are > generated by Logical_Flow path and some through the proposed solution. > Or am I talking about something else? > Assuming your current approach is accepted it means ovn-controller will automatically compute min MTU to use for multi-chassis ports. But if we add an explicit OVN port MTU configuration what do we do with the automatic computation? Do we leave it in place or do we remove it? If we remove it it means we'll break upgrades as northd is upgraded last and it will only set the OVN port binding MTU after it gets upgraded. If we leave it in place I'm not sure what the benefit of the explicit MTU is. I might be wrong though. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On Mon, May 22, 2023 at 9:55 AM Dumitru Ceara wrote: > > On 5/22/23 15:42, Ihar Hrachyshka wrote: > > On Mon, May 22, 2023 at 7:55 AM Dumitru Ceara wrote: > >> > >> On 5/17/23 18:46, Ihar Hrachyshka wrote: > >>> Thank you Dumitru! See below. > >>> > >>> On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote: > I'm not necessarily rejecting this change. I just wanted to bring up an > alternative approach (I'm not sure if it's possible to implement it > though): > > The CMS (e.g., Neutron) probably knows before hand the reduced MTU value > it should impose on the localnet port. That means it might be possible > to just implement all this through logical flows that use > 'check_pkt_larger()'. > > >>> > >>> I think I mentioned this alternative in my cover letter for the > >>> series. (At least one of the variations.) > >>> > >>> I don't understand the specifics of what you are describing here > >>> though. Yes, CMS may do calculations for the "desired path MTU". But > >>> what's the mechanism for CMS to pass this information down to the OVN > >>> controller? ACL? I don't think the space of possible actions that > >>> would allow CMS itself to define the behavior is available to CMS. > >>> > >>> Perhaps you mean (as I think we discussed before in private) the > >>> approach where OVN northd creates Logical_Flow objects that would > >>> implement the behavior. If so, yes that could be possible. But note > >>> that northd doesn't know the desired path MTU (because it's not CMS > >>> and OVN northbound database API doesn't support MTU primitive for > >>> ports.) So the best we could do in the alternative way is implement > >>> half of the solution via Logical_Flow (the one that sends ICMP replies > >>> for "too-big" frames), leaving the other half (tagging the "too-big" > >>> frames based on interface MTU for processing by the Logical_Flows) to > >>> OVN controller. I think I listed this variation of the approach in my > >>> cover letter, but please let me know if it's not sufficient. > >>> > >>> (The split could be avoided if OVN adds a MTU attribute to its ports. > >>> But I of course am not trying to tackle this in this series, for > >>> obvious reasons. But MTU for ports is - in my mind - a worthy feature > >>> to have in OVN, if you ask me.) > >> > >> I was referring to this last part: adding MTU for ports and ovn-northd > >> translating those to logical flows that use check_pkt_larger() and set > >> REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate > >> the ICMP error packets. But there's indeed no obvious way of doing that > >> because we need to act only on traffic that's destined to remote > >> hypervisors and that's not really an ovn-northd concept but more of an > >> ovn-controller one. > >> > >> If we go for your current approach and later add MTU for OVN ports, do > >> we need to be careful now in order to avoid complicated upgrades later? > >> > > > > I'm trying to think what complications could arise. Isn't it just > > generating effectively the same flows, now via Logical_Flow path? The > > flows act on local frames generated by regular VIF ports, so it > > shouldn't affect the rest of cluster that e.g. some of them are > > generated by Logical_Flow path and some through the proposed solution. > > Or am I talking about something else? > > > > Assuming your current approach is accepted it means ovn-controller will > automatically compute min MTU to use for multi-chassis ports. But if we > add an explicit OVN port MTU configuration what do we do with the > automatic computation? > > Do we leave it in place or do we remove it? We'll make OVN LSP `mtu` override interface-derived value. (If ovn-controller sees `mtu` set for LSP, it seizes its operation; otherwise it sets flows itself.) > > If we remove it it means we'll break upgrades as northd is upgraded last > and it will only set the OVN port binding MTU after it gets upgraded. > We'll have both for one LTS release and sunset the interface-derived value, if needed? > If we leave it in place I'm not sure what the benefit of the explicit > MTU is. I might be wrong though. There are benefits of OVN LSP API `mtu` regardless of Path MTU Discovery series. For one, OVN could enforce its MTU on VIF interfaces (set it in `mtu_request`). This would also potentially allow a chassisA to inject Path MTU Discovery flows when `mtu` is changed on chassisB independently of ports hosted on chassisA. Right now ovn-controller is aware of MTUs for the ports it owns, but not for the ports on other chassis. I wouldn't look at `mtu` as OVN API as just a means to simplify the task for Path MTU, there are other benefits too. > > Regards, > Dumitru > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 5/6] Implement MTU Path Discovery for multichassis ports
On 5/22/23 17:39, Ihar Hrachyshka wrote: > On Mon, May 22, 2023 at 9:55 AM Dumitru Ceara wrote: >> >> On 5/22/23 15:42, Ihar Hrachyshka wrote: >>> On Mon, May 22, 2023 at 7:55 AM Dumitru Ceara wrote: On 5/17/23 18:46, Ihar Hrachyshka wrote: > Thank you Dumitru! See below. > > On Tue, May 16, 2023 at 9:41 AM Dumitru Ceara wrote: >> I'm not necessarily rejecting this change. I just wanted to bring up an >> alternative approach (I'm not sure if it's possible to implement it >> though): >> >> The CMS (e.g., Neutron) probably knows before hand the reduced MTU value >> it should impose on the localnet port. That means it might be possible >> to just implement all this through logical flows that use >> 'check_pkt_larger()'. >> > > I think I mentioned this alternative in my cover letter for the > series. (At least one of the variations.) > > I don't understand the specifics of what you are describing here > though. Yes, CMS may do calculations for the "desired path MTU". But > what's the mechanism for CMS to pass this information down to the OVN > controller? ACL? I don't think the space of possible actions that > would allow CMS itself to define the behavior is available to CMS. > > Perhaps you mean (as I think we discussed before in private) the > approach where OVN northd creates Logical_Flow objects that would > implement the behavior. If so, yes that could be possible. But note > that northd doesn't know the desired path MTU (because it's not CMS > and OVN northbound database API doesn't support MTU primitive for > ports.) So the best we could do in the alternative way is implement > half of the solution via Logical_Flow (the one that sends ICMP replies > for "too-big" frames), leaving the other half (tagging the "too-big" > frames based on interface MTU for processing by the Logical_Flows) to > OVN controller. I think I listed this variation of the approach in my > cover letter, but please let me know if it's not sufficient. > > (The split could be avoided if OVN adds a MTU attribute to its ports. > But I of course am not trying to tackle this in this series, for > obvious reasons. But MTU for ports is - in my mind - a worthy feature > to have in OVN, if you ask me.) I was referring to this last part: adding MTU for ports and ovn-northd translating those to logical flows that use check_pkt_larger() and set REGBIT_PKT_LARGER. I was also hoping to see logical flows that generate the ICMP error packets. But there's indeed no obvious way of doing that because we need to act only on traffic that's destined to remote hypervisors and that's not really an ovn-northd concept but more of an ovn-controller one. If we go for your current approach and later add MTU for OVN ports, do we need to be careful now in order to avoid complicated upgrades later? >>> >>> I'm trying to think what complications could arise. Isn't it just >>> generating effectively the same flows, now via Logical_Flow path? The >>> flows act on local frames generated by regular VIF ports, so it >>> shouldn't affect the rest of cluster that e.g. some of them are >>> generated by Logical_Flow path and some through the proposed solution. >>> Or am I talking about something else? >>> >> >> Assuming your current approach is accepted it means ovn-controller will >> automatically compute min MTU to use for multi-chassis ports. But if we >> add an explicit OVN port MTU configuration what do we do with the >> automatic computation? >> >> Do we leave it in place or do we remove it? > > We'll make OVN LSP `mtu` override interface-derived value. (If > ovn-controller sees `mtu` set for LSP, it seizes its operation; > otherwise it sets flows itself.) > >> >> If we remove it it means we'll break upgrades as northd is upgraded last >> and it will only set the OVN port binding MTU after it gets upgraded. >> > > We'll have both for one LTS release and sunset the interface-derived > value, if needed? > Sure, that would work. >> If we leave it in place I'm not sure what the benefit of the explicit >> MTU is. I might be wrong though. > > There are benefits of OVN LSP API `mtu` regardless of Path MTU > Discovery series. For one, OVN could enforce its MTU on VIF interfaces > (set it in `mtu_request`). This would also potentially allow a > chassisA to inject Path MTU Discovery flows when `mtu` is changed on > chassisB independently of ports hosted on chassisA. Right now > ovn-controller is aware of MTUs for the ports it owns, but not for the > ports on other chassis. > > I wouldn't look at `mtu` as OVN API as just a means to simplify the > task for Path MTU, there are other benefits too. > Good points above. It makes sense to have the per port MTU as OVN API in general indeed. Thanks for the details! _
