[ovs-dev,v2,3/3] controller: Drop binding requests for paused virtual port.

Drop the binding requests for a virtual port if that port
set to pause in northd.

Signed-off-by: Mohammad Heib <mheib@redhat.com>
---
 controller/pinctrl.c | 39 ++++++++++++++++++-
 tests/ovn.at         | 91 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+), 2 deletions(-)

On 8/1/24 06:44, Mohammad Heib wrote:
> Drop the binding requests for a virtual port if that port
> set to pause in northd.
> 
> Signed-off-by: Mohammad Heib <mheib@redhat.com>
> ---
>   controller/pinctrl.c | 39 ++++++++++++++++++-
>   tests/ovn.at         | 91 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 7cbb0cf81..7420f2009 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -7057,11 +7057,16 @@ struct put_vport_binding {
>   
>       /* This vport record Only relevant if "new_record" is true. */
>       bool new_record;
> +    /* The creation time in pinctrl thread */
> +    long long int creation_time;
>   };
>   
>   /* Contains "struct put_vport_binding"s. */
>   static struct hmap put_vport_bindings;
>   
> +/* pause duration for port that set puased in northd. */
> +#define PAUSE_DURATION 10000
> +
>   /*
>    * Validate if the vport_binding record that was added
>    * by the pinctrl thread is still relevant and needs
> @@ -7145,7 +7150,7 @@ run_put_vport_binding(struct ovsdb_idl_txn *ovnsb_idl_txn OVS_UNUSED,
>                         struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
>                         struct ovsdb_idl_index *sbrec_port_binding_by_key,
>                         const struct sbrec_chassis *chassis,
> -                      const struct put_vport_binding *vpb)
> +                      struct put_vport_binding *vpb)
>   {
>       /* Convert logical datapath and logical port key into lport. */
>       const struct sbrec_port_binding *pb = lport_lookup_by_key(
> @@ -7159,6 +7164,35 @@ run_put_vport_binding(struct ovsdb_idl_txn *ovnsb_idl_txn OVS_UNUSED,
>           return;
>       }
>   
> +    if (smap_get(&pb->options, "binding_request_pause")) {
> +        long long int p_time = smap_get_ullong(&pb->options,
> +                      "binding_request_pause_ts", 0);
> +        /* Pause duration for this port still relevant, drop this
> +         * binding request, and set vpb->new_record=false to make sure
> +         * that it will be deleted from the list when flushing the list.
> +         */
> +        if ((p_time + PAUSE_DURATION) > vpb->creation_time) {
> +            vpb->new_record = false;
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +            VLOG_DBG_RL(&rl,
> +                         "Virtual lport %s drop binding request port "
> +                         "in pause state\n", pb->logical_port);
> +
> +            return;
> +        } else {
> +            VLOG_INFO("Virtual lport %s binding requests paused "
> +                       "for 10 seconds, resume binding requests handling.",

The log message here is a bit misleading, since "Virtual lport binding 
requests paused for 10 seconds" sounds like you are about to pause the 
requests for 10 seconds, not that the requests have been paused for 10 
seconds already.

In addition, saying "10 seconds" here is not a great idea since the 
PAUSE_DURATION could change. I think the message could just be "Virtual 
lport %s binding requests resumed". It's more succinct this way and 
doesn't try to refer to any potential values that could change.

> +                       pb->logical_port);
> +            struct smap options;
> +            smap_clone(&options, &pb->options);
> +            smap_remove(&options, "binding_request_pause");
> +            smap_remove(&options, "binding_request_pause_ts");
> +            sbrec_port_binding_set_options(pb, &options);
> +            smap_destroy(&options);
> +
> +        }
> +    }
> +
>       /* pinctrl module updates the port binding only for type 'virtual'. */
>       if (!strcmp(pb->type, "virtual")) {
>           const struct sbrec_port_binding *parent = lport_lookup_by_key(
> @@ -7187,7 +7221,7 @@ run_put_vport_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
>           return;
>       }
>   
> -    const struct put_vport_binding *vpb;
> +    struct put_vport_binding *vpb;
>       HMAP_FOR_EACH (vpb, hmap_node, &put_vport_bindings) {
>           run_put_vport_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
>                                 sbrec_port_binding_by_key, chassis, vpb);
> @@ -7232,6 +7266,7 @@ pinctrl_handle_bind_vport(
>       vpb->vport_key = vport_key;
>       vpb->vport_parent_key = vport_parent_key;
>       vpb->new_record = true;
> +    vpb->creation_time = time_msec();

Like with patch 2, since this is being synced with a timestamp from 
another machine, this should use time_wall_msec() instead of time_msec().

>       notify_pinctrl_main();
>   }
>   
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b31afbfb3..408505ee9 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -22442,6 +22442,97 @@ OVN_CLEANUP([hv1], [hv2])
>   AT_CLEANUP
>   ])
>   
> +# Create 10 HV's each have 3 VIF ports that all
> +# sends Garp at the same time to bind vport sw0-vir
> +# this will create high pressure on SB/North and will
> +# lead to a transaction dropping by SB.
> +#
> +# Northd must be able to detect such cases and pause
> +# binding requests for this specific port for a certain
> +# amount of time.
> +#
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([virtual ports - binding requests storm])
> +AT_KEYWORDS([virtual ports])
> +ovn_start
> +
> +send_garp() {
> +    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
> +    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}

Typically these days we try not to add new tests that write packets 
directly like this. Using fmt_pkt() is the preferred way to write tests 
since it is more expressive.

> +    as hv$hv ovs-appctl netdev-dummy/receive vif$hv$inport $request
> +}
> +
> +net_add n1
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl ls-add sw1
> +parents=""
> +for i in {1..9}; do
> +    sim_add hv$i
> +    as hv$i
> +    ovs-vsctl add-br br-phys
> +    ovn_attach n1 br-phys 192.168.0.$i
> +    ovs-appctl -t ovn-controller vlog/set dbg
> +
> +    for j in {1..3}; do
> +        check ovn-nbctl lsp-add sw0 sw0-h$i-p$j
> +        check ovn-nbctl lsp-set-addresses sw0-h$i-p$j "50:54:00:00:00:$i$j 10.0.0.$i$j 1000::$i$j"
> +        check ovn-nbctl lsp-set-port-security sw0-h$i-p$j "50:54:00:00:00:$i$j 10.0.0.$i$j 10.0.0.120 1000::$i$j"
> +        parents+=$"sw0-h$i-p$j,"
> +        ovs-vsctl -- add-port br-int vif$i$j -- \
> +                set interface vif$i$j \
> +                external-ids:iface-id=sw0-h$i-p$j \
> +                options:tx_pcap=hv$i/vif$i$j-tx.pcap \
> +                options:rxq_pcap=hv$i/vif$i$j-rx.pcap \
> +                ofport-request=$i$j
> +    done
> +done
> +
> +ovs-vsctl -- add-port br-int vifsw1 -- \
> +        set interface vifsw1 \
> +        external-ids:iface-id=sw1-p1 \
> +        options:tx_pcap=hv$i/vifsw1-tx.pcap \
> +        options:rxq_pcap=hv$i/vifsw1-rx.pcap \
> +        ofport-request=122
> +
> +check ovn-nbctl lsp-add sw0 sw0-vir
> +check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:10:10 10.0.0.120"
> +check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:10:10 10.0.0.120"
> +check ovn-nbctl lsp-set-type sw0-vir virtual
> +check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.120
> +check ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=$parents
> +
> +# Add an ACL that matches on sw0-vir being bound locally.
> +check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir") && ip4' allow
> +OVN_POPULATE_ARP
> +wait_for_ports_up
> +
> +# Start sending many Garp requests on randomly selected ports and chassis
> +# to pressure the SB/Northd
> +eth_dst=ffffffffffff
> +spa=$(ip_to_hex 10 0 0 120)
> +tpa=$(ip_to_hex 10 0 0 120)
> +while : ; do
> +    random_hv=$(shuf -i 1-9 -n 1)
> +    random_port=$(shuf -i 1-3 -n 1)
> +    eth_src=5054000000$random_hv$random_port
> +    send_garp $random_hv $random_port $eth_src $eth_dst $spa $tpa
> +    sleep 0.2
> +done &
> +pid1=$!
> +
> +OVS_WAIT_UNTIL([test 0 != `grep -c "Pausing virtual port sw0-vir from sending binding requests for few seconds." northd/ovn-northd.log`])
> +# Kill the Loop before exiting otherwise the subshell will keeps
> +# try to send pkts on HVs ovs interfaces which been cleaned and deleted
> +# and that will casue test to failed.
> +kill -9 $pid1
> +wait $pid1

I think the test should wait and then try to bind the virtual port after 
the pause duration and ensure that it works properly.

> +
> +for i in {1..9}; do
> +    OVN_CLEANUP_SBOX([hv$i])
> +done
> +AT_CLEANUP
> +])
> +
>   OVN_FOR_EACH_NORTHD([
>   # Run ovn-nbctl in daemon mode, change to a backup database and verify that
>   # an insert operation is not allowed.