[lldpmgrd] Fix potential race condition when interfaces are created (#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)
 
-    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 +152,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
 
-        proc = subprocess.Popen(lldpcli_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    def process_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_debug("Running command: '{}'".format(cmd))
+
+            proc = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
             (stdout, stderr) = proc.communicate()
 
-        if proc.returncode != 0:
+            # If the command succeeds, add the port name to our to_delete list.
-            log_error("Error running command '{}': {}".format(cmd, stderr))
+            # 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):
         """
@@ -157,6 +187,9 @@ class LldpManager(object):
         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)
@@ -164,16 +197,18 @@ class LldpManager(object):
 
         # Listen indefinitely for changes to the PORT table in the State DB
         while True:
-            (state, c, fd) = sel.select()
+            (state, c, fd) = sel.select(SELECT_TIMEOUT_MS)
-            if state != swsscommon.Select.OBJECT:
-                log_warning("sel.select() did not return swsscommon.Select.OBJECT")
-                continue
 
+            if state == swsscommon.Select.OBJECT:
                 (key, op, fvp) = sst.pop()
+
                 fvp_dict = dict(fvp)
 
                 if op == "SET" and fvp_dict.get("state") == "ok":
-                self.update_lldp_config_for_port(key)
+                    self.generate_pending_lldp_config_cmd_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

@@ -1 +0,0 @@
-Subproject commit 396961a038a38675d46f96eaa7b430b2a1f8701b

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 <gulv@microsoft.com>
+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