Hi Mark, Please see below.
regards, Vladislav Odintsov -----Original Message----- From: dev <[email protected]> on behalf of Mark Michelson <[email protected]> Date: Tuesday, 4 June 2024 at 03:45 To: Vladislav Odintsov <[email protected]>, "[email protected]" <[email protected]> Subject: Re: [ovs-dev] [PATCH ovn v5 2/2] northd: Add support for disabling vxlan mode. Hi Vladislav, I have just one comment below. On 5/3/24 04:13, Vladislav Odintsov wrote: > Commit [1] introduced a "VXLAN mode" concept. It brought a limitation > for available tunnel IDs because of lack of space in VXLAN VNI. > In VXLAN mode OVN is limited by 4095 datapaths (LRs or non-transit LSs) > and 2047 logical ports per datapath. > > Prior to this patch VXLAN mode was enabled automatically if at least one > chassis had encap of VXLAN type. In scenarios where one want to use > VXLAN only for HW VTEP (RAMP) switch, such limitation makes no sence. > > This patch adds support for explicit disabling of VXLAN mode via > Northbound database. > > 1: https://github.com/ovn-org/ovn/commit/b07f1bc3d068 > > Acked-By: Ihar Hrachyshka <[email protected]> > Fixes: b07f1bc3d068 ("Add VXLAN support for non-VTEP datapath bindings") > Signed-off-by: Vladislav Odintsov <[email protected]> > --- > NEWS | 4 ++++ > northd/en-global-config.c | 7 ++++++- > northd/northd.c | 10 ++++++++-- > northd/northd.h | 3 ++- > ovn-architecture.7.xml | 6 ++++++ > ovn-nb.xml | 10 ++++++++++ > tests/ovn-northd.at | 29 +++++++++++++++++++++++++++++ > 7 files changed, 65 insertions(+), 4 deletions(-) > > diff --git a/NEWS b/NEWS > index 3b5e93dc9..007b27f3d 100644 > --- a/NEWS > +++ b/NEWS > @@ -17,6 +17,10 @@ Post v24.03.0 > external-ids, the option is no longer needed as it became effectively > "true" for all scenarios. > - Added DHCPv4 relay support. > + - Added new global config option NB_Global:options:disable_vxlan_mode to > + extend available tunnel IDs space for datapaths from 4095 to 16711680 > + when running in "VXLAN mode". For more details see man ovn-nb(5) for > + mentioned option. > > OVN v24.03.0 - 01 Mar 2024 > -------------------------- > diff --git a/northd/en-global-config.c b/northd/en-global-config.c > index 873649a89..f5e2a8154 100644 > --- a/northd/en-global-config.c > +++ b/northd/en-global-config.c > @@ -115,7 +115,7 @@ en_global_config_run(struct engine_node *node , void *data) > config_data->svc_monitor_mac); > } > > - init_vxlan_mode(sbrec_chassis_table); > + init_vxlan_mode(&nb->options, sbrec_chassis_table); > char *max_tunid = xasprintf("%d", get_ovn_max_dp_key_local()); > smap_replace(options, "max_tunid", max_tunid); > free(max_tunid); > @@ -533,6 +533,11 @@ check_nb_options_out_of_sync(const struct nbrec_nb_global *nb, > return true; > } > > + if (config_out_of_sync(&nb->options, &config_data->nb_options, > + "disable_vxlan_mode", false)) { > + return true; > + } > + > return false; > } > > diff --git a/northd/northd.c b/northd/northd.c > index 0e0ae24db..7bdffe531 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -886,8 +886,14 @@ join_datapaths(const struct nbrec_logical_switch_table *nbrec_ls_table, > } > > void > -init_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table) > +init_vxlan_mode(const struct smap *nb_options, > + const struct sbrec_chassis_table *sbrec_chassis_table) > { > + if (smap_get_bool(nb_options, "disable_vxlan_mode", false)) { > + vxlan_mode = false; > + return; > + } > + > const struct sbrec_chassis *chassis; > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, sbrec_chassis_table) { > for (int i = 0; i < chassis->n_encaps; i++) { > @@ -17596,7 +17602,7 @@ ovnnb_db_run(struct northd_input *input_data, > use_common_zone = smap_get_bool(input_data->nb_options, "use_common_zone", > false); > > - init_vxlan_mode(input_data->sbrec_chassis_table); > + init_vxlan_mode(input_data->nb_options, input_data->sbrec_chassis_table); > > build_datapaths(ovnsb_txn, > input_data->nbrec_logical_switch_table, > diff --git a/northd/northd.h b/northd/northd.h > index be480003e..d0322e621 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -792,7 +792,8 @@ lr_has_multiple_gw_ports(const struct ovn_datapath *od) > } > > void > -init_vxlan_mode(const struct sbrec_chassis_table *sbrec_chassis_table); > +init_vxlan_mode(const struct smap *nb_options, > + const struct sbrec_chassis_table *sbrec_chassis_table); > > uint32_t get_ovn_max_dp_key_local(void); > > diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml > index 3ecb58933..f4eae340c 100644 > --- a/ovn-architecture.7.xml > +++ b/ovn-architecture.7.xml > @@ -2920,4 +2920,10 @@ > the future, gateways that do not support encapsulations with large amounts > of metadata may continue to have a reduced feature set. > </p> > + <p> > + <code>VXLAN mode</code> is recommended to be disabled if VXLAN encap at > + hypervisors is needed only to support HW VTEP L2 Gateway functionality. > + See man ovn-nb(5) for table <code>NB_Global</code> column > + <code>options</code> key <code>disable_vxlan_mode</code> for more details. > + </p> > </manpage> > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 5cb6ba640..84f1e07b6 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -381,6 +381,16 @@ > of SB changes would be very noticeable. > </column> > > + <column name="options" key="disable_vxlan_mode"> > + By default if at least one chassis in OVN cluster has VXLAN encap, > + northd will run in a <code>VXLAN mode</code>. See man > + ovn-architecture(7) <code>Tunnel Encapsulations</code> paragraph for > + more details. In case VXLAN encaps are needed on chassis only to > + support HW VTEP functionality and main encap type is GENEVE or STT, set > + this option to <code>false</code> to use default > + non-<code>VXLAN mode</code> tunnel IDs allocation logic. I think the logic is reversed in this paragraph. I think you actually want to set "disable_vxlan_mode" to "true" to use non-VXLAN mode allocation. Yeap, you're right. There should be true. One thing that might make this easier to document and to understand is to change the option to be positively-worded. IMO, it's much easier to understand "vxlan_mode=true" than it is to understand "disable_vxlan_mode=false". It also makes the code easier to understand, since "if (vxlan_mode)" is easier to process than "if (!disable_vxlan_mode)". Your point sounds reasonable with one note: vxlan_mode=true is the default value. User might want to only disable it with vxlan_mode=false. I'll address this in v6. > + </column> > + > <group title="Options for configuring interconnection route advertisement"> > <p> > These options control how routes are advertised between OVN > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 680d96675..9ddd7d177 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2847,6 +2847,35 @@ AT_CHECK( > get_tunnel_keys > AT_CHECK([test $lsp02 = 3 && test $ls1 = 123]) > > +AT_CLEANUP > +]) > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([check VXLAN mode disabling]) > +ovn_start > + > +# Create a fake chassis with vxlan encap to implicitly enable VXLAN mode. > +ovn-sbctl \ > + --id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="vxlan" \ > + -- --id=@c create chassis name=hv1 encaps=@e > + > +cmd="ovn-nbctl --wait=sb" > +for i in {1..4097..1}; do > + cmd="${cmd} -- ls-add lsw-${i}" > +done > + > +check $cmd > + > +check_row_count nb:Logical_Switch 4097 > +wait_row_count sb:Datapath_Binding 4095 > + > +OVS_WAIT_UNTIL([grep "all datapath tunnel ids exhausted" northd/ovn-northd.log]) > + > +# Explicitly disable VXLAN mode and check that two remaining datapaths were created. > +check ovn-nbctl set NB_Global . options:disable_vxlan_mode=true > + > +check_row_count nb:Logical_Switch 4097 > +wait_row_count sb:Datapath_Binding 4097 > + > AT_CLEANUP > ]) > _______________________________________________ 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
