Skip to content

[cfggen] Extend Template Argument to Support Batch Mode#4941

Merged
tahmed-dev merged 7 commits intosonic-net:masterfrom
tahmed-dev:taahme/cfggen-add-batch-mode
Aug 12, 2020
Merged

[cfggen] Extend Template Argument to Support Batch Mode#4941
tahmed-dev merged 7 commits intosonic-net:masterfrom
tahmed-dev:taahme/cfggen-add-batch-mode

Conversation

@tahmed-dev
Copy link
Contributor

@tahmed-dev tahmed-dev commented Jul 10, 2020

Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from service to one call only.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

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

- Why I did it
In order to reduce number of calls being made to sonic-cfggen during boot process

- How I did it
Added new mode (batch) to cfggen where multiple templates could be rendered and written to disk at once

- How to verify it

  1. Provided test case to verify the functionality
  2. Incorporate this new batch into interfaces-config.sh and I could see gain of ~3 sec
admin@str-s6000-acs-14:~$ time sudo ./interfaces-config-old.sh

real	0m4.826s
user	0m3.854s
sys	0m0.769s
admin@str-s6000-acs-14:~$ time sudo ./interfaces-config-new.sh

real	0m1.835s
user	0m1.444s
sys	0m0.316s
admin@str-s6000-acs-14:~$ time sudo ./interfaces-config-old.sh

real	0m4.743s
user	0m3.747s
sys	0m0.708s
admin@str-s6000-acs-14:~$ time sudo ./interfaces-config-new.sh

real	0m1.636s
user	0m1.297s
sys	0m0.268s

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch from 6d760d0 to ba40fd9 Compare July 10, 2020 19:07
@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 1 alert when merging ba40fd93dcd033fd210a638faaa23f97c1fa3d4d into 81a9d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch from ba40fd9 to ba85069 Compare July 10, 2020 19:43
@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 1 alert when merging b6d5f14c80a61cbf4a40fb822bfb63595b114174 into 81a9d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 1 alert when merging 7af2d6ad23781590eb8543248a207f2ece860742 into 81a9d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 1 alert when merging 18406037008789890eb2a2911c1743cd9944ddb0 into 81a9d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch from 1840603 to 5051ef0 Compare July 10, 2020 23:00
@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2020

This pull request introduces 1 alert when merging 5051ef050c248d08e17356afd8817e5476953321 into 81a9d5a - view on LGTM.com

new alerts:

  • 1 for Unused import

@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch from 5051ef0 to cf16bc6 Compare July 11, 2020 04:12
@lgtm-com
Copy link

lgtm-com bot commented Jul 11, 2020

This pull request introduces 1 alert when merging cf16bc6d4c5216374d7a2ed4329b1fab868b4dfb into 3154e04 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Jul 13, 2020

This pull request introduces 1 alert when merging b47aaeb46b7096fffbb7e54506624e4fecfdc10b into bf45e11 - view on LGTM.com

new alerts:

  • 1 for Unused import

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.

As a comment

@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch 2 times, most recently from 9136b91 to 9a20d3b Compare July 21, 2020 04:04
@tahmed-dev tahmed-dev force-pushed the taahme/cfggen-add-batch-mode branch from 9a20d3b to a27c408 Compare July 21, 2020 04:07
pavel-shirshov
pavel-shirshov previously approved these changes Aug 7, 2020
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.

looks good for me

Calls to cfggen take considerable time. With batch mode, ww will
have the ability to limit number of calls from service to one call
only.

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
for template_file, dest_file in args.template:
template = env.get_template(os.path.basename(template_file))
template_data = template.render(sorted_data)
if dest_file == "config-db":
Copy link
Collaborator

@qiluo-msft qiluo-msft Aug 12, 2020

Choose a reason for hiding this comment

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

"config-db" [](start = 28, length = 11)

This is magic string for a special mode. What if user want to write to a file called 'config-db' ?

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 user can write and rename the file later. Currently when we save config-db.json, we simply redirect stdout to the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can use -t template-2.j2,[config-db] to indicate this is config-db, not a config-db file. if user really generate a config-db file, then use -t template-2.j2,\[config-db\]

Copy link
Contributor Author

@tahmed-dev tahmed-dev Aug 12, 2020

Choose a reason for hiding this comment

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

the user can use this syntax -t template-2.j2,/config-db or -t template-2.j2,./config-db to write to file named config-db. The check is strictly done against config-db as keyword. The / or ./ would serve as a distinction between the keyword and a filename that happens to be the same as the keyword.

I can changed it to [config-db], however the premise of argument still exist. What if the file name is [config-db]. I tend to think this a user error since there is clear way if config-db is used as a file name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried the ./config-db and it does generate the file as intended:

admin@str-s6000-acs-14:~$ sudo sonic-cfggen -d -t /usr/share/sonic/templates/rsyslog.conf.j2,./config-db
admin@str-s6000-acs-14:~$ ls -alrt
total 85
-rw-r--r-- 1 admin admin   807 Apr 18  2019 .profile
-rw-r--r-- 1 admin admin  3526 Apr 18  2019 .bashrc
-rw-r--r-- 1 admin admin   220 Apr 18  2019 .bash_logout
drwxr-xr-x 1 root  root   4096 Aug 11 03:41 ..
drwxr-xr-x 1 admin admin  4096 Aug 12 21:16 .
-rw-r--r-- 1 root  root   1907 Aug 12 21:16 config-db

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comment

@tahmed-dev tahmed-dev requested a review from qiluo-msft August 12, 2020 18:46
@qiluo-msft qiluo-msft dismissed their stale review August 12, 2020 21:27

Unblock, still the design of command line could improve to prevent keyword

@tahmed-dev tahmed-dev changed the title [cfggen] Add Batch Mode [cfggen] Extend Template Argument to Support Batch Mode Aug 12, 2020
@tahmed-dev tahmed-dev merged commit f9edf6e into sonic-net:master Aug 12, 2020
@abdosi
Copy link
Contributor

abdosi commented Aug 15, 2020

@tahmed-dev Please create PR for 201911. Cherry-pick has conflict.

@tahmed-dev
Copy link
Contributor Author

@tahmed-dev Please create PR for 201911. Cherry-pick has conflict.

@abdosi PR:5200 raised for the cherry-pick

tahmed-dev added a commit to tahmed-dev/sonic-buildimage that referenced this pull request Oct 8, 2020
Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
tahmed-dev added a commit that referenced this pull request Oct 9, 2020
Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
lguohan pushed a commit that referenced this pull request Oct 30, 2020
Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
abdosi pushed a commit that referenced this pull request Dec 4, 2020
Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
abdosi pushed a commit that referenced this pull request Dec 22, 2020
Calls to cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-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 cfggen take considerable time. With batch mode, we will have the ability
to reduce number of calls from services.

Example of the batch mode command:
sonic-cfggen -t template-1.j2 -t template-2.j2,config-db -t template-3.j2,config-db -t template-4.j2,file1 -t template-5.j2,file2 --write-to-db.

template-1.j2 will be rendered to stdout since it is missing the dest part. stdout is default
config-db is a special keyword that will inject the rendered template into internal data structure. The internal data structure gets written to redis-db with --write-to-db switch. In the case the user would like to write to a file named config-db, it could be given as /config-db or ./config-db

signed-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.

5 participants