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

[sonic-cfggen] Fix sonic-cfggen crash issue when printing non-exists table/key #4131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhenggen-xu
Copy link
Collaborator

@zhenggen-xu zhenggen-xu commented Feb 8, 2020

[sonic-cfggen] Fix sonic-cfggen crash issue when printing non-exists table/key

Fix sonic-cfggen so docker-frr start.sh won't fail if no WARM_RESTART in configDB

In general, instead of sonic-cfggen crashing
we print empty string if we use -v or --var-json for variables not existing

Signed-off-by: Zhenggen Xu <zxu@linkedin.com>

This should be back-ported to 201911 and 201811 branches too.

- What I did
If the configDB has no warm-restart configurations, the start.sh in bgp docker would fail due to python exception as below:

Traceback (most recent call last):
  File "/usr/local/bin/sonic-cfggen", line 322, in <module>
    main()
  File "/usr/local/bin/sonic-cfggen", line 300, in main
    print(template.render(data))
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 430, in getattr
    return getattr(obj, attribute)
jinja2.exceptions.UndefinedError: 'WARM_RESTART' is undefined

- How I did it
Check the WARM_RESTART table first. If does not exist, won't check the bgp_eoiu info to avoid crash.

- How to verify it
After the fix:

without any warm restart configured or if we do "sudo config warm-restart bgp_eoiu false"
bgp docker will not start the bgp_eoiu_marker

if we do:  "sudo config warm-restart bgp_eoiu true"
We can see bgp_eoiu_marker started.

Additional tests:

Before fix:

admin@sonic:~$ sonic-cfggen -d -v abcd.ghij
Traceback (most recent call last):
  File "/usr/local/bin/sonic-cfggen", line 318, in <module>
    main()
  File "/usr/local/bin/sonic-cfggen", line 296, in main
    print(template.render(data))
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 430, in getattr
    return getattr(obj, attribute)
jinja2.exceptions.UndefinedError: 'abcd' is undefined
admin@sonic:~$ sonic-cfggen -d -v abcd.efgh
Traceback (most recent call last):
  File "/usr/local/bin/sonic-cfggen", line 318, in <module>
    main()
  File "/usr/local/bin/sonic-cfggen", line 296, in main
    print(template.render(data))
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "<template>", line 1, in top-level template code
  File "/usr/local/lib/python2.7/dist-packages/jinja2/environment.py", line 430, in getattr
    return getattr(obj, attribute)
jinja2.exceptions.UndefinedError: 'abcd' is undefined

admin@sonic:~$ sonic-cfggen -d --var-json abcd
Traceback (most recent call last):
  File "/usr/local/bin/sonic-cfggen", line 318, in <module>
    main()
  File "/usr/local/bin/sonic-cfggen", line 302, in main
    print(json.dumps(FormatConverter.to_serialized(data[args.var_json]), indent=4, cls=minigraph_encoder))
KeyError: 'abcd'

After fix:

admin@sonic:~$ sonic-cfggen -d -v abcd.efgh

admin@sonic:~$ sonic-cfggen -d --var-json abcd

- Description for the changelog

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

@zhenggen-xu zhenggen-xu changed the title [docker-frr] Fix the start.sh where it fails in case no WARM_RESTART in configDB [sonic-cfggen/docker-frr] Fix sonic-cfggen so docker-frr start.sh won't fail if no WARM_RESTART in configDB Feb 13, 2020
pavel-shirshov
pavel-shirshov previously approved these changes Feb 14, 2020
lguohan
lguohan previously approved these changes Apr 25, 2020
@lguohan
Copy link
Collaborator

lguohan commented Apr 25, 2020

@zhenggen-xu , it looks like your proposed change breaks the test. I do not whether we should change the test or your patch. any opinion?

@zhenggen-xu
Copy link
Collaborator Author

zhenggen-xu commented Apr 26, 2020

@lguohan it looks like the upstream merge broke some changes:
if args.var_json != None and args.var_json in data:

changed to:
if args.var_json != None

We need revert that line and see.

…'t fail if no WARM_RESTART in configDB

In general, instead of sonic-cfggen crashing
we print empty string if we use -v or --var-json for variables not existing

Signed-off-by: Zhenggen Xu <zxu@linkedin.com>
@zhenggen-xu
Copy link
Collaborator Author

Just reverted the merge issue and let's see the results.

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'd suggest to use is not None instead of != None

…table/key

Address review feedback

Signed-off-by: Zhenggen Xu <zxu@linkedin.com>
@zhenggen-xu zhenggen-xu changed the title [sonic-cfggen/docker-frr] Fix sonic-cfggen so docker-frr start.sh won't fail if no WARM_RESTART in configDB [sonic-cfggen] Fix sonic-cfggen crash issue when printing non-exists table/key Aug 14, 2020
@zhenggen-xu
Copy link
Collaborator Author

I'd suggest to use is not None instead of != None

In general, I might want to leave that to different PR, as the fix here was to handle exceptions not fixing existing style issue. I see many places in the file using "!= None", we probably should have some PR to address them all together. Anyway, I changed it so to get the PR moving forward.

@pavel-shirshov

@pavel-shirshov
Copy link
Contributor

@zhenggen-xu

Your code catches all exceptions. How can we find an issue if we have one? I'd suggest you to make except more specific, also as output something on the error, like "variable wasn't found" or "error on template rendering"? Also I'd suggest to return some exit code on the issue.

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.

3 participants