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.

Sign up for GitHub

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

Jump to bottom

Fix for LLDP portname exposed as MAC address bug #3152

Merged
merged 8 commits into from
Jul 17, 2019
Merged

Fix for LLDP portname exposed as MAC address bug #3152

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2e6ad45
Subscribe to both ConfigDB and AppDB
Jul 12, 2019
cd8d06c
the operstate file is not consistent
Jul 12, 2019
b76b9de
Addressing review comments
Jul 15, 2019
67ec30a
Remove check for PortInitDone and PortConfigDone
Jul 15, 2019
70b3686
Remove checking State DB for port creation
Jul 16, 2019
5ebac55
Check for key to be present before fetching it
Jul 16, 2019
21b17e0
Check if key is present before getting it
Jul 16, 2019
9170466
Addressing review comments
Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions dockers/docker-lldp-sv2/lldpmgrd
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,13 @@ class LldpManager(object):
port_table_dict = dict(fvp)

# Get the oper-status for the port
port_oper_status = port_table_dict.get("oper_status")
log_info("Port name {} oper status: {}".format(port_name, port_oper_status))
return port_oper_status == "up"

if port_table_dict.has_key("oper_status"):
port_oper_status = port_table_dict.get("oper_status")
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's don't output any error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return False
else:
log_error("Port '{}' not found in {} table in App DB".format(port_name, swsscommon.APP_PORT_TABLE_NAME))
return False
Expand Down Expand Up @@ -217,11 +220,12 @@ class LldpManager(object):
fvp_dict = dict(fvp)

# handle config change
if op in ["SET", "DEL"] and (fvp_dict.get("alias") or fvp_dict.get("description")) :
if self.is_port_up(key):
self.generate_pending_lldp_config_cmd_for_port(key)
else:
self.pending_cmds.pop(key, None)
if (fvp_dict.has_key("alias") or fvp_dict.has_key("description")):
if op in ["SET", "DEL"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we combine two ifs into one?

if self.is_port_up(key):
self.generate_pending_lldp_config_cmd_for_port(key)
jleveque marked this conversation as resolved.
Show resolved Hide resolved
else:
self.pending_cmds.pop(key, None)

(key, op, fvp) = sst_appdb.pop()
if (key != "PortInitDone") and (key != "PortConfigDone"):
Expand Down