Thanks Ales, Dumitru ! Dumitru, I'll work on the custom backport for 22.03 and 22.06. Thanks for applying to main.
Thanks Xavier On Mon, May 8, 2023 at 3:12 PM Dumitru Ceara <[email protected]> wrote: > On 5/5/23 11:21, Ales Musil wrote: > > On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <[email protected]> > wrote: > > > >> 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. > >> > >> ovn-installed was also not properly deleted on port or binding removal. > >> > >> Note that port->up is not properly removed on external_ids:iface-id > >> removal when > >> sb is read-only. This will be fixed in a further patch. > >> > >> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB > >> Port_Binding updates.") > >> > >> Signed-off-by: Xavier Simonart <[email protected]> > >> > >> --- > >> v2: - handled Dumitru's comments > >> - rebased on main > >> - updated state machine diagram as commented in v1 commit message > >> - remove ovn-installed on port deletion or external_ids:iface-id > >> removal. > >> - added test case > >> v3: - handled more comment from Dumitru > >> - fixed ovn-install not removed when ovs is read-only > >> - moved test case from unit (ovn.at) to system (system-ovn). > >> - test case connects to OVS db through tcp and uses iptables to > >> momentarly block the idl update > >> to simulate read-only ovsdb > >> v4: - handled comments from Ales: avoid using is_deleted outside of > >> tracked loops. > >> --- > >> controller/binding.c | 68 ++++++- > >> controller/binding.h | 11 ++ > >> controller/if-status.c | 204 ++++++++++++++++++--- > >> controller/if-status.h | 7 + > >> controller/ovn-controller.c | 5 + > >> tests/system-ovn.at | 349 ++++++++++++++++++++++++++++++++++++ > >> 6 files changed, 612 insertions(+), 32 deletions(-) > >> > >> diff --git a/controller/binding.c b/controller/binding.c > >> index 5df62baef..357e77609 100644 > >> --- a/controller/binding.c > >> +++ b/controller/binding.c > >> @@ -746,6 +746,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, > >> const struct sbrec_chassis *chassis_rec) > >> @@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings, > >> const char *pb_name, > >> } else if (b_lport->pb->chassis) { > >> VLOG_DBG("lport %s already claimed by other chassis", > >> b_lport->pb->logical_port); > >> + return true; > >> } > >> } > >> > >> @@ -834,6 +848,38 @@ local_binding_set_up(struct shash *local_bindings, > >> const char *pb_name, > >> } > >> } > >> > >> +void > >> +local_binding_remove_ovn_installed( > >> + struct shash *local_bindings, > >> + const struct ovsrec_interface_table *iface_table, > >> + const char *pb_name, bool ovs_readonly) > >> +{ > >> + if (ovs_readonly) { > >> + return; > >> + } > >> + struct local_binding *lbinding = > >> + local_binding_find(local_bindings, pb_name); > >> + if (lbinding && lbinding->iface) { > >> + const struct uuid *iface_uuid = &lbinding->iface->header_.uuid; > >> + remove_ovn_installed_for_uuid(iface_table, iface_uuid); > >> + } > >> +} > >> + > >> +void > >> +remove_ovn_installed_for_uuid(const struct ovsrec_interface_table > >> *iface_table, > >> + const struct uuid *iface_uuid) > >> +{ > >> + const struct ovsrec_interface *iface_rec = > >> + ovsrec_interface_table_get_for_uuid(iface_table, iface_uuid); > >> + if (iface_rec && smap_get_bool(&iface_rec->external_ids, > >> + OVN_INSTALLED_EXT_ID, false)) { > >> + VLOG_INFO("Removing iface %s ovn-installed in OVS", > >> + iface_rec->name); > >> + ovsrec_interface_update_external_ids_delkey(iface_rec, > >> + > OVN_INSTALLED_EXT_ID); > >> + } > >> +} > >> + > >> void > >> local_binding_set_down(struct shash *local_bindings, const char > *pb_name, > >> const struct sbrec_chassis *chassis_rec, > >> @@ -1239,7 +1285,9 @@ claim_lport(const struct sbrec_port_binding *pb, > >> return false; > >> } > >> } else { > >> - if (pb->n_up && !pb->up[0]) { > >> + if ((pb->n_up && !pb->up[0]) || > >> + !smap_get_bool(&iface_rec->external_ids, > >> + OVN_INSTALLED_EXT_ID, false)) { > >> if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, > >> sb_readonly); > >> } > >> @@ -1464,9 +1512,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: ""); > >> } > >> } > >> > >> @@ -2288,6 +2338,11 @@ consider_iface_release(const struct > >> ovsrec_interface *iface_rec, > >> return false; > >> } > >> } > >> + if (lbinding->iface && lbinding->iface->name) { > >> + if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr, > >> + lbinding->iface->name, > >> + > >> &lbinding->iface->header_.uuid); > >> + } > >> > >> } else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) { > >> /* lbinding is associated with a localport. Remove it from the > >> @@ -2558,6 +2613,7 @@ handle_deleted_lport(const struct > sbrec_port_binding > >> *pb, > >> if (ld) { > >> remove_pb_from_local_datapath(pb, > >> b_ctx_out, ld); > >> + if_status_mgr_release_iface(b_ctx_out->if_mgr, > pb->logical_port); > >> return; > >> } > >> > >> @@ -2581,6 +2637,7 @@ handle_deleted_lport(const struct > sbrec_port_binding > >> *pb, > >> remove_pb_from_local_datapath(pb, b_ctx_out, > >> ld); > >> } > >> + if_status_mgr_release_iface(b_ctx_out->if_mgr, > pb->logical_port); > >> } > >> } > >> > >> @@ -2627,6 +2684,11 @@ handle_deleted_vif_lport(const struct > >> sbrec_port_binding *pb, > >> } > >> > >> handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > >> + if (lbinding && lbinding->iface && lbinding->iface->name) { > >> + if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr, > >> + lbinding->iface->name, > >> + > >> &lbinding->iface->header_.uuid); > >> + } > >> return true; > >> } > >> > >> diff --git a/controller/binding.h b/controller/binding.h > >> index 6c3a98b02..0b9c6994f 100644 > >> --- a/controller/binding.h > >> +++ b/controller/binding.h > >> @@ -159,6 +159,14 @@ 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, > >> const struct sbrec_chassis *); > >> > >> +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 struct ovsrec_interface_table *iface_table, > >> + const char *pb_name, > >> + bool ovs_readonly); > >> + > >> void local_binding_set_up(struct shash *local_bindings, const char > >> *pb_name, > >> const struct sbrec_chassis *chassis_rec, > >> const char *ts_now_str, bool sb_readonly, > >> @@ -195,6 +203,9 @@ void set_pb_chassis_in_sbrec(const struct > >> sbrec_port_binding *pb, > >> const struct sbrec_chassis *chassis_rec, > >> bool is_set); > >> > >> +void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table > *, > >> + const struct uuid *); > >> + > >> /* Corresponds to each Port_Binding.type. */ > >> enum en_lport_type { > >> LP_UNKNOWN, > >> diff --git a/controller/if-status.c b/controller/if-status.c > >> index d1c14ac30..ac36a9fb9 100644 > >> --- a/controller/if-status.c > >> +++ b/controller/if-status.c > >> @@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status); > >> */ > >> > >> enum if_state { > >> - OIF_CLAIMED, /* Newly claimed interface. pb->chassis update > not > >> yet > >> - initiated. */ > >> - OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update > sent > >> to > >> - * SB (but update notification not confirmed, so > >> the > >> - * update may be resent in any of the following > >> states) > >> - * and for which flows are still being > installed. > >> - */ > >> - OIF_MARK_UP, /* Interface with flows successfully installed > in > >> OVS > >> - * but not yet marked "up" in the binding module > >> (in > >> - * SB and OVS databases). > >> - */ > >> - OIF_MARK_DOWN, /* Released interface but not yet marked "down" > in > >> the > >> - * binding module (in SB and/or OVS databases). > >> - */ > >> - OIF_INSTALLED, /* Interface flows programmed in OVS and binding > >> marked > >> - * "up" in the binding module. > >> - */ > >> + OIF_CLAIMED, /* Newly claimed interface. pb->chassis > update > >> not > >> + yet initiated. */ > >> + OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update > >> sent to > >> + * SB (but update notification not confirmed, > >> so the > >> + * update may be resent in any of the > following > >> + * states and for which flows are still being > >> + * installed. > >> + */ > >> + OIF_REM_OLD_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). > >> + */ > >> + OIF_MARK_DOWN, /* Released interface but not yet marked > "down" > >> in > >> + * the binding module (in SB and/or OVS > >> databases). > >> + */ > >> + OIF_INSTALLED, /* Interface flows programmed in OVS and > binding > >> + * marked "up" in the binding module. > >> + */ > >> OIF_MAX, > >> }; > >> > >> static const char *if_state_names[] = { > >> - [OIF_CLAIMED] = "CLAIMED", > >> - [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", > >> - [OIF_MARK_UP] = "MARK_UP", > >> - [OIF_MARK_DOWN] = "MARK_DOWN", > >> - [OIF_INSTALLED] = "INSTALLED", > >> + [OIF_CLAIMED] = "CLAIMED", > >> + [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS", > >> + [OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST", > >> + [OIF_MARK_UP] = "MARK_UP", > >> + [OIF_MARK_DOWN] = "MARK_DOWN", > >> + [OIF_INSTALLED] = "INSTALLED", > >> }; > >> > >> /* > >> @@ -109,11 +114,26 @@ static const char *if_state_names[] = { > >> * | | | - remove ovn-installed from ovsdb > >> | | | > >> * | | | mgr_update() > >> | | | > >> * | +----------------------+ - sbrec_update_chassis if needed > >> | | | > >> - * | | > >> | | | > >> - * | | mgr_run(seqno rcvd) > >> | | | > >> - * | | - set port up in sb > >> | | | > >> - * | release_iface | - set ovn-installed in ovs > >> | | | > >> - * | V > >> | | | > >> + * | | | > >> | | | > >> + * | | +----------------------------------------+ > >> | | | > >> + * | | | > >> | | | > >> + * | | mgr_run(seqno rcvd, ovn-installed present) | > >> | | | > >> + * | V | > >> | | | > >> + * | +--------------------+ | > >> | | | > >> + * | | | mgr_run() | > >> | | | > >> + * +--- | REM_OLD_OVN_INST | - remove ovn-installed in ovs | > >> | | | > >> + * | +--------------------+ | > >> | | | > >> + * | | | > >> | | | > >> + * | | | > >> | | | > >> + * | | mgr_update( ovn_installed not present) | > >> | | | > >> + * | | | > >> | | | > >> + * | | +-------------------------------------------+ > >> | | | > >> + * | | | > >> | | | > >> + * | | | mgr_run(seqno rcvd, ovn-installed not present) > >> | | | > >> + * | | | - set port up in sb > >> | | | > >> + * | | | - set ovn-installed in ovs > >> | | | > >> + * |release_iface | | > >> | | | > >> + * | V V > >> | | | > >> * | +----------------------+ > >> | | | > >> * | | | mgr_run() > >> | | | > >> * +-- | MARK_UP | - set port up in sb > >> | | | > >> @@ -155,6 +175,9 @@ struct if_status_mgr { > >> /* All local interfaces, mapping from 'iface-id' to 'struct > >> ovs_iface'. */ > >> struct shash ifaces; > >> > >> + /* local interfaces which need ovn-install removal */ > >> + struct shash ovn_uninstall_hash; > >> + > >> /* All local interfaces, stored per state. */ > >> struct hmapx ifaces_per_state[OIF_MAX]; > >> > >> @@ -170,15 +193,20 @@ struct if_status_mgr { > >> static struct ovs_iface *ovs_iface_create(struct if_status_mgr *, > >> const char *iface_id, > >> enum if_state ); > >> +static void add_to_ovn_uninstall_hash(struct if_status_mgr *, const > char > >> *, > >> + const struct uuid *); > >> static void ovs_iface_destroy(struct if_status_mgr *, struct ovs_iface > *); > >> +static void ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char > >> *name); > >> static void ovs_iface_set_state(struct if_status_mgr *, struct > ovs_iface > >> *, > >> enum if_state); > >> > >> static void if_status_mgr_update_bindings( > >> struct if_status_mgr *mgr, struct local_binding_data *binding_data, > >> const struct sbrec_chassis *, > >> + const struct ovsrec_interface_table *iface_table, > >> bool sb_readonly, bool ovs_readonly); > >> > >> +static void ovn_uninstall_hash_account_mem(const char *name, bool > erase); > >> struct if_status_mgr * > >> if_status_mgr_create(void) > >> { > >> @@ -189,6 +217,7 @@ if_status_mgr_create(void) > >> hmapx_init(&mgr->ifaces_per_state[i]); > >> } > >> shash_init(&mgr->ifaces); > >> + shash_init(&mgr->ovn_uninstall_hash); > >> return mgr; > >> } > >> > >> @@ -202,6 +231,11 @@ if_status_mgr_clear(struct if_status_mgr *mgr) > >> } > >> ovs_assert(shash_is_empty(&mgr->ifaces)); > >> > >> + SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) { > >> + ovn_uninstall_hash_destroy(mgr, node->data); > >> + } > >> + ovs_assert(shash_is_empty(&mgr->ovn_uninstall_hash)); > >> + > >> for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > >> ovs_assert(hmapx_is_empty(&mgr->ifaces_per_state[i])); > >> } > >> @@ -212,6 +246,7 @@ if_status_mgr_destroy(struct if_status_mgr *mgr) > >> { > >> if_status_mgr_clear(mgr); > >> shash_destroy(&mgr->ifaces); > >> + shash_destroy(&mgr->ovn_uninstall_hash); > >> for (size_t i = 0; i < ARRAY_SIZE(mgr->ifaces_per_state); i++) { > >> hmapx_destroy(&mgr->ifaces_per_state[i]); > >> } > >> @@ -238,6 +273,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, > >> switch (iface->state) { > >> case OIF_CLAIMED: > >> case OIF_INSTALL_FLOWS: > >> + case OIF_REM_OLD_OVN_INST: > >> case OIF_MARK_UP: > >> /* Nothing to do here. */ > >> break; > >> @@ -274,6 +310,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_REM_OLD_OVN_INST: > >> case OIF_MARK_UP: > >> case OIF_INSTALLED: > >> /* Properly mark interfaces "down" if their flows were already > >> @@ -305,6 +342,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_REM_OLD_OVN_INST: > >> case OIF_MARK_UP: > >> case OIF_INSTALLED: > >> /* Properly mark interfaces "down" if their flows were already > >> @@ -346,12 +384,33 @@ if_status_handle_claims(struct if_status_mgr *mgr, > >> return rc; > >> } > >> > >> +static void > >> +clean_ovn_installed(struct if_status_mgr *mgr, > >> + const struct ovsrec_interface_table *iface_table) > >> +{ > >> + struct shash_node *node; > >> + > >> + SHASH_FOR_EACH_SAFE (node, &mgr->ovn_uninstall_hash) { > >> + const struct uuid *iface_uuid = node->data; > >> + remove_ovn_installed_for_uuid(iface_table, iface_uuid); > >> + free(node->data); > >> + char *node_name = shash_steal(&mgr->ovn_uninstall_hash, node); > >> + ovn_uninstall_hash_account_mem(node_name, true); > >> + free(node_name); > >> + } > >> +} > >> + > >> void > >> if_status_mgr_update(struct if_status_mgr *mgr, > >> struct local_binding_data *binding_data, > >> const struct sbrec_chassis *chassis_rec, > >> + const struct ovsrec_interface_table *iface_table, > >> + bool ovs_readonly, > >> bool sb_readonly) > >> { > >> + if (!ovs_readonly) { > >> + clean_ovn_installed(mgr, iface_table); > >> + } > >> if (!binding_data) { > >> return; > >> } > >> @@ -359,6 +418,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_REM_OLD_OVN_INST to OIF_MARK_UP. > >> + */ > >> + HMAPX_FOR_EACH_SAFE (node, > >> &mgr->ifaces_per_state[OIF_REM_OLD_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); > >> + } > >> + } > >> + > >> /* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set > >> their > >> * pb->chassis. However, the update might still be in fly > >> (confirmation > >> * not received yet) or pb->chassis was overwitten by another > chassis. > >> @@ -450,10 +520,23 @@ if_status_mgr_update(struct if_status_mgr *mgr, > >> } > >> } > >> > >> +void > >> +if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr, > >> + const char *name, > >> + const struct uuid *uuid) > >> +{ > >> + VLOG_DBG("Adding %s to list of interfaces for which to remove " > >> + "ovn-installed", name); > >> + if (!shash_find_data(&mgr->ovn_uninstall_hash, name)) { > >> + add_to_ovn_uninstall_hash(mgr, name, uuid); > >> + } > >> +} > >> + > >> void > >> if_status_mgr_run(struct if_status_mgr *mgr, > >> struct local_binding_data *binding_data, > >> const struct sbrec_chassis *chassis_rec, > >> + const struct ovsrec_interface_table *iface_table, > >> bool sb_readonly, bool ovs_readonly) > >> { > >> struct ofctrl_acked_seqnos *acked_seqnos = > >> @@ -471,12 +554,25 @@ 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_REM_OLD_OVN_INST > >> state. > >> + */ > >> + if (!binding_data || > >> + local_binding_is_ovn_installed(&binding_data->bindings, > >> + iface->id)) { > >> + ovs_iface_set_state(mgr, iface, OIF_REM_OLD_OVN_INST); > >> + } else { > >> + ovs_iface_set_state(mgr, iface, OIF_MARK_UP); > >> + } > >> } > >> ofctrl_acked_seqnos_destroy(acked_seqnos); > >> > >> /* Update binding states. */ > >> if_status_mgr_update_bindings(mgr, binding_data, chassis_rec, > >> + iface_table, > >> sb_readonly, ovs_readonly); > >> } > >> > >> @@ -492,6 +588,18 @@ ovs_iface_account_mem(const char *iface_id, bool > >> erase) > >> } > >> } > >> > >> +static void > >> +ovn_uninstall_hash_account_mem(const char *name, bool erase) > >> +{ > >> + uint32_t size = (strlen(name) + sizeof(struct uuid) + > >> + sizeof(struct shash_node)); > >> + if (erase) { > >> + ifaces_usage -= size; > >> + } else { > >> + ifaces_usage += size; > >> + } > >> +} > >> + > >> static struct ovs_iface * > >> ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id, > >> enum if_state state) > >> @@ -506,6 +614,16 @@ ovs_iface_create(struct if_status_mgr *mgr, const > >> char *iface_id, > >> return iface; > >> } > >> > >> +static void > >> +add_to_ovn_uninstall_hash(struct if_status_mgr *mgr, const char *name, > >> + const struct uuid *uuid) > >> +{ > >> + struct uuid *new_uuid = xzalloc(sizeof *new_uuid); > >> + memcpy(new_uuid, uuid, sizeof(*new_uuid)); > >> + shash_add(&mgr->ovn_uninstall_hash, name, new_uuid); > >> + ovn_uninstall_hash_account_mem(name, false); > >> +} > >> + > >> static void > >> ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface) > >> { > >> @@ -521,6 +639,23 @@ ovs_iface_destroy(struct if_status_mgr *mgr, struct > >> ovs_iface *iface) > >> free(iface); > >> } > >> > >> +static void > >> +ovn_uninstall_hash_destroy(struct if_status_mgr *mgr, char *name) > >> +{ > >> + struct shash_node *node = shash_find(&mgr->ovn_uninstall_hash, > name); > >> + char *node_name = NULL; > >> + if (node) { > >> + free(node->data); > >> + VLOG_DBG("Interface name %s destroy", name); > >> + node_name = shash_steal(&mgr->ovn_uninstall_hash, node); > >> + ovn_uninstall_hash_account_mem(name, true); > >> + free(node_name); > >> + } else { > >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > >> + VLOG_WARN_RL(&rl, "Interface name %s not found", name); > >> + } > >> +} > >> + > >> static void > >> ovs_iface_set_state(struct if_status_mgr *mgr, struct ovs_iface *iface, > >> enum if_state state) > >> @@ -539,6 +674,7 @@ static void > >> if_status_mgr_update_bindings(struct if_status_mgr *mgr, > >> struct local_binding_data *binding_data, > >> const struct sbrec_chassis *chassis_rec, > >> + const struct ovsrec_interface_table > >> *iface_table, > >> bool sb_readonly, bool ovs_readonly) > >> { > >> if (!binding_data) { > >> @@ -558,7 +694,17 @@ 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_REM_OLD_OVN_INST state. > >> + */ > >> + HMAPX_FOR_EACH (node, > &mgr->ifaces_per_state[OIF_REM_OLD_OVN_INST]) { > >> + struct ovs_iface *iface = node->data; > >> + > >> + local_binding_remove_ovn_installed(bindings, iface_table, > >> 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. > >> */ > >> diff --git a/controller/if-status.h b/controller/if-status.h > >> index 5bd187a25..203764946 100644 > >> --- a/controller/if-status.h > >> +++ b/controller/if-status.h > >> @@ -17,6 +17,7 @@ > >> #define IF_STATUS_H 1 > >> > >> #include "openvswitch/shash.h" > >> +#include "lib/vswitch-idl.h" > >> > >> #include "binding.h" > >> > >> @@ -35,9 +36,12 @@ 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 *, > >> const struct sbrec_chassis *chassis, > >> + const struct ovsrec_interface_table > >> *iface_table, > >> + bool ovs_readonly, > >> bool sb_readonly); > >> void if_status_mgr_run(struct if_status_mgr *mgr, struct > >> local_binding_data *, > >> const struct sbrec_chassis *, > >> + const struct ovsrec_interface_table > *iface_table, > >> bool sb_readonly, bool ovs_readonly); > >> void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, > >> struct simap *usage); > >> @@ -48,5 +52,8 @@ bool if_status_handle_claims(struct if_status_mgr > *mgr, > >> const struct sbrec_chassis *chassis_rec, > >> struct hmap *tracked_datapath, > >> bool sb_readonly); > >> +void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr, > >> + const char *name, > >> + const struct uuid *uuid); > >> > >> # endif /* controller/if-status.h */ > >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > >> index c094cb74d..2d8fee4d6 100644 > >> --- a/controller/ovn-controller.c > >> +++ b/controller/ovn-controller.c > >> @@ -5225,6 +5225,9 @@ main(int argc, char *argv[]) > >> > stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, > >> time_msec()); > >> if_status_mgr_update(if_mgr, binding_data, chassis, > >> + ovsrec_interface_table_get( > >> + ovs_idl_loop.idl), > >> + !ovs_idl_txn, > >> !ovnsb_idl_txn); > >> stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, > >> time_msec()); > >> @@ -5254,6 +5257,8 @@ main(int argc, char *argv[]) > >> stopwatch_start(IF_STATUS_MGR_RUN_STOPWATCH_NAME, > >> time_msec()); > >> if_status_mgr_run(if_mgr, binding_data, chassis, > >> + ovsrec_interface_table_get( > >> + ovs_idl_loop.idl), > >> !ovnsb_idl_txn, !ovs_idl_txn); > >> stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, > >> time_msec()); > >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at > >> index b46f67636..cae44edee 100644 > >> --- a/tests/system-ovn.at > >> +++ b/tests/system-ovn.at > >> @@ -10667,3 +10667,352 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > >> port patch-.*/d > >> /connection dropped.*/d"]) > >> AT_CLEANUP > >> ]) > >> + > >> +# This tests port->up/down and ovn-installed after adding and removing > >> Ports and Interfaces. > >> +# 3 Conditions x 3 tests: > >> +# - 3 Conditions: > >> +# - In normal conditions > >> +# - Remove interface while starting and stopping SB and Controller > >> +# - Remove and add back interface while starting and stopping SB and > >> Controller > >> +# - 3 tests: > >> +# - Add/Remove Logical Port > >> +# - Add/Remove iface-id > >> +# - Add/Remove Interface > >> +# Each tests/conditions checks for > >> +# - Port_binding->chassis > >> +# - Port up or down > >> +# - ovn-installed > >> +OVN_FOR_EACH_NORTHD([ > >> +AT_SETUP([ovn-install on slow ovsdb]) > >> +AT_KEYWORDS([ovn-install]) > >> + > >> +OVS_TRAFFIC_VSWITCHD_START() > >> +# Restart ovsdb-server, this time with tcp > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> +start_daemon ovsdb-server --remote=punix:"$OVS_RUNDIR"/db.sock > >> --remote=ptcp:0:127.0.0.1 > >> + > >> +ovn_start > >> +ADD_BR([br-int]) > >> + > >> +# Set external-ids in br-int needed for ovn-controller > >> +check ovs-vsctl \ > >> + -- set Open_vSwitch . external-ids:system-id=hv1 \ > >> + -- set Open_vSwitch . > >> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > >> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > >> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > >> + -- set bridge br-int fail-mode=secure > >> other-config:disable-in-band=true > >> + > >> +# Start ovn-controller > >> +PARSE_LISTENING_PORT([$ovs_base/ovsdb-server.log], [TCP_PORT]) > >> +start_daemon ovn-controller tcp:127.0.0.1:$TCP_PORT > >> + > >> +check ovn-nbctl ls-add ls1 > >> +check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16 > >> + > >> +check ovn-nbctl --wait=hv sync > >> + > >> +add_logical_ports() { > >> + echo Adding logical ports > >> + check ovn-nbctl lsp-add ls1 lsp1 > >> + check ovn-nbctl lsp-add ls1 lsp2 > >> +} > >> + > >> +remove_logical_ports() { > >> + echo Removing logical ports > >> + check ovn-nbctl lsp-del lsp1 > >> + check ovn-nbctl lsp-del lsp2 > >> +} > >> + > >> +add_ovs_interface() { > >> + echo Adding interface $1 $2 > >> + ovs-vsctl --no-wait -- add-port br-int $1 \ > >> + -- set Interface $1 external_ids:iface-id=$2 \ > >> + -- set Interface $1 type=internal > >> +} > >> +add_ovs_interfaces() { > >> + add_ovs_interface vif1 lsp1 > >> + add_ovs_interface vif2 lsp2 > >> +} > >> +remove_ovs_interface() { > >> + echo Removing interface $1 > >> + check ovs-vsctl --no-wait -- del-port $1 > >> +} > >> +remove_ovs_interfaces() { > >> + remove_ovs_interface vif1 > >> + remove_ovs_interface vif2 > >> +} > >> +add_iface_ids() { > >> + echo Adding iface-id vif1 lsp1 > >> + ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1 > >> + echo Adding iface-id vif2 lsp2 > >> + ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2 > >> +} > >> +remove_iface_id() { > >> + echo Removing iface-id $1 > >> + check ovs-vsctl remove Interface $1 external_ids iface-id > >> +} > >> +remove_iface_ids() { > >> + remove_iface_id vif1 > >> + remove_iface_id vif2 > >> +} > >> +wait_for_local_bindings() { > >> + OVS_WAIT_UNTIL( > >> + [test `ovs-appctl -t ovn-controller debug/dump-local-bindings | > >> grep interface | wc -l` -eq 2], > >> + [kill -CONT $(cat ovn-sb/ovsdb-server.pid)] > >> + ) > >> +} > >> +sleep_sb() { > >> + echo SB going to sleep > >> + AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)]) > >> +} > >> +wake_up_sb() { > >> + echo SB waking up > >> + AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)]) > >> +} > >> +sleep_controller() { > >> + echo Controller going to sleep > >> + ovn-appctl debug/pause > >> + OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) = > >> "xpaused"]) > >> +} > >> + > >> +stop_ovsdb_controller_updates() { > >> + TCP_PORT=$1 > >> + echo Stopping updates from ovn-controller to ovsdb using port > $TCP_PORT > >> + on_exit 'iptables -C INPUT -p tcp --destination-port $TCP_PORT -j > DROP > >> 2>/dev/null && iptables -D INPUT -p tcp --destination-port $TCP_PORT -j > >> DROP' > >> + iptables -A INPUT -p tcp --destination-port $TCP_PORT -j DROP > >> +} > >> +restart_ovsdb_controller_updates() { > >> + TCP_PORT=$1 > >> + echo Restarting updates from ovn-controller to ovsdb > >> + iptables -D INPUT -p tcp --destination-port $TCP_PORT -j DROP > >> +} > >> +wake_up_controller() { > >> + echo Controller waking up > >> + ovn-appctl debug/resume > >> +} > >> +ensure_controller_run() { > >> +# We want to make sure controller could run at least one full loop. > >> +# We can't use wait=hv as sb might be sleeping. > >> +# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop, > >> and not just the unixctl handling > >> + OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) = > >> "xrunning"]) > >> + OVS_WAIT_UNTIL([test x$(ovn-appctl -t ovn-controller debug/status) = > >> "xrunning"]) > >> +} > >> +sleep_ovsdb() { > >> + echo OVSDB going to sleep > >> + AT_CHECK([kill -STOP $(cat ovsdb-server.pid)]) > >> +} > >> +wake_up_ovsdb() { > >> + echo OVSDB waking up > >> + AT_CHECK([kill -CONT $(cat ovsdb-server.pid)]) > >> +} > >> +check_ovn_installed() { > >> + OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif1 > >> external_ids:ovn-installed` = '"true"']) > >> + OVS_WAIT_UNTIL([test `ovs-vsctl get Interface vif2 > >> external_ids:ovn-installed` = '"true"']) > >> +} > >> +check_ovn_uninstalled() { > >> + OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif2 > >> external_ids:ovn-installed` = x]) > >> + OVS_WAIT_UNTIL([test x`ovs-vsctl get Interface vif1 > >> external_ids:ovn-installed` = x]) > >> +} > >> +check_ports_up() { > >> + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true']) > >> + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true']) > >> +} > >> +check_ports_down() { > >> + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false']) > >> + OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false']) > >> +} > >> + > >> +check_ports_bound() { > >> + ch=$(fetch_column Chassis _uuid name=hv1) > >> + wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch > >> + wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch > >> +} > >> +check_ports_unbound() { > >> + wait_column "" Port_Binding chassis logical_port=lsp1 > >> + wait_column "" Port_Binding chassis logical_port=lsp2 > >> +} > >> +add_logical_ports > >> +add_ovs_interfaces > >> +wait_for_local_bindings > >> +wait_for_ports_up > >> +check ovn-nbctl --wait=hv sync > >> +############################################################ > >> +########## Remove interface while removing iface-id ######## > >> +############################################################ > >> +AS_BOX(["Remove interface while removing iface-id"]) > >> +stop_ovsdb_controller_updates $TCP_PORT > >> +remove_iface_id vif1 > >> +ensure_controller_run > >> +# OVSDB should be seen as ro now > >> +remove_iface_id vif2 > >> +ensure_controller_run > >> +# Controller delaying ovn-install removal for vif2 as ovsdb ro > >> +sleep_controller > >> +restart_ovsdb_controller_updates $TCP_PORT > >> +remove_ovs_interface vif2 > >> +# vif2, for which we want to remove ovn-install, is deleted > >> +wake_up_controller > >> +check_ovn_uninstalled > >> +check_ports_down > >> +check_ports_unbound > >> +add_ovs_interface vif2 lsp2 > >> +add_iface_ids > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> +############################################################ > >> +################### Add/Remove iface-id #################### > >> +############################################################ > >> +AS_BOX(["iface-id removal and added back (no sleeping sb or > controller)"]) > >> +remove_iface_ids > >> +check_ovn_uninstalled > >> +check_ports_down > >> +check_ports_unbound > >> +add_iface_ids > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> + > >> +AS_BOX(["iface-id removal"]) > >> +sleep_sb > >> +remove_iface_ids > >> +ensure_controller_run > >> +sleep_controller > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_uninstalled > >> +# Port_down not always set on iface-id removal > >> +# check_ports_down > >> +# Port_Binding(chassis) not always removed on iface-id removal > >> +# check_ports_unbound > >> +add_iface_ids > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["iface-id removal 2"]) > >> +# Block IDL from ovn-controller to OVSDB > >> +stop_ovsdb_controller_updates $TCP_PORT > >> +remove_iface_id vif2 > >> +ensure_controller_run > >> + > >> +# OVSDB should now be seen as read-only by ovn-controller > >> +remove_iface_id vif1 > >> +ensure_controller_run > >> + > >> +# Restart connection from ovn-controller to OVSDB > >> +restart_ovsdb_controller_updates $TCP_PORT > >> +check_ovn_uninstalled > >> +check_ports_down > >> +check_ports_unbound > >> + > >> +add_iface_ids > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["iface-id removal and added back"]) > >> +sleep_sb > >> +remove_iface_ids > >> +ensure_controller_run > >> +sleep_controller > >> +add_iface_ids > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> +############################################################ > >> +###################### Add/Remove Interface ################ > >> +############################################################ > >> +AS_BOX(["Interface removal and added back (no sleeping sb or > >> controller)"]) > >> +remove_ovs_interfaces > >> +check_ovn_uninstalled > >> +check_ports_down > >> +check_ports_unbound > >> +add_ovs_interfaces > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["Interface removal"]) > >> +sleep_sb > >> +remove_ovs_interfaces > >> +ensure_controller_run > >> +sleep_controller > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_uninstalled > >> +# Port_down not always set on Interface removal > >> +# check_ports_down > >> +# Port_Binding(chassis) not always removed on Interface removal > >> +# check_ports_unbound > >> +add_ovs_interfaces > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["Interface removal and added back"]) > >> +sleep_sb > >> +remove_ovs_interfaces > >> +ensure_controller_run > >> +sleep_controller > >> +add_ovs_interfaces > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> +check ovn-nbctl --wait=hv sync > >> +############################################################ > >> +###################### Add/Remove Logical Port ############# > >> +############################################################ > >> +AS_BOX(["Logical port removal and added back (no sleeping sb or > >> controller)"]) > >> +remove_logical_ports > >> +check_ovn_uninstalled > >> +check_ports_unbound > >> +sleep_ovsdb > >> +add_logical_ports > >> +ensure_controller_run > >> +wake_up_ovsdb > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["Logical port removal"]) > >> +sleep_sb > >> +remove_logical_ports > >> +ensure_controller_run > >> +sleep_controller > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_uninstalled > >> +check_ports_unbound > >> +add_logical_ports > >> +check ovn-nbctl --wait=hv sync > >> + > >> +AS_BOX(["Logical port removal and added back"]) > >> +sleep_sb > >> +remove_logical_ports > >> +ensure_controller_run > >> +sleep_controller > >> +add_logical_ports > >> +wake_up_sb > >> +wake_up_controller > >> +check_ovn_installed > >> +check_ports_up > >> +check_ports_bound > >> + > >> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >> + > >> +as ovn-sb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as ovn-nb > >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >> + > >> +as northd > >> +OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > >> + > >> +as > >> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >> +/connection dropped.*/d"]) > >> +AT_CLEANUP > >> +]) > >> + > >> -- > >> 2.31.1 > >> > >> > > Looks good to me, thanks. > > > > Reviewed-by: Ales Musil <[email protected]> > > > > Thanks, Xavier and Ales! > > I applied this to the main branch and backported it to stable branches > down to 22.09. Xavier, the patch didn't apply cleanly to 22.06 and > 22.03. Do you have time to prepare a custom backport? Otherwise I'll > look into it. > > Regards, > Dumitru > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
