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