Fix for LLDP portname exposed as MAC address bug by sumukhatv · Pull Request #3152 · sonic-net/sonic-buildimage

sumukhatv · 2019-07-12T23:28:01Z

- What I did
Fixed the LLDP portname bug
- How I did it
By subscribing to notifications from APP DB and Config DB to apply the lldp port config command
- How to verify it
We should not see portname as MAC address in the lldp messages
- Description for the changelog

Subscribe to Config DB and Application DB to get notifications to apply the LLDP port config correctly

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

to get notifications to apply LLDP port config

Removing this since it is not serving any purpose

jleveque

As comments. Also, please add a few more comments in the code to briefly explain the new logic.

dockers/docker-lldp-sv2/lldpmgrd

This is not prteset in Config DB

lguohan · 2019-07-16T15:37:45Z

retest mellanox please

lguohan · 2019-07-16T15:37:54Z

retest vs please

dockers/docker-lldp-sv2/lldpmgrd

pavel-shirshov · 2019-07-16T20:37:19Z

dockers/docker-lldp-sv2/lldpmgrd

+                log_info("Port name {} oper status: {}".format(port_name, port_oper_status))
+                return port_oper_status == "up"
+            else:
+                log_error("Port name {} does not have key oper_status".format(port_name))


Let's don't output any error here?

For multiple cases we return False. Outputting an error message is the only way to find out the reason for False.

In reply to: 304107933 [](ancestors = 304107933)

That key should always be present. I think this log message is helpful for catching unexpected circumstances.

The key will not be presented when we creating this table.
So on sonic initialization you can catch some "errors" here.
I think it's not an error at all.

Good point. We might see some false alarms at boot time and when reloading config. Maybe it is better to simply remove it.

pavel-shirshov · 2019-07-16T21:37:54Z

dockers/docker-lldp-sv2/lldpmgrd

+
+                    # handle config change
+                    if (fvp_dict.has_key("alias") or fvp_dict.has_key("description")):
+                        if op in ["SET", "DEL"]:


can we combine two ifs into one?

dockers/docker-lldp-sv2/lldpmgrd

The permissions got changed inadverently in this PR: #3152

* Subscribe to both ConfigDB and AppDB to get notifications to apply LLDP port config * the operstate file is not consistent Removing this since it is not serving any purpose * Remove check for PortInitDone and PortConfigDone This is not prteset in Config DB * Remove checking State DB for port creation * Check for key to be present before fetching it * Addressing review comments

The permissions got changed inadverently in this PR: #3152

…lly (#19273) #### Why I did it src/sonic-swss ``` * 70eb766 - (HEAD -> master, origin/master, origin/HEAD) ADD VOQ COUNTERS(SAI_SWITCH_STAT_PACKET_INTEGRITY_DROP, SAI_QUEUE_ST…T_CREDIT_WD_DELETED_PACKETS) support for VOQ/Fabric switches (#3152) (10 hours ago) [saksarav-nokia] ``` #### How I did it #### How to verify it #### Description for the changelog

…lly (#19554) #### Why I did it src/sonic-swss ``` * d3073b7c - (HEAD -> 202405, origin/202405) [muxorch] Fixing bug with updateRoute and mux neighbors (#3187) (19 hours ago) [Nikola Dancejic] * b16d6b2a - ADD VOQ COUNTERS(SAI_SWITCH_STAT_PACKET_INTEGRITY_DROP, SAI_QUEUE_ST…T_CREDIT_WD_DELETED_PACKETS) support for VOQ/Fabric switches (#3152) (19 hours ago) [saksarav-nokia] * 12a95e57 - Revamp module build script to make it work for 5.15 on Ubuntu 20.04 (#3212) (19 hours ago) [Saikrishna Arcot] * 87cf38e0 - Fix in switchorch: unsupported attribute causes skipping of processing the rest of configurations (#3209) (19 hours ago) [Amir] * 8f333b6 - [subnet decap] Support decap rule generation based on T0 VIP route (#3183) (5 weeks ago) [Longxiang Lyu] * 9bcb9b6 - Fixing appl_db FABRIC_MONITOR notification issue. (#3176) (5 weeks ago) [jfeng-arista] * fff544e - Rotate record file before writing new log. (#3158) (5 weeks ago) [mint570] * 80f5207 - Add SWSS support for link event damping feature (#2933) (5 weeks ago) [Roy Yi] * b3ebfc4 - [muxorch] Using bulker to program routes/neighbors during switchover (#3148) (5 weeks ago) [Nikola Dancejic] ``` #### How I did it #### How to verify it #### Description for the changelog

Sumukha Tumkur Vani added 2 commits July 12, 2019 16:15

Subscribe to both ConfigDB and AppDB

2e6ad45

to get notifications to apply LLDP port config

the operstate file is not consistent

cd8d06c

Removing this since it is not serving any purpose

sumukhatv requested review from jleveque, lguohan and pavel-shirshov July 12, 2019 23:28

jleveque suggested changes Jul 12, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

pavel-shirshov suggested changes Jul 13, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

dockers/docker-lldp-sv2/lldpmgrd Show resolved Hide resolved

jleveque added the Bug 🐛 label Jul 13, 2019

Addressing review comments

b76b9de

jleveque suggested changes Jul 15, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

Remove check for PortInitDone and PortConfigDone

67ec30a

This is not prteset in Config DB

Remove checking State DB for port creation

70b3686

jleveque approved these changes Jul 16, 2019

View reviewed changes

pavel-shirshov reviewed Jul 16, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

pavel-shirshov reviewed Jul 16, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

pavel-shirshov reviewed Jul 16, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Outdated Show resolved Hide resolved

Check for key to be present before fetching it

5ebac55

sumukhatv requested review from jleveque and pavel-shirshov July 16, 2019 20:16

pavel-shirshov reviewed Jul 16, 2019

View reviewed changes

Check if key is present before getting it

21b17e0

jleveque approved these changes Jul 16, 2019

View reviewed changes

pavel-shirshov reviewed Jul 16, 2019

View reviewed changes

dockers/docker-lldp-sv2/lldpmgrd Show resolved Hide resolved

Addressing review comments

9170466

pavel-shirshov approved these changes Jul 17, 2019

View reviewed changes

sumukhatv merged commit ac9934a into sonic-net:master Jul 17, 2019

sumukhatv deleted the lldp_portname branch July 17, 2019 23:04

pavel-shirshov added Request for 201811 Branch Request for 201904 Branch 🏂 labels Jul 17, 2019

sumukhatv pushed a commit that referenced this pull request Jul 18, 2019

Reverting permission changes on lldpmgrd (#3184)

6e37e23

The permissions got changed inadverently in this PR: #3152

yxieca added the Included in 201811 Branch label Jul 19, 2019

yxieca pushed a commit that referenced this pull request Jul 19, 2019

Reverting permission changes on lldpmgrd (#3184)

3375fb2

The permissions got changed inadverently in this PR: #3152

Conversation

sumukhatv commented Jul 12, 2019

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lguohan commented Jul 16, 2019

Uh oh!

lguohan commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

sumukhatv Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

jleveque Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

jleveque Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants