From 26a8a1204e873109537c81462ad1457cf38c2f9e Mon Sep 17 00:00:00 2001 From: gechiang <62408185+gechiang@users.noreply.github.com> Date: Mon, 29 Nov 2021 10:39:42 -0800 Subject: [PATCH] Prevent other notification event storms to keep enqueue unchecked and drained all memory that leads to crashing the switch router (#968) --- syncd/NotificationQueue.cpp | 50 +++++++++++++++-- syncd/NotificationQueue.h | 17 +++++- unittest/syncd/Makefile.am | 3 +- unittest/syncd/TestNotificationQueue.cpp | 68 ++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 unittest/syncd/TestNotificationQueue.cpp diff --git a/syncd/NotificationQueue.cpp b/syncd/NotificationQueue.cpp index 8c25c25c55ae..c684142424a9 100644 --- a/syncd/NotificationQueue.cpp +++ b/syncd/NotificationQueue.cpp @@ -8,9 +8,13 @@ using namespace syncd; #define MUTEX std::lock_guard _lock(m_mutex); NotificationQueue::NotificationQueue( - _In_ size_t queueLimit): + _In_ size_t queueLimit, + _In_ size_t consecutiveThresholdLimit): m_queueSizeLimit(queueLimit), - m_dropCount(0) + m_thresholdLimit(consecutiveThresholdLimit), + m_dropCount(0), + m_lastEventCount(0), + m_lastEvent(SAI_SWITCH_NOTIFICATION_NAME_FDB_EVENT) { SWSS_LOG_ENTER(); @@ -30,16 +34,52 @@ bool NotificationQueue::enqueue( MUTEX; SWSS_LOG_ENTER(); + bool candidateToDrop = false; + std::string currentEvent; /* * If the queue exceeds the limit, then drop all further FDB events This is * a temporary solution to handle high memory usage by syncd and the * notification queue keeps growing. The permanent solution would be to * make this stateful so that only the *latest* event is published. + * + * We have also seen other notification storms that can also cause this queue issue + * So the new scheme is to keep the last notification event and its consecutive count + * If threshold limit reached and the consecutive count also reached then this notification + * will also be dropped regardless of its event type to protect the device from crashing due to + * running out of memory */ auto queueSize = m_queue.size(); + currentEvent = kfvKey(item); + if (currentEvent == m_lastEvent) + { + m_lastEventCount++; + } + else + { + m_lastEventCount = 1; + m_lastEvent = currentEvent; + } + if (queueSize >= m_queueSizeLimit) + { + /* Too many queued up already check if notification fits condition to e dropped + * 1. All FDB events should be dropped at this point. + * 2. All other notification events will start to drop if it reached the consecutive threshold limit + */ + if (currentEvent == SAI_SWITCH_NOTIFICATION_NAME_FDB_EVENT) + { + candidateToDrop = true; + } + else + { + if (m_lastEventCount >= m_thresholdLimit) + { + candidateToDrop = true; + } + } + } - if (queueSize < m_queueSizeLimit || kfvKey(item) != SAI_SWITCH_NOTIFICATION_NAME_FDB_EVENT) // TODO use enum instead of strings + if (!candidateToDrop) { m_queue.push(item); @@ -51,9 +91,9 @@ bool NotificationQueue::enqueue( if (!(m_dropCount % NOTIFICATION_QUEUE_DROP_COUNT_INDICATOR)) { SWSS_LOG_NOTICE( - "Too many messages in queue (%zu), dropped %zu FDB events!", + "Too many messages in queue (%zu), dropped (%zu), lastEventCount (%zu) Dropping %s !", queueSize, - m_dropCount); + m_dropCount, m_lastEventCount, m_lastEvent.c_str()); } return false; diff --git a/syncd/NotificationQueue.h b/syncd/NotificationQueue.h index bf0b2364d4f2..12459ff6afa3 100644 --- a/syncd/NotificationQueue.h +++ b/syncd/NotificationQueue.h @@ -16,9 +16,17 @@ extern "C" { * Value based on typical L2 deployment with 256k MAC entries and * some extra buffer for other events like port-state, q-deadlock etc * + * Note: We recently found a case where SAI continuously sending switch notification events + * that also caused the queue to keep growing and eventually exhaust all system memory and crashed. + * So a new detection/limit scheme is being implemented by keeping track of the last Event + * and if the current Event matches the last Event, then the last Event Count will get + * incremented and this count will also be used as part of the equation to ensure this + * notification should also be dropped if the queue size limit has already reached and not + * just dropping FDB events only. * TODO: move to config, also this limit only applies to fdb notifications */ #define DEFAULT_NOTIFICATION_QUEUE_SIZE_LIMIT (300000) +#define DEFAULT_NOTIFICATION_CONSECUTIVE_THRESHOLD (1000) namespace syncd { @@ -27,7 +35,8 @@ namespace syncd public: NotificationQueue( - _In_ size_t limit = DEFAULT_NOTIFICATION_QUEUE_SIZE_LIMIT); + _In_ size_t limit = DEFAULT_NOTIFICATION_QUEUE_SIZE_LIMIT, + _In_ size_t consecutiveThresholdLimit = DEFAULT_NOTIFICATION_CONSECUTIVE_THRESHOLD); virtual ~NotificationQueue(); @@ -49,6 +58,12 @@ namespace syncd size_t m_queueSizeLimit; + size_t m_thresholdLimit; + size_t m_dropCount; + + size_t m_lastEventCount; + + std::string m_lastEvent; }; } diff --git a/unittest/syncd/Makefile.am b/unittest/syncd/Makefile.am index e4f2f2661b14..5148f70a9401 100644 --- a/unittest/syncd/Makefile.am +++ b/unittest/syncd/Makefile.am @@ -9,7 +9,8 @@ tests_SOURCES = main.cpp \ MockableSaiInterface.cpp \ MockHelper.cpp \ TestCommandLineOptions.cpp \ - TestFlexCounter.cpp + TestFlexCounter.cpp \ + TestNotificationQueue.cpp tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON) tests_LDADD = $(LDADD_GTEST) $(top_srcdir)/syncd/libSyncd.a -lhiredis -lswsscommon -lpthread -L$(top_srcdir)/lib/.libs -lsairedis -L$(top_srcdir)/meta/.libs -lsaimetadata -lsaimeta -lzmq $(CODE_COVERAGE_LIBS) diff --git a/unittest/syncd/TestNotificationQueue.cpp b/unittest/syncd/TestNotificationQueue.cpp new file mode 100644 index 000000000000..3485208577a3 --- /dev/null +++ b/unittest/syncd/TestNotificationQueue.cpp @@ -0,0 +1,68 @@ +#include "NotificationQueue.h" + +#include "sairediscommon.h" + +#include + +using namespace syncd; + +static std::string fdbData = +"[{\"fdb_entry\":\"{\\\"bvid\\\":\\\"oid:0x260000000005be\\\",\\\"mac\\\":\\\"52:54:00:86:DD:7A\\\",\\\"switch_id\\\":\\\"oid:0x21000000000000\\\"}\"," +"\"fdb_event\":\"SAI_FDB_EVENT_LEARNED\"," +"\"list\":[{\"id\":\"SAI_FDB_ENTRY_ATTR_TYPE\",\"value\":\"SAI_FDB_ENTRY_TYPE_DYNAMIC\"},{\"id\":\"SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID\",\"value\":\"oid:0x3a000000000660\"}]}]"; + +static std::string sscData = "{\"status\":\"SAI_SWITCH_OPER_STATUS_UP\",\"switch_id\":\"oid:0x2100000000\"}"; + +TEST(NotificationQueue, EnqueueLimitTest) +{ + bool status; + int i; + std::vector fdbEntry; + std::vector sscEntry; + + // Set up a queue with limit at 5 and threshold at 3 where after this is reached event starts dropping + syncd::NotificationQueue testQ(5, 3); + + // Try queue up 5 fake FDB event and expect them to be added successfully + swss::KeyOpFieldsValuesTuple fdbItem(SAI_SWITCH_NOTIFICATION_NAME_FDB_EVENT, fdbData, fdbEntry); + for (i = 0; i < 5; ++i) + { + status = testQ.enqueue(fdbItem); + EXPECT_EQ(status, true); + } + + // On the 6th fake FDB event expect it to be dropped right away + status = testQ.enqueue(fdbItem); + EXPECT_EQ(status, false); + + // Add 2 switch state change events expect both are accepted as consecutive limit not yet reached + swss::KeyOpFieldsValuesTuple sscItem(SAI_SWITCH_NOTIFICATION_NAME_SWITCH_STATE_CHANGE, sscData, sscEntry); + for (i = 0; i < 2; ++i) + { + status = testQ.enqueue(sscItem); + EXPECT_EQ(status, true); + } + + // On the 3rd consecutive switch state change event expect it to be dropped + status = testQ.enqueue(sscItem); + EXPECT_EQ(status, false); + + // Add a fake FDB event to cause the consecutive event signature to change while this FDB event is dropped + status = testQ.enqueue(fdbItem); + EXPECT_EQ(status, false); + + // Add 2 switch state change events expect both are accepted as consecutive limit not yet reached + for (i = 0; i < 2; ++i) + { + status = testQ.enqueue(sscItem); + EXPECT_EQ(status, true); + } + + // Add 2 switch state change events expect both are dropped as consecutive limit has already reached + for (i = 0; i < 2; ++i) + { + status = testQ.enqueue(sscItem); + EXPECT_EQ(status, false); + } +} +