On Mon, Nov 21, 2022 at 3:00 PM Mark Michelson <[email protected]> wrote: > > Hi Ihar, I have some comments down below. > > One thing I noticed is that nothing ever removes the ovn-chassis-idx > entries from the OVS other_config column. By my interpretation, each > time ovn-controller restarts, it will use a new index, until after the > 37th time, it will fail to get an index and cannot start up. We probably > should remove the local chassis index when ovn-controller shuts down. >
You are right, thank you. I will update. > On 10/18/22 14:31, Ihar Hrachyshka wrote: > > This is in preparation to support multiple separate controller instances > > with distinct chassis names operating on the same vswitchd instance. > > > > To avoid conflicts, this patch introduces a unique "index" (from 0-9a-z > > range) into the port name. Each chassis allocates a separate index for > > itself on startup. The index is then stored in > > Open_vSwitch:other_config:ovn-chassis-idx-<chassis_name> key. > > > > An alternative would be including source chassis name into the port > > name, but the length is limited by IFNAMSIZ defined in kernel, which is > > 15. > > > > Signed-off-by: Ihar Hrachyshka <[email protected]> > > --- > > controller/encaps.c | 24 ++++++++++++---- > > controller/encaps.h | 2 ++ > > controller/ovn-controller.c | 57 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 77 insertions(+), 6 deletions(-) > > > > diff --git a/controller/encaps.c b/controller/encaps.c > > index 9647ba507..a82088d42 100644 > > --- a/controller/encaps.c > > +++ b/controller/encaps.c > > @@ -22,6 +22,7 @@ > > #include "lib/vswitch-idl.h" > > #include "openvswitch/vlog.h" > > #include "lib/ovn-sb-idl.h" > > +#include "lib/ovsdb-idl.h" > > #include "ovn-controller.h" > > #include "smap.h" > > > > @@ -59,6 +60,7 @@ struct tunnel_ctx { > > struct sset port_names; > > > > struct ovsdb_idl_txn *ovs_txn; > > + const struct ovsrec_open_vswitch_table *ovs_table; > > const struct ovsrec_bridge *br_int; > > const struct sbrec_chassis *this_chassis; > > }; > > @@ -68,14 +70,23 @@ struct chassis_node { > > const struct ovsrec_bridge *bridge; > > }; > > > > +static const char *get_chassis_idx(struct tunnel_ctx *tc) > > +{ > > + const struct ovsrec_open_vswitch *cfg = > > + ovsrec_open_vswitch_table_first(tc->ovs_table); > > + char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", > > tc->this_chassis->name); > > + const char *idx = smap_get_def(&cfg->other_config, idx_key, ""); > > + free(idx_key); > > + return idx; > > +} > > + > > static char * > > tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) > > { > > - int i; > > - > > - for (i = 0; i < UINT16_MAX; i++) { > > - char *port_name; > > - port_name = xasprintf("ovn-%.6s-%x", chassis_id, i); > > + for (int i = 0; i < UINT16_MAX; i++) { > > + const char *idx = get_chassis_idx(tc); > > + char *port_name = xasprintf( > > + "ovn%s-%.*s-%x", idx, strlen(idx) ? 5 : 6, chassis_id, i); > > > > if (!sset_contains(&tc->port_names, port_name)) { > > return port_name; > > @@ -400,7 +411,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, > > .chassis = SHASH_INITIALIZER(&tc.chassis), > > .port_names = SSET_INITIALIZER(&tc.port_names), > > .br_int = br_int, > > - .this_chassis = this_chassis > > + .this_chassis = this_chassis, > > + .ovs_table = ovs_table, > > }; > > > > tc.ovs_txn = ovs_idl_txn; > > diff --git a/controller/encaps.h b/controller/encaps.h > > index 25d44b034..16032f15b 100644 > > --- a/controller/encaps.h > > +++ b/controller/encaps.h > > @@ -18,6 +18,8 @@ > > > > #include <stdbool.h> > > > > +#define CHASSIS_IDX_PREFIX "ovn-chassis-idx-" > > + > > struct ovsdb_idl; > > struct ovsdb_idl_txn; > > struct ovsrec_bridge; > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 9969d317f..5c2a4b6ef 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -3527,6 +3527,57 @@ check_northd_version(struct ovsdb_idl *ovs_idl, > > struct ovsdb_idl *ovnsb_idl, > > return true; > > } > > > > +static void > > +store_chassis_index_if_needed( > > + const struct ovsrec_open_vswitch_table *ovs_table) > > +{ > > + const struct ovsrec_open_vswitch *cfg = > > + ovsrec_open_vswitch_table_first(ovs_table); > > + const char *chassis_id = get_ovs_chassis_id(ovs_table); > > + > > + char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", chassis_id); > > + const char *chassis_idx = smap_get(&cfg->other_config, idx_key); > > + if (!chassis_idx) { > > + /* collect all indices so far consumed by other chassis */ > > + struct sset used_indices = SSET_INITIALIZER(&used_indices); > > + struct smap_node *node; > > + SMAP_FOR_EACH (node, &cfg->other_config) { > > + if (!strncmp(node->key, CHASSIS_IDX_PREFIX, 16)) { > > Nit: Using a bare "16" here is not fantastic. A couple of ways to > improve this would be to use: > * sizeof(CHASSIS_IDX_PREFIX) - 1 > * strlen(CHASSIS_IDX_PREFIX) > If you use strlen(), you'll want to evaluate that outside the loop and > set a local variable to that value, then use the local variable in the > loop. > > size_t len = strlen(CHASSIS_IDX_PREFIX); > SMAP_FOR_EACH(node, &cfg->other_config) { > if (!strncmp(node->key, CHASSIS_IDX_PREFIX, len) { > } > > > + sset_add(&used_indices, node->value); > > + } > > + } > > + /* first chassis on the host */ > > + if (!sset_contains(&used_indices, "")) { > > I think it's a bit strange that the first chassis index is the empty > string. Why not just start with "0" ? > This is to avoid renaming tunnel ports for the base case of a single controller running on a node. I should add a comment explaining it. > > + ovsrec_open_vswitch_update_other_config_setkey( > > + cfg, idx_key, ""); > > + goto out; > > + } > > + /* next chassis get an alphanum index allocated */ > > + char idx[] = "0"; > > + for (char i = '0'; i <= '9'; i++) { > > + idx[0] = i; > > + if (!sset_contains(&used_indices, idx)) { > > + ovsrec_open_vswitch_update_other_config_setkey( > > + cfg, idx_key, idx); > > + goto out; > > + } > > + } > > + for (char i = 'a'; i <= 'z'; i++) { > > + idx[0] = i; > > + if (!sset_contains(&used_indices, idx)) { > > + ovsrec_open_vswitch_update_other_config_setkey( > > + cfg, idx_key, idx); > > + goto out; > > + } > > + } > > + /* all indices consumed; it's safer to abort */ > > + VLOG_ERR("All unique controller indices consumed. Exiting."); > > + exit(EXIT_FAILURE); > > + } > > +out: > > + free(idx_key); > > +} > > + > > int > > main(int argc, char *argv[]) > > { > > @@ -4078,6 +4129,12 @@ main(int argc, char *argv[]) > > } > > } > > > > + static bool chassis_idx_stored = false; > > + if (ovs_idl_txn && !chassis_idx_stored) { > > + store_chassis_index_if_needed(ovs_table); > > + chassis_idx_stored = true; > > + } > > + > > if (ovsdb_idl_has_ever_connected(ovnsb_idl_loop.idl) && > > northd_version_match) { > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
