OVN checks whether ovn-installed is already present (in OVS) before updating it. This might cause ovn-installed related issues in the following case: - (1) ovn-installed is present - (2) we claim the interface - (3) we update ovs, removing ovn-installed and start installing flows - (4) (next loop), after flows installed, we check if ovn-installed is absent, and if yes, we update OVS with ovn-installed. However, in step4, if OVS is still busy from step 3, ovn-installed is read as present; hence OVN controller does not update it and move to INSTALLED state.
Note that this does not happen with writing port up in SBDB because Port status changes will hit I-P. This patch will require updating the state machine description in the "controller: avoid recomputes triggered by SBDB Port_Binding updates." patch when it's merged, as it changes the if-status state-machine. Signed-off-by: Xavier Simonart <[email protected]> --- controller/binding.c | 35 ++++++++++++++++++++++++++++++++-- controller/binding.h | 6 ++++++ controller/if-status.c | 43 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 2279570f9..157c381cf 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -643,6 +643,19 @@ local_binding_get_lport_ofport(const struct shash *local_bindings, u16_to_ofp(lbinding->iface->ofport[0]) : 0; } +bool +local_binding_is_ovn_installed(struct shash *local_bindings, + const char *pb_name) +{ + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + if (lbinding && lbinding->iface) { + return smap_get_bool(&lbinding->iface->external_ids, + OVN_INSTALLED_EXT_ID, false); + } + return false; +} + bool local_binding_is_up(struct shash *local_bindings, const char *pb_name) { @@ -715,6 +728,22 @@ local_binding_set_up(struct shash *local_bindings, const char *pb_name, } } +void +local_binding_remove_ovn_installed(struct shash *local_bindings, + const char *pb_name, bool ovs_readonly) +{ + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + + if (!ovs_readonly && lbinding && lbinding->iface + && smap_get_bool(&lbinding->iface->external_ids, + OVN_INSTALLED_EXT_ID, false)) { + VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name); + ovsrec_interface_update_external_ids_delkey(lbinding->iface, + OVN_INSTALLED_EXT_ID); + } +} + void local_binding_set_down(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, @@ -1297,9 +1326,11 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, const char *requested_chassis_option = smap_get( &pb->options, "requested-chassis"); VLOG_INFO_RL(&rl, - "Not claiming lport %s, chassis %s requested-chassis %s", + "Not claiming lport %s, chassis %s requested-chassis %s " + "pb->chassis %s", pb->logical_port, b_ctx_in->chassis_rec->name, - requested_chassis_option ? requested_chassis_option : "[]"); + requested_chassis_option ? requested_chassis_option : "[]", + pb->chassis ? pb->chassis->name: ""); } } diff --git a/controller/binding.h b/controller/binding.h index 1fed06674..445bdd9f2 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -152,6 +152,12 @@ 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_ovn_installed(struct shash *local_bindings, + const char *pb_name); +void local_binding_remove_ovn_installed(struct shash *local_bindings, + const char *pb_name, + bool ovs_readonly); + bool local_binding_is_down(struct shash *local_bindings, const char *pb_name); void local_binding_set_up(struct shash *local_bindings, const char *pb_name, const struct sbrec_chassis *chassis_rec, diff --git a/controller/if-status.c b/controller/if-status.c index ad61844d8..fe544c6ec 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -57,6 +57,9 @@ enum if_state { OIF_INSTALL_FLOWS, /* Already claimed interface for which flows are still * being installed. */ + OIF_REMOVE_OVN_INST,/* Interface with flows successfully installed in OVS, + * but with ovn-installed still in OVSDB. + */ OIF_MARK_UP, /* Interface with flows successfully installed in OVS * but not yet marked "up" in the binding module (in * SB and OVS databases). @@ -73,6 +76,7 @@ enum if_state { static const char *if_state_names[] = { [OIF_CLAIMED] = "CLAIMED", [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", + [OIF_REMOVE_OVN_INST] = "REMOVE_OVN_INST", [OIF_MARK_UP] = "MARK_UP", [OIF_MARK_DOWN] = "MARK_DOWN", [OIF_INSTALLED] = "INSTALLED", @@ -169,6 +173,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, const char *iface_id) switch (iface->state) { case OIF_CLAIMED: case OIF_INSTALL_FLOWS: + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: /* Nothing to do here. */ break; @@ -199,6 +204,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) /* Not yet fully installed interfaces can be safely deleted. */ ovs_iface_destroy(mgr, iface); break; + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: /* Properly mark interfaces "down" if their flows were already @@ -230,6 +236,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) /* Not yet fully installed interfaces can be safely deleted. */ ovs_iface_destroy(mgr, iface); break; + case OIF_REMOVE_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: /* Properly mark interfaces "down" if their flows were already @@ -257,6 +264,17 @@ if_status_mgr_update(struct if_status_mgr *mgr, struct shash *bindings = &binding_data->bindings; struct hmapx_node *node; + /* Move all interfaces that have been confirmed without ovn-installed, + * from OIF_REMOVE_OVN_INST to OIF_MARK_UP. + */ + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) { + struct ovs_iface *iface = node->data; + + if (!local_binding_is_ovn_installed(bindings, iface->id)) { + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + } + } + /* Move all interfaces that have been confirmed "up" by the binding module, * from OIF_MARK_UP to OIF_INSTALLED. */ @@ -325,7 +343,19 @@ if_status_mgr_run(struct if_status_mgr *mgr, iface->install_seqno)) { continue; } - ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + /* Wait for ovn-installed to be absent before moving to MARK_UP state. + * Most of the times ovn-installed is already absent and hence we will + * not have to wait. + * If there is no binding_data, we can't determine if ovn-installed is + * present or not; hence also go to the OIF_REMOVE_OVN_INST state. + */ + if (!binding_data || + local_binding_is_ovn_installed(&binding_data->bindings, + iface->id)) { + ovs_iface_set_state(mgr, iface, OIF_REMOVE_OVN_INST); + } else { + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); + } } ofctrl_acked_seqnos_destroy(acked_seqnos); @@ -412,7 +442,16 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr, sb_readonly, ovs_readonly); } - /* Notifiy the binding module to set "up" all bindings that have had + /* Notify the binding module to remove "ovn-installed" for all bindings + * in the OIF_REMOVE_OVN_INST state. + */ + HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) { + struct ovs_iface *iface = node->data; + + local_binding_remove_ovn_installed(bindings, iface->id, ovs_readonly); + } + + /* Notify the binding module to set "up" all bindings that have had * their flows installed but are not yet marked "up" in the binding * module. */ -- 2.31.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
