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
---
 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


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to