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 7 commits
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
106 changes: 64 additions & 42 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 @@ -71,18 +71,6 @@ def signal_handler(sig, frame):
else:
log_warning("Caught unhandled signal '" + sig + "'")

# ========================== Helpers ==================================

def is_port_exist(port_name):
filename = "/sys/class/net/%s/operstate" % port_name
if os.path.exists(filename):
with open(filename) as fp:
state = fp.read()
return "up" in state
else:
filename = "/sys/class/net/%s/ifindex" % port_name
return os.path.exists(filename)

# ============================== Classes ==============================

class LldpManager(object):
Expand All @@ -101,20 +89,44 @@ class LldpManager(object):
REDIS_TIMEOUT_MS = 0

def __init__(self):
# Open a handle to the State database
self.state_db = swsscommon.DBConnector(swsscommon.STATE_DB,
self.REDIS_HOSTNAME,
self.REDIS_PORT,
self.REDIS_TIMEOUT_MS)

# Open a handle to the Config database
self.config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB,
self.REDIS_HOSTNAME,
self.REDIS_PORT,
self.REDIS_TIMEOUT_MS)

# Open a handle to the Application database
self.appl_db = swsscommon.DBConnector(swsscommon.APPL_DB,
self.REDIS_HOSTNAME,
self.REDIS_PORT,
self.REDIS_TIMEOUT_MS)

self.pending_cmds = {}

def is_port_up(self, port_name):
"""
Determine if a port is up or down by looking into the oper-status for the port in
PORT TABLE in the Application DB
"""
# Retrieve all entires for this port from the Port table
port_table = swsscommon.Table(self.appl_db, swsscommon.APP_PORT_TABLE_NAME)
(status, fvp) = port_table.get(port_name)
if status:
# Convert list of tuples to a dictionary
port_table_dict = dict(fvp)

# Get the oper-status for the port
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

def generate_pending_lldp_config_cmd_for_port(self, port_name):
"""
For port `port_name`, look up the description and alias in the Config database,
Expand Down Expand Up @@ -157,11 +169,6 @@ class LldpManager(object):
to_delete = []

for (port_name, cmd) in self.pending_cmds.iteritems():
if not is_port_exist(port_name):
# it doesn't make any sense to configure lldpd if the target port does not exist
# let's postpone the command for the next iteration
continue

log_debug("Running command: '{}'".format(cmd))

proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Expand All @@ -185,37 +192,52 @@ class LldpManager(object):
def run(self):
"""
Subscribes to notifications of changes in the PORT table
of the Redis State database.
Subscribe to STATE_DB - get notified of the creation of an interface
of the Redis Config/Application database.
Subscribe to APP_DB - get port oper status
Subscribe to CONFIG_DB - get notified of port config changes
Update LLDP configuration accordingly.
"""
# Set select timeout to 10 seconds
SELECT_TIMEOUT_MS = 1000 * 10

# Subscribe to PORT table notifications in the State DB
sel = swsscommon.Select()
sst = swsscommon.SubscriberStateTable(self.state_db, swsscommon.STATE_PORT_TABLE_NAME)
sel.addSelectable(sst)
sst = swsscommon.SubscriberStateTable(self.config_db, swsscommon.CFG_PORT_TABLE_NAME)
sel.addSelectable(sst)

# Listen for changes to the PORT table in the STATE_DB and CONFIG_DB
# Subscribe to PORT table notifications in the Config DB
sst_confdb = swsscommon.SubscriberStateTable(self.config_db, swsscommon.CFG_PORT_TABLE_NAME)
sel.addSelectable(sst_confdb)

# Subscribe to PORT table notifications in the App DB
sst_appdb = swsscommon.SubscriberStateTable(self.appl_db, swsscommon.APP_PORT_TABLE_NAME)
sel.addSelectable(sst_appdb)

# Listen for changes to the PORT table in the CONFIG_DB and APP_DB
while True:
(state, c) = sel.select(SELECT_TIMEOUT_MS)
pavel-shirshov marked this conversation as resolved.
Show resolved Hide resolved

if state == swsscommon.Select.OBJECT:
(key, op, fvp) = sst.pop()

fvp_dict = dict(fvp)

# handle creation
if op == "SET" and fvp_dict.get("state") == "ok":
self.generate_pending_lldp_config_cmd_for_port(key)

# handle config change
if op in ["SET", "DEL"] and (fvp_dict.get("alias") or fvp_dict.get("description")) :
self.generate_pending_lldp_config_cmd_for_port(key)
(key, op, fvp) = sst_confdb.pop()
if fvp:
fvp_dict = dict(fvp)

# handle config change
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"):
if fvp:
fvp_dict = dict(fvp)

# handle port status change
if fvp_dict.has_key("oper_status"):
if "up" in fvp_dict.get("oper_status"):
pavel-shirshov marked this conversation as resolved.
Show resolved Hide resolved
self.generate_pending_lldp_config_cmd_for_port(key)
else:
self.pending_cmds.pop(key, None)

# Process all pending commands
self.process_pending_cmds()
Expand Down