When two chassis claim the same port, if a port is in the MARK_UP
state when the other chassis claims it, then ovn-controller was
sometimes using 100% cpu, as if-status was fighting to add ovn-install
(as in the MARK_UP state) and binding was fighting to remove it (as
the port was bound to the other chassis).
Signed-off-by: Xavier Simonart <[email protected]>
---
v2: - rebase on origin/main for scapy fix.
v3: - rebase on origin/main.
Signed-off-by: Xavier Simonart <[email protected]>
---
controller/binding.c | 15 +++++----------
controller/if-status.c | 27 +++++++++++++++++++++------
controller/if-status.h | 3 +--
tests/ovn.at | 3 +++
4 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index a6e02bb5e..9dee39be3 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -1608,8 +1608,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
/* Release the lport if there is no lbinding. */
if (lbinding_set && !can_bind) {
if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
- b_lport->lbinding->iface->name,
- &b_lport->lbinding->iface->header_.uuid);
+ b_lport->lbinding->iface);
}
if (!lbinding_set || !can_bind) {
@@ -1626,8 +1625,7 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
pb->logical_port)) {
update_lport_tracking(pb, b_ctx_out->tracked_dp_bindings, false);
if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
- b_lport->lbinding->iface->name,
- &b_lport->lbinding->iface->header_.uuid);
+ b_lport->lbinding->iface);
}
return true;
@@ -2092,8 +2090,7 @@ build_local_bindings(struct binding_ctx_in *b_ctx_in,
* This can happen if iface-id was removed as we recompute.
*/
if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
- iface_rec->name,
- &iface_rec->header_.uuid);
+ iface_rec);
}
}
}
@@ -2521,8 +2518,7 @@ consider_iface_release(const struct ovsrec_interface
*iface_rec,
}
if (lbinding->iface && lbinding->iface->name) {
if_status_mgr_remove_ovn_installed(b_ctx_out->if_mgr,
- lbinding->iface->name,
- &lbinding->iface->header_.uuid);
+ lbinding->iface);
}
} else if (b_lport && b_lport->type == LP_LOCALPORT) {
@@ -2852,8 +2848,7 @@ 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);
+ lbinding->iface);
}
return true;
}
diff --git a/controller/if-status.c b/controller/if-status.c
index ada78a18b..dc7b66ee2 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -71,6 +71,8 @@ enum if_state {
* but not yet marked "up" in the binding module (in
* SB and OVS databases).
*/
+ OIF_UNINSTALLED, /* Interface from whom ovn-install must be removed
+ */
OIF_INSTALLED, /* Interface flows programmed in OVS and binding
* marked "up" in the binding module.
*/
@@ -89,6 +91,7 @@ static const char *if_state_names[] = {
[OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST",
[OIF_MARK_UP] = "MARK_UP",
[OIF_MARK_DOWN] = "MARK_DOWN",
+ [OIF_UNINSTALLED] = "UNINSTALLED",
[OIF_INSTALLED] = "INSTALLED",
[OIF_UPDATE_PORT] = "UPDATE_PORT",
};
@@ -326,6 +329,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
/* Nothing to do here. */
break;
case OIF_INSTALLED:
+ case OIF_UNINSTALLED:
case OIF_MARK_DOWN:
case OIF_UPDATE_PORT:
ovs_iface_set_state(mgr, iface, OIF_CLAIMED);
@@ -362,6 +366,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
const char *iface_id)
case OIF_REM_OLD_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
+ case OIF_UNINSTALLED:
/* Properly mark interfaces "down" if their flows were already
* programmed in OVS.
*/
@@ -402,6 +407,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const
char *iface_id,
case OIF_REM_OLD_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
+ case OIF_UNINSTALLED:
/* Properly mark interfaces "down" if their flows were already
* programmed in OVS.
*/
@@ -634,13 +640,22 @@ 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)
+ const struct ovsrec_interface *iface_rec)
{
- 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);
+ if (!shash_find_data(&mgr->ovn_uninstall_hash, iface_rec->name)) {
+ VLOG_DBG("Adding %s to list of interfaces for which to remove "
+ "ovn-installed", iface_rec->name);
+ add_to_ovn_uninstall_hash(mgr, iface_rec->name,
+ &iface_rec->header_.uuid);
+ }
+
+ /* Move out of MARK_UP state which would add ovn-install back. */
+ const char *iface_id = smap_get(&iface_rec->external_ids, "iface-id");
+ if (iface_id) {
+ struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
+ if (iface && iface->state == OIF_MARK_UP) {
+ ovs_iface_set_state(mgr, iface, OIF_UNINSTALLED);
+ }
}
}
diff --git a/controller/if-status.h b/controller/if-status.h
index 4ae5ad481..eb91b62fd 100644
--- a/controller/if-status.h
+++ b/controller/if-status.h
@@ -61,8 +61,7 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
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,
- const struct uuid *uuid);
+ const struct ovsrec_interface *iface_rec);
uint16_t if_status_mgr_iface_get_mtu(const struct if_status_mgr *mgr,
const char *iface_id);
bool if_status_mgr_iface_update(const struct if_status_mgr *mgr,
diff --git a/tests/ovn.at b/tests/ovn.at
index 2b43cc62c..70b69b3ad 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -16463,6 +16463,9 @@ max_claims=20
AT_CHECK([test "${hv1_claims}" -le "${max_claims}"], [0], [])
AT_CHECK([test "${hv2_claims}" -le "${max_claims}"], [0], [])
+n_uninstalled=$(as hv1 grep -c 'Removing iface vif ovn-installed in' hv1/ovn-controller.log)
+echo "$n_uninstalled times 'Removing iface vif ovn-installed'"
+AT_CHECK([test "${n_uninstalled}" -le "10"], [0], [])
check ovn-nbctl --wait=hv lsp-del lsp0
CHECK_AFTER_RECOMPUTE([hv1], [hv1])
