Thanks Han for your comments. I've handled what you spotted + added more scenarios, for 3 chassis, switching roles of chassis from main to additional and back, to have some chassis not claiming the binding. I hope this resolves your concerns.
Ihar On Wed, May 11, 2022 at 2:22 AM Han Zhou <[email protected]> wrote: > > > > On Thu, May 5, 2022 at 6:38 AM Ihar Hrachyshka <[email protected]> wrote: > > > > When the option is set to a comma separated list of chassis names, OVN > > will attempt to bind the port at any number of other locations in > > addition to the main chassis. > > > > This is useful in live migration scenarios where it's important to > > prepare the environment for workloads to move to, avoiding costly flow > > configuration at the moment of the final port binding location change. > > > > This may also be useful in port mirroring scenarios. > > > > Corresponding database fields (pb->additional_chassis, > > pb->requested_additional_chassis, pb->additional_encap) are introduced > > as part of the patch. These are similar to fields designed to track the > > main chassis (->chassis, ->requested_chassis, ->encap). But because we > > support any number of additional chassis, these fields are lists. > > > > Signed-off-by: Ihar Hrachyshka <[email protected]> > > Thanks Ihar. Please check my comments below. > (Please allow me some more time to review the rest of the patches in this > series.) > > > --- > > NEWS | 1 + > > controller/binding.c | 276 ++++++++++++++++++++++++++++++++++++------- > > controller/lport.c | 42 ++++--- > > northd/northd.c | 62 ++++++---- > > northd/ovn-northd.c | 4 +- > > ovn-nb.xml | 20 +++- > > ovn-sb.ovsschema | 17 ++- > > ovn-sb.xml | 63 ++++++++-- > > tests/ovn.at | 89 ++++++++++++++ > > 9 files changed, 476 insertions(+), 98 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index dbe89e9cf..3fedcfeed 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -7,6 +7,7 @@ Post v22.03.0 > > - Support NAT for logical routers with multiple distributed gateway > > ports. > > - Add global option (NB_Global.options:default_acl_drop) to enable > > implicit drop behavior on logical switches with ACLs applied. > > + - Support list of chassis for > > Logical_Switch_Port:options:requested-chassis. > > > > OVN v22.03.0 - 11 Mar 2022 > > -------------------------- > > diff --git a/controller/binding.c b/controller/binding.c > > index e8c96a64a..20f0fd11b 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -922,6 +922,142 @@ claimed_lport_set_up(const struct sbrec_port_binding > > *pb, > > } > > } > > > > +typedef void (*set_func)(const struct sbrec_port_binding *pb, > > + const struct sbrec_encap *); > > + > > +static bool > > +update_port_encap_if_needed(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec, > > + const struct ovsrec_interface *iface_rec, > > + bool sb_readonly) > > +{ > > + const struct sbrec_encap *encap_rec = > > + sbrec_get_port_encap(chassis_rec, iface_rec); > > + if ((encap_rec && pb->encap != encap_rec) || > > + (!encap_rec && pb->encap)) { > > + if (sb_readonly) { > > + return false; > > + } > > + sbrec_port_binding_set_encap(pb, encap_rec); > > + } > > + return true; > > +} > > + > > +static void > > +append_additional_encap(const struct sbrec_port_binding *pb, > > + const struct sbrec_encap *encap) > > +{ > > + struct sbrec_encap **additional_encap = xmalloc( > > + (pb->n_additional_encap + 1) * (sizeof *additional_encap)); > > + memcpy(additional_encap, pb->additional_encap, > > + pb->n_additional_encap * (sizeof *additional_encap)); > > + additional_encap[pb->n_additional_encap] = (struct sbrec_encap *) > > encap; > > + sbrec_port_binding_set_additional_encap( > > + pb, additional_encap, pb->n_additional_encap + 1); > > + free(additional_encap); > > +} > > + > > +static void > > +remove_additional_encap_for_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis > > *chassis_rec) > > +{ > > + struct sbrec_encap **additional_encap = xmalloc( > > + pb->n_additional_encap * (sizeof *additional_encap)); > > + > > + int idx = 0; > > + for (int i = 0; i < pb->n_additional_encap; i++) { > > + if (!strcmp(pb->additional_encap[i]->chassis_name, > > + chassis_rec->name)) { > > + continue; > > + } > > + additional_encap[idx++] = pb->additional_encap[i]; > > + } > > + sbrec_port_binding_set_additional_encap(pb, additional_encap, idx); > > + free(additional_encap); > > +} > > + > > +static bool > > +update_port_additional_encap_if_needed( > > + const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec, > > + const struct ovsrec_interface *iface_rec, > > + bool sb_readonly) > > +{ > > + const struct sbrec_encap *encap_rec = > > + sbrec_get_port_encap(chassis_rec, iface_rec); > > + if (encap_rec) { > > + for (int i = 0; i < pb->n_additional_encap; i++) { > > + if (pb->additional_encap[i] == encap_rec) { > > + return true; > > + } > > + } > > + if (sb_readonly) { > > + return false; > > + } > > + append_additional_encap(pb, encap_rec); > > + } > > + return true; > > +} > > + > > +static bool > > +is_additional_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec) > > +{ > > + for (int i = 0; i < pb->n_additional_chassis; i++) { > > + if (pb->additional_chassis[i] == chassis_rec) { > > + return true; > > + } > > + } > > + return false; > > +} > > + > > +static bool > > +is_requested_additional_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec) > > +{ > > + for (int i = 0; i < pb->n_requested_additional_chassis; i++) { > > + if (pb->requested_additional_chassis[i] == chassis_rec) { > > + return true; > > + } > > + } > > + return false; > > +} > > + > > +static void > > +append_additional_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec) > > +{ > > + struct sbrec_chassis **additional_chassis = xmalloc( > > + (pb->n_additional_chassis + 1) * (sizeof *additional_chassis)); > > + memcpy(additional_chassis, pb->additional_chassis, > > + pb->n_additional_chassis * (sizeof *additional_chassis)); > > + additional_chassis[pb->n_additional_chassis] = ( > > + (struct sbrec_chassis *) chassis_rec); > > + sbrec_port_binding_set_additional_chassis( > > + pb, additional_chassis, pb->n_additional_chassis + 1); > > + free(additional_chassis); > > +} > > + > > +static void > > +remove_additional_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec) > > +{ > > + struct sbrec_chassis **additional_chassis = xmalloc( > > + (pb->n_additional_chassis - 1) * (sizeof *additional_chassis)); > > + int idx = 0; > > + for (int i = 0; i < pb->n_additional_chassis; i++) { > > + if (pb->additional_chassis[i] == chassis_rec) { > > + continue; > > + } > > + additional_chassis[idx++] = pb->additional_chassis[i]; > > + } > > + sbrec_port_binding_set_additional_chassis( > > + pb, additional_chassis, pb->n_additional_chassis - 1); > > + free(additional_chassis); > > + > > + remove_additional_encap_for_chassis(pb, chassis_rec); > > +} > > + > > /* Returns false if lport is not claimed due to 'sb_readonly'. > > * Returns true otherwise. > > */ > > @@ -938,38 +1074,56 @@ claim_lport(const struct sbrec_port_binding *pb, > > claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, > > if_mgr); > > } > > > > - if (pb->chassis != chassis_rec) { > > - if (sb_readonly) { > > - return false; > > - } > > + if (!pb->requested_chassis || pb->requested_chassis == chassis_rec) { > > According to the logic in lport_can_bind_on_this_chassis(), it is possible > that the requested_chassis is NULL but "requested-chassis" option has this > chassis as the main chassis, we still need to claim the port. > > > + if (pb->chassis != chassis_rec) { > > + if (sb_readonly) { > > + return false; > > + } > > > > - if (pb->chassis) { > > - VLOG_INFO("Changing chassis for lport %s from %s to %s.", > > - pb->logical_port, pb->chassis->name, > > - chassis_rec->name); > > - } else { > > - VLOG_INFO("Claiming lport %s for this chassis.", > > pb->logical_port); > > - } > > - for (int i = 0; i < pb->n_mac; i++) { > > - VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); > > + if (pb->chassis) { > > + VLOG_INFO("Changing chassis for lport %s from %s to %s.", > > + pb->logical_port, pb->chassis->name, > > + chassis_rec->name); > > + } else { > > + VLOG_INFO("Claiming lport %s for this chassis.", > > + pb->logical_port); > > + } > > + for (int i = 0; i < pb->n_mac; i++) { > > + VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); > > + } > > + > > + sbrec_port_binding_set_chassis(pb, chassis_rec); > > + if (is_additional_chassis(pb, chassis_rec)) { > > + remove_additional_chassis(pb, chassis_rec); > > + } > > } > > + } else if (is_requested_additional_chassis(pb, chassis_rec)) { > > Similar to the above, need to check the "requested-chassis" option as well. > It would better that "lport_can_bind_on_this_chassis" returns the information > (CANNOT_BIND, CAN_BIND_AS_MAIN, CAN_BIND_AS_ADDITIONAL) directly and pass the > information, so that we don't have to repeat the checking here. > > > + if (!is_additional_chassis(pb, chassis_rec)) { > > + if (sb_readonly) { > > + return false; > > + } > > > > - sbrec_port_binding_set_chassis(pb, chassis_rec); > > + VLOG_INFO("Claiming lport %s for this additional chassis.", > > + pb->logical_port); > > + for (int i = 0; i < pb->n_mac; i++) { > > + VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]); > > + } > > In the previous branch it checked if it is in additional chassis then remove > it from there. Similarly, shall we checked here in this branch if it is the > current chassis then unset it? Or are there any considerations? > It would be better to add test cases to cover the scenario that a LSP's > requested-chassis and additional-chassis are updated back and force, > including below combinations: > - request-chassis is set but it not claimed, additional chassis is set and > claimed > - a chassis is moved from requested-chassis to requested-additional-chassis > for the LSP > - a chassis is moved from requested-additional-chassis to requested-chassis > for the LSP > etc. > > > > > - if (tracked_datapaths) { > > - update_lport_tracking(pb, tracked_datapaths, true); > > + append_additional_chassis(pb, chassis_rec); > > } > > } > > > > + if (tracked_datapaths) { > > + update_lport_tracking(pb, tracked_datapaths, true); > > + } > > In the original logic, it is added to the tracked_datapaths here only if the > port is newly claimed, but now it is blindly added, which looks incorrect and > could lead to incremental processing problems. > > > + > > /* Check if the port encap binding, if any, has changed */ > > - struct sbrec_encap *encap_rec = > > - sbrec_get_port_encap(chassis_rec, iface_rec); > > - if ((encap_rec && pb->encap != encap_rec) || > > - (!encap_rec && pb->encap)) { > > - if (sb_readonly) { > > - return false; > > - } > > - sbrec_port_binding_set_encap(pb, encap_rec); > > + if (pb->chassis == chassis_rec) { > > + return update_port_encap_if_needed( > > + pb, chassis_rec, iface_rec, sb_readonly); > > + } else if (is_additional_chassis(pb, chassis_rec)) { > > + return update_port_additional_encap_if_needed( > > + pb, chassis_rec, iface_rec, sb_readonly); > > } > > > > return true; > > @@ -983,7 +1137,8 @@ claim_lport(const struct sbrec_port_binding *pb, > > * Caller should make sure that this is the case. > > */ > > static bool > > -release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly) > > +release_lport_main_chassis(const struct sbrec_port_binding *pb, > > + bool sb_readonly) > > { > > if (pb->encap) { > > if (sb_readonly) { > > @@ -1011,11 +1166,42 @@ release_lport_(const struct sbrec_port_binding *pb, > > bool sb_readonly) > > } > > > > static bool > > -release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, > > +release_lport_additional_chassis(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec, > > + bool sb_readonly) > > +{ > > + if (pb->additional_encap) { > > + if (sb_readonly) { > > + return false; > > + } > > + remove_additional_encap_for_chassis(pb, chassis_rec); > > + } > > + > > + if (is_additional_chassis(pb, chassis_rec)) { > > + if (sb_readonly) { > > + return false; > > + } > > + remove_additional_chassis(pb, chassis_rec); > > + } > > + > > + VLOG_INFO("Releasing lport %s from this additional chassis.", > > + pb->logical_port); > > + return true; > > +} > > + > > +static bool > > +release_lport(const struct sbrec_port_binding *pb, > > + const struct sbrec_chassis *chassis_rec, bool sb_readonly, > > struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr) > > { > > - if (!release_lport_(pb, sb_readonly)) { > > - return false; > > + if (pb->chassis == chassis_rec) { > > + if (!release_lport_main_chassis(pb, sb_readonly)) { > > + return false; > > + } > > + } else if (is_additional_chassis(pb, chassis_rec)) { > > + if (!release_lport_additional_chassis(pb, chassis_rec, > > sb_readonly)) { > > + return false; > > + } > > } > > > > update_lport_tracking(pb, tracked_datapaths, false); > > @@ -1034,7 +1220,8 @@ is_binding_lport_this_chassis(struct binding_lport > > *b_lport, > > const struct sbrec_chassis *chassis) > > { > > return (b_lport && b_lport->pb && chassis && > > - b_lport->pb->chassis == chassis); > > + (b_lport->pb->chassis == chassis > > + || is_additional_chassis(b_lport->pb, chassis))); > > } > > > > /* Returns 'true' if the 'lbinding' has binding lports of type > > LP_CONTAINER, > > @@ -1059,7 +1246,7 @@ release_binding_lport(const struct sbrec_chassis > > *chassis_rec, > > { > > if (is_binding_lport_this_chassis(b_lport, chassis_rec)) { > > remove_related_lport(b_lport->pb, b_ctx_out); > > - if (!release_lport(b_lport->pb, sb_readonly, > > + if (!release_lport(b_lport->pb, chassis_rec, sb_readonly, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr)) { > > return false; > > @@ -1108,22 +1295,21 @@ consider_vif_lport_(const struct sbrec_port_binding > > *pb, > > } else { > > /* We could, but can't claim the lport. */ > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > - VLOG_INFO_RL(&rl, > > - "Not claiming lport %s, chassis %s " > > - "requested-chassis %s", > > - pb->logical_port, > > - b_ctx_in->chassis_rec->name, > > - pb->requested_chassis ? > > - pb->requested_chassis->name : "(option points > > at " > > - "non-existent " > > - "chassis)"); > > + const char *requested_chassis_option = smap_get( > > + &pb->options, "requested-chassis"); > > + VLOG_INFO_RL(&rl, > > + "Not claiming lport %s, chassis %s requested-chassis %s", > > + pb->logical_port, b_ctx_in->chassis_rec->name, > > + requested_chassis_option ? requested_chassis_option : > > "[]"); > > } > > } > > > > - if (pb->chassis == b_ctx_in->chassis_rec) { > > + if (pb->chassis == b_ctx_in->chassis_rec > > + || is_additional_chassis(pb, b_ctx_in->chassis_rec)) { > > /* Release the lport if there is no lbinding. */ > > if (!lbinding_set || !can_bind) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + return release_lport(pb, b_ctx_in->chassis_rec, > > + !b_ctx_in->ovnsb_idl_txn, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr); > > } > > @@ -1245,7 +1431,8 @@ consider_container_lport(const struct > > sbrec_port_binding *pb, > > * if it was bound earlier. */ > > if (is_binding_lport_this_chassis(container_b_lport, > > b_ctx_in->chassis_rec)) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + return release_lport(pb, b_ctx_in->chassis_rec, > > + !b_ctx_in->ovnsb_idl_txn, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr); > > } > > @@ -1339,7 +1526,7 @@ consider_localport(const struct sbrec_port_binding > > *pb, > > /* If the port binding is claimed, then release it as localport is > > claimed > > * by any ovn-controller. */ > > if (pb->chassis == b_ctx_in->chassis_rec) { > > - if (!release_lport_(pb, !b_ctx_in->ovnsb_idl_txn)) { > > + if (!release_lport_main_chassis(pb, !b_ctx_in->ovnsb_idl_txn)) { > > return false; > > } > > Shouldn't we also check here if the localport is claimed by this chassis as > an additional-chassis, release it? > > > > > @@ -1374,7 +1561,8 @@ consider_nonvif_lport_(const struct > > sbrec_port_binding *pb, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr); > > } else if (pb->chassis == b_ctx_in->chassis_rec) { > > - return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, > > + return release_lport(pb, b_ctx_in->chassis_rec, > > + !b_ctx_in->ovnsb_idl_txn, > > b_ctx_out->tracked_dp_bindings, > > b_ctx_out->if_mgr); > > Similar to the above scenario, should we check is_additional_chassis() here? > > Thanks, > Han > > > > > > } > > diff --git a/controller/lport.c b/controller/lport.c > > index 5ad40f6d3..ed63608a2 100644 > > --- a/controller/lport.c > > +++ b/controller/lport.c > > @@ -112,25 +112,35 @@ bool > > lport_can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec, > > const struct sbrec_port_binding *pb) > > { > > - /* We need to check for presence of the requested-chassis option in > > - * addittion to checking the pb->requested_chassis column because this > > - * column will be set to NULL whenever the option points to a > > non-existent > > - * chassis. As the controller routinely clears its own chassis record > > this > > - * might occur more often than one might think. */ > > + if (pb->requested_chassis == chassis_rec) { > > + return true; > > + } > > + > > + for (int i = 0; i < pb->n_requested_additional_chassis; i++) { > > + if (pb->requested_additional_chassis[i] == chassis_rec) { > > + return true; > > + } > > + } > > + > > const char *requested_chassis_option = smap_get(&pb->options, > > "requested-chassis"); > > - if (requested_chassis_option && requested_chassis_option[0] > > - && !pb->requested_chassis) { > > - /* The requested-chassis option is set, but the requested_chassis > > - * column is not filled. This means that the chassis the option > > - * points to is currently not running, or is in the process of > > starting > > - * up. In this case we must fall back to comparing the strings to > > - * avoid release/claim thrashing. */ > > - return !strcmp(requested_chassis_option, chassis_rec->name) > > - || !strcmp(requested_chassis_option, chassis_rec->hostname); > > + if (!requested_chassis_option || !strcmp("", > > requested_chassis_option)) { > > + return true; > > + } > > + > > + char *tokstr = xstrdup(requested_chassis_option); > > + char *save_ptr = NULL; > > + char *chassis; > > + for (chassis = strtok_r(tokstr, ",", &save_ptr); chassis != NULL; > > + chassis = strtok_r(NULL, ",", &save_ptr)) { > > + if (!strcmp(chassis, chassis_rec->name) > > + || !strcmp(chassis, chassis_rec->hostname)) { > > + free(tokstr); > > + return true; > > + } > > } > > - return !requested_chassis_option || !requested_chassis_option[0] > > - || chassis_rec == pb->requested_chassis; > > + free(tokstr); > > + return false; > > } > > > > const struct sbrec_datapath_binding * > > diff --git a/northd/northd.c b/northd/northd.c > > index a56666297..55919188e 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3177,31 +3177,53 @@ ovn_port_update_sbrec_chassis( > > const struct ovn_port *op) > > { > > const char *requested_chassis; /* May be NULL. */ > > - bool reset_requested_chassis = false; > > + > > + int n_requested_chassis = 0; > > + struct sbrec_chassis **requested_chassis_sb = xcalloc( > > + n_requested_chassis, sizeof *requested_chassis_sb); > > + > > requested_chassis = smap_get(&op->nbsp->options, > > "requested-chassis"); > > if (requested_chassis) { > > - const struct sbrec_chassis *chassis = chassis_lookup( > > - sbrec_chassis_by_name, sbrec_chassis_by_hostname, > > - requested_chassis); > > - if (chassis) { > > - sbrec_port_binding_set_requested_chassis(op->sb, chassis); > > - } else { > > - reset_requested_chassis = true; > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT( > > - 1, 1); > > - VLOG_WARN_RL( > > - &rl, > > - "Unknown chassis '%s' set as " > > - "options:requested-chassis on LSP '%s'.", > > - requested_chassis, op->nbsp->name); > > - } > > - } else if (op->sb->requested_chassis) { > > - reset_requested_chassis = true; > > - } > > - if (reset_requested_chassis) { > > + char *tokstr = xstrdup(requested_chassis); > > + char *save_ptr = NULL; > > + char *chassis; > > + for (chassis = strtok_r(tokstr, ",", &save_ptr); chassis != NULL; > > + chassis = strtok_r(NULL, ",", &save_ptr)) { > > + const struct sbrec_chassis *chassis_sb = chassis_lookup( > > + sbrec_chassis_by_name, sbrec_chassis_by_hostname, chassis); > > + if (chassis_sb) { > > + requested_chassis_sb = xrealloc( > > + requested_chassis_sb, > > + ++n_requested_chassis * (sizeof > > *requested_chassis_sb)); > > + requested_chassis_sb[n_requested_chassis - 1] = ( > > + (struct sbrec_chassis *) chassis_sb); > > + } else { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT( > > + 1, 1); > > + VLOG_WARN_RL( > > + &rl, > > + "Unknown chassis '%s' set in " > > + "options:requested-chassis on LSP '%s'.", > > + chassis, op->nbsp->name); > > + } > > + } > > + free(tokstr); > > + } > > + > > + if (n_requested_chassis > 0) { > > + sbrec_port_binding_set_requested_chassis(op->sb, > > + *requested_chassis_sb); > > + } else { > > sbrec_port_binding_set_requested_chassis(op->sb, NULL); > > } > > + if (n_requested_chassis > 1) { > > + sbrec_port_binding_set_requested_additional_chassis( > > + op->sb, &requested_chassis_sb[1], n_requested_chassis - 1); > > + } else { > > + sbrec_port_binding_set_requested_additional_chassis(op->sb, NULL, > > 0); > > + } > > + free(requested_chassis_sb); > > } > > > > static void > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 0a0f85010..1cfc2024d 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -100,7 +100,9 @@ static const char *rbac_fdb_update[] = > > static const char *rbac_port_binding_auth[] = > > {""}; > > static const char *rbac_port_binding_update[] = > > - {"chassis", "encap", "up", "virtual_parent"}; > > + {"chassis", "additional_chassis", > > + "encap", "additional_encap", > > + "up", "virtual_parent"}; > > > > static const char *rbac_mac_binding_auth[] = > > {""}; > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 9010240a8..3a5c7d3ec 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -1020,12 +1020,20 @@ > > </p> > > > > <column name="options" key="requested-chassis"> > > - If set, identifies a specific chassis (by name or hostname) that > > - is allowed to bind this port. Using this option will prevent > > - thrashing between two chassis trying to bind the same port during > > - a live migration. It can also prevent similar thrashing due to a > > - mis-configuration, if a port is accidentally created on more than > > - one chassis. > > + <p> > > + If set, identifies a specific chassis (by name or hostname) > > that > > + is allowed to bind this port. Using this option will prevent > > + thrashing between two chassis trying to bind the same port > > during > > + a live migration. It can also prevent similar thrashing due to > > a > > + mis-configuration, if a port is accidentally created on more > > than > > + one chassis. > > + </p> > > + > > + <p> > > + If set to a comma separated list, the first entry identifies > > the > > + main chassis and the rest are one or more additional chassis > > that > > + are allowed to bind the same port. > > + </p> > > </column> > > > > <column name="options" key="iface-id-ver"> > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index 66664c840..fcd74b918 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > "version": "20.22.0", > > - "cksum": "1686121686 27471", > > + "cksum": "2805878321 28417", > > "tables": { > > "SB_Global": { > > "columns": { > > @@ -218,10 +218,18 @@ > > "refTable": "Chassis", > > "refType": "weak"}, > > "min": 0, "max": 1}}, > > + "additional_chassis": {"type": {"key": {"type": "uuid", > > + "refTable": > > "Chassis", > > + "refType": "weak"}, > > + "min": 0, "max": > > "unlimited"}}, > > "encap": {"type": {"key": {"type": "uuid", > > "refTable": "Encap", > > "refType": "weak"}, > > "min": 0, "max": 1}}, > > + "additional_encap": {"type": {"key": {"type": "uuid", > > + "refTable": "Encap", > > + "refType": "weak"}, > > + "min": 0, "max": "unlimited"}}, > > "mac": {"type": {"key": "string", > > "min": 0, > > "max": "unlimited"}}, > > @@ -236,7 +244,12 @@ > > "requested_chassis": {"type": {"key": {"type": "uuid", > > "refTable": > > "Chassis", > > "refType": "weak"}, > > - "min": 0, "max": 1}}}, > > + "min": 0, "max": 1}}, > > + "requested_additional_chassis": { > > + "type": {"key": {"type": "uuid", > > + "refTable": > > "Chassis", > > + "refType": "weak"}, > > + "min": 0, "max": > > "unlimited"}}}, > > "indexes": [["datapath", "tunnel_key"], ["logical_port"]], > > "isRoot": true}, > > "MAC_Binding": { > > diff --git a/ovn-sb.xml b/ovn-sb.xml > > index 1ffb24e7a..ff9cb8c83 100644 > > --- a/ovn-sb.xml > > +++ b/ovn-sb.xml > > @@ -2867,9 +2867,17 @@ tcp.flags = RST; > > </column> > > > > <column name="encap"> > > - Points to supported encapsulation configurations to transmit > > - logical dataplane packets to this chassis. Each entry is a <ref > > - table="Encap"/> record that describes the configuration. > > + Points to preferred encapsulation configuration to transmit > > + logical dataplane packets to this chassis. The entry is reference > > to > > + a <ref table="Encap"/> record. > > + </column> > > + > > + <column name="additional_encap"> > > + Points to preferred encapsulation configuration to transmit > > + logical dataplane packets to this additional chassis. The entry is > > + reference to a <ref table="Encap"/> record. > > + > > + See also <ref column="additional_chassis"/>. > > </column> > > > > <column name="chassis"> > > @@ -2921,6 +2929,13 @@ tcp.flags = RST; > > > > </column> > > > > + <column name="additional_chassis"> > > + The meaning of this column is the same as for the > > + <ref column="chassis"/>. The column is used to track an additional > > + physical location of the logical port. Used with regular (empty > > + <ref column="type"/>) port bindings. > > + </column> > > + > > <column name="gateway_chassis"> > > <p> > > A list of <ref table="Gateway_Chassis"/>. > > @@ -3079,6 +3094,28 @@ tcp.flags = RST; > > db="OVN_Northbound"/> > > is defined and contains a string matching the name or hostname of > > an > > existing chassis. > > + > > + See also > > + <ref table="Port_Binding" column="requested_additional_chassis"/>. > > + </column> > > + <column name="requested_additional_chassis"> > > + This column exists so that the ovn-controller can effectively > > monitor > > + all <ref table="Port_Binding"/> records destined for it, and is a > > + supplement to the <ref > > + table="Port_Binding" > > + column="options" > > + key="requested-chassis"/> option when multiple chassis are listed. > > + > > + This column must be a list of > > + <ref table="Chassis"/> records. This is populated by > > + <code>ovn-northd</code> when the <ref > > + table="Logical_Switch_Port" > > + column="options" > > + key="requested-chassis" > > + db="OVN_Northbound"/> > > + is defined as a list of chassis names or hostnames. > > + > > + See also <ref table="Port_Binding" column="requested_chassis"/>. > > </column> > > </group> > > > > @@ -3238,12 +3275,20 @@ tcp.flags = RST; > > </p> > > > > <column name="options" key="requested-chassis"> > > - If set, identifies a specific chassis (by name or hostname) that > > - is allowed to bind this port. Using this option will prevent > > - thrashing between two chassis trying to bind the same port during > > - a live migration. It can also prevent similar thrashing due to a > > - mis-configuration, if a port is accidentally created on more than > > - one chassis. > > + <p> > > + If set, identifies a specific chassis (by name or hostname) that > > + is allowed to bind this port. Using this option will prevent > > + thrashing between two chassis trying to bind the same port during > > + a live migration. It can also prevent similar thrashing due to a > > + mis-configuration, if a port is accidentally created on more than > > + one chassis. > > + </p> > > + > > + <p> > > + If set to a comma separated list, the first entry identifies the > > main > > + chassis and the rest are one or more additional chassis that are > > + allowed to bind the same port. > > + </p> > > </column> > > > > <column name="options" key="iface-id-ver"> > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 799a6aabd..9210b256b 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -13917,6 +13917,95 @@ OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([options:multiple requested-chassis for logical port]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.11 > > + > > +sim_add hv2 > > +as hv2 > > +check ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.12 > > + > > +check ovn-nbctl ls-add ls0 > > +check ovn-nbctl lsp-add ls0 lsp0 > > + > > +# Allow only chassis hv1 to bind logical port lsp0. > > +check ovn-nbctl lsp-set-options lsp0 requested-chassis=hv1 > > + > > +as hv1 check ovs-vsctl -- add-port br-int lsp0 -- \ > > + set Interface lsp0 external-ids:iface-id=lsp0 > > +as hv2 check ovs-vsctl -- add-port br-int lsp0 -- \ > > + set Interface lsp0 external-ids:iface-id=lsp0 > > + > > +wait_row_count Chassis 1 name=hv1 > > +wait_row_count Chassis 1 name=hv2 > > +hv1_uuid=$(fetch_column Chassis _uuid name=hv1) > > +hv2_uuid=$(fetch_column Chassis _uuid name=hv2) > > + > > +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp0 > > +wait_column "$hv1_uuid" Port_Binding requested_chassis logical_port=lsp0 > > +wait_column "" Port_Binding additional_chassis logical_port=lsp0 > > +wait_column "" Port_Binding requested_additional_chassis logical_port=lsp0 > > + > > +# Request port binding at an additional chassis > > +check ovn-nbctl lsp-set-options lsp0 \ > > + requested-chassis=hv1,hv2 > > + > > +wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp0 > > +wait_column "$hv1_uuid" Port_Binding requested_chassis logical_port=lsp0 > > +wait_column "$hv2_uuid" Port_Binding additional_chassis logical_port=lsp0 > > +wait_column "$hv2_uuid" Port_Binding requested_additional_chassis > > logical_port=lsp0 > > + > > +# Check that setting iface:encap-ip populates Port_Binding:additional_encap > > +wait_row_count Encap 2 chassis_name=hv1 > > +wait_row_count Encap 2 chassis_name=hv2 > > +encap_hv1_uuid=$(fetch_column Encap _uuid chassis_name=hv1 type=geneve) > > +encap_hv2_uuid=$(fetch_column Encap _uuid chassis_name=hv2 type=geneve) > > + > > +wait_column "" Port_Binding encap logical_port=lsp0 > > +wait_column "" Port_Binding additional_encap logical_port=lsp0 > > + > > +as hv1 check ovs-vsctl -- \ > > + set Interface lsp0 external-ids:encap-ip=192.168.0.11 > > +as hv2 check ovs-vsctl -- \ > > + set Interface lsp0 external-ids:encap-ip=192.168.0.12 > > + > > +wait_column "$encap_hv1_uuid" Port_Binding encap logical_port=lsp0 > > +wait_column "$encap_hv2_uuid" Port_Binding additional_encap > > logical_port=lsp0 > > + > > +# Complete moving the binding to the new location > > +check ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2 > > + > > +wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0 > > +wait_column "$hv2_uuid" Port_Binding requested_chassis logical_port=lsp0 > > +wait_column "" Port_Binding additional_chassis logical_port=lsp0 > > +wait_column "" Port_Binding requested_additional_chassis logical_port=lsp0 > > + > > +# Check that additional_encap is cleared > > +wait_column "" Port_Binding additional_encap logical_port=lsp0 > > + > > +# Check that abrupted port migration clears additional_encap > > +check ovn-nbctl lsp-set-options lsp0 \ > > + requested-chassis=hv2,hv1 > > +wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0 > > +wait_column "$hv2_uuid" Port_Binding requested_chassis logical_port=lsp0 > > +wait_column "$hv1_uuid" Port_Binding additional_chassis logical_port=lsp0 > > +wait_column "$hv1_uuid" Port_Binding requested_additional_chassis > > logical_port=lsp0 > > +check ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2 > > +wait_column "" Port_Binding additional_encap logical_port=lsp0 > > + > > +OVN_CLEANUP([hv1],[hv2]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD([ > > AT_SETUP([options:requested-chassis for logical port]) > > ovn_start > > -- > > 2.34.1 > > > > _______________________________________________ > > 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
