[ovs-dev] [PATCH ovn 1/1] ovn-controller: Check for tunnel change in multi-vtep case is incorrect

[venugopal iyer]

 Hi, Numan:
I'll add the Fixes tag, let me think about how to write a test case for this.
thanks!
-venu

    On Wednesday, September 23, 2020, 10:18:00 AM PDT, Numan Siddique <numans at ovn.org> wrote:  
 
 On Wed, Sep 23, 2020 at 10:40 PM <venugopali at nvidia.com> wrote:

> From: venu iyer <venugopali at nvidia.com>
>
> Prior to multi-vtep, there was one tunnel for each type, since
> there was only one encap-ip. So, the check in
> chassis_tunnels_changed():
>        sset_count(encap_type_set) != encap_type_count
> worked. However, with multiple IPs per tunnel type, the above
> check won't work. So, once multiple encap-ips are configured,
> ovn-controller will always keep updating the encap list in
> the SB (due to the above check); this causes a lot of unnecessary
> churn, including recomputing the flows etc. which will put
> ovn-controller in overdrive thereby consuming lot of CPU cycles
> (see almost 100%).
>
> Verified ovn-controller cpu utilization with the fix (and also
> that SB doesn't keep constantly updating). make check didn't
> show any additional failures.
>
> Signed-off-by: venu iyer (venugopali at nvidia.com)
> ---
>

hi Venu,

I didn't look into the patch. But I have a couple of requests.
Can you please put the Fixes tag with the commit id which introduced
multi-vtep ?
And also a test case to cover this scenario ?

Thanks
Numan


>  controller/chassis.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index a365188e8..a6dfb92df 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -415,14 +415,15 @@ chassis_tunnels_changed(const struct sset
> *encap_type_set,
>                          const char *encap_csum,
>                          const struct sbrec_chassis *chassis_rec)
>  {
> -    size_t encap_type_count = 0;
> +    struct sset chassis_rec_encap_type_set;
>
> +    sset_init(&chassis_rec_encap_type_set);
>      for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
>
>          if (!sset_contains(encap_type_set, chassis_rec->encaps[i]->type))
> {
>              return true;
>          }
> -        encap_type_count++;
> +        sset_add(&chassis_rec_encap_type_set,
> chassis_rec->encaps[i]->type);
>
>          if (!sset_contains(encap_ip_set, chassis_rec->encaps[i]->ip)) {
>              return true;
> @@ -441,7 +442,8 @@ chassis_tunnels_changed(const struct sset
> *encap_type_set,
>          return true;
>      }
>
> -    if (sset_count(encap_type_set) != encap_type_count) {
> +    if (sset_count(encap_type_set) !=
> +            sset_count(&chassis_rec_encap_type_set)) {
>          return true;
>      }
>
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
dev at openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev