On 12/18/24 4:54 PM, Felix Huettner via dev wrote: > In case we find a LRP (or a LSP connected to a LRP) that has > options:active-active-lrp set we ignore it during normal processing in > join_logical_ports. > > We add an additional section at the end where we then use these ports to > generate derived Port_Bindings for each LRP + LSP combination once for > each matching ovn-aa-port-mappings entry. > > In the end this gives us the same result as if someone would have > precreated a LRP + LSP combination for each ovn-aa-port-mappings in the > northbound. However it allows our users to benefit from active-active > routing without their CMS needing to know about this feature (besides > the active-active-lrp setting). > > Signed-off-by: Felix Huettner <[email protected]> > ---
Hi Felix, > NEWS | 6 + > controller/ovn-controller.8.xml | 30 ++++ > lib/automake.mk | 2 + > lib/lrp-index.c | 43 +++++ > lib/lrp-index.h | 25 +++ > lib/ovn-util.c | 97 +++++++++++ > lib/ovn-util.h | 11 ++ > northd/en-northd.c | 4 + > northd/inc-proc-northd.c | 6 + > northd/northd.c | 278 ++++++++++++++++++++++++++++-- > northd/northd.h | 10 ++ > ovn-nb.xml | 27 +++ > tests/ovn-macros.at | 17 ++ > tests/ovn.at | 290 ++++++++++++++++++++++++++++++++ > 14 files changed, 832 insertions(+), 14 deletions(-) > create mode 100644 lib/lrp-index.c > create mode 100644 lib/lrp-index.h > > diff --git a/NEWS b/NEWS > index d2b3ee460..124ce60a2 100644 > --- a/NEWS > +++ b/NEWS > @@ -12,6 +12,12 @@ Post v24.09.0 > and "routing-protocols" are now also usable on distributed gateway > ports. > - ovn-nb: Changed schema of ovn-nb to make networks optional within > Logical > Router Ports. > + - Add the option "active-active-lrp" to LRPs. If set northd will clone > this Nit: maybe just call this "active-active"? Or something else that doesn't include the "-lrp" suffix. It's applied on a router port so it's kind of clear that it's a router port specific option. > + LRP based on its HA_Chassis_Group and the > + other_config:ovn-active-active-mapping of the defined chassis. In > + combination with the dynamic routing features this allows operators to > + integrate OVN into the network fabric in a highly available way without > + significant (or any) changes to the CMS. > > OVN v24.09.0 - 13 Sep 2024 > -------------------------- > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index 6a7d676af..10407351b 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -404,6 +404,36 @@ > If the value is zero, it disables the inactivity probe. > </p> > </dd> > + <dt><code>external_ids:ovn-active-active-mappings</code></dt> > + <dd> > + <p> > + Setting is used for the chassis specific values of the > + <code>options:active-active-lrp</code> in the northbound database. > + > + The following is an example of such an option: > + > <code>ovn-active-active-mappings="phys;00:fe:fe:fe:fe:01,172.16.0.10/25;00:fe:fe:fe:fe:33,172.17.0.10/25|phys2;00:aa:bb:cc:dd:ee,192.168.0.10/24"</code> > + > + This configures 2 separate active-active mapping for two external > + networks. > + > + <ul> > + <li> > + For the external network named <code>phys</code> > + there will be two LRPs generated for this chassis. One with MAC > + <code>00:fe:fe:fe:fe:01</code> and IP > + <code>172.16.0.10/25</code>. The other with MAC > + <code>00:fe:fe:fe:fe:33</code> and IP > + <code>172.17.0.10/25</code>. > + </li> > + <li> > + For the external network named <code>phys2</code> > + there will be one LRPs generated for this chassis. With MAC > + <code>00:aa:bb:cc:dd:ee</code> and IP > + <code>192.168.0.10/24</code>. > + </li> > + </ul> I'm really worried that this will create problems in the future. It's in my opinion better and cleaner if we store such configuration in a properly typed (through DB schema) field. Please see my comments below where I suggest an explicit NB configuration instead. > + </p> > + </dd> > </dl> > > <p> > diff --git a/lib/automake.mk b/lib/automake.mk > index 25e516406..b43f8a7f3 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -26,6 +26,8 @@ lib_libovn_la_SOURCES = \ > lib/ovn-parallel-hmap.c \ > lib/ip-mcast-index.c \ > lib/ip-mcast-index.h \ > + lib/lrp-index.c \ > + lib/lrp-index.h \ > lib/mac-binding-index.c \ > lib/mac-binding-index.h \ > lib/mcast-group-index.c \ > diff --git a/lib/lrp-index.c b/lib/lrp-index.c > new file mode 100644 > index 000000000..ac64c4b45 > --- /dev/null > +++ b/lib/lrp-index.c > @@ -0,0 +1,43 @@ > +/* Missing copyright. > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > +#include "lib/lrp-index.h" > +#include "lib/ovn-nb-idl.h" > + > +struct ovsdb_idl_index * > +lrp_index_create(struct ovsdb_idl *idl) > +{ > + return ovsdb_idl_index_create1(idl, &nbrec_logical_router_port_col_name); > +} > + > + > +/* Finds and returns the lrp with the given 'name', or NULL if no such > + * lrp exists. */ > +const struct nbrec_logical_router_port * > +lrp_lookup_by_name(struct ovsdb_idl_index *nbrec_lrp_by_name, > + const char *name) > +{ > + struct nbrec_logical_router_port *target = > + nbrec_logical_router_port_index_init_row(nbrec_lrp_by_name); > + nbrec_logical_router_port_index_set_name(target, name); > + > + struct nbrec_logical_router_port *retval = > + nbrec_logical_router_port_index_find(nbrec_lrp_by_name, target); > + > + nbrec_logical_router_port_index_destroy_row(target); > + > + return retval; > +} > + > diff --git a/lib/lrp-index.h b/lib/lrp-index.h > new file mode 100644 > index 000000000..2c56933fc > --- /dev/null > +++ b/lib/lrp-index.h > @@ -0,0 +1,25 @@ > +/* Missing copyright. > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + [...] > @@ -2301,13 +2347,19 @@ join_logical_ports_lrp(struct hmap *ports, > return op; > } > > +struct active_active_port { > + const struct nbrec_logical_switch_port *nbsp; > + const struct nbrec_logical_router_port *nbrp; > + struct ovn_datapath *switch_dp; > + struct ovn_datapath *router_dp; > +}; > + > > static struct ovn_port * > create_cr_port(struct ovn_port *op, struct hmap *ports, > struct ovs_list *both_dbs, struct ovs_list *nb_only) > { > - char *redirect_name = ovn_chassis_redirect_name( > - op->nbsp ? op->nbsp->name : op->nbrp->name); > + char *redirect_name = ovn_chassis_redirect_name(op->key); > > struct ovn_port *crp = ovn_port_find(ports, redirect_name); > if (crp && crp->sb && crp->sb->datapath == op->od->sb) { > @@ -2352,6 +2404,8 @@ peer_needs_cr_port_creation(struct ovn_port *op) > > static void > join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, > + struct ovsdb_idl_index *nbrec_lrp_by_name, > + struct ovsdb_idl_index *sbrec_chassis_by_name, > struct hmap *ls_datapaths, struct hmap *lr_datapaths, > struct hmap *ports, unsigned long *queue_id_bitmap, > struct hmap *tag_alloc_table, struct ovs_list *sb_only, > @@ -2361,6 +2415,8 @@ join_logical_ports(const struct > sbrec_port_binding_table *sbrec_pb_table, > ovs_list_init(nb_only); > ovs_list_init(both); > > + struct shash active_active_ports = > SHASH_INITIALIZER(&active_active_ports); > + > const struct sbrec_port_binding *sb; > SBREC_PORT_BINDING_TABLE_FOR_EACH (sb, sbrec_pb_table) { > struct ovn_port *op = ovn_port_create(ports, sb->logical_port, > @@ -2377,6 +2433,20 @@ join_logical_ports(const struct > sbrec_port_binding_table *sbrec_pb_table, > = od->nbr->ports[i]; > > struct lport_addresses lrp_networks; > + > + if (lrport_is_active_active(nbrp)) { > + struct ovn_port *op = ovn_port_find_bound(ports, nbrp->name); > + if (op) { > + ovs_list_remove(&op->list); > + } > + struct active_active_port *aap = xzalloc( > + sizeof(struct active_active_port)); > + aap->nbrp = nbrp; > + aap->router_dp = od; > + shash_add(&active_active_ports, nbrp->name, aap); > + continue; > + } > + > if (!extract_lrp_networks(nbrp, &lrp_networks)) { > static struct vlog_rate_limit rl > = VLOG_RATE_LIMIT_INIT(5, 1); > @@ -2394,6 +2464,16 @@ join_logical_ports(const struct > sbrec_port_binding_table *sbrec_pb_table, > for (size_t i = 0; i < od->nbs->n_ports; i++) { > const struct nbrec_logical_switch_port *nbsp > = od->nbs->ports[i]; > + const struct nbrec_logical_router_port *nbrp > + = lsp_get_peer(nbrec_lrp_by_name, nbsp); > + if (lrport_is_active_active(nbrp)) { > + struct active_active_port *aap = > + shash_find_data(&active_active_ports, nbrp->name); > + ovs_assert(aap); nit: no real need for assert, we'd crash anyway if aap is NULL. > + aap->nbsp = nbsp; > + aap->switch_dp = od; > + continue; > + } Can't we avoid the lookup and the index all together if we do this as part of the 'Connect logical router ports, and logical switch ports of type "router", to their peers.' section, lower in join_logical_ports()? > join_logical_ports_lsp(ports, nb_only, both, od, nbsp, > nbsp->name, queue_id_bitmap, > tag_alloc_table); > @@ -2472,6 +2552,109 @@ join_logical_ports(const struct > sbrec_port_binding_table *sbrec_pb_table, > } > } > [snip - most of the code to generate "implicit ports"] > diff --git a/northd/northd.h b/northd/northd.h > index 9457a7be6..1d837f151 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -64,6 +64,7 @@ struct northd_input { > const struct chassis_features *features; > > /* Indexes */ > + struct ovsdb_idl_index *nbrec_lrp_by_name; > struct ovsdb_idl_index *sbrec_chassis_by_name; > struct ovsdb_idl_index *sbrec_chassis_by_hostname; > struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name; > @@ -662,6 +663,15 @@ struct ovn_port { > /* Only used for the router type LSP whose peer is l3dgw_port */ > bool enable_router_port_acl; > > + /* Used for active-active port bindings to store the data they where > + * generated from */ > + bool is_active_active; Nit: this is equivalent to "!!aa_chassis_name". > + char *aa_chassis_name; > + size_t aa_chassis_index; > + /* The following value is only set on the lrp side of an > + * active-active port binding */ > + char *aa_mac; Nit: I'd use more explicit names. "active_active_*" is in my opinion better than "aa_*". > + > /* Reference of lflows generated for this ovn_port. > * > * This data is initialized and destroyed by the en_northd node, but > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 8373ddb99..9896d9310 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -3672,6 +3672,33 @@ or > learned by the <code>ovn-ic</code> daemon. > </p> > </column> > + > + <column name="options" key="active-active-lrp" > + type='{"type": "boolean"}'> > + <p> > + If set to true this turns this LRP and associated LSP into a > template > + that will be used by ovn-northd to generate multiple LRP/LSP > + combinations. > + One or multiple LRP/LSP combinations will be built for each chassis > + specified in the <ref column="ha_chassis_group"/> of this LRP > + independent of their priority. > + The amount of LRP/LSP combinations per chassis as well as their IP > + and MAC addresses are determined based on the value of > + <ref columns="external_ids" key="ovn-active-active-mappings" > + db="Open_vSwitch"/> on that chassis. > + > + The MAC and IP configuration of this LRP in the northbound database > + is ignored. > + </p> I'm not sure I like this way of configuring things to be honest. In my opinion it just confuses users and can be detrimental when trying to debug OVN issues. Also, we're splitting MAC/IP configuration between the Northbound database and each chassis local OVS database. Am I understanding correctly that in the end all this is just a syntactic sugar to avoid having the CMS configure all LRP/LSP combinations? If so, the CMS still has to manage the per-chassis ovn-active-active-mappings so I'm not really sure where the benefit is. Personally, I think I'd prefer if we had an explicit configuration in the northbound database. Are you concerned that that increases the NB size too much? Or is there any other issue I'm missing here? > + <p> > + The LRP must be connected via its LSP to an external network. > + > + Having multiple such LRPs on a single router is not supported. > + > + Having multiple such LRPs on different routers connected to the > same > + external network is also not supported. > + </p> > + </column> I have the feeling that most of the code added for this patch (to generate logical ports) should actually live on the CMS side and be used to provision the NB explicitly. What do you think? Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
