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

Fix issue#14 lldp-syncd throws exception from get_sys_capability_list #22

Merged
merged 2 commits into from
May 10, 2019

Conversation

chenkelly
Copy link
Contributor

What I did
To fix issue #14 and add test cases to parse sys_capability_list for remote devices.

Why I did it
Current get_sys_capability_list don't consider the system name of remote device is null case.
If devices don't configure host name, it will send out LLDPPDU whose System Name TLV is null string.
When sonic device learned the remote device, the chassis object of lldp_json does not include system name string.
The capability_list does not get successfully by applying current statement, capability_list = if_attributes['chassis'].values()[0]['capability'].
We can execute lldpctl command to see the difference.
root@sonic:~# docker exec -it lldp bash
root@sonic:/# /usr/sbin/lldpctl -f json
[Remote device without system name]

    "chassis": {
      ...
      "capability": [
        {
          "type": "Bridge",
          "enabled": true
        },
        {
          "type": "Router",
          "enabled": true
        }
      ]
    }

[Remote device with system name]

    "chassis": {
      "system_name": {
        ......
        "capability": [
          {
            "type": "Bridge",
            "enabled": true
          },
          {
            "type": "Router",
            "enabled": true
          }
        ]
      }
    }

How I verified it
Add test case.

sonic-dbsyncd$ pytest -v
================================================= test session starts ==================================================
platform linux2 -- Python 2.7.15rc1, pytest-3.3.2, py-1.5.2, pluggy-0.6.0 -- /usr/bin/python2
cachedir: .cache
rootdir: /mnt/d/lldp_syncd/sonic-dbsyncd, inifile:
collected 9 items

tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_loc_chassis PASSED                                        [ 11%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_multiple_mgmt_ip PASSED                                   [ 22%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_json PASSED                                         [ 33%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_short PASSED                                        [ 44%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_parse_short_short PASSED                                  [ 55%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_remote_sys_capability_list PASSED                         [ 66%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_single_mgmt_ip PASSED                                     [ 77%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_sync_roundtrip PASSED                                     [ 88%]
tests/test_lldpSyncDaemon.py::TestLldpSyncDaemon::test_timeparse PASSED                                          [100%]

Signed-off-by: kelly_chen [email protected]

…sys_capability_list

The get_sys_capability_list() shall consider the system name of remote device is null case.
@chenkelly
Copy link
Contributor Author

Hi @qiluo-msft, could you help to review this modification? Thanks very much.

@qiluo-msft
Copy link
Contributor

@nikos-github Could you help review?

Copy link
Contributor

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

Thanks for the fix and detailed explanation!

@qiluo-msft qiluo-msft merged commit 282ed87 into sonic-net:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants