[ovs-dev] lflow: Add missing sample flow.

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 <xsimonar@redhat.com>
---
 controller/ovn-controller.c | 13 +++++++++
 tests/ovn.at                | 58 ++++++++++++++++++++++++++++++++-----
 2 files changed, 64 insertions(+), 7 deletions(-)

Thank you for this Xavier,

I have one minor suggestion below. It can be fixed when this is merged.

Acked-by: Mark Michelson <mmichels@redhat.com>

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 <xsimonar@redhat.com>
> ---
>   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