Thank you for this Xavier,
I have one minor suggestion below. It can be fixed when this is merged.
Acked-by: Mark Michelson <[email protected]>
On 4/26/24 06:52, Xavier Simonart wrote:
If lflow inode had to be recomputed (e.g. due to non_vif_data change), then
there could be
some missing sample actions.
This issue was highlighted by some flaky failures of test "Check default openflow
flows".
The test "Check default openflow flows" has been updated to create a
race-condition
in which ovn-controller handles both ovsdb Flow_Sample_Collector_Set changes
and ofport
change for a geneve interface at the same time.
Finally, the test has also been updated as
- It used to print many "printf %s\n: command not found" due to IFS changes.
- It was using a non-existing check_debug function.
Fixes: 5b1476709d7c ("controller: only sample flow if Collector Set exists")
Signed-off-by: Xavier Simonart <[email protected]>
---
controller/ovn-controller.c | 13 +++++++++
tests/ovn.at | 58 ++++++++++++++++++++++++++++++++-----
2 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 23269af83..a7dff53eb 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4036,6 +4036,8 @@ en_lflow_output_run(struct engine_node *node, void *data)
EN_OVSDB_GET(engine_get_input("OVS_bridge", node));
const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
const char *chassis_id = get_ovs_chassis_id(ovs_table);
+ const struct ovsrec_flow_sample_collector_set_table *flow_collector_table =
+ EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node));
struct ovsdb_idl_index *sbrec_chassis_by_name =
engine_ovsdb_node_get_index(
@@ -4049,6 +4051,17 @@ en_lflow_output_run(struct engine_node *node, void *data)
ovs_assert(br_int && chassis);
+ const struct ovsrec_flow_sample_collector_set *set;
+ OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (set,
+ flow_collector_table) {
+ if (set->bridge == br_int) {
+ struct ed_type_lflow_output *lfo = data;
+ flow_collector_ids_clear(&lfo->collector_ids);
+ flow_collector_ids_init_from_table(&lfo->collector_ids,
+ flow_collector_table);
+ }
+ }
+
struct ed_type_lflow_output *fo = data;
struct ovn_desired_flow_table *lflow_table = &fo->flow_table;
struct ovn_extend_table *group_table = &fo->group_table;
diff --git a/tests/ovn.at b/tests/ovn.at
index 79c9524c6..2c8d15134 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -35118,7 +35118,10 @@ check_default_flows() {
# Check that every drop flow gets sampled.
check_sample_drops() {
-
+ hv=hv$1
+ remote_hv=hv$((${1}%2 + 1))
+ race_condition=$2
+ ovs-vsctl destroy Flow_Sample_Collector_Set 123
check ovn-nbctl -- remove NB_Global . options debug_drop_collector_set \
-- remove NB_Global . options debug_drop_domain_id
check ovn-nbctl --wait=hv sync
@@ -35128,14 +35131,52 @@ check_sample_drops() {
# Take match part of flows that contain "drop".
drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, priority=.*
')"
+ if [[ x$race_condition = x"true" ]]; then
+ sleep_controller $hv
+ # Get ofport used by the geneve interface
+ OVS_WAIT_UNTIL([
+ ofport=$(as $hv ovs-vsctl --bare --columns ofport find Interface
name=ovn-${remote_hv}-0)
+ test 1 -le $ofport
+ ])
+
+ # Add a vif while ovn-controller sleeps, and make it request the
ofport used by the geneve interface.
+ # This used to cause the geneve interface to change ofport.
Suggestion: s/This used to/This is used to/
"This used to" sounds like something was true in the past but is no
longer true now. I got confused when I looked below and saw that the
geneve ofport is changing, since the comment makes it sound like that
shouldn't be happening any more.
Saying "This is used to..." makes it more clear that you are purposely
invoking a current behavior, rather than documenting a past behavior
that you are trying to ensure is no longer present.
+ ovs-vsctl -- add-port br-int vif3 -- set interface vif3\
+ options:tx_pcap=${hv}/vif3-tx.pcap \
+ options:rxq_pcap=${hv}/vif3-rx.pcap \
+ ofport-request=$ofport
+ OVS_WAIT_UNTIL([
+ vif_ofport=$(as $hv ovs-vsctl --bare --columns ofport find
Interface name=vif3)
+ test 1 -le $vif_ofport
+ ])
+ # For the geneve interface ofport change to happen...
+ ovs-vsctl -- add-port br-int vif4 -- set interface vif4\
+ options:tx_pcap=${hv}/vif4-tx.pcap \
+ options:rxq_pcap=${hv}/vif4-rx.pcap
+ OVS_WAIT_UNTIL([
+ new_ofport=$(as $hv ovs-vsctl --bare --columns ofport find
Interface name=ovn-${remote_hv}-0)
+ test $ofport -ne $new_ofport
+ ])
+ fi
+
ovs-vsctl --id=@br get Bridge br-int -- \
--id=@i create IPFIX targets=\"192.168.1.1\" -- \
create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i
check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \
-- set NB_Global . options:debug_drop_domain_id="1"
- check ovn-nbctl --wait=hv sync
+ if [[ x$race_condition = x"true" ]]; then
+ # Wait sb as ovn-controller sleeps.
+ check ovn-nbctl --wait=sb sync
+ # Wake up ovn-controller. It should most of the times receive non-vif
ofport change and sb changes at the same time.
+ wake_up_controller $hv
+ # Check twice ovn-controller is running to guarantee if run a full
loop.
+ OVS_WAIT_UNTIL([test x$(as $hv ovn-appctl -t ovn-controller debug/status) =
"xrunning"])
+ OVS_WAIT_UNTIL([test x$(as $hv ovn-appctl -t ovn-controller debug/status) =
"xrunning"])
+ else
+ check ovn-nbctl --wait=hv sync
+ fi
ovs-ofctl dump-flows --no-stats br-int > oflows_sample
AT_CAPTURE_FILE([oflows_sample])
@@ -35143,6 +35184,8 @@ check_sample_drops() {
save_IFS=$IFS
IFS=$'\n'
for flow in $drop_matches; do
+ # Restore IFS to avoid "printf %s\n: command not found" errors.
+ IFS=$save_IFS
AT_CHECK([grep "${flow}actions=.*sample.*" oflows_sample], [0], [ignore],
[ignore], [echo "Flow $flow has a drop and did not get sampled"])
done
IFS=$save_IFS
@@ -35155,9 +35198,9 @@ check_drops() {
check_default_flows
as hv1
- check_sample_drops
+ check_sample_drops 1 $1
as hv2
- check_sample_drops
+ check_sample_drops 2 $1
}
# Logical network:
@@ -35225,7 +35268,8 @@ ovs-vsctl -- add-port br-int hv2-vif1 -- \
wait_for_ports_up
check ovn-nbctl --wait=hv sync
-check_debug
+check_drops
+check_drops true
# Add stateless ACL
check ovn-nbctl --wait=sb \
@@ -35233,7 +35277,7 @@ check ovn-nbctl --wait=sb \
check ovn-nbctl --wait=sb \
-- acl-add ls2 from-lport 100 'ip4' allow-stateless
-check_debug
+check_drops
check ovn-nbctl --wait=sb acl-del ls1
check ovn-nbctl --wait=sb acl-del ls2
@@ -35244,7 +35288,7 @@ check ovn-nbctl --wait=sb \
check ovn-nbctl --wait=sb \
-- acl-add ls2 from-lport 100 "udp" allow-related
-check_debug
+check_drops
check ovn-nbctl --wait=sb acl-del ls1
check ovn-nbctl --wait=sb acl-del ls2
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
