| Message ID | 20241001151719.1627801-3-xsimonar@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Postpone ports performance fixes. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Oct 1, 2024 at 5:17 PM Xavier Simonart <xsimonar@redhat.com> wrote: > 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 <xsimonar@redhat.com> > > --- > v2: rebase on origin/main for scapy fix. > --- > 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 3b9bc8620..8c6f5aabe 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; > @@ -2062,8 +2060,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); > } > } > } > @@ -2491,8 +2488,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) { > @@ -2824,8 +2820,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 826b52051..f52d7f3e2 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -16552,6 +16552,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]) > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
diff --git a/controller/binding.c b/controller/binding.c index 3b9bc8620..8c6f5aabe 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; @@ -2062,8 +2060,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); } } } @@ -2491,8 +2488,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) { @@ -2824,8 +2820,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 826b52051..f52d7f3e2 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -16552,6 +16552,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])
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 <xsimonar@redhat.com> --- v2: rebase on origin/main for scapy fix. --- 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(-)