-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[vstest] Virtual chssis config rendering from default_config.json #8008
Conversation
Fixed issue introduced by PR#7857. The problem was that introduction of using init_cfg.json.j2 template did not have instruction in init_config.json.j2 to merge the virtual chassis default_config.json. Ths fix is to have a separate invokiation of sonic-cfggen to combine init_config.json and virtual chassis default_config.json Signed-off-by: vedganes <[email protected]>
sonic-cfggen -t /usr/share/sonic/templates/init_cfg.json.j2 -a "{\"system_mac\": \"$SYSTEM_MAC_ADDRESS\"}" > /etc/sonic/init_cfg.json | ||
|
||
if [[ -f /usr/share/sonic/virtual_chassis/default_config.json ]]; then | ||
sonic-cfggen -j /etc/sonic/init_cfg.json -j /usr/share/sonic/virtual_chassis/default_config.json --print-data > /tmp/init_cfg.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dont we write directly to /etc/sonic/init_cfg.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template /usr/share/sonic/templates/init_cfg.json.j2 does not have j2 code to take the /usr/share/sonic/virtual_chassis/default_config.json contents as data and generate the result with the default_config,json contents in /etc/sonic/init_cfg.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but do you need to write to /tmp/init_cfg.json or can you update init_cfg.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is possible to update the init_cfg.json directly since sonic-cfggen loads all the contents first as data and then produces the output. But for here, I just followed the model used for generating config_db.json (given just below these lines from 43-45) to appear consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vganesan-nokia Does this mean that "-t" & "-j" does not work together in sonic-cfggen?
-j JSON, --json JSON json file that contains additional variables
-t TEMPLATE, --template TEMPLATE
render the data with the template file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When -t option is used the template file, the contents of the json file given with option -j are loaded as data to the template. If the template does not have explicit j2 code to take these data, the rendering will not have the json files contents in the output.
So, it IS possible to make -t and -j options work together if the template has appropriate j2 instructions to take the contents of the json file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for details, now I understand.
@stepanblyschak , can you please review? |
@prsunny, would you please merge this PR so that we can merge the dependent PR sonic-net/sonic-swss#1811? Once both of these PRs are merged, we can add back the skipped virtual chassis tests. |
@prsunny, @anshuv-mfst, would you please make arrangements to get this PR merged? Merging this will enable merging sonic-net/sonic-swss#1811 and adding back the skipped virtual chassis tests in test_virtual_chassis.py. I am also preparing a UT update PR as requested in PR sonic-net/sonic-swss#1823, which requires enabling back the skipped tests in test_viratual_chassis.py. |
This reverts commit facdef5. With the changes in platfrom/vs/docker-sonic-vs/start.sh done via sonic-buildimage PR sonic-net/sonic-buildimage#8008 all tests will pass. No need to skip the tests. So reverting this commit
This reverts commit facdef5. With the changes in platfrom/vs/docker-sonic-vs/start.sh done via sonic-buildimage PR sonic-net/sonic-buildimage#8008 all tests will pass. No need to skip the tests. So reverting this commit
This reverts commit facdef5. With the changes in platfrom/vs/docker-sonic-vs/start.sh done via sonic-buildimage PR sonic-net/sonic-buildimage#8008 all tests will pass. No need to skip the tests. So reverting this commit
Why I did it
To fix VS virtual_chassis tests failure issue sonic-net/sonic-swss#1809
How I did it
There was problem in rendering virtual chassis default_config.json into config_db.json. This problem was introduced by changes in PR #7857. The problem was that introduction of using init_cfg.json.j2 template did not have instruction in init_config.json.j2 to merge the virtual chassis default_config.json. Ths fix is to have a separate invokiation of sonic-cfggen to combine init_config.json and virtual chassis default_config.json
Note: This PR depends on sonic-swss PR sonic-net/sonic-swss#1811
How to verify it
VS test for test_virtual_chassis.py passes all tests consistently.