On Fri, Jan 17, 2025 at 12:29 PM Lorenzo Bianconi
<[email protected]> wrote:
>
> Similar to commit 1431276926 ("controller: fix ovn patch port
> incremental processing"), update peer logical flows when the related
> patch port is removed.
>
> Reported-at: https://issues.redhat.com/browse/FDP-947
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Changes in v3:
> - fallback to full recompute when type column in SB.port_binding tale is
> updated
> - update pb patch port in 'removed' case in physical_handle_flows_for_lport()
> Changes in v2:
> - use OFTABLE_LOG_TO_PHY for table 65
Thanks Lorenzo.
I applied this patch to main with the below changes in the test code.
Once the tests pass on my github repo for the branches 24.09 and
24.03, I'll backport to them.
-------------------------------------------
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index b2c90409bd..0dd39ad97d 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -1,5 +1,52 @@
AT_BANNER([ovn-controller])
+m4_divert_push([PREPARE_TESTS])
+# Checks if the provided engine node recomputed or not
+# and if it computed or not.
+# 1st argument is the engine node.
+# 2nd argument is the expected recompute state.
+# Possible values are - "norecompute" and "recompute".
+# 3rd argument is the expected compute state.
+# Possible values are - "nocompute" and "compute".
+#
+# Eg. 'check_controller_engine_stats hv1 logical_flow_output
recompute nocompute'
+# will verify that if the lflow engine node recompute stat is > 0 and
+# compute stat is equal to 0. It fails otherwise.
+check_controller_engine_stats() {
+ hv=$1
+ node=$2
+ recompute=$3
+ compute=$4
+
+ echo "Checking engine stats for node $node : recompute - \
+$recompute : compute - $compute"
+
+ node_stat=$(as $hv ovn-appctl -t ovn-controller inc-engine/show-stats $node)
+ # node_stat will be of this format :
+ # - Node: logical_flow_output - recompute: 3 - compute: 0 - cancel: 0
+ node_recompute_ct=$(echo $node_stat | cut -d '-' -f2 | cut -d ':' -f2)
+ node_compute_ct=$(echo $node_stat | cut -d '-' -f3 | cut -d ':' -f2)
+
+ if [[ "$recompute" == "norecompute" ]]; then
+ # node should not be recomputed
+ echo "Expecting $node recompute count - $node_recompute_ct to be 0"
+ check test "$node_recompute_ct" -eq "0"
+ else
+ echo "Expecting $node recompute count - $node_recompute_ct not to be 0"
+ check test "$node_recompute_ct" -ne "0"
+ fi
+
+ if [[ "$compute" == "nocompute" ]]; then
+ # node should not be computed
+ echo "Expecting $node compute count - $node_compute_ct to be 0"
+ check test "$node_compute_ct" -eq "0"
+ else
+ echo "Expecting $node compute count - $node_compute_ct not to be 0"
+ check test "$node_compute_ct" -ne "0"
+ fi
+}
+m4_divert_pop([PREPARE_TESTS])
+
OVN_FOR_EACH_NORTHD([
AT_SETUP([ovn-controller - ovn-bridge-mappings])
AT_KEYWORDS([ovn])
@@ -918,11 +965,9 @@ check ovn-nbctl lrp-add lr0 rp-ls1
00:00:01:01:02:03 192.168.1.254/24
OVS_WAIT_UNTIL([as hv1 ovs-ofctl dump-flows br-int | grep
table=OFTABLE_LOCAL_OUTPUT | grep -q
"reg15=0x${port},metadata=0x${meta}"])
# Check we have a full recompute if type column is updated
-AT_CHECK([ovn-appctl -t ovn-controller inc-engine/clear-stats])
-check ovn-nbctl lsp-set-type ls0-rp localnet
-AT_CHECK([ovn-appctl -t ovn-controller inc-engine/show-stats | grep
physical_flow_output -A 1 | awk '/recompute/{print $3}'],[0],[dnl
-1
-])
+check as hv1 ovn-appctl -t ovn-controller inc-engine/clear-stats
+check ovn-nbctl --wait=hv lsp-set-type ls0-rp localnet
+check_controller_engine_stats hv1 physical_flow_output recompute nocompute
OVN_CLEANUP([hv1])
AT_CLEANUP
diff --git a/tests/ovn.at b/tests/ovn.at
index 651543461d..f0eca6ae41 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -40788,7 +40788,7 @@ wait_for_ports_up
check ovn-nbctl --wait=hv set logical_router_port lr-ls
options:requested-tnl-key=42
ls_tnl_key=$(fetch_column datapath_binding tunnel_key external_ids:name=ls)
-AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
metadata=0x${ls_tnl_key} | grep -q load:0x2a->NXM_NX_REG14])
+AT_CHECK([ovs-ofctl dump-flows br-int
table=OFTABLE_LOG_TO_PHY,metadata=0x${ls_tnl_key} | grep -q
load:0x2a->NXM_NX_REG14])
check ovn-nbctl lrp-del lr-ls
check ovn-nbctl \
@@ -40796,7 +40796,7 @@ check ovn-nbctl
\
-- set logical_router_port lr-ls options:requested-tnl-key=43
check ovn-nbctl --wait=hv sync
-AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
metadata=0x${ls_tnl_key} | grep -q load:0x2b->NXM_NX_REG14])
+AT_CHECK([ovs-ofctl dump-flows br-int
table=OFTABLE_LOG_TO_PHY,metadata=0x${ls_tnl_key} | grep -q
load:0x2b->NXM_NX_REG14])
OVN_CLEANUP([hv1])
AT_CLEANUP
--------------------------------------------------------------------------
Thanks
Numan
> ---
> controller/ovn-controller.c | 5 ++++
> controller/physical.c | 20 +++++++++++-----
> tests/ovn-controller.at | 7 ++++++
> tests/ovn.at | 47 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 73 insertions(+), 6 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 890c8f988..8c44c88e3 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -4566,6 +4566,11 @@ pflow_output_sb_port_binding_handler(struct
> engine_node *node,
> */
> const struct sbrec_port_binding *pb;
> SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb, p_ctx.port_binding_table)
> {
> + /* Trigger a full recompute if type column is updated. */
> + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) {
> + destroy_physical_ctx(&p_ctx);
> + return false;
> + }
> bool removed = sbrec_port_binding_is_deleted(pb);
> if (!physical_handle_flows_for_lport(pb, removed, &p_ctx,
> &pfo->flow_table)) {
> diff --git a/controller/physical.c b/controller/physical.c
> index c56c73c20..1da2dd226 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -2411,13 +2411,21 @@ physical_handle_flows_for_lport(const struct
> sbrec_port_binding *pb,
> physical_multichassis_reprocess(pb, p_ctx, flow_table);
> }
>
> - if (!removed) {
> + /* Always update pb and the configured peer for patch ports. */
> + if (!removed || !strcmp(pb->type, "patch")) {
> physical_eval_port_binding(p_ctx, pb, flow_table);
> - if (!strcmp(pb->type, "patch")) {
> - const struct sbrec_port_binding *peer =
> - get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb);
> - if (peer) {
> - physical_eval_port_binding(p_ctx, peer, flow_table);
> + }
> +
> + if (!strcmp(pb->type, "patch")) {
> + if (removed) {
> + ofctrl_remove_flows(flow_table, &pb->header_.uuid);
> + }
> + const struct sbrec_port_binding *peer =
> + get_binding_peer(p_ctx->sbrec_port_binding_by_name, pb);
> + if (peer) {
> + physical_eval_port_binding(p_ctx, peer, flow_table);
> + if (removed) {
> + ofctrl_remove_flows(flow_table, &peer->header_.uuid);
> }
> }
> }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index df0158cd6..b2c90409b 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -917,6 +917,13 @@ check ovn-nbctl lrp-add lr0 rp-ls1 00:00:01:01:02:03
> 192.168.1.254/24
>
> OVS_WAIT_UNTIL([as hv1 ovs-ofctl dump-flows br-int | grep
> table=OFTABLE_LOCAL_OUTPUT | grep -q "reg15=0x${port},metadata=0x${meta}"])
>
> +# Check we have a full recompute if type column is updated
> +AT_CHECK([ovn-appctl -t ovn-controller inc-engine/clear-stats])
> +check ovn-nbctl lsp-set-type ls0-rp localnet
> +AT_CHECK([ovn-appctl -t ovn-controller inc-engine/show-stats | grep
> physical_flow_output -A 1 | awk '/recompute/{print $3}'],[0],[dnl
> +1
> +])
> +
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> ])
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 0ea403fd7..1297c5c51 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -40649,3 +40649,50 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int
> table=OFTABLE_PHY_TO_LOG | grep -v
> OVN_CLEANUP([hv1],[hv2])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([requested-tnl-key-recompute])
> +AT_KEYWORDS([requested-tnl-key-recompute])
> +
> +m4_define([OFTABLE_LOG_TO_PHY], [65])
> +
> +ovn_start
> +net_add n1
> +
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl lsp-add ls lsp -- lsp-set-addresses lsp "00:00:10:01:02:01
> 10.0.0.1"
> +
> +check ovn-nbctl lr-add lr
> +check ovn-nbctl set logical_router lr options:mac_binding_age_threshold=3600
> +check ovn-nbctl lrp-add lr lr-ls 00:00:00:00:ff:01 10.0.0.254/24
> +check ovn-nbctl lsp-add ls ls-lr
> +check ovn-nbctl lsp-set-type ls-lr router
> +check ovn-nbctl lsp-set-addresses ls-lr router
> +check ovn-nbctl lsp-set-options ls-lr router-port=lr-ls
> +
> +sim_add hv1
> +
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys
> +ovs-vsctl add-port br-int vif -- set Interface vif external-ids:iface-id=lsp
> +
> +check ovn-nbctl --wait=hv sync
> +wait_for_ports_up
> +
> +check ovn-nbctl --wait=hv set logical_router_port lr-ls
> options:requested-tnl-key=42
> +ls_tnl_key=$(fetch_column datapath_binding tunnel_key external_ids:name=ls)
> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
> metadata=0x${ls_tnl_key} | grep -q load:0x2a->NXM_NX_REG14])
> +
> +check ovn-nbctl lrp-del lr-ls
> +check ovn-nbctl \
> + -- lrp-add lr lr-ls 00:00:00:00:10:00 192.168.10.1/24 \
> + -- set logical_router_port lr-ls options:requested-tnl-key=43
> +check ovn-nbctl --wait=hv sync
> +
> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_LOG_TO_PHY | grep
> metadata=0x${ls_tnl_key} | grep -q load:0x2b->NXM_NX_REG14])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.48.0
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
