Thanks, Numan!
-venu
On Monday, September 28, 2020, 3:34:20 AM PDT, Numan Siddique
<[email protected]> wrote:
On Fri, Sep 25, 2020 at 11:44 PM <[email protected]> wrote:
>
> From: venu iyer <[email protected]>
>
> 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.
>
> Fixes: Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> Signed-off-by: venu iyer <[email protected]>
Thanks for the verson 3.
Instead of having multiple returns in the function - chassis_tunnels_changed(),
I modified the function to have just one return and applied the patch
to master and backported
to branch-20.,09 and branch-20.06.
Below are the changes I did on top of your patch
**************************************
diff --git a/controller/chassis.c b/controller/chassis.c
index 7d78366e34..7748fb94cf 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -417,42 +417,46 @@ chassis_tunnels_changed(const struct sset *encap_type_set,
{
struct sset chassis_rec_encap_type_set =
SSET_INITIALIZER(&chassis_rec_encap_type_set);
+ bool changed = false;
for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
if (!sset_contains(encap_type_set, chassis_rec->encaps[i]->type)) {
- sset_destroy(&chassis_rec_encap_type_set);
- return true;
+ changed = true;
+ break;
}
sset_add(&chassis_rec_encap_type_set, chassis_rec->encaps[i]->type);
if (!sset_contains(encap_ip_set, chassis_rec->encaps[i]->ip)) {
- sset_destroy(&chassis_rec_encap_type_set);
- return true;
+ changed = true;
+ break;
}
if (strcmp(smap_get_def(&chassis_rec->encaps[i]->options, "csum", ""),
encap_csum)) {
- sset_destroy(&chassis_rec_encap_type_set);
- return true;
+ changed = true;
+ break;
}
}
- size_t tunnel_count =
- sset_count(encap_type_set) * sset_count(encap_ip_set);
+ if (!changed) {
+ size_t tunnel_count =
+ sset_count(encap_type_set) * sset_count(encap_ip_set);
- if (tunnel_count != chassis_rec->n_encaps) {
- sset_destroy(&chassis_rec_encap_type_set);
- return true;
+ if (tunnel_count != chassis_rec->n_encaps) {
+ changed = true;
+ }
}
- if (sset_count(encap_type_set) !=
- sset_count(&chassis_rec_encap_type_set)) {
- sset_destroy(&chassis_rec_encap_type_set);
- return true;
+ if (!changed) {
+ if (sset_count(encap_type_set) !=
+ sset_count(&chassis_rec_encap_type_set)) {
+ changed = true;
+ }
}
+
sset_destroy(&chassis_rec_encap_type_set);
- return false;
+ return changed;
}
/*
diff --git a/tests/ovn.at b/tests/ovn.at
index bd50157058..7769b69ed0 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22321,8 +22321,7 @@ ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.1 24 geneve
# Get the encap rec, should be just one - with geneve/192.168.0.1
-encap_rec=`ovn-sbctl --data=bare --no-heading --column encaps list chassis hv1`
-#echo "Encap Rec before mvtep is $encap_rec"
+encap_rec=$(ovn-sbctl --data=bare --no-heading --column encaps list
chassis hv1)
# Set multiple IPs
as hv1
@@ -22331,8 +22330,7 @@ ovs-vsctl \
# Check if the encap_rec changed - should have, no need to
# compare the exact values.
-encap_rec_mvtep=`ovn-sbctl --data=bare --no-heading --column encaps
list chassis hv1`
-#echo "Encap Rec after mvtep is $encap_rec_mvtep"
+encap_rec_mvtep=$(ovn-sbctl --data=bare --no-heading --column encaps
list chassis hv1)
AT_CHECK([test "$encap_rec" != "$encap_rec_mvtep"], [0], [])
@@ -22340,8 +22338,7 @@ AT_CHECK([test "$encap_rec" !=
"$encap_rec_mvtep"], [0], [])
sleep 2
# Check if the encap_rec changed (it should not have)
-encap_rec_mvtep1=`ovn-sbctl --data=bare --no-heading --column encaps
list chassis hv1`
-#echo "Encap Rec after wait is $encap_rec_mvtep1"
+encap_rec_mvtep1=$(ovn-sbctl --data=bare --no-heading --column encaps
list chassis hv1)
AT_CHECK([test "$encap_rec_mvtep" == "$encap_rec_mvtep1"], [0], [])
**************************
Thanks
Numan
> ---
> controller/chassis.c | 15 ++++++++++----
> tests/ovn.at | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index a365188e8..7d78366e3 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -415,21 +415,25 @@ 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_INITIALIZER(&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)) {
> + sset_destroy(&chassis_rec_encap_type_set);
> 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)) {
> + sset_destroy(&chassis_rec_encap_type_set);
> return true;
> }
>
> if (strcmp(smap_get_def(&chassis_rec->encaps[i]->options, "csum",
>""),
> encap_csum)) {
> + sset_destroy(&chassis_rec_encap_type_set);
> return true;
> }
> }
> @@ -438,13 +442,16 @@ chassis_tunnels_changed(const struct sset
> *encap_type_set,
> sset_count(encap_type_set) * sset_count(encap_ip_set);
>
> if (tunnel_count != chassis_rec->n_encaps) {
> + sset_destroy(&chassis_rec_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)) {
> + sset_destroy(&chassis_rec_encap_type_set);
> return true;
> }
> -
> + sset_destroy(&chassis_rec_encap_type_set);
> return false;
> }
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b6c8622ba..bd5015705 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -22299,3 +22299,51 @@ ovn-sbctl --columns chassis --bare list port_binding
> sw0-p2
>
> OVN_CLEANUP([hv1])
> AT_CLEANUP
> +
> +#
> +# When multiple encap-ips are configued, make sure the
> +# ovn-controller doesn't get into a state of constantly
> +# updating the SB Chassis's encap information. The test
> +# configures multiple vteps and waits for a couple
> +# of seconds to makes sure the SB encap info remains
> +# unchanged.
> +#
> +#
> +AT_SETUP([ovn -- multi-vtep SB Chassis encap updates])
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +
> +ovs-vsctl add-br br-phys
> +# Just set the encap type to be geneve for this test.
> +ovn_attach n1 br-phys 192.168.0.1 24 geneve
> +
> +# Get the encap rec, should be just one - with geneve/192.168.0.1
> +encap_rec=`ovn-sbctl --data=bare --no-heading --column encaps list chassis
> hv1`
> +#echo "Encap Rec before mvtep is $encap_rec"
> +
> +# Set multiple IPs
> +as hv1
> +ovs-vsctl \
> + -- set Open_vSwitch . external-ids:ovn-encap-ip="192.168.0.1,192.168.1.1"
> +
> +# Check if the encap_rec changed - should have, no need to
> +# compare the exact values.
> +encap_rec_mvtep=`ovn-sbctl --data=bare --no-heading --column encaps list
> chassis hv1`
> +#echo "Encap Rec after mvtep is $encap_rec_mvtep"
> +
> +AT_CHECK([test "$encap_rec" != "$encap_rec_mvtep"], [0], [])
> +
> +# now, wait for a couple of secs - should be enough time, I suppose.
> +sleep 2
> +
> +# Check if the encap_rec changed (it should not have)
> +encap_rec_mvtep1=`ovn-sbctl --data=bare --no-heading --column encaps list
> chassis hv1`
> +#echo "Encap Rec after wait is $encap_rec_mvtep1"
> +
> +AT_CHECK([test "$encap_rec_mvtep" == "$encap_rec_mvtep1"], [0], [])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
> 2.17.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
