Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[frr] Reduce Calls to SONiC Cfggen #5176

Merged

Conversation

tahmed-dev
Copy link
Contributor

@tahmed-dev tahmed-dev commented Aug 14, 2020

Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed tamer.ahmed@microsoft.com

- Why I did it
Reduce time required when invoking frr

- How I did it
Used template batch mode to batch together three calls into one call to sonic-cfggen

- How to verify it

root@str-s6000-acs-14:/# time ./docker_init_old.sh

real	0m12.873s
user	0m10.486s
sys	0m1.624s
root@str-s6000-acs-14:/# time ./docker_init_new.sh

real	0m3.572s
user	0m2.927s
sys	0m0.499s
root@str-s6000-acs-14:/# diff /etc/supervisor/conf.d/supervisord.conf.old /etc/supervisor/conf.d/supervisord.conf.new 
root@str-s6000-acs-14:/# diff /etc/frr/bgpd.conf.old /etc/frr/bgpd.conf.new 
root@str-s6000-acs-14:/# diff /etc/frr/zebra.conf.old /etc/frr/zebra.conf.new 
root@str-s6000-acs-14:/# diff /etc/frr/staticd.conf.old /etc/frr/staticd.conf.new 
root@str-s6000-acs-14:/# diff /usr/sbin/bgp-isolate.old /usr/sbin/bgp-isolate.new 
root@str-s6000-acs-14:/# diff /usr/sbin/bgp-unisolate.old /usr/sbin/bgp-unisolate.new 
root@str-s6000-acs-14:/# 

Update for the case when just once call is placed t sonic-cfggen:

root@str-s6000-acs-14:/# time ./docker_init_old.sh
CONFIG_TYPE=separated

real	0m12.117s
user	0m9.803s
sys	0m1.523s
root@str-s6000-acs-14:/# time ./docker_init_new.sh
CONFIG_TYPE=separated

real	0m2.598s
user	0m1.986s
sys	0m0.383s
root@str-s6000-acs-14:/# diff /etc/supervisor/conf.d/supervisord.conf.old /etc/supervisor/conf.d/supervisord.conf.new 
root@str-s6000-acs-14:/# diff /etc/frr/bgpd.conf.old /etc/frr/bgpd.conf.new 
root@str-s6000-acs-14:/# diff /etc/frr/zebra.conf.old /etc/frr/zebra.conf.new
root@str-s6000-acs-14:/# diff /etc/frr/staticd.conf.old /etc/frr/staticd.conf.new
root@str-s6000-acs-14:/# diff /usr/sbin/bgp-isolate.old /usr/sbin/bgp-isolate.new
root@str-s6000-acs-14:/# diff /usr/sbin/bgp-unisolate.old /usr/sbin/bgp-unisolate.new
root@str-s6000-acs-14:/# 

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006

@tahmed-dev tahmed-dev force-pushed the taahme/frr-reduce-cfggen-calls branch from 22d39a7 to 84ef3b3 Compare August 14, 2020 00:10
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
@tahmed-dev tahmed-dev force-pushed the taahme/frr-reduce-cfggen-calls branch from 84ef3b3 to a5f2de0 Compare August 14, 2020 01:07
@tahmed-dev tahmed-dev marked this pull request as ready for review August 14, 2020 01:34
@lguohan
Copy link
Collaborator

lguohan commented Aug 17, 2020

@pavel-shirshov to review

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have couple questions

CFGGEN_PARAMS=" \
-d \
-y /etc/sonic/constants.yml \
-t /usr/share/sonic/templates/supervisord/frr_vars.j2 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need frr_vars?
Also why you have only source after -t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the frr_vars is a template file that pulls information about the config_type. This variable later decides if BGP service is integrate into vtysh or not. The content of the frr_vars is retreiving single variable for now:
{{ DEVICE_METADATA["localhost"]["docker_routing_config_mode"] }}
Lack of destination after the first argument, means this template is going to be rendered to stdout.

In comparison, commit a5f2de does docker initialization with two calls to cfggen. The time difference between the two approaches is about 1sec.

Copy link
Contributor

@pavel-shirshov pavel-shirshov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tahmed-dev tahmed-dev merged commit a10c5bf into sonic-net:master Aug 17, 2020
abdosi pushed a commit that referenced this pull request Oct 7, 2020
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
lguohan pushed a commit that referenced this pull request Oct 30, 2020
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
abdosi pushed a commit that referenced this pull request Dec 4, 2020
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
abdosi pushed a commit that referenced this pull request Dec 22, 2020
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen to two calls during startup when starting frr service.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants