Routing-stack warm-reboot feature. (sonic-net#602)

* Routing-stack warm-reboot feature. Please refer to the corresponding design document for more details: sonic-net/SONiC#239 The following manual UT plan has been successfully executed. UT automation pending. Physical topology formed by various BGP peers connecting to the DUT. Both non-ecmp and ecmp prefixes are learned/tested. Testcase Suite 1: Making use of “pkill -9 bgpd && pkill -9 zebra” as trigger. ============= IPv4 prefixes ========== * Restart zebra/bgpd: - Verify that all prefixes are properly stale-marked and that no change is pushed to AppDB during reconciliation. - Result: PASSED * Restart zebra/bgpd and add one new non-ecmp IPv4 prefix. - To produce a route-state-inconsistency, add prefix in adjacent node before bgp sessions are re-established. - Verify that new-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and withdraw one non-ecmp IPv4 prefix. - To produce a route-state-inconsistency, remove prefix in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and add one new path to an IPv4 ecmp-prefix. - To produce a route-state-inconsistency, add prefix-path in adjacent node before bgp sessions are re-established. - Verify that new prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and withdraw one ecmp-path from an existing ecmp IPv4 prefix. - To produce a route-state-inconsistency, remove prefix-path in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Eventually, during reconciliation, this path will be eliminated as it’s not being refreshed by remote-peer. - Result: PASSED IPv6 prefixes ========== * Restart zebra/bgpd: - Verify that all prefixes are properly stale-marked and that no change is pushed to AppDB during reconciliation. - Result: PASSED * Restart zebra/bgpd and add one new non-ecmp IPv6 prefix. - To produce a route-state-inconsistency, add prefix in adjacent node before bgp sessions are re-established. - Verify that new-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and withdraw one non-ecmp IPv6 prefix. - To produce a route-state-inconsistency, remove prefix in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and add one new path to an IPv6 ecmp-prefix. - To produce a route-state-inconsistency, add prefix-path in adjacent node before bgp sessions are re-established. - Verify that new prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart zebra/bgpd and withdraw one ecmp-path from an existing ecmp IPv6 prefix. - To produce a route-state-inconsistency, remove prefix-path in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Eventually, during reconciliation, this path will be eliminated as it’s not being refreshed by remote-peer. - Result: PASSED Testcase Suite 2: Making use of sudo service bgp restart” as trigger. ============= IPv4 prefixes ========== * Restart bgp service/docker: - Verify that all prefixes are properly stale-marked and that no change is pushed to AppDB during reconciliation. - Result: PASSED * Restart bgp service/docker and add one new non-ecmp IPv4 prefix. - To produce a route-state-inconsistency, add prefix in adjacent node before bgp sessions are re-established. - Verify that new-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and withdraw one non-ecmp IPv4 prefix. - To produce a route-state-inconsistency, remove prefix in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and add one new path to an IPv4 ecmp-prefix. - To produce a route-state-inconsistency, add prefix-path in adjacent node before bgp sessions are re-established. - Verify that new prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and withdraw one ecmp-path from an existing ecmp IPv4 prefix. - To produce a route-state-inconsistency, remove prefix-path in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Eventually, during reconciliation, this path will be eliminated as it’s not being refreshed by remote-peer. - Result: PASSED IPv6 prefixes ========== * Restart bgp service/docker: - Verify that all prefixes are properly stale-marked and that no change is pushed to AppDB during reconciliation. - Result: PASSED * Restart bgp service/docker and add one new non-ecmp IPv6 prefix. - To produce a route-state-inconsistency, add prefix in adjacent node before bgp sessions are re-established. - Verify that new-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and withdraw one non-ecmp IPv6 prefix. - To produce a route-state-inconsistency, remove prefix in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and add one new path to an IPv6 ecmp-prefix. - To produce a route-state-inconsistency, add prefix-path in adjacent node before bgp sessions are re-established. - Verify that new prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Result: PASSED * Restart bgp service/docker and withdraw one ecmp-path from an existing ecmp IPv6 prefix. - To produce a route-state-inconsistency, remove prefix-path in adjacent node before bgp sessions are re-established. - Verify that deleted-prefix-path msg is NOT pushed down to AppDB till reconciliation takes place. - Eventually, during reconciliation, this path will be eliminated as it’s not being refreshed by remote-peer. - Result: PASSED RB= G=lnos-reviewers R=pchaudhary,pmao,rmolina,samaity,sfardeen,zxu A= Signed-off-by: Rodny Molina <rmolina@linkedin.com> * Renaming 'restoration' code and making minor adjustments to fpmsyncd Signed-off-by: Rodny Molina <rmolina@linkedin.com> * Eliminating 'state' associated to field-value-tuples Code has been refactored to reduce the complexity associated to carrying 'state' for every received FV-tuple. Obviously, there's no free-lunch here, this is only possible at the expense of more memory utilization: we are now using two different buffers to store 'old' and 'new' state. Yet, this is a relatively-small price to pay for a much simpler implementation. Signed-off-by: Rodny Molina <rmolina@linkedin.com> * Adding UTs to cover routing-warm-reboot logic. Signed-off-by: Rodny Molina <rmolina@linkedin.com> * Fixing an issue with warm-reboot UTs that prevented an existing test-case from passing As part of these changes i'm also modifying the ip-addresses of my UT setup to avoid clashes with existing/previous testcases. Signed-off-by: Rodny Molina <rmolina@linkedin.com> * Making some small adjustments * Addressing review comments for my UT code * Addressing more review comments. * Refactoring UTs to rely on pubsub notifications instead of logs
lguohan · Nov 10, 2018 · f380685 · f380685
1 parent 9fbcb60
commit f380685
Show file tree

Hide file tree

Showing 8 changed files with 1,290 additions and 24 deletions.
diff --git a/fpmsyncd/Makefile.am b/fpmsyncd/Makefile.am
@@ -1,4 +1,4 @@
-INCLUDES = -I $(top_srcdir) -I $(FPM_PATH)
+INCLUDES = -I $(top_srcdir) -I $(top_srcdir)/warmrestart -I $(FPM_PATH)
 
 bin_PROGRAMS = fpmsyncd
 
@@ -8,9 +8,8 @@ else
 DBGFLAGS = -g
 endif
 
-fpmsyncd_SOURCES = fpmsyncd.cpp fpmlink.cpp routesync.cpp
+fpmsyncd_SOURCES = fpmsyncd.cpp fpmlink.cpp routesync.cpp $(top_srcdir)/warmrestart/warmRestartHelper.cpp $(top_srcdir)/warmrestart/warmRestartHelper.h
 
 fpmsyncd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON)
 fpmsyncd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON)
 fpmsyncd_LDADD = -lnl-3 -lnl-route-3 -lswsscommon
-
diff --git a/fpmsyncd/fpmsyncd.cpp b/fpmsyncd/fpmsyncd.cpp
@@ -1,13 +1,24 @@
 #include <iostream>
 #include "logger.h"
 #include "select.h"
+#include "selectabletimer.h"
 #include "netdispatcher.h"
+#include "warmRestartHelper.h"
 #include "fpmsyncd/fpmlink.h"
 #include "fpmsyncd/routesync.h"
 
+
 using namespace std;
 using namespace swss;
 
+
+/*
+ * Default warm-restart timer interval for routing-stack app. To be used only if
+ * no explicit value has been defined in configuration.
+ */
+const uint32_t DEFAULT_ROUTING_RESTART_INTERVAL = 120;
+
+
 int main(int argc, char **argv)
 {
  swss::Logger::linkToDbNative("fpmsyncd");
@@ -18,25 +29,75 @@ int main(int argc, char **argv)
  NetDispatcher::getInstance().registerMessageHandler(RTM_NEWROUTE, &sync);
  NetDispatcher::getInstance().registerMessageHandler(RTM_DELROUTE, &sync);
 
- while (1)
+ while (true)
  {
  try
  {
  FpmLink fpm;
  Select s;
+ SelectableTimer warmStartTimer(timespec{0, 0});
 
- cout << "Waiting for connection..." << endl;
+ /*
+ * Pipeline should be flushed right away to deal with state pending
+ * from previous try/catch iterations.
+ */
+ pipeline.flush();
+
+ cout << "Waiting for fpm-client connection..." << endl;
  fpm.accept();
  cout << "Connected!" << endl;
 
  s.addSelectable(&fpm);
+
+ /* If warm-restart feature is enabled, execute 'restoration' logic */
+ bool warmStartEnabled = sync.m_warmStartHelper.checkAndStart();
+ if (warmStartEnabled)
+ {
+ /* Obtain warm-restart timer defined for routing application */
+ uint32_t warmRestartIval = sync.m_warmStartHelper.getRestartTimer();
+ if (!warmRestartIval)
+ {
+ warmStartTimer.setInterval(timespec{DEFAULT_ROUTING_RESTART_INTERVAL, 0});
+ }
+ else
+ {
+ warmStartTimer.setInterval(timespec{warmRestartIval, 0});
+ }
+
+ /* Execute restoration instruction and kick off warm-restart timer */
+ if (sync.m_warmStartHelper.runRestoration())
+ {
+ warmStartTimer.start();
+ s.addSelectable(&warmStartTimer);
+ }
+ }
+
  while (true)
  {
  Selectable *temps;
- /* Reading FPM messages forever (and calling "readData" to read them) */
+
+ /* Reading FPM messages forever (and calling "readMe" to read them) */
  s.select(&temps);
- pipeline.flush();
- SWSS_LOG_DEBUG("Pipeline flushed");
+
+ /*
+ * Upon expiration of the warm-restart timer, proceed to run the
+ * reconciliation process and remove warm-restart timer from
+ * select() loop.
+ */
+ if (warmStartEnabled && temps == &warmStartTimer)
+ {
+ SWSS_LOG_NOTICE("Warm-Restart timer expired.");
+ sync.m_warmStartHelper.reconcile();
+ s.removeSelectable(&warmStartTimer);
+
+ pipeline.flush();
+ SWSS_LOG_DEBUG("Pipeline flushed");
+ }
+ else if (!warmStartEnabled || sync.m_warmStartHelper.isReconciled())
+ {
+ pipeline.flush();
+ SWSS_LOG_DEBUG("Pipeline flushed");
+ }
  }
  }
  catch (FpmLink::FpmConnectionClosedException &e)

diff --git a/fpmsyncd/routesync.cpp b/fpmsyncd/routesync.cpp
@@ -14,7 +14,8 @@ using namespace std;
 using namespace swss;
 
 RouteSync::RouteSync(RedisPipeline *pipeline) :
- m_routeTable(pipeline, APP_ROUTE_TABLE_NAME, true)
+ m_routeTable(pipeline, APP_ROUTE_TABLE_NAME, true),
+ m_warmStartHelper(pipeline, &m_routeTable, APP_ROUTE_TABLE_NAME, "bgp", "bgp")
 {
  m_nl_sock = nl_socket_alloc();
  nl_connect(m_nl_sock, NETLINK_ROUTE);
@@ -38,10 +39,31 @@ void RouteSync::onMsg(int nlmsg_type, struct nl_object *obj)
  return;
  }
 
+ /*
+ * Upon arrival of a delete msg we could either push the change right away,
+ * or we could opt to defer it if we are going through a warm-reboot cycle.
+ */
+ bool warmRestartInProgress = m_warmStartHelper.inProgress();
+
  if (nlmsg_type == RTM_DELROUTE)
  {
- m_routeTable.del(destipprefix);
- return;
+ if (!warmRestartInProgress)
+ {
+ m_routeTable.del(destipprefix);
+ return;
+ }
+ else
+ {
+ SWSS_LOG_INFO("Warm-Restart mode: Receiving delete msg: %s\n",
+ destipprefix);
+
+ vector<FieldValueTuple> fvVector;
+ const KeyOpFieldsValuesTuple kfv = std::make_tuple(destipprefix,
+ DEL_COMMAND,
+ fvVector);
+ m_warmStartHelper.insertRefreshMap(kfv);
+ return;
+ }
  }
  else if (nlmsg_type != RTM_NEWROUTE)
  {
@@ -118,8 +140,29 @@ void RouteSync::onMsg(int nlmsg_type, struct nl_object *obj)
  vector<FieldValueTuple> fvVector;
  FieldValueTuple nh("nexthop", nexthops);
  FieldValueTuple idx("ifname", ifnames);
+
  fvVector.push_back(nh);
  fvVector.push_back(idx);
- m_routeTable.set(destipprefix, fvVector);
- SWSS_LOG_DEBUG("RoutTable set: %s %s %s\n", destipprefix, nexthops.c_str(), ifnames.c_str());
+
+ if (!warmRestartInProgress)
+ {
+ m_routeTable.set(destipprefix, fvVector);
+ SWSS_LOG_DEBUG("RouteTable set msg: %s %s %s\n",
+ destipprefix, nexthops.c_str(), ifnames.c_str());
+ }
+
+ /*
+ * During routing-stack restarting scenarios route-updates will be temporarily
+ * put on hold by warm-reboot logic.
+ */
+ else
+ {
+ SWSS_LOG_INFO("Warm-Restart mode: RouteTable set msg: %s %s %s\n",
+ destipprefix, nexthops.c_str(), ifnames.c_str());
+
+ const KeyOpFieldsValuesTuple kfv = std::make_tuple(destipprefix,
+ SET_COMMAND,
+ fvVector);
+ m_warmStartHelper.insertRefreshMap(kfv);
+ }
 }
diff --git a/fpmsyncd/routesync.h b/fpmsyncd/routesync.h
@@ -4,6 +4,8 @@
 #include "dbconnector.h"
 #include "producerstatetable.h"
 #include "netmsg.h"
+#include "warmRestartHelper.h"
+
 
 namespace swss {
 
@@ -16,10 +18,12 @@ class RouteSync : public NetMsg
 
  virtual void onMsg(int nlmsg_type, struct nl_object *obj);
 
+ WarmStartHelper m_warmStartHelper;
+
 private:
- ProducerStateTable m_routeTable;
- struct nl_cache *m_link_cache;
- struct nl_sock *m_nl_sock;
+ ProducerStateTable  m_routeTable;
+ struct nl_cache  *m_link_cache;
+ struct nl_sock  *m_nl_sock;
 };
 
 }

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -290,6 +290,26 @@ def stop_swss(self):
  cmd += "supervisorctl stop {}; ".format(pname)
  self.runcmd(['sh', '-c', cmd])
 
+ def start_zebra(dvs):
+ dvs.runcmd(['sh', '-c', 'supervisorctl start zebra'])
+
+ # Let's give zebra a chance to connect to FPM.
+ time.sleep(5)
+
+ def stop_zebra(dvs):
+ dvs.runcmd(['sh', '-c', 'pkill -x zebra'])
+ time.sleep(1)
+
+ def start_fpmsyncd(dvs):
+ dvs.runcmd(['sh', '-c', 'supervisorctl start fpmsyncd'])
+
+ # Let's give fpmsyncd a chance to connect to Zebra.
+ time.sleep(5)
+
+ def stop_fpmsyncd(dvs):
+ dvs.runcmd(['sh', '-c', 'pkill -x fpmsyncd'])
+ time.sleep(1)
+
  def init_asicdb_validator(self):
  self.asicdb = AsicDbValidator(self)
 
@@ -334,13 +354,18 @@ def get_logs(self, modname=None):
  raise RuntimeError("Failed to unpack the archive.")
  os.system("chmod a+r -R log")
 
- def add_log_marker(self):
+ def add_log_marker(self, file=None):
  marker = "=== start marker {} ===".format(datetime.now().isoformat())
- self.ctn.exec_run("logger {}".format(marker))
+
+ if file:
+ self.runcmd(['sh', '-c', "echo \"{}\" >> {}".format(marker, file)])
+ else:
+ self.ctn.exec_run("logger {}".format(marker))
+
  return marker
 
  def SubscribeAppDbObject(self, objpfx):
- r = redis.Redis(unix_socket_path=self.redis_sock, db=swsscommon.APP_DB)
+ r = redis.Redis(unix_socket_path=self.redis_sock, db=swsscommon.APPL_DB)
  pubsub = r.pubsub()
  pubsub.psubscribe("__keyspace@0__:%s*" % objpfx)
  return pubsub
@@ -375,26 +400,40 @@ def CountSubscribedObjects(self, pubsub, ignore=None, timeout=10):
  return (nadd, ndel)
 
  def GetSubscribedAppDbObjects(self, pubsub, ignore=None, timeout=10):
- r = redis.Redis(unix_socket_path=self.redis_sock, db=swsscommon.APP_DB)
+ r = redis.Redis(unix_socket_path=self.redis_sock, db=swsscommon.APPL_DB)
 
  addobjs = []
  delobjs = []
  idle = 0
+ prev_key = None
 
  while True and idle < timeout:
  message = pubsub.get_message()
  if message:
  print message
  key = message['channel'].split(':', 1)[1]
+ # In producer/consumer_state_table scenarios, every entry will
+ # show up twice for every push/pop operation, so skip the second
+ # one to avoid double counting.
+ if key != None and key == prev_key:
+ continue
+ # Skip instructions with meaningless keys. To be extended in the
+ # future to other undesired keys.
+ if key == "ROUTE_TABLE_KEY_SET" or key == "ROUTE_TABLE_DEL_SET":
+ continue
  if ignore:
  fds = message['channel'].split(':')
  if fds[2] in ignore:
  continue
+
  if message['data'] == 'hset':
+ (_, k) = key.split(':', 1)
  value=r.hgetall(key)
- addobjs.append({'key':k, 'vals':value})
+ addobjs.append({'key':json.dumps(k), 'vals':json.dumps(value)})
+ prev_key = key
  elif message['data'] == 'del':
- delobjs.append(key)
+ (_, k) = key.split(':', 1)
+ delobjs.append({'key':json.dumps(k)})
  idle = 0
  else:
  time.sleep(1)
@@ -424,7 +463,8 @@ def GetSubscribedAsicDbObjects(self, pubsub, ignore=None, timeout=10):
  (_, t, k) = key.split(':', 2)
  addobjs.append({'type':t, 'key':k, 'vals':value})
  elif message['data'] == 'del':
- delobjs.append(key)
+ (_, t, k) = key.split(':', 2)
+ delobjs.append({'key':k})
  idle = 0
  else:
  time.sleep(1)