When VIF ports are claimed on a chassis, SBDB Port_Binding table is updated.
If the SBDB IDL is still is read-only ("in transaction") when such a update
is required, the update is not possible and recompute is triggered through
I+P failure.This situation can happen: - after updating Port_Binding->chassis to SBDB for one port, in a following iteration, ovn-controller handles Interface:external_ids:ovn-installed (for the same port) while SBDB is still read-only. - after updating Port_Binding->chassis to SBDB for one port, in a following iteration, ovn-controller updates Port_Binding->chassis for another port, while SBDB is still read-only. This patch prevent the recompute, by having the if-status module updating the Port_Binding chassis (if needed), when possible. This does not delay Port_Binding chassis update compared to before this patch: Port_Binding chassis will be updated as soon as SBDB is again writable, as it was the case, through a recompute, before this patch. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253 Signed-off-by: Xavier Simonart <[email protected]> --- controller/binding.c | 124 ++++++++++++++++++++++++++---------- controller/binding.h | 9 ++- controller/if-status.c | 31 +++++++-- controller/if-status.h | 6 +- controller/ovn-controller.c | 6 +- tests/ovn.at | 55 +++++++++++++++- 6 files changed, 184 insertions(+), 47 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index e5ba56b25..d917d8775 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -657,11 +657,15 @@ local_binding_get_lport_ofport(const struct shash *local_bindings, } bool -local_binding_is_up(struct shash *local_bindings, const char *pb_name) +local_binding_is_up(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *chassis_rec) { struct local_binding *lbinding = local_binding_find(local_bindings, pb_name); struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding); + if (b_lport && b_lport->pb->chassis != chassis_rec) { + return false; + } if (lbinding && b_lport && lbinding->iface) { if (b_lport->pb->n_up && !b_lport->pb->up[0]) { return false; @@ -673,13 +677,23 @@ local_binding_is_up(struct shash *local_bindings, const char *pb_name) } bool -local_binding_is_down(struct shash *local_bindings, const char *pb_name) +local_binding_is_down(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *chassis_rec) { struct local_binding *lbinding = local_binding_find(local_bindings, pb_name); struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding); + if (b_lport) { + if (b_lport->pb->chassis == chassis_rec) { + return false; + } else if (b_lport->pb->chassis) { + VLOG_DBG("lport %s already claimed by other chassis", + b_lport->pb->logical_port); + } + } + if (!lbinding) { return true; } @@ -894,6 +908,48 @@ get_lport_type_str(enum en_lport_type lport_type) OVS_NOT_REACHED(); } +static void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, + const struct sbrec_chassis *chassis_rec) +{ + if (pb->chassis != chassis_rec) { + 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); + } +} + +static void clear_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb) +{ + if (pb->chassis) { + sbrec_port_binding_set_chassis(pb, NULL); + } +} + +void +local_binding_set_pb(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *chassis_rec) +{ + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + struct binding_lport *b_lport = local_binding_get_primary_lport(lbinding); + + if (b_lport) { + if (chassis_rec) { + set_pb_chassis_in_sbrec(b_lport->pb, chassis_rec); + } else { + clear_pb_chassis_in_sbrec(b_lport->pb); + } + } +} + /* For newly claimed ports, if 'notify_up' is 'false': * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'. * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for @@ -906,23 +962,37 @@ get_lport_type_str(enum en_lport_type lport_type) * it's explicitly set to 'false'. * This is to ensure compatibility with older versions of ovn-northd. */ -static void +static bool claimed_lport_set_up(const struct sbrec_port_binding *pb, const struct sbrec_port_binding *parent_pb, const struct sbrec_chassis *chassis_rec, - bool notify_up, struct if_status_mgr *if_mgr) + bool notify_up, struct if_status_mgr *if_mgr, + bool sb_readonly) { + if (!notify_up) { bool up = true; if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) { - sbrec_port_binding_set_up(pb, &up, 1); + if (!sb_readonly) { + sbrec_port_binding_set_up(pb, &up, 1); + } else if (pb->n_up && !pb->up[0]) { + return false; + } } - return; + if (sb_readonly && (pb->chassis != chassis_rec)) { + /* Handle the cases where if_status_mgr does not claim the + * interface.In those cases, if we do not set pb->chassis in sb + * now (as readonly), we might not do it later ... + */ + return false; + } + return true; } if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) { if_status_mgr_claim_iface(if_mgr, pb->logical_port); } + return true; } /* Returns false if lport is not claimed due to 'sb_readonly'. @@ -937,27 +1007,15 @@ claim_lport(const struct sbrec_port_binding *pb, struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr) { - if (!sb_readonly) { - claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, if_mgr); - } + if (!claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, + if_mgr, sb_readonly)) { + return false; + } 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); + if (!sb_readonly) { + set_pb_chassis_in_sbrec(pb, chassis_rec); } - 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 (tracked_datapaths) { update_lport_tracking(pb, tracked_datapaths, true); @@ -974,7 +1032,6 @@ claim_lport(const struct sbrec_port_binding *pb, } sbrec_port_binding_set_encap(pb, encap_rec); } - return true; } @@ -986,7 +1043,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_(const struct sbrec_port_binding *pb, bool sb_readonly, + struct if_status_mgr *if_mgr) { if (pb->encap) { if (sb_readonly) { @@ -995,11 +1053,13 @@ release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly) sbrec_port_binding_set_encap(pb, NULL); } + /* if sb readonly, pb->chassis unset through if-status if present */ if (pb->chassis) { - if (sb_readonly) { + if (!sb_readonly) { + sbrec_port_binding_set_chassis(pb, NULL); + } else if (!if_status_mgr_iface_is_present(if_mgr, pb->logical_port)) { return false; } - sbrec_port_binding_set_chassis(pb, NULL); } if (pb->virtual_parent) { @@ -1009,7 +1069,8 @@ release_lport_(const struct sbrec_port_binding *pb, bool sb_readonly) sbrec_port_binding_set_virtual_parent(pb, NULL); } - VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port); + VLOG_INFO("Releasing lport %s from this chassis (sb_readonly=%d)", + pb->logical_port, sb_readonly); return true; } @@ -1017,7 +1078,7 @@ static bool release_lport(const struct sbrec_port_binding *pb, bool sb_readonly, struct hmap *tracked_datapaths, struct if_status_mgr *if_mgr) { - if (!release_lport_(pb, sb_readonly)) { + if (!release_lport_(pb, sb_readonly, if_mgr)) { return false; } @@ -1342,10 +1403,9 @@ 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_(pb, !b_ctx_in->ovnsb_idl_txn, b_ctx_out->if_mgr)) { return false; } - remove_related_lport(pb, b_ctx_out); } diff --git a/controller/binding.h b/controller/binding.h index 430a8d9b1..45202a321 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -151,14 +151,17 @@ const struct sbrec_port_binding *local_binding_get_primary_pb( ofp_port_t local_binding_get_lport_ofport(const struct shash *local_bindings, const char *pb_name); -bool local_binding_is_up(struct shash *local_bindings, const char *pb_name); -bool local_binding_is_down(struct shash *local_bindings, const char *pb_name); +bool local_binding_is_up(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *); +bool local_binding_is_down(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *); void local_binding_set_up(struct shash *local_bindings, const char *pb_name, const char *ts_now_str, bool sb_readonly, bool ovs_readonly); void local_binding_set_down(struct shash *local_bindings, const char *pb_name, bool sb_readonly, bool ovs_readonly); - +void local_binding_set_pb(struct shash *local_bindings, const char *pb_name, + const struct sbrec_chassis *chassis_rec); void binding_register_ovs_idl(struct ovsdb_idl *); void binding_run(struct binding_ctx_in *, struct binding_ctx_out *); bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn, diff --git a/controller/if-status.c b/controller/if-status.c index dbdf8b66f..e36df22a0 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -115,6 +115,7 @@ static void ovs_iface_set_state(struct if_status_mgr *, struct ovs_iface *, static void if_status_mgr_update_bindings( struct if_status_mgr *mgr, struct local_binding_data *binding_data, + const struct sbrec_chassis *chassis_rec, bool sb_readonly, bool ovs_readonly); struct if_status_mgr * @@ -181,6 +182,13 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char *iface_id) } } +bool +if_status_mgr_iface_is_present(struct if_status_mgr *mgr, const char *iface_id) +{ + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id); + return (!!iface); +} + void if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) { @@ -247,7 +255,8 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) void if_status_mgr_update(struct if_status_mgr *mgr, - struct local_binding_data *binding_data) + struct local_binding_data *binding_data, + const struct sbrec_chassis *chassis_rec) { if (!binding_data) { return; @@ -262,7 +271,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) { struct ovs_iface *iface = node->data; - if (local_binding_is_up(bindings, iface->id)) { + if (local_binding_is_up(bindings, iface->id, chassis_rec)) { ovs_iface_set_state(mgr, iface, OIF_INSTALLED); } } @@ -273,7 +282,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { struct ovs_iface *iface = node->data; - if (local_binding_is_down(bindings, iface->id)) { + if (local_binding_is_down(bindings, iface->id, chassis_rec)) { ovs_iface_destroy(mgr, iface); } } @@ -306,6 +315,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *binding_data, + const struct sbrec_chassis *chassis_rec, bool sb_readonly, bool ovs_readonly) { struct ofctrl_acked_seqnos *acked_seqnos = @@ -328,8 +338,8 @@ if_status_mgr_run(struct if_status_mgr *mgr, ofctrl_acked_seqnos_destroy(acked_seqnos); /* Update binding states. */ - if_status_mgr_update_bindings(mgr, binding_data, sb_readonly, - ovs_readonly); + if_status_mgr_update_bindings(mgr, binding_data, chassis_rec, + sb_readonly, ovs_readonly); } static void @@ -390,6 +400,7 @@ ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface, static void if_status_mgr_update_bindings(struct if_status_mgr *mgr, struct local_binding_data *binding_data, + const struct sbrec_chassis *chassis_rec, bool sb_readonly, bool ovs_readonly) { if (!binding_data) { @@ -404,6 +415,9 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, */ HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) { struct ovs_iface *iface = node->data; + if (!sb_readonly) { + local_binding_set_pb(bindings, iface->id, chassis_rec); + } local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly); } @@ -415,7 +429,9 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, char *ts_now_str = xasprintf("%lld", time_wall_msec()); HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) { struct ovs_iface *iface = node->data; - + if (!sb_readonly) { + local_binding_set_pb(bindings, iface->id, chassis_rec); + } local_binding_set_up(bindings, iface->id, ts_now_str, sb_readonly, ovs_readonly); } @@ -426,6 +442,9 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, */ HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { struct ovs_iface *iface = node->data; + if (!sb_readonly) { + local_binding_set_pb(bindings, iface->id, NULL); + } local_binding_set_down(bindings, iface->id, sb_readonly, ovs_readonly); } diff --git a/controller/if-status.h b/controller/if-status.h index ff4aa760e..2c55eb18e 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -31,10 +31,14 @@ void if_status_mgr_claim_iface(struct if_status_mgr *, const char *iface_id); void if_status_mgr_release_iface(struct if_status_mgr *, const char *iface_id); void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); -void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *); +void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *, + const struct sbrec_chassis *chassis); void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, + const struct sbrec_chassis *chassis, bool sb_readonly, bool ovs_readonly); void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, struct simap *usage); +bool if_status_mgr_iface_is_present(struct if_status_mgr *mgr, + const char *iface_id); # endif /* controller/if-status.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 5a6274eb2..994aebdfb 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -3912,7 +3912,7 @@ main(int argc, char *argv[]) runtime_data ? &runtime_data->lbinding_data : NULL; stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, time_msec()); - if_status_mgr_update(if_mgr, binding_data); + if_status_mgr_update(if_mgr, binding_data, chassis); stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, time_msec()); @@ -3938,8 +3938,8 @@ main(int argc, char *argv[]) time_msec()); stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME, time_msec()); - if_status_mgr_run(if_mgr, binding_data, !ovnsb_idl_txn, - !ovs_idl_txn); + if_status_mgr_run(if_mgr, binding_data, chassis, + !ovnsb_idl_txn, !ovs_idl_txn); stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, time_msec()); } diff --git a/tests/ovn.at b/tests/ovn.at index 6a0a169c1..403fbc85f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13934,6 +13934,7 @@ ovn_attach n1 br-phys 192.168.0.11 ovs-vsctl -- add-port br-int hv1-vif0 -- \ set Interface hv1-vif0 ofport-request=1 + sim_add hv2 as hv2 ovs-vsctl add-br br-phys @@ -13983,7 +13984,10 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep actions=output:1], echo "verifying that lsp0 binding moves when requested-chassis is changed" ovn-nbctl lsp-set-options lsp0 requested-chassis=hv2 -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this chassis" hv1/ovn-controller.log)]) + +# We might see multiple "Releasing lport ...", when sb is read only +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this chassis" hv1/ovn-controller.log)]) + wait_column "$hv2_uuid" Port_Binding chassis logical_port=lsp0 # (6) Chassis hv2 should add flows and hv1 should not. @@ -14029,7 +14033,7 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], [0], [ig AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=65 | grep actions=output:1], [0], [ignore]) check ovn-nbctl --wait=hv lsp-set-options lsp0 requested-chassis=non-existant-chassis -OVS_WAIT_UNTIL([test 1 = $(grep -c "Releasing lport lsp0 from this chassis" hv1/ovn-controller.log)]) +OVS_WAIT_UNTIL([test 1 -le $(grep -c "Releasing lport lsp0 from this chassis" hv1/ovn-controller.log)]) check ovn-nbctl --wait=hv sync wait_column '' Port_Binding chasssi logical_port=lsp0 AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=0 | grep in_port=1], [1], []) @@ -30595,3 +30599,50 @@ test_mac_binding_flows hv2 00\:00\:00\:00\:10\:10 1 OVN_CLEANUP([hv1], [hv2]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([recomputes]) +ovn_start + +# Add chassis +net_add n1 +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys +ovn_attach n1 br-phys 192.168.0.1 + +add_switch_ports() { + for port in $(seq $1 $2); do + logical_switch_port=lsp${port} + check ovn-nbctl lsp-add ls1 $logical_switch_port + check ovn-nbctl lsp-set-addresses $logical_switch_port dynamic + + as hv1 ovs-vsctl \ + -- add-port br-int vif${port} \ + -- set Interface vif${port} external_ids:iface-id=$logical_switch_port + done + + check ovn-nbctl --wait=hv sync + AT_CHECK([test $(ovn-appctl -t ovn-controller coverage/read-counter lflow_run) == $lflow_run]) +} + +check ovn-nbctl ls-add ls1 +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16 +check ovn-nbctl set Logical_Switch ls1 other_config:exclude_ips=10.1.255.254 + +check ovn-nbctl lr-add lr1 +check ovn-nbctl lsp-add ls1 lsp0 -- set Logical_Switch_Port lsp0 type=router options:router-port=lrp0 addresses=dynamic +check ovn-nbctl lrp-add lr1 lrp0 "f0:00:00:01:00:01" 10.1.255.254/16 +check ovn-nbctl lr-nat-add lr1 snat 10.2.0.1 10.1.0.0/16 + +check ovn-nbctl --wait=hv sync +lflow_run=$(ovn-appctl -t ovn-controller coverage/read-counter lflow_run) + +add_switch_ports 1 50 +add_switch_ports 51 100 +add_switch_ports 101 150 + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) -- 2.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
