[LLDP] Add lldpmgrd Daemon to Manage LLDP Configuration by jleveque · Pull Request #1428 · sonic-net/sonic-buildimage

jleveque · 2018-02-28T00:42:27Z

New lldpmgrd solution replaces previous reconfigure.sh script, which had two known issues:
- reconfigure.sh would occasionally crash due to call to sonic-cfggen crashing with the exception jinja2.exceptions.UndefinedError: 'PORT' is undefined if PORT table wasn't available at the time
- reconfigure.sh would wait for all interfaces to exist, reconfigure all interfaces, then wait for all interfaces to go away before repeating the process. However, this only worked if ALL interfaces were destoyed and then recreated. If any number of interfaces less than the total number of interfaces were destroyed and recreated, their LLDP configuration would never be updated.
New lldpmgrd solution subscribes to notifications in the PORT table of the Redis State database and applies LLDP configuration on an interface-by-interface basis when interfaces are created.
Also clean up and refactor Dockerfile Jinja2 template
One minor issue with the lldpmgrd implementation is that sel.select() blocks and is not interrupted by signals, therefore calling docker stop lldp will not return until it times out after 10 seconds.

pavel-shirshov

As commented

pavel-shirshov · 2018-02-28T01:10:55Z

rules/docker-lldp-sv2.mk

Do you need LIBSWSSCOMMON here?

it uses python-binding of libswsscommon for getting table notification from redis db.

pavel-shirshov · 2018-02-28T01:12:31Z

dockers/docker-lldp-sv2/lldpmgrd

What about removing of an interface. Do you catch such case?

No. We decided this was unnecessary.

pavel-shirshov · 2018-02-28T01:13:28Z

dockers/docker-lldp-sv2/lldpmgrd

probably it's better to get exit code from the command too.

I'm already checking the return code from the command on line 145 (if proc.returncode != 0:)

pavel-shirshov · 2018-02-28T01:14:54Z

dockers/docker-lldp-sv2/lldpmgrd

I think 'if not status' would be better here

pavel-shirshov · 2018-02-28T01:16:11Z

dockers/docker-lldp-sv2/lldpmgrd

'if not status'

pavel-shirshov · 2018-02-28T01:17:59Z

therefore calling docker stop lldp will not return until it times out after 10 seconds.
After 10 seconds docker just kill -9 everything in the container.

jleveque · 2018-02-28T01:37:43Z

@pavel-shirshov: I understand that Docker sends a SIGKILL after 10 seconds, I'm just stating that with the current implementation of this daemon, the LLDP docker will always take the full 10 seconds to stop.

pavel-shirshov · 2018-02-28T20:55:34Z

dockers/docker-lldp-sv2/lldpmgrd

Are you sure you always have "state" in fvp_dict?
Probably better

"state" in fvp_dict and fvp_dict["state"] == "ok"

?

It should always be there, but it can't hurt to check. Will update shortly.

lguohan · 2018-03-01T08:00:03Z

dockers/docker-lldp-sv2/lldpmgrd

to be complete, we should also listen to CFG_DEVICE_NEIGHBOR_TABLE_NAME change, since if user configure this table, we may need to update the message we send out. I understand this is not an issue now, but we should add an comment to mark as TODO.

I do not think we should return here, in case there is no description, we should send "ldpcli configure ports {0} lldp portidsubtype local {1}". this will ensure we send out the correct port name instead of mac address.

lguohan · 2018-03-01T08:01:04Z

dockers/docker-lldp-sv2/lldpmgrd

to be complete, we should also listen to PORT_TABLE_NAME change, same reason as above.

lguohan · 2018-03-01T08:01:43Z

dockers/docker-lldp-sv2/lldpmgrd

In case there is no alias, then we should use port name to send out.

lguohan

need to handle no alias case, need to handle no neighbor description case.

…as dependency

- Remove reconfigure.sh

…, omit description

lguohan · 2018-03-02T05:25:18Z

dockers/docker-lldp-sv2/lldpmgrd

+        of the Redis State database. When we are notified of the creation of an
+        interface, update LLDP configuration accordingly.
+        """
+        # Subscribe to PORT table notifications in the State DB


as I noted in my previous comments, we should also listen CFG_PORT_TABLE and CFG_NEIGBHRO_TABLE in case port alias change or neighbor info change, then we can send updated information to the neighbor. We do not need to do this now, but we should mark it as TODO in the comment.

I added a TODO in the file header comment (lines 12-13). I think you missed it.

lguohan · 2018-03-02T05:25:56Z

as I noted in my previous comments, we should also listen CFG_PORT_TABLE and CFG_NEIGBHRO_TABLE in case port alias change or neighbor info change, then we can send updated information to the neighbor. We do not need to do this now, but we should mark it as TODO in the comment.

Commits include: * src/sonic-utilities c7e46c9...42cab68 (3): > [consutil] Look for udevprefix.conf file under platform dir, not plugins (#1431) > [ci]: download from sonic-buildimage.vs artifact (#1428) > [storyteller] sort output by time and improve lag support (#1430)

sonic-net#6768 change the kvm artifact name from kvm to vs Signed-off-by: Guohan Lu <[email protected]>

Update SAI submodule v1.9 with the following fixes 7594e53 (HEAD, origin/v1.9) Skip brcm teardown assertion (sonic-net#1423) (sonic-net#1428) 0c33f4a [FIX]Fix the circular reference issue when build sai header py (sonic-net#1427) 7e0fc24 Add support for building under Doxygen 1.9.1 (sonic-net#1414) (sonic-net#1424) 8ecf3ef [Fix]Correct enum check on branch 1.9 (sonic-net#1418) e2b2f39 Add Thrift 0.14.1 compatibility (sonic-net#1403) (sonic-net#1416)

jleveque added the Enhancement ➕ label Feb 28, 2018

jleveque self-assigned this Feb 28, 2018

jleveque requested review from lguohan, pavel-shirshov and stcheng February 28, 2018 00:42

pavel-shirshov suggested changes Feb 28, 2018

View reviewed changes

pavel-shirshov reviewed Feb 28, 2018

View reviewed changes

pavel-shirshov approved these changes Feb 28, 2018

View reviewed changes

lguohan reviewed Mar 1, 2018

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated

Copy link

Collaborator

lguohan Mar 1, 2018

Choose a reason for hiding this comment

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

to be complete, we should also listen to PORT_TABLE_NAME change, same reason as above.

lguohan reviewed Mar 1, 2018

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated

Copy link

Collaborator

lguohan Mar 1, 2018

Choose a reason for hiding this comment

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

In case there is no alias, then we should use port name to send out.

lguohan requested changes Mar 1, 2018

View reviewed changes

Joe LeVeque added 7 commits March 1, 2018 23:34

Refactor Dockerfile.j2; Add dependencies to makefile; Add swsscommon …

5dfe6e8

…as dependency

[LLDP] Add lldpmgrd which manages LLDP configuration

0211ac2

- Remove reconfigure.sh

Address review comments

2ca0e3c

Check for SET operation; also make code more readable

eb06a41

Ensure state element exists in fvp_dict before accessing

e817b0a

Check return value of sel.select()

7edeacb

If no port alias, use port name; if no neighbor host name or portname…

1df26db

…, omit description

lguohan reviewed Mar 2, 2018

View reviewed changes

lguohan approved these changes Mar 2, 2018

View reviewed changes

jleveque merged commit c689253 into sonic-net:master Mar 3, 2018

jleveque deleted the lldpmgrd branch March 3, 2018 00:46

jleveque mentioned this pull request Mar 3, 2018

LLDP configurations are not applied to all ports #1208

Closed

jleveque mentioned this pull request Feb 13, 2021

[sonic-utilities] Update submodule #6777

Merged

stepanblyschak pushed a commit to stepanblyschak/sonic-buildimage that referenced this pull request May 10, 2021

[ci]: download from sonic-buildimage.vs artifact (sonic-net#1428)

18cd8ba

sonic-net#6768 change the kvm artifact name from kvm to vs Signed-off-by: Guohan Lu <[email protected]>

Conversation

jleveque commented Feb 28, 2018

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov commented Feb 28, 2018

Uh oh!

jleveque commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleveque Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleveque Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jleveque Feb 28, 2018 •

edited

Loading

jleveque Mar 2, 2018 •

edited

Loading