[lldpmgrd] Fix potential race condition when interfaces are created (s…

…onic-net#1469)

.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/*

.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

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)
@@ -75,25 +81,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)
+
+        self.pending_cmds = {}
 
-    def update_lldp_config_for_port(self, port_name):
+    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,38 +152,63 @@ 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 process_pending_cmds(self):
+        # List of port names (keys of elements) to delete from self.pending_cmds
+        to_delete = []
 
-        proc = subprocess.Popen(lldpcli_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        for (port_name, cmd) in self.pending_cmds.iteritems():
+            log_debug("Running command: '{}'".format(cmd))
 
-        (stdout, stderr) = proc.communicate()
+            proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
-        if proc.returncode != 0:
-            log_error("Error running command '{}': {}".format(cmd, stderr))
+            (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_warning("Command failed '{}': {}".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):
         """
         Infinite loop. Subscribes to notifications of changes in the PORT table
         of the Redis State database. When we are notified of the creation of an
         interface, 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)
 
         # 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)
+            # Process all pending commands
+            self.process_pending_cmds()
 
 
 # ============================= Functions =============================

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

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 <tag_name>" 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)

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 <[email protected]>
+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
+
+

src/lldpd/patch/series

@@ -0,0 +1,2 @@
+# This series applies on GIT commit 396961a038a38675d46f96eaa7b430b2a1f8701b
+0001-return-error-when-port-does-not-exist.patch