diff mbox series

[ovs-dev,2/3] binding: handle pb->chassis and pb->up from if-status module

Message ID 20230918164647.3144917-2-xsimonar@redhat.com
State Superseded
Headers show
Series [ovs-dev,1/3] binding: slight refactor if no local binding in consider_iface_release | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Xavier Simonart Sept. 18, 2023, 4:46 p.m. UTC
Before this patch, if-status module handled pb->chassis and pb->up
for vif ports, but not for non-VIF ports.
This caused a few potential issues:
- It was difficult to check that a no-vif port has been previously claimed
  as there was no state in if-status. Hence it was difficult to always release
  such a port when pb->chassis was set to a different chassis.
  This issue will be fixed in a future patch.
- Releasing such ports caused ovn-controller to log warnings such as
  "Trying to delete unknown ports".
- If sb is readonly at the time of the claim, it forced the module to recompute.

This patch fixed issues two and three by moving the work of setting pb->chassis
and pb->up to the if-status module.

Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 controller/binding.c        | 122 ++++++++++++++++++++++++---------
 controller/binding.h        |  18 +++++
 controller/if-status.c      | 130 ++++++++++++++++++++++++++++--------
 controller/if-status.h      |   9 ++-
 controller/ovn-controller.c |   6 +-
 tests/ovn.at                |  94 ++++++++++++++++++++++++++
 6 files changed, 318 insertions(+), 61 deletions(-)
diff mbox series

Patch

diff --git a/controller/binding.c b/controller/binding.c
index dbd2d52b7..fff631e7f 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1200,7 +1200,7 @@  local_binding_set_pb(struct shash *local_bindings, const char *pb_name,
  *   Updates the 'pb->up' field only if it's explicitly set to 'false'.
  *   This is to ensure compatibility with older versions of ovn-northd.
  */
-static bool
+bool
 claimed_lport_set_up(const struct sbrec_port_binding *pb,
                      const struct sbrec_port_binding *parent_pb,
                      bool sb_readonly)
@@ -1213,6 +1213,8 @@  claimed_lport_set_up(const struct sbrec_port_binding *pb,
     if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
         if (!sb_readonly) {
             if (pb->n_up) {
+                VLOG_INFO("Setting lport %s up in Southbound",
+                          pb->logical_port);
                 sbrec_port_binding_set_up(pb, &up, 1);
             }
         } else if (pb->n_up && !pb->up[0]) {
@@ -1347,39 +1349,26 @@  claim_lport(const struct sbrec_port_binding *pb,
             }
             update_tracked = true;
 
-            if (!notify_up) {
-                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
-                    return false;
-                }
-                if (sb_readonly) {
-                    return false;
-                }
-                set_pb_chassis_in_sbrec(pb, chassis_rec, true);
-            } else {
-                if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
-                                          sb_readonly, can_bind);
-            }
+            if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
+                                      sb_readonly, can_bind, notify_up,
+                                      parent_pb);
             register_claim_timestamp(pb->logical_port, now);
             sset_find_and_delete(postponed_ports, pb->logical_port);
         } else {
-            if (!notify_up) {
-                if (!claimed_lport_set_up(pb, parent_pb, sb_readonly)) {
-                    return false;
-                }
-            } else {
-                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,
-                                              iface_rec, sb_readonly,
-                                              can_bind);
-                }
+            if ((pb->n_up && !pb->up[0]) ||
+                (notify_up && !smap_get_bool(&iface_rec->external_ids,
+                               OVN_INSTALLED_EXT_ID, false))) {
+                if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
+                                          iface_rec, sb_readonly,
+                                          can_bind, notify_up,
+                                          parent_pb);
             }
         }
     } else if (can_bind == CAN_BIND_AS_ADDITIONAL) {
         if (!is_additional_chassis(pb, chassis_rec)) {
             if_status_mgr_claim_iface(if_mgr, pb, chassis_rec, iface_rec,
-                                      sb_readonly, can_bind);
+                                      sb_readonly, can_bind, notify_up,
+                                      parent_pb);
             update_tracked = true;
         }
     }
@@ -2446,15 +2435,15 @@  consider_iface_release(const struct ovsrec_interface *iface_rec,
             remove_related_lport(b_lport->pb, b_ctx_out);
         }
 
-        /* Clear the iface of the local binding. */
-        lbinding->iface = NULL;
-
         /* Check if the lbinding has children of type PB_CONTAINER.
          * If so, don't delete the local_binding. */
         if (!is_lbinding_container_parent(lbinding)) {
             local_binding_delete(lbinding, local_bindings, binding_lports,
                                  b_ctx_out->if_mgr);
         }
+        /* Clear the iface of the local binding. */
+        lbinding->iface = NULL;
+
     }
 
     remove_local_lports(iface_id, b_ctx_out);
@@ -3247,7 +3236,7 @@  local_binding_delete(struct local_binding *lbinding,
                      struct if_status_mgr *if_mgr)
 {
     shash_find_and_delete(local_bindings, lbinding->name);
-    if_status_mgr_delete_iface(if_mgr, lbinding->name);
+    if_status_mgr_delete_iface(if_mgr, lbinding->name, lbinding->iface);
     local_binding_destroy(lbinding, binding_lports);
 }
 
@@ -3464,6 +3453,79 @@  port_binding_set_down(const struct sbrec_chassis *chassis_rec,
         }
 }
 
+void
+port_binding_set_up(const struct sbrec_chassis *chassis_rec,
+                      const struct sbrec_port_binding_table *pb_table,
+                      const char *iface_id,
+                      const struct uuid *pb_uuid)
+{
+    const struct sbrec_port_binding *pb =
+        sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
+    if (!pb) {
+        VLOG_DBG("port_binding already deleted for %s", iface_id);
+        return;
+    }
+    if (pb->n_up && !pb->up[0]) {
+        bool up = true;
+        sbrec_port_binding_set_up(pb, &up, 1);
+        VLOG_INFO("Setting lport %s up in Southbound", pb->logical_port);
+        set_pb_chassis_in_sbrec(pb, chassis_rec, true);
+    }
+}
+
+void
+port_binding_set_pb(const struct sbrec_chassis *chassis_rec,
+                      const struct sbrec_port_binding_table *pb_table,
+                      const char *iface_id,
+                      const struct uuid *pb_uuid,
+                      enum can_bind bind_type)
+{
+    const struct sbrec_port_binding *pb =
+        sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
+    if (!pb) {
+        VLOG_DBG("port_binding already deleted for %s", iface_id);
+        return;
+    }
+    if (bind_type == CAN_BIND_AS_MAIN) {
+        set_pb_chassis_in_sbrec(pb, chassis_rec, true);
+    } else  if (bind_type == CAN_BIND_AS_ADDITIONAL) {
+        set_pb_additional_chassis_in_sbrec(pb, chassis_rec, true);
+    }
+}
+
+bool
+port_binding_pb_chassis_is_set(const struct sbrec_chassis *chassis_rec,
+                   const struct sbrec_port_binding_table *pb_table,
+                   const struct uuid *pb_uuid)
+{
+    const struct sbrec_port_binding *pb =
+        sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
+
+    if (pb && ((pb->chassis == chassis_rec) ||
+        is_additional_chassis(pb, chassis_rec))) {
+        return true;
+    }
+    return false;
+}
+
+bool
+port_binding_is_up(const struct sbrec_chassis *chassis_rec,
+                   const struct sbrec_port_binding_table *pb_table,
+                   const struct uuid *pb_uuid)
+{
+    const struct sbrec_port_binding *pb =
+        sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid);
+
+    if (!pb || pb->chassis != chassis_rec) {
+        return false;
+    }
+
+    if (pb->n_up && !pb->up[0]) {
+        return false;
+    }
+    return true;
+}
+
 static void
 binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly)
 {
diff --git a/controller/binding.h b/controller/binding.h
index 47df668a2..133badf3f 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -217,6 +217,18 @@  void port_binding_set_down(const struct sbrec_chassis *chassis_rec,
                            const struct sbrec_port_binding_table *pb_table,
                            const char *iface_id,
                            const struct uuid *pb_uuid);
+void port_binding_set_up(const struct sbrec_chassis *chassis_rec,
+                      const struct sbrec_port_binding_table *pb_table,
+                      const char *iface_id,
+                      const struct uuid *pb_uuid);
+void port_binding_set_pb(const struct sbrec_chassis *chassis_rec,
+                      const struct sbrec_port_binding_table *pb_table,
+                      const char *iface_id,
+                      const struct uuid *pb_uuid,
+                      enum can_bind bind_type);
+bool port_binding_pb_chassis_is_set(const struct sbrec_chassis *chassis_rec,
+                   const struct sbrec_port_binding_table *pb_table,
+                   const struct uuid *pb_uuid);
 
 /* This structure represents a logical port (or port binding)
  * which is associated with 'struct local_binding'.
@@ -258,4 +270,10 @@  void update_qos(struct ovsdb_idl_index * sbrec_port_binding_by_name,
                 const struct ovsrec_open_vswitch_table *ovs_table,
                 const struct ovsrec_bridge_table *bridge_table);
 
+bool claimed_lport_set_up(const struct sbrec_port_binding *pb,
+                     const struct sbrec_port_binding *parent_pb,
+                     bool sb_readonly);
+bool port_binding_is_up(const struct sbrec_chassis *chassis_rec,
+                   const struct sbrec_port_binding_table *pb_table,
+                   const struct uuid *pb_uuid);
 #endif /* controller/binding.h */
diff --git a/controller/if-status.c b/controller/if-status.c
index 6c5afc866..a907dbbaa 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -177,7 +177,9 @@  static const char *if_state_names[] = {
 
 struct ovs_iface {
     char *id;               /* Extracted from OVS external_ids.iface_id. */
+    char *name;             /* OVS iface name. */
     struct uuid pb_uuid;    /* Port_binding uuid */
+    struct uuid parent_pb_uuid;    /* Port_binding uuid */
     enum if_state state;    /* State of the interface in the state machine. */
     uint32_t install_seqno; /* Seqno at which this interface is expected to
                              * be fully programmed in OVS.  Only used in state
@@ -185,6 +187,7 @@  struct ovs_iface {
                              */
     uint16_t mtu;           /* Extracted from OVS interface.mtu field. */
     enum can_bind bind_type;/* CAN_BIND_AS_MAIN or CAN_BIND_AS_ADDITIONAL */
+    bool notify_up;
 };
 
 static uint64_t ifaces_usage;
@@ -224,6 +227,7 @@  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,
+    const struct sbrec_port_binding_table *pb_table,
     bool sb_readonly, bool ovs_readonly);
 
 static void ovn_uninstall_hash_account_mem(const char *name, bool erase);
@@ -273,12 +277,22 @@  if_status_mgr_destroy(struct if_status_mgr *mgr)
     free(mgr);
 }
 
+/* notify_up controls whether we wait for flows to be updated
+ * before setting the interface up.
+ * Non-VIF ports are reported up as soon as they are claimed
+ * to maintain compatibility with older versions.
+ * See aae25e6 binding: Correctly set Port_Binding.up for container/virtual
+ * ports.
+ */
+
 void
 if_status_mgr_claim_iface(struct if_status_mgr *mgr,
                           const struct sbrec_port_binding *pb,
                           const struct sbrec_chassis *chassis_rec,
                           const struct ovsrec_interface *iface_rec,
-                          bool sb_readonly, enum can_bind bind_type)
+                          bool sb_readonly, enum can_bind bind_type,
+                          bool notify_up,
+                          const struct sbrec_port_binding *parent_pb)
 {
     const char *iface_id = pb->logical_port;
     struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
@@ -287,8 +301,13 @@  if_status_mgr_claim_iface(struct if_status_mgr *mgr,
         iface = ovs_iface_create(mgr, iface_id, iface_rec, OIF_CLAIMED);
     }
     iface->bind_type = bind_type;
+    iface->notify_up = notify_up;
 
     memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid));
+    if (parent_pb) {
+        memcpy(&iface->parent_pb_uuid, &parent_pb->header_.uuid,
+               sizeof(iface->pb_uuid));
+    }
     if (!sb_readonly) {
         if (bind_type == CAN_BIND_AS_MAIN) {
             set_pb_chassis_in_sbrec(pb, chassis_rec, true);
@@ -357,7 +376,8 @@  if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id)
 }
 
 void
-if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
+if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id,
+                           const struct ovsrec_interface *iface_rec)
 {
     struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
 
@@ -365,6 +385,12 @@  if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id)
         return;
     }
 
+    if (iface_rec && strcmp(iface->name, iface_rec->name)) {
+        VLOG_DBG("Interface %s not deleted as port %s bound to %s",
+                 iface_rec->name, iface_id, iface->name);
+        return;
+    }
+
     switch (iface->state) {
     case OIF_CLAIMED:
     case OIF_INSTALL_FLOWS:
@@ -394,6 +420,7 @@  if_status_handle_claims(struct if_status_mgr *mgr,
                         struct local_binding_data *binding_data,
                         const struct sbrec_chassis *chassis_rec,
                         struct hmap *tracked_datapath,
+                        const struct sbrec_port_binding_table *pb_table,
                         bool sb_readonly)
 {
     if (!binding_data || sb_readonly) {
@@ -406,9 +433,15 @@  if_status_handle_claims(struct if_status_mgr *mgr,
     bool rc = false;
     HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
         struct ovs_iface *iface = node->data;
-        VLOG_INFO("if_status_handle_claims for %s", iface->id);
-        local_binding_set_pb(bindings, iface->id, chassis_rec,
-                             tracked_datapath, true, iface->bind_type);
+        VLOG_INFO("if_status_handle_claims for %s, notify_up = %d", iface->id,
+                  iface->notify_up);
+        if (iface->notify_up) {
+            local_binding_set_pb(bindings, iface->id, chassis_rec,
+                                 tracked_datapath, true, iface->bind_type);
+        } else {
+            port_binding_set_pb(chassis_rec, pb_table, iface->id,
+                                &iface->pb_uuid, iface->bind_type);
+        }
         rc = true;
     }
     return rc;
@@ -470,18 +503,32 @@  if_status_mgr_update(struct if_status_mgr *mgr,
      */
     HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
         struct ovs_iface *iface = node->data;
-
-        if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
-            chassis_rec)) {
-            if (!sb_readonly) {
-                local_binding_set_pb(bindings, iface->id, chassis_rec,
-                                     NULL, true, iface->bind_type);
-            } else {
-                continue;
+        if (iface->notify_up) {
+            if (!local_bindings_pb_chassis_is_set(bindings, iface->id,
+                chassis_rec)) {
+                if (!sb_readonly) {
+                    local_binding_set_pb(bindings, iface->id, chassis_rec,
+                                         NULL, true, iface->bind_type);
+                } else {
+                    continue;
+                }
+            }
+            if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
+                ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
+            }
+        } else {
+            if (!port_binding_pb_chassis_is_set(chassis_rec, pb_table,
+                                                &iface->pb_uuid)) {
+                if (!sb_readonly) {
+                    port_binding_set_pb(chassis_rec, pb_table, iface->id,
+                                        &iface->pb_uuid, iface->bind_type);
+                } else {
+                    continue;
+                }
+            }
+            if (port_binding_is_up(chassis_rec, pb_table, &iface->pb_uuid)) {
+                ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
             }
-        }
-        if (local_binding_is_up(bindings, iface->id, chassis_rec)) {
-            ovs_iface_set_state(mgr, iface, OIF_INSTALLED);
         }
     }
 
@@ -528,9 +575,13 @@  if_status_mgr_update(struct if_status_mgr *mgr,
             /* No need to to update pb->chassis as already done
              * in if_status_handle_claims or if_status_mgr_claim_iface
              */
-            ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
-            iface->install_seqno = mgr->iface_seqno + 1;
-            new_ifaces = true;
+            if (iface->notify_up) {
+                ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
+                iface->install_seqno = mgr->iface_seqno + 1;
+                new_ifaces = true;
+            } else {
+                ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
+            }
         }
     } else {
         HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) {
@@ -587,6 +638,7 @@  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,
+                  const struct sbrec_port_binding_table *pb_table,
                   bool sb_readonly, bool ovs_readonly)
 {
     struct ofctrl_acked_seqnos *acked_seqnos =
@@ -622,15 +674,18 @@  if_status_mgr_run(struct if_status_mgr *mgr,
 
     /* Update binding states. */
     if_status_mgr_update_bindings(mgr, binding_data, chassis_rec,
-                                  iface_table,
+                                  iface_table, pb_table,
                                   sb_readonly, ovs_readonly);
 }
 
 static void
-ovs_iface_account_mem(const char *iface_id, bool erase)
+ovs_iface_account_mem(const char *iface_id, char *iface_name, bool erase)
 {
     uint32_t size = (strlen(iface_id) + sizeof(struct ovs_iface) +
                      sizeof(struct shash_node));
+    if (iface_name) {
+        size += strlen(iface_name);
+    }
     if (erase) {
         ifaces_usage -= size;
     } else {
@@ -682,12 +737,18 @@  ovs_iface_create(struct if_status_mgr *mgr, const char *iface_id,
 {
     struct ovs_iface *iface = xzalloc(sizeof *iface);
 
-    VLOG_DBG("Interface %s create.", iface_id);
+    VLOG_DBG("Interface %s create for iface %s.", iface_id,
+             iface_rec ? iface_rec->name : "");
     iface->id = xstrdup(iface_id);
     shash_add_nocopy(&mgr->ifaces, iface->id, iface);
     ovs_iface_set_state(mgr, iface, state);
-    ovs_iface_account_mem(iface_id, false);
-    if_status_mgr_iface_update(mgr, iface_rec);
+    if (iface_rec) {
+        ovs_iface_account_mem(iface_id, iface_rec->name, false);
+        if_status_mgr_iface_update(mgr, iface_rec);
+        iface->name = xstrdup(iface_rec->name);
+    } else {
+        ovs_iface_account_mem(iface_id, NULL, false);
+    }
     return iface;
 }
 
@@ -711,7 +772,8 @@  ovs_iface_destroy(struct if_status_mgr *mgr, struct ovs_iface *iface)
     if (node) {
         shash_steal(&mgr->ifaces, node);
     }
-    ovs_iface_account_mem(iface->id, true);
+    ovs_iface_account_mem(iface->id, iface->name, true);
+    free(iface->name);
     free(iface->id);
     free(iface);
 }
@@ -752,6 +814,7 @@  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,
+                              const struct sbrec_port_binding_table *pb_table,
                               bool sb_readonly, bool ovs_readonly)
 {
     if (!binding_data) {
@@ -788,9 +851,20 @@  if_status_mgr_update_bindings(struct if_status_mgr *mgr,
     char *ts_now_str = xasprintf("%lld", time_wall_msec());
     HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_MARK_UP]) {
         struct ovs_iface *iface = node->data;
-
-        local_binding_set_up(bindings, iface->id, chassis_rec, ts_now_str,
-                             sb_readonly, ovs_readonly);
+        if (iface->notify_up) {
+            local_binding_set_up(bindings, iface->id, chassis_rec, ts_now_str,
+                                 sb_readonly, ovs_readonly);
+        } else if (!sb_readonly) {
+            const struct sbrec_port_binding *pb =
+                sbrec_port_binding_table_get_for_uuid(pb_table,
+                                                      &iface->pb_uuid);
+            if (pb) {
+                const struct sbrec_port_binding *parent_pb =
+                    sbrec_port_binding_table_get_for_uuid(pb_table,
+                                                  &iface->parent_pb_uuid);
+                claimed_lport_set_up(pb, parent_pb, sb_readonly);
+            }
+        }
     }
     free(ts_now_str);
 
diff --git a/controller/if-status.h b/controller/if-status.h
index 9714f6d8d..4ae5ad481 100644
--- a/controller/if-status.h
+++ b/controller/if-status.h
@@ -32,9 +32,12 @@  void if_status_mgr_claim_iface(struct if_status_mgr *,
                                const struct sbrec_port_binding *pb,
                                const struct sbrec_chassis *chassis_rec,
                                const struct ovsrec_interface *iface_rec,
-                               bool sb_readonly, enum can_bind bind_type);
+                               bool sb_readonly, enum can_bind bind_type,
+                               bool notify_up,
+                               const struct sbrec_port_binding *parent_pb);
 void if_status_mgr_release_iface(struct if_status_mgr *, const char *iface_id);
-void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id);
+void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id,
+                                const struct ovsrec_interface *iface_rec);
 
 void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *,
                           const struct sbrec_chassis *chassis,
@@ -45,6 +48,7 @@  void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *,
 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,
+                       const struct sbrec_port_binding_table *pb_table,
                        bool sb_readonly, bool ovs_readonly);
 void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr,
                                     struct simap *usage);
@@ -54,6 +58,7 @@  bool if_status_handle_claims(struct if_status_mgr *mgr,
                              struct local_binding_data *binding_data,
                              const struct sbrec_chassis *chassis_rec,
                              struct hmap *tracked_datapath,
+                             const struct sbrec_port_binding_table *pb_table,
                              bool sb_readonly);
 void if_status_mgr_remove_ovn_installed(struct if_status_mgr *mgr,
                                         const char *name,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 859d9cab9..b09ac047e 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1815,6 +1815,8 @@  runtime_data_sb_ro_handler(struct engine_node *node, void *data)
         engine_ovsdb_node_get_index(
                 engine_get_input("SB_chassis", node),
                 "name");
+    const struct sbrec_port_binding_table *pb_table =
+        EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
 
     if (chassis_id) {
         chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
@@ -1829,7 +1831,7 @@  runtime_data_sb_ro_handler(struct engine_node *node, void *data)
                                     &rt_data->lbinding_data,
                                     chassis,
                                     &rt_data->tracked_dp_bindings,
-                                    sb_readonly)) {
+                                    pb_table, sb_readonly)) {
             engine_set_node_state(node, EN_UPDATED);
             rt_data->tracked = true;
         }
@@ -5871,6 +5873,8 @@  main(int argc, char *argv[])
                     if_status_mgr_run(if_mgr, binding_data, chassis,
                                       ovsrec_interface_table_get(
                                                   ovs_idl_loop.idl),
+                                      sbrec_port_binding_table_get(
+                                                 ovnsb_idl_loop.idl),
                                       !ovnsb_idl_txn, !ovs_idl_txn);
                     stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
                                    time_msec());
diff --git a/tests/ovn.at b/tests/ovn.at
index ba5ce298a..b4f146590 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -37004,3 +37004,97 @@  AT_CHECK([grep -c "NXT_CT_FLUSH_ZONE" hv1/ovs-vswitchd.log], [0], [dnl
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([virtual port claim race condition])
+AT_KEYWORDS([virtual ports])
+ovn_start
+
+send_garp() {
+    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
+    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
+}
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovn-appctl vlog/set dbg
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+ovs-vsctl -- add-port br-int hv1-vif3 -- \
+    set interface hv1-vif3 \
+    options:tx_pcap=hv1/vif3-tx.pcap \
+    options:rxq_pcap=hv1/vif3-rx.pcap \
+    ofport-request=3
+
+ovs-appctl -t ovn-controller vlog/set dbg
+
+ovn-nbctl ls-add sw0
+
+check ovn-nbctl lsp-add sw0 sw0-vir
+check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
+check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
+check ovn-nbctl lsp-set-type sw0-vir virtual
+check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
+check ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1
+
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 10.0.0.10 1000::3"
+
+# Create a logical router and attach both logical switches
+check ovn-nbctl lr-add lr0
+check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 1000::a/64
+check ovn-nbctl lsp-add sw0 sw0-lr0
+check ovn-nbctl lsp-set-type sw0-lr0 router
+check ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
+check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
+
+wait_for_ports_up
+ovn-nbctl --wait=hv sync
+hv1_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv1"`
+
+# Try to bind sw0-vir directly to an OVS port. This should be ignored by
+# ovn-controller.
+as hv1 ovs-vsctl set interface hv1-vif3 external-ids:iface-id=sw0-vir
+
+# Make sb to sleep, so that claim of sw0-vir (through pinctrl) and hv1-vif3 can be handled within same idl by controller
+sleep_sb
+
+# From sw0-p0 send GARP for 10.0.0.10. hv1 should claim sw0-vir
+# and sw0-p1 should be its virtual_parent.
+eth_src=505400000003
+eth_dst=ffffffffffff
+spa=$(ip_to_hex 10 0 0 10)
+tpa=$(ip_to_hex 10 0 0 10)
+send_garp 1 1 $eth_src $eth_dst $spa $tpa
+
+OVS_WAIT_UNTIL([test 1 = `cat hv1/ovn-controller.log | grep "pinctrl received  packet-in" | \
+grep opcode=BIND_VPORT | grep OF_Table_ID=29 | wc -l`])
+
+sleep_controller hv1
+
+# Cleanup hv1-vif3. This should not interfere with sw0-vir claim
+as hv1 ovs-vsctl del-port hv1-vif3
+
+wake_up_sb
+ovn-nbctl --wait=sb sync
+wake_up_controller hv1
+check ovn-nbctl --wait=hv sync
+
+wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid
+check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1
+wait_for_ports_up sw0-vir
+check ovn-nbctl --wait=hv sync
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+