diff mbox series

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

Message ID 20200923171108.8620-1-venugopali@nvidia.com
State Superseded
Headers show
Series [ovs-dev,1/1] ovn-controller: Check for tunnel change in multi-vtep case is incorrect | expand

Commit Message

Venugopal Iyer Sept. 23, 2020, 5:11 p.m. UTC 
From: venu iyer <venugopali@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@nvidia.com)
---
 controller/chassis.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Numan Siddique Sept. 23, 2020, 5:17 p.m. UTC | #1 
On Wed, Sep 23, 2020 at 10:40 PM <venugopali@nvidia.com> wrote:

> From: venu iyer <venugopali@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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
venugopal iyer Sept. 23, 2020, 8:16 p.m. UTC | #2 
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@ovn.org> wrote:  
 
 On Wed, Sep 23, 2020 at 10:40 PM <venugopali@nvidia.com> wrote:

> From: venu iyer <venugopali@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@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
diff mbox series

Patch

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;
     }