| Message ID | 20240423115333.2796843-5-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Numan Siddique |
| Headers | show |
| Series | Fix I+P versus recompute differences. | 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 | success | github build: passed |
On Tue, Apr 23, 2024 at 7:54 AM Xavier Simonart <xsimonar@redhat.com> wrote: > > When a port was claimed by two chassis, both chassis were fighting for the port; > a claim was postponed if the port was claimed recently. > However, there were two issues: > - Not all the flows were properly removed when the remote chassis was claiming > the port. > - A port was not properly released if claim was postponed when the port_binding > was deleted. > > There were also multiple cases causing ovn-controller to always wake-up > immediately as a port was not removed from postponed_port list. > - Changing port type to localport while port claim was postponed. > - Deleting parent of a container port while parent was postponed. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> Acked-by: Numan Siddique <numans@ovn.org> Numan > --- > controller/binding.c | 29 +++++++++++++++++++++++++++-- > tests/ovn.at | 10 ++++++++++ > 2 files changed, 37 insertions(+), 2 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 1499ceae1..0bef5dc42 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1270,6 +1270,18 @@ update_port_additional_encap_if_needed( > return true; > } > > +static bool > +is_requested_additional_chassis(const struct sbrec_port_binding *pb, > + const struct sbrec_chassis *chassis_rec) > +{ > + for (size_t i = 0; i < pb->n_requested_additional_chassis; i++) { > + if (pb->requested_additional_chassis[i] == chassis_rec) { > + return true; > + } > + } > + return false; > +} > + > bool > is_additional_chassis(const struct sbrec_port_binding *pb, > const struct sbrec_chassis *chassis_rec) > @@ -1587,6 +1599,15 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, > b_ctx_out->if_mgr); > } > } > + if (pb->chassis != b_ctx_in->chassis_rec > + && !is_requested_additional_chassis(pb, b_ctx_in->chassis_rec) > + && if_status_is_port_claimed(b_ctx_out->if_mgr, > + 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); > + } > > return true; > } > @@ -1787,7 +1808,8 @@ consider_localport(const struct sbrec_port_binding *pb, > struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; > struct local_binding *lbinding = local_binding_find(local_bindings, > pb->logical_port); > - > + /* Make sure there is no previous postponed port claim */ > + sset_find_and_delete(b_ctx_out->postponed_ports, pb->logical_port); > if (!lbinding) { > return true; > } > @@ -2754,11 +2776,14 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb, > binding_lport_delete(binding_lports, b_lport); > } > > - if (bound && lbinding && lport_type == LP_VIF) { > + if ((lbinding && lport_type == LP_VIF) && > + (bound || sset_find_and_delete(b_ctx_out->postponed_ports, > + pb->logical_port))) { > /* We need to release the container/virtual binding lports (if any) if > * deleted 'pb' type is LP_VIF. */ > struct binding_lport *c_lport; > LIST_FOR_EACH (c_lport, list_node, &lbinding->binding_lports) { > + sset_find_and_delete(b_ctx_out->postponed_ports, c_lport->name); > remove_local_lports(c_lport->pb->logical_port, b_ctx_out); > if (!release_binding_lport(b_ctx_in->chassis_rec, c_lport, > !b_ctx_in->ovnsb_idl_txn, > diff --git a/tests/ovn.at b/tests/ovn.at > index b68678472..74c5bccc0 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -16413,6 +16413,10 @@ ovn_start > > ovn-nbctl ls-add ls0 > ovn-nbctl lsp-add ls0 lsp0 > +ovn-nbctl lsp-add ls0 lsp1 > +ovn-nbctl lsp-add ls0 lsp2 > +check ovn-nbctl lsp-add ls0 lsp-cont1 lsp0 1 > + > > net_add n1 > for i in 1 2; do > @@ -16426,6 +16430,8 @@ for i in 1 2; do > as hv$i > ovs-vsctl -- add-port br-int vif \ > -- set Interface vif external-ids:iface-id=lsp0 > + ovs-vsctl -- add-port br-int vif$i \ > + -- set Interface vif$i external-ids:iface-id=lsp$i > done > > # give controllers some time to fight for the port binding > @@ -16443,6 +16449,10 @@ max_claims=20 > AT_CHECK([test "${hv1_claims}" -le "${max_claims}"], [0], []) > AT_CHECK([test "${hv2_claims}" -le "${max_claims}"], [0], []) > > +check ovn-nbctl --wait=hv lsp-del lsp0 > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) > +CHECK_FLOWS_AFTER_RECOMPUTE([hv2], [hv2]) > + > OVN_CLEANUP([hv1],[hv2]) > > AT_CLEANUP > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index 1499ceae1..0bef5dc42 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1270,6 +1270,18 @@ update_port_additional_encap_if_needed( return true; } +static bool +is_requested_additional_chassis(const struct sbrec_port_binding *pb, + const struct sbrec_chassis *chassis_rec) +{ + for (size_t i = 0; i < pb->n_requested_additional_chassis; i++) { + if (pb->requested_additional_chassis[i] == chassis_rec) { + return true; + } + } + return false; +} + bool is_additional_chassis(const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis_rec) @@ -1587,6 +1599,15 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, b_ctx_out->if_mgr); } } + if (pb->chassis != b_ctx_in->chassis_rec + && !is_requested_additional_chassis(pb, b_ctx_in->chassis_rec) + && if_status_is_port_claimed(b_ctx_out->if_mgr, + 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); + } return true; } @@ -1787,7 +1808,8 @@ consider_localport(const struct sbrec_port_binding *pb, struct shash *local_bindings = &b_ctx_out->lbinding_data->bindings; struct local_binding *lbinding = local_binding_find(local_bindings, pb->logical_port); - + /* Make sure there is no previous postponed port claim */ + sset_find_and_delete(b_ctx_out->postponed_ports, pb->logical_port); if (!lbinding) { return true; } @@ -2754,11 +2776,14 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb, binding_lport_delete(binding_lports, b_lport); } - if (bound && lbinding && lport_type == LP_VIF) { + if ((lbinding && lport_type == LP_VIF) && + (bound || sset_find_and_delete(b_ctx_out->postponed_ports, + pb->logical_port))) { /* We need to release the container/virtual binding lports (if any) if * deleted 'pb' type is LP_VIF. */ struct binding_lport *c_lport; LIST_FOR_EACH (c_lport, list_node, &lbinding->binding_lports) { + sset_find_and_delete(b_ctx_out->postponed_ports, c_lport->name); remove_local_lports(c_lport->pb->logical_port, b_ctx_out); if (!release_binding_lport(b_ctx_in->chassis_rec, c_lport, !b_ctx_in->ovnsb_idl_txn, diff --git a/tests/ovn.at b/tests/ovn.at index b68678472..74c5bccc0 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -16413,6 +16413,10 @@ ovn_start ovn-nbctl ls-add ls0 ovn-nbctl lsp-add ls0 lsp0 +ovn-nbctl lsp-add ls0 lsp1 +ovn-nbctl lsp-add ls0 lsp2 +check ovn-nbctl lsp-add ls0 lsp-cont1 lsp0 1 + net_add n1 for i in 1 2; do @@ -16426,6 +16430,8 @@ for i in 1 2; do as hv$i ovs-vsctl -- add-port br-int vif \ -- set Interface vif external-ids:iface-id=lsp0 + ovs-vsctl -- add-port br-int vif$i \ + -- set Interface vif$i external-ids:iface-id=lsp$i done # give controllers some time to fight for the port binding @@ -16443,6 +16449,10 @@ max_claims=20 AT_CHECK([test "${hv1_claims}" -le "${max_claims}"], [0], []) AT_CHECK([test "${hv2_claims}" -le "${max_claims}"], [0], []) +check ovn-nbctl --wait=hv lsp-del lsp0 +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1]) +CHECK_FLOWS_AFTER_RECOMPUTE([hv2], [hv2]) + OVN_CLEANUP([hv1],[hv2]) AT_CLEANUP
When a port was claimed by two chassis, both chassis were fighting for the port; a claim was postponed if the port was claimed recently. However, there were two issues: - Not all the flows were properly removed when the remote chassis was claiming the port. - A port was not properly released if claim was postponed when the port_binding was deleted. There were also multiple cases causing ovn-controller to always wake-up immediately as a port was not removed from postponed_port list. - Changing port type to localport while port claim was postponed. - Deleting parent of a container port while parent was postponed. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/binding.c | 29 +++++++++++++++++++++++++++-- tests/ovn.at | 10 ++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-)