Message ID | 20230419124118.3576664-1-xsimonar@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v3,1/2] ovn-controller: fixed ovn-installed not always properly added or removed. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
As a note: The newly added system test sometimes fails as it is finding some warnings in the controller.log (e.g. 2023-04-19T13:18:29.536Z|00087|if_status|WARN|Trying to release unknown interface lsp1) This can happen in the following scenario: - logical port is created - OVS interfaces is added w/ iface-id - Port is claimed, setting pb->chassis in SB. SB very slow - Removing OVS interfaces - iface deleted in if-status (if_status_mgr_release_iface destroying the iface in CLAIMED state) - pb->chassis update received from sb => port released, iface released from iface-status This issue existed before this patch and has been fixed in the next patch. On Wed, Apr 19, 2023 at 2:41 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > > --- > 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 > --- > controller/binding.c | 81 ++++++++- > controller/binding.h | 9 + > controller/if-status.c | 199 ++++++++++++++++++---- > controller/if-status.h | 6 + > controller/ovn-controller.c | 3 + > tests/system-ovn.at | 321 ++++++++++++++++++++++++++++++++++++ > 6 files changed, 587 insertions(+), 32 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 5df62baef..4e79c1c87 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,51 @@ local_binding_set_up(struct shash *local_bindings, > const char *pb_name, > } > } > > +static void > +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name) > +{ > + if (lbinding && lbinding->iface && > + smap_get_bool(&lbinding->iface->external_ids, > + OVN_INSTALLED_EXT_ID, false)) { > + /* If iface has been deleted, do not try to delete a key from it > */ > + if (!ovsrec_interface_is_deleted(lbinding->iface)) { > + 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_remove_ovn_installed(struct shash *local_bindings, > + const char *pb_name, bool ovs_readonly) > +{ > + if (ovs_readonly) { > + return; > + } > + struct local_binding *lbinding = > + local_binding_find(local_bindings, pb_name); > + remove_ovn_installed(lbinding, pb_name); > +} > + > +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)) { > + /* If iface has been deleted, do not try to delete a key from it > */ > + if (!ovsrec_interface_is_deleted(iface_rec)) { > + 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 +1298,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 +1525,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 +2351,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 +2626,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 +2650,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 +2697,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..fdf59b813 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -159,6 +159,12 @@ 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 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 +201,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..7caa65aca 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,7 +193,10 @@ 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); > > @@ -179,6 +205,7 @@ static void if_status_mgr_update_bindings( > const struct sbrec_chassis *, > 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 +216,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 +230,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 +245,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 +272,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 +309,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 +341,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 +383,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 +417,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,6 +519,18 @@ 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, > @@ -471,7 +552,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_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); > > @@ -492,6 +585,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 +611,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 +636,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) > @@ -558,7 +690,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_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->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..6341b5dc6 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,6 +36,8 @@ 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 *, > @@ -48,5 +51,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 e170e9262..8ddc21f5e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5243,6 +5243,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()); > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 96d1c295e..bf15d2aac 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10784,3 +10784,324 @@ 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_interfaces() { > + echo Adding interfaces > + ovs-vsctl --no-wait -- add-port br-int vif1 \ > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > + -- set Interface vif1 type=internal > + ovs-vsctl --no-wait -- add-port br-int vif2 \ > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > + -- set Interface vif2 type=internal > +} > +remove_ovs_interfaces() { > + echo Removing interfaces > + check ovs-vsctl --no-wait -- del-port vif1 > + check ovs-vsctl --no-wait -- del-port vif2 > +} > +add_iface_ids() { > + echo Adding back iface-id > + ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1 > + 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() { > + echo Removing iface-id > + 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 > +############################################################ > +################### 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 > >
On Wed, Apr 19, 2023 at 2:41 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > Hi Xavier, I have two comments down below, other than that it looks good. > > --- > 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 > --- > controller/binding.c | 81 ++++++++- > controller/binding.h | 9 + > controller/if-status.c | 199 ++++++++++++++++++---- > controller/if-status.h | 6 + > controller/ovn-controller.c | 3 + > tests/system-ovn.at | 321 ++++++++++++++++++++++++++++++++++++ > 6 files changed, 587 insertions(+), 32 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 5df62baef..4e79c1c87 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,51 @@ local_binding_set_up(struct shash *local_bindings, > const char *pb_name, > } > } > > +static void > +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name) > +{ > + if (lbinding && lbinding->iface && > + smap_get_bool(&lbinding->iface->external_ids, > + OVN_INSTALLED_EXT_ID, false)) { > + /* If iface has been deleted, do not try to delete a key from it > */ > + if (!ovsrec_interface_is_deleted(lbinding->iface)) { > This call is not "safe" to use outside of a tracked loop AFAIU. Maybe it could be actually replaced with the updated "remove_ovn_installed_for_uuid". WDYT? > + 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_remove_ovn_installed(struct shash *local_bindings, > + const char *pb_name, bool ovs_readonly) > +{ > + if (ovs_readonly) { > + return; > + } > + struct local_binding *lbinding = > + local_binding_find(local_bindings, pb_name); > + remove_ovn_installed(lbinding, pb_name); > +} > + > +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)) { > + /* If iface has been deleted, do not try to delete a key from it > */ > + if (!ovsrec_interface_is_deleted(iface_rec)) { > Same as before this check is not "safe" to do outside of the tracked loop AFAIU. But IMO it is not needed. If the interface would be deleted the "ovsrec_interface_table_get_for_uuid" should return NULL so in this case this check is redundant. > + 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 +1298,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 +1525,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 +2351,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 +2626,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 +2650,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 +2697,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..fdf59b813 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -159,6 +159,12 @@ 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 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 +201,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..7caa65aca 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,7 +193,10 @@ 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); > > @@ -179,6 +205,7 @@ static void if_status_mgr_update_bindings( > const struct sbrec_chassis *, > 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 +216,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 +230,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 +245,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 +272,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 +309,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 +341,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 +383,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 +417,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,6 +519,18 @@ 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, > @@ -471,7 +552,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_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); > > @@ -492,6 +585,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 +611,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 +636,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) > @@ -558,7 +690,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_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->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..6341b5dc6 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,6 +36,8 @@ 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 *, > @@ -48,5 +51,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 e170e9262..8ddc21f5e 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5243,6 +5243,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()); > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 96d1c295e..bf15d2aac 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10784,3 +10784,324 @@ 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_interfaces() { > + echo Adding interfaces > + ovs-vsctl --no-wait -- add-port br-int vif1 \ > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > + -- set Interface vif1 type=internal > + ovs-vsctl --no-wait -- add-port br-int vif2 \ > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > + -- set Interface vif2 type=internal > +} > +remove_ovs_interfaces() { > + echo Removing interfaces > + check ovs-vsctl --no-wait -- del-port vif1 > + check ovs-vsctl --no-wait -- del-port vif2 > +} > +add_iface_ids() { > + echo Adding back iface-id > + ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1 > + 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() { > + echo Removing iface-id > + 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 > +############################################################ > +################### 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 > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
diff --git a/controller/binding.c b/controller/binding.c index 5df62baef..4e79c1c87 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,51 @@ local_binding_set_up(struct shash *local_bindings, const char *pb_name, } } +static void +remove_ovn_installed(struct local_binding *lbinding, const char *pb_name) +{ + if (lbinding && lbinding->iface && + smap_get_bool(&lbinding->iface->external_ids, + OVN_INSTALLED_EXT_ID, false)) { + /* If iface has been deleted, do not try to delete a key from it */ + if (!ovsrec_interface_is_deleted(lbinding->iface)) { + 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_remove_ovn_installed(struct shash *local_bindings, + const char *pb_name, bool ovs_readonly) +{ + if (ovs_readonly) { + return; + } + struct local_binding *lbinding = + local_binding_find(local_bindings, pb_name); + remove_ovn_installed(lbinding, pb_name); +} + +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)) { + /* If iface has been deleted, do not try to delete a key from it */ + if (!ovsrec_interface_is_deleted(iface_rec)) { + 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 +1298,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 +1525,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 +2351,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 +2626,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 +2650,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 +2697,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..fdf59b813 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -159,6 +159,12 @@ 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 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 +201,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..7caa65aca 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,7 +193,10 @@ 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); @@ -179,6 +205,7 @@ static void if_status_mgr_update_bindings( const struct sbrec_chassis *, 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 +216,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 +230,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 +245,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 +272,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 +309,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 +341,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 +383,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 +417,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,6 +519,18 @@ 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, @@ -471,7 +552,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_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); @@ -492,6 +585,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 +611,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 +636,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) @@ -558,7 +690,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_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->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..6341b5dc6 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,6 +36,8 @@ 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 *, @@ -48,5 +51,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 e170e9262..8ddc21f5e 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5243,6 +5243,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()); diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 96d1c295e..bf15d2aac 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10784,3 +10784,324 @@ 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_interfaces() { + echo Adding interfaces + ovs-vsctl --no-wait -- add-port br-int vif1 \ + -- set Interface vif1 external_ids:iface-id=lsp1 \ + -- set Interface vif1 type=internal + ovs-vsctl --no-wait -- add-port br-int vif2 \ + -- set Interface vif2 external_ids:iface-id=lsp2 \ + -- set Interface vif2 type=internal +} +remove_ovs_interfaces() { + echo Removing interfaces + check ovs-vsctl --no-wait -- del-port vif1 + check ovs-vsctl --no-wait -- del-port vif2 +} +add_iface_ids() { + echo Adding back iface-id + ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1 + 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() { + echo Removing iface-id + 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 +############################################################ +################### 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 +]) +
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 <xsimonar@redhat.com> --- 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 --- controller/binding.c | 81 ++++++++- controller/binding.h | 9 + controller/if-status.c | 199 ++++++++++++++++++---- controller/if-status.h | 6 + controller/ovn-controller.c | 3 + tests/system-ovn.at | 321 ++++++++++++++++++++++++++++++++++++ 6 files changed, 587 insertions(+), 32 deletions(-)