From 0f418083ce0cae9649573df7cf6f00253e7e67cd Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Tue, 6 Mar 2018 23:31:50 +0000 Subject: [PATCH 1/4] [lldmgrd] Fix potential race condition with interface bring-up --- dockers/docker-lldp-sv2/lldpmgrd | 62 +++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/dockers/docker-lldp-sv2/lldpmgrd b/dockers/docker-lldp-sv2/lldpmgrd index dce64c10c2a3..1ea40a25d952 100755 --- a/dockers/docker-lldp-sv2/lldpmgrd +++ b/dockers/docker-lldp-sv2/lldpmgrd @@ -75,25 +75,29 @@ class LldpManager(object): Attributes: state_db: Handle to Redis State database via swsscommon lib config_db: Handle to Redis Config database via swsscommon lib + pending_cmds: Dictionary where key is port name, value is pending + LLDP configuration command to run """ REDIS_HOSTNAME = "localhost" REDIS_PORT = 6379 - REDIS_TIMEOUT_USECS = 0 + 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_USECS) + 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_USECS) + self.REDIS_TIMEOUT_MS) - def update_lldp_config_for_port(self, port_name): + self.pending_cmds = {} + + def generate_pending_lldp_config_cmd_for_port(self, port_name): """ For port `port_name`, look up the neighboring device's hostname and corresponding port alias in the Config database, then form the @@ -142,14 +146,34 @@ class LldpManager(object): else: log_info("Unable to retrieve neighbor information for port '{}'. Not adding port description.".format(port_name)) - log_info("Running command: '{}'".format(lldpcli_cmd)) + # Add the command to our dictionary of pending commands, overwriting any + # previous pending command for this port + self.pending_cmds[port_name] = lldpcli_cmd + + def run_pending_cmds(self): + # List of port names (keys of elements) to delete from self.pending_cmds + to_delete = [] + + for (port_name, cmd) in self.pending_cmds.iteritems(): + log_info("Running command: '{}'".format(cmd)) + + proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - proc = subprocess.Popen(lldpcli_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + (stdout, stderr) = proc.communicate() - (stdout, stderr) = proc.communicate() + # If the command succeeds, add the port name to our to_delete list. + # We will delete this command from self.pending_cmds below. + # If the command fails, log a message, but don't delete the command + # from self.pending_cmds, so that the command will be retried the + # next time this method is called. + if proc.returncode == 0: + to_delete.append(port_name) + else: + log_error("Error running command '{}': {}".format(cmd, stderr)) - if proc.returncode != 0: - log_error("Error running command '{}': {}".format(cmd, stderr)) + # Delete all successful commands from self.pending_cmds + for port_name in to_delete: + self.pending_cmds.pop(port_name, None) def run(self): """ @@ -157,6 +181,8 @@ class LldpManager(object): of the Redis State database. When we are notified of the creation of an interface, update LLDP configuration accordingly. """ + SELECT_TIMEOUT_MS = 1000 + # Subscribe to PORT table notifications in the State DB sel = swsscommon.Select() sst = swsscommon.SubscriberStateTable(self.state_db, swsscommon.STATE_PORT_TABLE_NAME) @@ -164,16 +190,18 @@ class LldpManager(object): # Listen indefinitely for changes to the PORT table in the State DB while True: - (state, c, fd) = sel.select() - if state != swsscommon.Select.OBJECT: - log_warning("sel.select() did not return swsscommon.Select.OBJECT") - continue + (state, c, fd) = sel.select(SELECT_TIMEOUT_MS) + + if state == swsscommon.Select.OBJECT: + (key, op, fvp) = sst.pop() + + fvp_dict = dict(fvp) - (key, op, fvp) = sst.pop() - fvp_dict = dict(fvp) + if op == "SET" and fvp_dict.get("state") == "ok": + self.generate_pending_lldp_config_cmd_for_port(key) - if op == "SET" and fvp_dict.get("state") == "ok": - self.update_lldp_config_for_port(key) + # Run all pending commands + self.run_pending_cmds() # ============================= Functions ============================= From 7b63e7c8f656408bce7bab1183cad41cac03dd9d Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Wed, 7 Mar 2018 01:01:29 +0000 Subject: [PATCH 2/4] Remove lldpd submodule, now clone source repo, add patch that causes lldpctl/lldpcli to return an error code if port doesn't exist --- .gitignore | 3 ++ .gitmodules | 3 -- rules/lldpd.mk | 13 +++++-- src/lldpd | 1 - src/lldpd/Makefile | 32 +++++++++++++++ ...eturn-error-when-port-does-not-exist.patch | 39 +++++++++++++++++++ src/lldpd/patch/series | 2 + 7 files changed, 85 insertions(+), 8 deletions(-) delete mode 160000 src/lldpd create mode 100644 src/lldpd/Makefile create mode 100644 src/lldpd/patch/0001-return-error-when-port-does-not-exist.patch create mode 100644 src/lldpd/patch/series diff --git a/.gitignore b/.gitignore index 98ef49099546..b75083ce4d57 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,9 @@ src/libnl3/* !src/libnl3/Makefile src/libteam/* !src/libteam/Makefile +src/lldpd/* +!src/lldpd/Makefile +!src/lldpd/patch/ src/mpdecimal/* !src/mpdecimal/Makefile src/python3/* diff --git a/.gitmodules b/.gitmodules index 5517948e5f0e..e0cfc0869d05 100755 --- a/.gitmodules +++ b/.gitmodules @@ -26,9 +26,6 @@ [submodule "src/sonic-py-swsssdk"] path = src/sonic-py-swsssdk url = https://github.com/Azure/sonic-py-swsssdk.git -[submodule "src/lldpd"] - path = src/lldpd - url = https://github.com/vincentbernat/lldpd.git [submodule "src/sonic-snmpagent"] path = src/sonic-snmpagent url = https://github.com/Azure/sonic-snmpagent diff --git a/rules/lldpd.mk b/rules/lldpd.mk index 65cfb61b3eee..10f3a200fc94 100644 --- a/rules/lldpd.mk +++ b/rules/lldpd.mk @@ -1,12 +1,17 @@ # lldpd package -LLDPD_VERSION = 0.9.5-0 +LLDPD_VERSION = 0.9.5 -LLDPD = lldpd_$(LLDPD_VERSION)_amd64.deb +LLDPD = lldpd_$(LLDPD_VERSION)-0_amd64.deb $(LLDPD)_DEPENDS += $(LIBSNMP_DEV) $(LLDPD)_RDEPENDS += $(LIBSNMP) $(LLDPD)_SRC_PATH = $(SRC_PATH)/lldpd -SONIC_DPKG_DEBS += $(LLDPD) +SONIC_MAKE_DEBS += $(LLDPD) -LIBLLDPCTL = liblldpctl-dev_$(LLDPD_VERSION)_amd64.deb +LIBLLDPCTL = liblldpctl-dev_$(LLDPD_VERSION)-0_amd64.deb $(eval $(call add_derived_package,$(LLDPD),$(LIBLLDPCTL))) + +# Export these variables so they can be used in a sub-make +export LLDPD_VERSION +export LLDPD +export LIBLLDPCTL diff --git a/src/lldpd b/src/lldpd deleted file mode 160000 index 396961a038a3..000000000000 --- a/src/lldpd +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 396961a038a38675d46f96eaa7b430b2a1f8701b diff --git a/src/lldpd/Makefile b/src/lldpd/Makefile new file mode 100644 index 000000000000..c34062505d4e --- /dev/null +++ b/src/lldpd/Makefile @@ -0,0 +1,32 @@ +.ONESHELL: +SHELL = /bin/bash +.SHELLFLAGS += -e + +MAIN_TARGET = $(LLDPD) +DERIVED_TARGETS = $(LIBLLDPCTL) + +$(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% : + # Remove any stale files + rm -rf ./lldpd + + # Clone lldpd repo + git clone https://github.com/vincentbernat/lldpd.git + pushd ./lldpd + + # Reset HEAD to the commit of the proper tag + # NOTE: Using "git checkout " here detaches our HEAD, + # which stg doesn't like, so we use this method instead + git reset --hard $(LLDPD_VERSION) + + # Apply patches + stg init + stg import -s ../patch/series + + # Build source and Debian packages + dpkg-buildpackage -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS) + popd + + # Move the newly-built .deb packages to the destination directory + mv $* $(DERIVED_TARGETS) $(DEST)/ + +$(addprefix $(DEST)/, $(DERIVED_TARGETS)): $(DEST)/% : $(DEST)/$(MAIN_TARGET) diff --git a/src/lldpd/patch/0001-return-error-when-port-does-not-exist.patch b/src/lldpd/patch/0001-return-error-when-port-does-not-exist.patch new file mode 100644 index 000000000000..e17d09a386d4 --- /dev/null +++ b/src/lldpd/patch/0001-return-error-when-port-does-not-exist.patch @@ -0,0 +1,39 @@ +From 15e692fb82a61203624829cdd872315211920077 Mon Sep 17 00:00:00 2001 +From: Guohan Lu +Date: Tue, 6 Mar 2018 09:36:51 +0000 +Subject: [PATCH] return error when port does not exist + +--- + src/client/conf-lldp.c | 8 ++++++++ + 1 file changed, 8 insertions(+) + +diff --git a/src/client/conf-lldp.c b/src/client/conf-lldp.c +index c16219b..b51e4eb 100644 +--- a/src/client/conf-lldp.c ++++ b/src/client/conf-lldp.c +@@ -148,6 +148,8 @@ cmd_portid_type_local(struct lldpctl_conn_t *conn, struct writer *w, + const char *name; + const char *id = cmdenv_get(env, "port-id"); + const char *descr = cmdenv_get(env, "port-descr"); ++ const char *portname = cmdenv_get(env, "ports"); ++ int find_port = 0; + + log_debug("lldpctl", "lldp PortID TLV Subtype Local port-id '%s' port-descr '%s'", id, descr); + +@@ -165,6 +167,12 @@ cmd_portid_type_local(struct lldpctl_conn_t *conn, struct writer *w, + log_warnx("lldpctl", "unable to set LLDP Port Description for %s." + " %s", name, lldpctl_last_strerror(conn)); + } ++ find_port = 1; ++ } ++ ++ if (!find_port) { ++ log_warnx("lldpctl", "cannot find port %s", portname); ++ return 0; + } + + return 1; +-- +2.7.4 + + diff --git a/src/lldpd/patch/series b/src/lldpd/patch/series new file mode 100644 index 000000000000..8aa5ab8d8f3f --- /dev/null +++ b/src/lldpd/patch/series @@ -0,0 +1,2 @@ +# This series applies on GIT commit 396961a038a38675d46f96eaa7b430b2a1f8701b +0001-return-error-when-port-does-not-exist.patch From a5c3b6224bbc9c95731ce4573733d82d2ec72715 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Wed, 7 Mar 2018 19:38:37 +0000 Subject: [PATCH 3/4] Increase select() timeout to 10 seconds; Add log_debug() function; Modify some log message levels --- dockers/docker-lldp-sv2/lldpmgrd | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/dockers/docker-lldp-sv2/lldpmgrd b/dockers/docker-lldp-sv2/lldpmgrd index 1ea40a25d952..b9c9c93a1cec 100755 --- a/dockers/docker-lldp-sv2/lldpmgrd +++ b/dockers/docker-lldp-sv2/lldpmgrd @@ -31,6 +31,12 @@ SYSLOG_IDENTIFIER = "lldpmgrd" # ========================== Syslog wrappers ========================== +def log_debug(msg): + syslog.openlog(SYSLOG_IDENTIFIER) + syslog.syslog(syslog.LOG_DEBUG, msg) + syslog.closelog() + + def log_info(msg): syslog.openlog(SYSLOG_IDENTIFIER) syslog.syslog(syslog.LOG_INFO, msg) @@ -155,7 +161,7 @@ class LldpManager(object): to_delete = [] for (port_name, cmd) in self.pending_cmds.iteritems(): - log_info("Running command: '{}'".format(cmd)) + log_debug("Running command: '{}'".format(cmd)) proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) @@ -169,7 +175,7 @@ class LldpManager(object): if proc.returncode == 0: to_delete.append(port_name) else: - log_error("Error running command '{}': {}".format(cmd, stderr)) + log_warning("Command failed '{}': {}".format(cmd, stderr)) # Delete all successful commands from self.pending_cmds for port_name in to_delete: @@ -181,7 +187,8 @@ class LldpManager(object): of the Redis State database. When we are notified of the creation of an interface, update LLDP configuration accordingly. """ - SELECT_TIMEOUT_MS = 1000 + # Set select timeout to 10 seconds + SELECT_TIMEOUT_MS = 1000 * 10 # Subscribe to PORT table notifications in the State DB sel = swsscommon.Select() From 78962759501e50489b9334e138b62a41324f9489 Mon Sep 17 00:00:00 2001 From: Joe LeVeque Date: Wed, 7 Mar 2018 19:56:38 +0000 Subject: [PATCH 4/4] Rename: run_pending_cmds() -> process_pending_cmds() --- dockers/docker-lldp-sv2/lldpmgrd | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dockers/docker-lldp-sv2/lldpmgrd b/dockers/docker-lldp-sv2/lldpmgrd index b9c9c93a1cec..1d13b1aaafd4 100755 --- a/dockers/docker-lldp-sv2/lldpmgrd +++ b/dockers/docker-lldp-sv2/lldpmgrd @@ -156,7 +156,7 @@ class LldpManager(object): # previous pending command for this port self.pending_cmds[port_name] = lldpcli_cmd - def run_pending_cmds(self): + def process_pending_cmds(self): # List of port names (keys of elements) to delete from self.pending_cmds to_delete = [] @@ -207,8 +207,8 @@ class LldpManager(object): if op == "SET" and fvp_dict.get("state") == "ok": self.generate_pending_lldp_config_cmd_for_port(key) - # Run all pending commands - self.run_pending_cmds() + # Process all pending commands + self.process_pending_cmds() # ============================= Functions =============================