Re: [ovs-dev] [PATCH ovn 4/5] controller: Handle postponed ports claims.
On Tue, Apr 23, 2024 at 7:54 AM Xavier Simonart 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
Acked-by: Numan Siddique
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,
> + _lport->lbinding->iface->header_.uuid);
> +}
>
> return true;
> }
> @@ -1787,7 +1808,8 @@ consider_localport(const struct sbrec_port_binding *pb,
> struct shash *local_bindings = _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, >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
>
>
[ovs-dev] [PATCH ovn 4/5] controller: Handle postponed ports claims.
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
---
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,
+ _lport->lbinding->iface->header_.uuid);
+}
return true;
}
@@ -1787,7 +1808,8 @@ consider_localport(const struct sbrec_port_binding *pb,
struct shash *local_bindings = _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, >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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
