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