From 66a942ee6db8c4aafcf25b2d91a6155577c2b564 Mon Sep 17 00:00:00 2001 From: PINS Working Group Date: Tue, 5 Oct 2021 15:56:44 -0700 Subject: [PATCH] [orchagent, cfgmgr] Add response publisher and state recording * Add response publisher. * Add APPL STATE DB recording. * Add response path into vrforch. Submission containing materials of a third party: Copyright Google LLC; Licensed under Apache 2.0 Co-authored-by: Runming Wu Co-authored-by: Yilan Ji Co-authored-by: Akarsh Gupta Co-authored-by: Jay Hu Signed-off-by: Don Newton don@opennetworking.org --- cfgmgr/Makefile.am | 26 +- cfgmgr/buffermgrd.cpp | 4 + cfgmgr/coppmgrd.cpp | 4 + cfgmgr/intfmgrd.cpp | 4 + cfgmgr/macsecmgrd.cpp | 4 + cfgmgr/natmgrd.cpp | 5 +- cfgmgr/nbrmgrd.cpp | 4 + cfgmgr/portmgrd.cpp | 4 + cfgmgr/sflowmgrd.cpp | 4 + cfgmgr/teammgrd.cpp | 4 + cfgmgr/tunnelmgrd.cpp | 4 + cfgmgr/vlanmgrd.cpp | 4 + cfgmgr/vrfmgrd.cpp | 4 + cfgmgr/vxlanmgrd.cpp | 4 + orchagent/Makefile.am | 3 +- orchagent/main.cpp | 69 +++-- orchagent/orch.cpp | 68 ++-- orchagent/orch.h | 3 + orchagent/request_parser.cpp | 2 + orchagent/request_parser.h | 7 + orchagent/response_publisher.cpp | 170 ++++++++++ orchagent/response_publisher.h | 54 ++++ orchagent/response_publisher_interface.h | 36 +++ orchagent/return_code.h | 308 +++++++++++++++++++ orchagent/vrforch.cpp | 57 ++-- tests/mock_tests/Makefile.am | 1 + tests/mock_tests/database_config.json | 5 + tests/mock_tests/fake_response_publisher.cpp | 23 ++ tests/test_vrf.py | 103 ++++++- 29 files changed, 910 insertions(+), 78 deletions(-) create mode 100644 orchagent/response_publisher.cpp create mode 100644 orchagent/response_publisher.h create mode 100644 orchagent/response_publisher_interface.h create mode 100644 orchagent/return_code.h create mode 100644 tests/mock_tests/fake_response_publisher.cpp diff --git a/cfgmgr/Makefile.am b/cfgmgr/Makefile.am index dcd652498c5..39574d69dd3 100644 --- a/cfgmgr/Makefile.am +++ b/cfgmgr/Makefile.am @@ -23,67 +23,67 @@ else DBGFLAGS = -g endif -vlanmgrd_SOURCES = vlanmgrd.cpp vlanmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +vlanmgrd_SOURCES = vlanmgrd.cpp vlanmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h vlanmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vlanmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vlanmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -teammgrd_SOURCES = teammgrd.cpp teammgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +teammgrd_SOURCES = teammgrd.cpp teammgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h teammgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) teammgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) teammgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -portmgrd_SOURCES = portmgrd.cpp portmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +portmgrd_SOURCES = portmgrd.cpp portmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h portmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) portmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) portmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -intfmgrd_SOURCES = intfmgrd.cpp intfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +intfmgrd_SOURCES = intfmgrd.cpp intfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) intfmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -buffermgrd_SOURCES = buffermgrd.cpp buffermgr.cpp buffermgrdyn.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +buffermgrd_SOURCES = buffermgrd.cpp buffermgr.cpp buffermgrdyn.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h buffermgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) buffermgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) buffermgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -vrfmgrd_SOURCES = vrfmgrd.cpp vrfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +vrfmgrd_SOURCES = vrfmgrd.cpp vrfmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h vrfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vrfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vrfmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -nbrmgrd_SOURCES = nbrmgrd.cpp nbrmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +nbrmgrd_SOURCES = nbrmgrd.cpp nbrmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h nbrmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(LIBNL_CFLAGS) nbrmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) $(LIBNL_CPPFLAGS) nbrmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) $(LIBNL_LIBS) -vxlanmgrd_SOURCES = vxlanmgrd.cpp vxlanmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +vxlanmgrd_SOURCES = vxlanmgrd.cpp vxlanmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h vxlanmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vxlanmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) vxlanmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -sflowmgrd_SOURCES = sflowmgrd.cpp sflowmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +sflowmgrd_SOURCES = sflowmgrd.cpp sflowmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h sflowmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) sflowmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) sflowmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -natmgrd_SOURCES = natmgrd.cpp natmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +natmgrd_SOURCES = natmgrd.cpp natmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h natmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) natmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) natmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -coppmgrd_SOURCES = coppmgrd.cpp coppmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +coppmgrd_SOURCES = coppmgrd.cpp coppmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h coppmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) coppmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) coppmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -tunnelmgrd_SOURCES = tunnelmgrd.cpp tunnelmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +tunnelmgrd_SOURCES = tunnelmgrd.cpp tunnelmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h tunnelmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) tunnelmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) tunnelmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) -macsecmgrd_SOURCES = macsecmgrd.cpp macsecmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h +macsecmgrd_SOURCES = macsecmgrd.cpp macsecmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp $(top_srcdir)/orchagent/response_publisher.cpp shellcmd.h macsecmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) macsecmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI) macsecmgrd_LDADD = $(COMMON_LIBS) $(SAIMETA_LIBS) diff --git a/cfgmgr/buffermgrd.cpp b/cfgmgr/buffermgrd.cpp index 9f4d9792375..bdaec8553f4 100644 --- a/cfgmgr/buffermgrd.cpp +++ b/cfgmgr/buffermgrd.cpp @@ -33,6 +33,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/coppmgrd.cpp b/cfgmgr/coppmgrd.cpp index 1995405cd64..60b0a2442a9 100644 --- a/cfgmgr/coppmgrd.cpp +++ b/cfgmgr/coppmgrd.cpp @@ -29,6 +29,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/intfmgrd.cpp b/cfgmgr/intfmgrd.cpp index d6ed18526e0..9ed36533335 100644 --- a/cfgmgr/intfmgrd.cpp +++ b/cfgmgr/intfmgrd.cpp @@ -29,6 +29,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/macsecmgrd.cpp b/cfgmgr/macsecmgrd.cpp index f77e3d8c07e..913c0ac4eef 100644 --- a/cfgmgr/macsecmgrd.cpp +++ b/cfgmgr/macsecmgrd.cpp @@ -38,6 +38,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/natmgrd.cpp b/cfgmgr/natmgrd.cpp index 7e2aeba4a25..c2baf7eb879 100644 --- a/cfgmgr/natmgrd.cpp +++ b/cfgmgr/natmgrd.cpp @@ -52,6 +52,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; mutex gDbMutex; NatMgr *natmgr = NULL; @@ -200,4 +204,3 @@ int main(int argc, char **argv) } return -1; } - diff --git a/cfgmgr/nbrmgrd.cpp b/cfgmgr/nbrmgrd.cpp index d9b68290368..338d8d9d0d5 100644 --- a/cfgmgr/nbrmgrd.cpp +++ b/cfgmgr/nbrmgrd.cpp @@ -33,6 +33,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/portmgrd.cpp b/cfgmgr/portmgrd.cpp index b0f0c887ddf..180bbc1d632 100644 --- a/cfgmgr/portmgrd.cpp +++ b/cfgmgr/portmgrd.cpp @@ -28,6 +28,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/sflowmgrd.cpp b/cfgmgr/sflowmgrd.cpp index 0436ad5f004..7de5f15a2d3 100644 --- a/cfgmgr/sflowmgrd.cpp +++ b/cfgmgr/sflowmgrd.cpp @@ -28,6 +28,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/teammgrd.cpp b/cfgmgr/teammgrd.cpp index e38456eebe2..66bfa4b6d23 100644 --- a/cfgmgr/teammgrd.cpp +++ b/cfgmgr/teammgrd.cpp @@ -17,6 +17,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; bool received_sigterm = false; diff --git a/cfgmgr/tunnelmgrd.cpp b/cfgmgr/tunnelmgrd.cpp index d419b2b8867..0165eb94b5f 100644 --- a/cfgmgr/tunnelmgrd.cpp +++ b/cfgmgr/tunnelmgrd.cpp @@ -31,6 +31,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/vlanmgrd.cpp b/cfgmgr/vlanmgrd.cpp index 88e4745758b..b69dc781228 100644 --- a/cfgmgr/vlanmgrd.cpp +++ b/cfgmgr/vlanmgrd.cpp @@ -36,6 +36,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/vrfmgrd.cpp b/cfgmgr/vrfmgrd.cpp index 556b937901f..735e59191d6 100644 --- a/cfgmgr/vrfmgrd.cpp +++ b/cfgmgr/vrfmgrd.cpp @@ -29,6 +29,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; diff --git a/cfgmgr/vxlanmgrd.cpp b/cfgmgr/vxlanmgrd.cpp index 809c580f822..d47893a614e 100644 --- a/cfgmgr/vxlanmgrd.cpp +++ b/cfgmgr/vxlanmgrd.cpp @@ -34,6 +34,10 @@ bool gSwssRecord = false; bool gLogRotate = false; ofstream gRecordOfs; string gRecordFile; +bool gResponsePublisherRecord = false; +bool gResponsePublisherLogRotate = false; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; /* Global database mutex */ mutex gDbMutex; MacAddress gMacAddress; diff --git a/orchagent/Makefile.am b/orchagent/Makefile.am index cf789b027ff..2006fb44c51 100644 --- a/orchagent/Makefile.am +++ b/orchagent/Makefile.am @@ -83,7 +83,8 @@ orchagent_SOURCES = \ macsecorch.cpp \ lagid.cpp \ bfdorch.cpp \ - srv6orch.cpp + srv6orch.cpp \ + response_publisher.cpp orchagent_SOURCES += flex_counter/flex_counter_manager.cpp flex_counter/flex_counter_stat_manager.cpp orchagent_SOURCES += debug_counter/debug_counter.cpp debug_counter/drop_counter.cpp diff --git a/orchagent/main.cpp b/orchagent/main.cpp index aacae5fe97e..281e6424c2e 100644 --- a/orchagent/main.cpp +++ b/orchagent/main.cpp @@ -13,6 +13,8 @@ extern "C" { #include #include #include +#include +#include #include #include @@ -54,8 +56,10 @@ int gBatchSize = DEFAULT_BATCH_SIZE; bool gSairedisRecord = true; bool gSwssRecord = true; +bool gResponsePublisherRecord = true; bool gLogRotate = false; bool gSaiRedisLogRotate = false; +bool gResponsePublisherLogRotate = false; bool gSyncMode = false; sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC; string gAsicInstance; @@ -64,6 +68,12 @@ extern bool gIsNatSupported; ofstream gRecordOfs; string gRecordFile; +ofstream gResponsePublisherRecordOfs; +string gResponsePublisherRecordFile; + +#define SAIREDIS_RECORD_ENABLE 0x1 +#define SWSS_RECORD_ENABLE (0x1 << 1) +#define RESPONSE_PUBLISHER_RECORD_ENABLE (0x1 << 2) string gMySwitchType = ""; int32_t gVoqMySwitchId = -1; @@ -76,11 +86,13 @@ void usage() { cout << "usage: orchagent [-h] [-r record_type] [-d record_location] [-f swss_rec_filename] [-j sairedis_rec_filename] [-b batch_size] [-m MAC] [-i INST_ID] [-s] [-z mode] [-k bulk_size]" << endl; cout << " -h: display this message" << endl; - cout << " -r record_type: record orchagent logs with type (default 3)" << endl; + cout << " -r record_type: record orchagent logs with type (default 7)" << endl; + cout << " Bit 0: sairedis.rec, Bit 1: swss.rec, Bit 2: responsepublisher.rec. For example:" << endl; cout << " 0: do not record logs" << endl; cout << " 1: record SAI call sequence as sairedis.rec" << endl; cout << " 2: record SwSS task sequence as swss.rec" << endl; cout << " 3: enable both above two records" << endl; + cout << " 7: enable sairedis.rec, swss.rec and responsepublisher.rec" << endl; cout << " -d record_location: set record logs folder location (default .)" << endl; cout << " -b batch_size: set consumer table pop operation batch size (default 128)" << endl; cout << " -m MAC: set switch MAC address" << endl; @@ -99,6 +111,7 @@ void sighup_handler(int signo) */ gLogRotate = true; gSaiRedisLogRotate = true; + gResponsePublisherLogRotate = true; } void syncd_apply_view() @@ -321,6 +334,7 @@ int main(int argc, char **argv) string record_location = "."; string swss_rec_filename = "swss.rec"; string sairedis_rec_filename = "sairedis.rec"; + int record_type = 7; // All recordings enabled by default. while ((opt = getopt(argc, argv, "b:m:r:f:j:d:i:hsz:k:")) != -1) { @@ -346,28 +360,13 @@ int main(int argc, char **argv) gMacAddress = MacAddress(optarg); break; case 'r': - if (!strcmp(optarg, "0")) - { - gSairedisRecord = false; - gSwssRecord = false; - } - else if (!strcmp(optarg, "1")) - { - gSwssRecord = false; - } - else if (!strcmp(optarg, "2")) - { - gSairedisRecord = false; - } - else if (!strcmp(optarg, "3")) - { - continue; /* default behavior */ - } - else - { - usage(); - exit(EXIT_FAILURE); - } + // Disable all recordings if atoi() fails i.e. returns 0 due to + // invalid command line argument. + record_type = atoi(optarg); + if (record_type < 0 || record_type > 7) { + usage(); + exit(EXIT_FAILURE); + } break; case 'd': record_location = optarg; @@ -434,6 +433,14 @@ int main(int argc, char **argv) attr.value.ptr = (void *)on_fdb_event; attrs.push_back(attr); + // Initialize recording parameters. + gSairedisRecord = + (record_type & SAIREDIS_RECORD_ENABLE) == SAIREDIS_RECORD_ENABLE; + gSwssRecord = (record_type & SWSS_RECORD_ENABLE) == SWSS_RECORD_ENABLE; + gResponsePublisherRecord = + (record_type & RESPONSE_PUBLISHER_RECORD_ENABLE) == + RESPONSE_PUBLISHER_RECORD_ENABLE; + /* Disable/enable SwSS recording */ if (gSwssRecord) { @@ -447,6 +454,22 @@ int main(int argc, char **argv) gRecordOfs << getTimestamp() << "|recording started" << endl; } + // Disable/Enable response publisher recording. + if (gResponsePublisherRecord) { + gResponsePublisherRecordFile = + record_location + "/" + "responsepublisher.rec"; + gResponsePublisherRecordOfs.open(gResponsePublisherRecordFile, + std::ofstream::out | std::ofstream::app); + if (!gResponsePublisherRecordOfs.is_open()) { + SWSS_LOG_ERROR("Failed to open Response Publisher recording file %s", + gResponsePublisherRecordFile.c_str()); + gResponsePublisherRecord = false; + } else { + gResponsePublisherRecordOfs << getTimestamp() << "|recording started" + << endl; + } + } + attr.id = SAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFY; attr.value.ptr = (void *)on_port_state_change; attrs.push_back(attr); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 226512f388e..2b6237ddcc8 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -1,6 +1,8 @@ #include #include #include +#include +#include #include #include "timestamp.h" #include "orch.h" @@ -197,9 +199,16 @@ size_t Consumer::refillToSync() auto subTable = dynamic_cast(consumerTable); if (subTable != NULL) { - std::deque entries; - subTable->pops(entries); - return addToSync(entries); + size_t update_size = 0; + size_t total_size = 0; + do + { + std::deque entries; + subTable->pops(entries); + update_size = addToSync(entries); + total_size += update_size; + } while (update_size != 0); + return total_size; } else { @@ -215,10 +224,13 @@ void Consumer::execute() { SWSS_LOG_ENTER(); - std::deque entries; - getConsumerTable()->pops(entries); - - addToSync(entries); + size_t update_size = 0; + do + { + std::deque entries; + getConsumerTable()->pops(entries); + update_size = addToSync(entries); + } while (update_size != 0); drain(); } @@ -729,9 +741,9 @@ task_process_status Orch::handleSaiCreateStatus(sai_api_t api, sai_status_t stat SWSS_LOG_WARN("SAI_STATUS_SUCCESS is not expected in handleSaiCreateStatus"); return task_success; default: - SWSS_LOG_ERROR("Encountered failure in create operation, exiting orchagent, SAI API: %s, status: %s", + SWSS_LOG_ERROR("Encountered failure in create operation, SAI API: %s, status: %s", sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); - exit(EXIT_FAILURE); + return task_failed; } } return task_need_retry; @@ -775,9 +787,9 @@ task_process_status Orch::handleSaiSetStatus(sai_api_t api, sai_status_t status, exit(EXIT_FAILURE); } default: - SWSS_LOG_ERROR("Encountered failure in set operation, exiting orchagent, SAI API: %s, status: %s", + SWSS_LOG_ERROR("Encountered failure in set operation, SAI API: %s, status: %s", sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); - exit(EXIT_FAILURE); + return task_failed; } return task_need_retry; @@ -803,9 +815,9 @@ task_process_status Orch::handleSaiRemoveStatus(sai_api_t api, sai_status_t stat SWSS_LOG_WARN("SAI_STATUS_SUCCESS is not expected in handleSaiRemoveStatus"); return task_success; default: - SWSS_LOG_ERROR("Encountered failure in remove operation, exiting orchagent, SAI API: %s, status: %s", + SWSS_LOG_ERROR("Encountered failure in remove operation, SAI API: %s, status: %s", sai_serialize_api(api).c_str(), sai_serialize_status(status).c_str()); - exit(EXIT_FAILURE); + return task_failed; } return task_need_retry; } @@ -866,6 +878,7 @@ void Orch2::doTask(Consumer &consumer) while (it != consumer.m_toSync.end()) { bool erase_from_queue = true; + ReturnCode rc; try { request_.parse(it->second); @@ -883,26 +896,44 @@ void Orch2::doTask(Consumer &consumer) } else { - SWSS_LOG_ERROR("Wrong operation. Check RequestParser: %s", op.c_str()); + rc = ReturnCode(StatusCode::SWSS_RC_INVALID_PARAM) + << "Wrong operation, only supporting SET and DEL, but got " + << op; + SWSS_LOG_ERROR("%s", rc.message().c_str()); } } catch (const std::invalid_argument& e) { - SWSS_LOG_ERROR("Parse error: %s", e.what()); + rc = ReturnCode(StatusCode::SWSS_RC_INVALID_PARAM) + << "Parse error: " << e.what(); + SWSS_LOG_ERROR("%s", rc.message().c_str()); } catch (const std::logic_error& e) { - SWSS_LOG_ERROR("Logic error: %s", e.what()); + rc = ReturnCode(StatusCode::SWSS_RC_INTERNAL) + << "Logic error: " << e.what(); + SWSS_LOG_ERROR("%s", rc.message().c_str()); + SWSS_RAISE_CRITICAL_STATE(rc.message()); } catch (const std::exception& e) { - SWSS_LOG_ERROR("Exception was catched in the request parser: %s", e.what()); + rc = ReturnCode(StatusCode::SWSS_RC_INTERNAL) + << "Exception was catched in the request parser: " << e.what(); + SWSS_LOG_ERROR("%s", rc.message().c_str()); + SWSS_RAISE_CRITICAL_STATE(rc.message()); } catch (...) { - SWSS_LOG_ERROR("Unknown exception was catched in the request parser"); + rc = ReturnCode(StatusCode::SWSS_RC_UNKNOWN) + << "Unknown exception was catched in the request parser"; + SWSS_LOG_ERROR("%s", rc.message().c_str()); } request_.clear(); + if (!rc.ok()) + { + m_publisher.publish(consumer.getTableName(), kfvKey(it->second), + kfvFieldsValues(it->second), rc); + } if (erase_from_queue) { @@ -914,4 +945,3 @@ void Orch2::doTask(Consumer &consumer) } } } - diff --git a/orchagent/orch.h b/orchagent/orch.h index 9801f09f7d0..ab44bbbc315 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -20,6 +20,7 @@ extern "C" { #include "notificationconsumer.h" #include "selectabletimer.h" #include "macaddress.h" +#include "response_publisher.h" const char delimiter = ':'; const char list_item_delimiter = ','; @@ -243,6 +244,8 @@ class Orch virtual task_process_status handleSaiRemoveStatus(sai_api_t api, sai_status_t status, void *context = nullptr); virtual task_process_status handleSaiGetStatus(sai_api_t api, sai_status_t status, void *context = nullptr); bool parseHandleSaiStatusFailure(task_process_status status); + + ResponsePublisher m_publisher{"APPL_STATE_DB"}; private: void removeMeFromObjsReferencedByMe(type_map &type_maps, const std::string &table, const std::string &obj_name, const std::string &field, const std::string &old_referenced_obj_name); void addConsumer(swss::DBConnector *db, std::string tableName, int pri = default_orch_pri); diff --git a/orchagent/request_parser.cpp b/orchagent/request_parser.cpp index 70b4351119e..48516a2068f 100644 --- a/orchagent/request_parser.cpp +++ b/orchagent/request_parser.cpp @@ -35,6 +35,7 @@ void Request::clear() operation_.clear(); full_key_.clear(); attr_names_.clear(); + full_attr_items_.clear(); key_item_strings_.clear(); key_item_mac_addresses_.clear(); attr_item_strings_.clear(); @@ -142,6 +143,7 @@ void Request::parseKey(const KeyOpFieldsValuesTuple& request) void Request::parseAttrs(const KeyOpFieldsValuesTuple& request) { const auto not_found = std::end(request_description_.attr_item_types); + full_attr_items_ = kfvFieldsValues(request); for (auto i = kfvFieldsValues(request).begin(); i != kfvFieldsValues(request).end(); i++) diff --git a/orchagent/request_parser.h b/orchagent/request_parser.h index 1fd110977c4..e1814c54f29 100644 --- a/orchagent/request_parser.h +++ b/orchagent/request_parser.h @@ -85,6 +85,12 @@ class Request return attr_names_; } + const std::vector& getFullAttrFields() const + { + assert(is_parsed_); + return full_attr_items_; + } + const std::string& getAttrString(const std::string& attr_name) const { assert(is_parsed_); @@ -209,6 +215,7 @@ class Request std::unordered_map key_item_ip_prefix_; std::unordered_map key_item_uint_; std::unordered_set attr_names_; + std::vector full_attr_items_; // FIXME: Make one union with all the values, except string std::unordered_map attr_item_strings_; std::unordered_map attr_item_bools_; diff --git a/orchagent/response_publisher.cpp b/orchagent/response_publisher.cpp new file mode 100644 index 00000000000..448e1c52de9 --- /dev/null +++ b/orchagent/response_publisher.cpp @@ -0,0 +1,170 @@ +#include "response_publisher.h" + +#include +#include +#include +#include + +#include "timestamp.h" + +extern bool gResponsePublisherRecord; +extern bool gResponsePublisherLogRotate; +extern std::ofstream gResponsePublisherRecordOfs; +extern std::string gResponsePublisherRecordFile; + +namespace { + +// Returns the component string that we need to prepend for sending the error +// message. +// Returns an empty string if the status is OK. +// Returns "[SAI] " if the ReturnCode is generated from a SAI status code. +// Else, returns "[OrchAgent] ". +std::string PrependedComponent(const ReturnCode& status) { + constexpr char* kOrchagentComponent = "[OrchAgent] "; + constexpr char* kSaiComponent = "[SAI] "; + if (status.ok()) { + return ""; + } + if (status.isSai()) { + return kSaiComponent; + } + return kOrchagentComponent; +} + +void PerformLogRotate() { + if (!gResponsePublisherLogRotate) { + return; + } + gResponsePublisherLogRotate = false; + + gResponsePublisherRecordOfs.close(); + gResponsePublisherRecordOfs.open(gResponsePublisherRecordFile); + if (!gResponsePublisherRecordOfs.is_open()) { + SWSS_LOG_ERROR("Failed to reopen Response Publisher record file %s: %s", + gResponsePublisherRecordFile.c_str(), strerror(errno)); + } +} + +void RecordDBWrite(const std::string& table, const std::string& key, + const std::vector& attrs, + const std::string& op) { + if (!gResponsePublisherRecord) { + return; + } + + std::string s = table + ":" + key + "|" + op; + for (const auto& attr : attrs) { + s += "|" + fvField(attr) + ":" + fvValue(attr); + } + + PerformLogRotate(); + gResponsePublisherRecordOfs << swss::getTimestamp() << "|" << s << std::endl; +} + +void RecordResponse(const std::string& response_channel, const std::string& key, + const std::vector& attrs, + const std::string& status) { + if (!gResponsePublisherRecord) { + return; + } + + std::string s = response_channel + ":" + key + "|" + status; + for (const auto& attr : attrs) { + s += "|" + fvField(attr) + ":" + fvValue(attr); + } + + PerformLogRotate(); + gResponsePublisherRecordOfs << swss::getTimestamp() << "|" << s << std::endl; +} + +} // namespace + +ResponsePublisher::ResponsePublisher(const std::string& dbName) + : m_db(dbName, 0) {} + +void ResponsePublisher::publish( + const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, + const std::vector& state_attrs, bool replace) { + // Write to the DB only if: + // 1) A write operation is being performed and state attributes are specified. + // 2) A successful delete operation. + if ((intent_attrs.size() && state_attrs.size()) || + (status.ok() && !intent_attrs.size())) { + writeToDB(table, key, state_attrs, + intent_attrs.size() ? SET_COMMAND : DEL_COMMAND, replace); + } + + std::string response_channel = "APPL_DB_" + table + "_RESPONSE_CHANNEL"; + if (m_notifiers.find(table) == m_notifiers.end()) { + m_notifiers[table] = + std::make_unique(&m_db, response_channel); + } + + auto intent_attrs_copy = intent_attrs; + // Add error message as the first field-value-pair. + swss::FieldValueTuple err_str("err_str", + PrependedComponent(status) + status.message()); + intent_attrs_copy.insert(intent_attrs_copy.begin(), err_str); + // Sends the response to the notification channel. + m_notifiers[table]->send(status.codeStr(), key, intent_attrs_copy); + RecordResponse(response_channel, key, intent_attrs_copy, status.codeStr()); +} + +void ResponsePublisher::publish( + const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, bool replace) { + // If status is OK then intent attributes need to be written in + // APPL_STATE_DB. In this case, pass the intent attributes as state + // attributes. In case of a failure status, nothing needs to be written in + // APPL_STATE_DB. + std::vector state_attrs; + if (status.ok()) { + state_attrs = intent_attrs; + } + publish(table, key, intent_attrs, status, state_attrs, replace); +} + +void ResponsePublisher::writeToDB( + const std::string& table, const std::string& key, + const std::vector& values, const std::string& op, + bool replace) { + if (m_tables.find(table) == m_tables.end()) { + m_tables[table] = std::make_unique(&m_db, table); + } + + auto attrs = values; + if (op == SET_COMMAND) { + if (replace) { + m_tables[table]->del(key); + } + if (!values.size()) { + attrs.push_back(swss::FieldValueTuple("NULL", "NULL")); + } + + // Write to DB only if the key does not exist or non-NULL attributes are + // being written to the entry. + std::vector fv; + if (!m_tables[table]->get(key, fv)) { + m_tables[table]->set(key, attrs); + RecordDBWrite(table, key, attrs, op); + return; + } + for (auto it = attrs.cbegin(); it != attrs.cend();) { + if (it->first == "NULL") { + it = attrs.erase(it); + } else { + it++; + } + } + if (attrs.size()) { + m_tables[table]->set(key, attrs); + RecordDBWrite(table, key, attrs, op); + } + } else if (op == DEL_COMMAND) { + m_tables[table]->del(key); + RecordDBWrite(table, key, {}, op); + } +} diff --git a/orchagent/response_publisher.h b/orchagent/response_publisher.h new file mode 100644 index 00000000000..342f992f925 --- /dev/null +++ b/orchagent/response_publisher.h @@ -0,0 +1,54 @@ +#pragma once + +#include +#include +#include +#include + +#include "dbconnector.h" +#include "notificationproducer.h" +#include "response_publisher_interface.h" +#include "table.h" + +// This class performs two tasks when publish is called: +// 1. Sends a notification into the redis channel. +// 2. Writes the operation into the DB. +class ResponsePublisher : public ResponsePublisherInterface { + public: + explicit ResponsePublisher(const std::string& dbName); + virtual ~ResponsePublisher() = default; + + // Intent attributes are the attributes sent in the notification into the + // redis channel. + // State attributes are the list of attributes that need to be written in + // the DB namespace. These might be different from intent attributes. For + // example: + // 1) If only a subset of the intent attributes were successfully applied, the + // state attributes shall be different from intent attributes. + // 2) If additional state changes occur due to the intent attributes, more + // attributes need to be added in the state DB namespace. + // 3) Invalid attributes are excluded from the state attributes. + // State attributes will be written into the DB even if the status code + // consists of an error. + void publish(const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, + const std::vector& state_attrs, + bool replace = false) override; + + void publish(const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, bool replace = false) override; + + void writeToDB(const std::string& table, const std::string& key, + const std::vector& values, + const std::string& op, bool replace = false) override; + + private: + swss::DBConnector m_db; + // Maps table names to tables. + std::unordered_map> m_tables; + // Maps table names to notifiers. + std::unordered_map> + m_notifiers; +}; diff --git a/orchagent/response_publisher_interface.h b/orchagent/response_publisher_interface.h new file mode 100644 index 00000000000..92d364a500f --- /dev/null +++ b/orchagent/response_publisher_interface.h @@ -0,0 +1,36 @@ +#pragma once + +#include + +#include "return_code.h" +#include "table.h" + +class ResponsePublisherInterface { + public: + virtual ~ResponsePublisherInterface() = default; + + // Publishes the response status. + // If intent attributes are empty, it is a delete operation. + // What "publish" needs to do is completely up to implementation. + // This API does not include redis DB namespace. So if implementation chooses + // to write to a redis DB, it will need to use a fixed namespace. + // The replace flag indicates the state attributes will replace the old ones. + virtual void publish(const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, + const std::vector& state_attrs, + bool replace = false) = 0; + + // Publishes response status. If response status is OK then also writes the + // intent attributes into the DB. + // The replace flag indicates a replace operation. + virtual void publish(const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, bool replace = false) = 0; + + // Write to DB only. This API does not send notification. + // The replace flag indicates the new attributes will replace the old ones. + virtual void writeToDB(const std::string& table, const std::string& key, + const std::vector& values, + const std::string& op, bool replace = false) = 0; +}; diff --git a/orchagent/return_code.h b/orchagent/return_code.h new file mode 100644 index 00000000000..67265ece9c2 --- /dev/null +++ b/orchagent/return_code.h @@ -0,0 +1,308 @@ +#pragma once + +#include +#include +#include +#include +#include +#include + +#include "sai_serialize.h" +#include "status_code_util.h" + +extern "C" { +#include "sai.h" +} + +using swss::StatusCode; + +// RETURN_IF_ERROR evaluates an expression that returns a ReturnCode. If the +// result is not ok, returns the result. Otherwise, continues. +// +// Example: +// ReturnCode Foo() {...} +// ReturnCode Bar() { +// RETURN_IF_ERROR(Foo()); +// return ReturnCode(); +// } +#define RETURN_IF_ERROR(expr) \ + do { \ + ReturnCode RETURN_IF_ERROR_RC_ = expr; \ + if (!RETURN_IF_ERROR_RC_.ok()) return RETURN_IF_ERROR_RC_; \ + } while (0) + +// LOG_ERROR_AND_RETURN evaluates an expression that should returns an error +// ReturnCode. Logs the error message in the ReturnCode by calling +// SWSS_LOG_ERROR and returns. +#define LOG_ERROR_AND_RETURN(expr) \ + do { \ + ReturnCode LOG_ERROR_AND_RETURN_RC_ = expr; \ + SWSS_LOG_ERROR("%s", LOG_ERROR_AND_RETURN_RC_.message().c_str()); \ + return LOG_ERROR_AND_RETURN_RC_; \ + } while (0) + +// Same as RETURN_IF_ERROR, plus a call of SWSS_LOG_ERROR for the return code +// error message. +#define LOG_AND_RETURN_IF_ERROR(expr) \ + do { \ + ReturnCode LOG_AND_RETURN_IF_ERROR_RC_ = expr; \ + if (!LOG_AND_RETURN_IF_ERROR_RC_.ok()) { \ + SWSS_LOG_ERROR("%s", LOG_AND_RETURN_IF_ERROR_RC_.message().c_str()); \ + return LOG_AND_RETURN_IF_ERROR_RC_; \ + } \ + } while (0) + +#define RETURNCODE_MACROS_IMPL_CONCAT_INNER_(x, y) x##y + +#define RETURNCODE_MACROS_IMPL_CONCAT_(x, y) \ + RETURNCODE_MACROS_IMPL_CONCAT_INNER_(x, y) + +// ASSIGN_OR_RETURN evaluates an expression that returns a ReturnCodeOr. If the +// result is ok, the value is saved to dest. Otherwise, the ReturnCode is +// returned. +// +// Example: +// ReturnCodeOr Foo() {...} +// ReturnCode Bar() { +// ASSIGN_OR_RETURN(int value, Foo()); +// std::cout << "value: " << value; +// return ReturnCode(); +// } +#define ASSIGN_OR_RETURN(dest, expr) \ + auto RETURNCODE_MACROS_IMPL_CONCAT_(ASSIGN_OR_RETURN_RESULT_, __LINE__) = \ + expr; \ + if (!RETURNCODE_MACROS_IMPL_CONCAT_(ASSIGN_OR_RETURN_RESULT_, __LINE__) \ + .ok()) { \ + return RETURNCODE_MACROS_IMPL_CONCAT_(ASSIGN_OR_RETURN_RESULT_, __LINE__) \ + .status(); \ + } \ + dest = std::move( \ + RETURNCODE_MACROS_IMPL_CONCAT_(ASSIGN_OR_RETURN_RESULT_, __LINE__) \ + .value()) + +// CHECK_ERROR_AND_LOG evaluates an expression that returns a sai_status_t. If +// the result is not SAI_STATUS_SUCCESS, it will log an error message. +// +// Example: +// CHECK_ERROR_AND_LOG( +// sai_router_intfs_api->set_router_interface_attribute(...), +// "error message" << " stream"); +#define CHECK_ERROR_AND_LOG(expr, msg_stream) \ + do { \ + sai_status_t CHECK_ERROR_AND_LOG_SAI_ = expr; \ + if (CHECK_ERROR_AND_LOG_SAI_ != SAI_STATUS_SUCCESS) { \ + std::stringstream CHECK_ERROR_AND_LOG_SS_; \ + CHECK_ERROR_AND_LOG_SS_ << msg_stream; \ + SWSS_LOG_ERROR("%s SAI_STATUS: %s", \ + CHECK_ERROR_AND_LOG_SS_.str().c_str(), \ + sai_serialize_status(CHECK_ERROR_AND_LOG_SAI_).c_str()); \ + } \ + } while (0) + +// CHECK_ERROR_AND_LOG_AND_RETURN evaluates an expression that returns a +// sai_status_t. If the result is not SAI_STATUS_SUCCESS, it will log an error +// message and return a ReturnCode. +// +// Example: +// CHECK_ERROR_AND_LOG_AND_RETURN( +// sai_router_intfs_api->set_router_interface_attribute(...), +// "error message" << " stream"); +#define CHECK_ERROR_AND_LOG_AND_RETURN(expr, msg_stream) \ + do { \ + sai_status_t CHECK_ERROR_AND_LOG_AND_RETURN_SAI_ = expr; \ + if (CHECK_ERROR_AND_LOG_AND_RETURN_SAI_ != SAI_STATUS_SUCCESS) { \ + ReturnCode CHECK_ERROR_AND_LOG_AND_RETURN_RC_ = \ + ReturnCode(CHECK_ERROR_AND_LOG_AND_RETURN_SAI_) << msg_stream; \ + SWSS_LOG_ERROR( \ + "%s SAI_STATUS: %s", \ + CHECK_ERROR_AND_LOG_AND_RETURN_RC_.message().c_str(), \ + sai_serialize_status(CHECK_ERROR_AND_LOG_AND_RETURN_SAI_).c_str()); \ + return CHECK_ERROR_AND_LOG_AND_RETURN_RC_; \ + } \ + } while (0) + +// This macro raises critical state to indicate that something is seriously +// wrong in the system. Currently, this macro just logs an error message. +// TODO: Implement this macro. +#define SWSS_RAISE_CRITICAL_STATE(err_str) \ + do { \ + std::string err_msge = err_str; \ + SWSS_LOG_ERROR("Orchagent is in critical state: %s", err_msge.c_str()); \ + } while (0) + +// RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL returns an error status of +// SWSS_RC_INTERNAL. It also logs the error message and reports critical state. +#define RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL(msg_stream) \ + do { \ + ReturnCode RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL_RC_ = \ + ReturnCode(StatusCode::SWSS_RC_INTERNAL) << msg_stream; \ + SWSS_LOG_ERROR( \ + "%s", RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL_RC_.message().c_str()); \ + SWSS_RAISE_CRITICAL_STATE( \ + RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL_RC_.message()); \ + return RETURN_INTERNAL_ERROR_AND_RAISE_CRITICAL_RC_; \ + } while (0) + +class ReturnCode { + public: + ReturnCode() + : status_(StatusCode::SWSS_RC_SUCCESS), + stream_(std::ios_base::out | std::ios_base::ate), + is_sai_(false) {} + + ReturnCode(const StatusCode& status, const std::string& message = "") + : status_(status), + stream_(std::ios_base::out | std::ios_base::ate), + is_sai_(false) { + stream_ << message; + } + + ReturnCode(const sai_status_t& status, const std::string& message = "") + : stream_(std::ios_base::out | std::ios_base::ate), is_sai_(true) { + if (m_saiStatusCodeLookup.find(status) == m_saiStatusCodeLookup.end()) { + status_ = StatusCode::SWSS_RC_UNKNOWN; + } else { + status_ = m_saiStatusCodeLookup[status]; + } + stream_ << message; + } + + ReturnCode(const ReturnCode& return_code) + : stream_(std::ios_base::out | std::ios_base::ate) { + status_ = return_code.status_; + stream_ << return_code.stream_.str(); + is_sai_ = return_code.is_sai_; + } + + ReturnCode& operator=(const ReturnCode& return_code) { + status_ = return_code.status_; + stream_.str(return_code.stream_.str()); + is_sai_ = return_code.is_sai_; + return *this; + } + + ~ReturnCode() = default; + + bool ok() const { return status_ == StatusCode::SWSS_RC_SUCCESS; } + + StatusCode code() const { return status_; } + + std::string codeStr() const { return swss::statusCodeToStr(status_); } + + std::string message() const { + if (stream_.str().empty()) { + return codeStr(); + } + return stream_.str(); + } + + ReturnCode& prepend(const std::string& msg) { + const std::string& tmp = stream_.str(); + stream_.str(msg + tmp); + return *this; + } + + std::string toString() const { return codeStr() + ":" + message(); } + + // Whether the ReturnCode is generated from a SAI status code or not. + bool isSai() const { return is_sai_; } + + template + ReturnCode& operator<<(T val) { + stream_ << val; + return *this; + } + + bool operator==(const ReturnCode& x) const { + return status_ == x.status_ && message() == x.message(); + } + + bool operator!=(const ReturnCode& x) const { + return status_ != x.status_ || message() != x.message(); + } + + bool operator==(const StatusCode& x) const { return status_ == x; } + + bool operator!=(const StatusCode& x) const { return status_ != x; } + + private: + // SAI codes that are not included in this lookup map will map to + // SWSS_RC_UNKNOWN. This includes the general SAI failure: SAI_STATUS_FAILURE. + std::unordered_map m_saiStatusCodeLookup = { + {SAI_STATUS_SUCCESS, StatusCode::SWSS_RC_SUCCESS}, + {SAI_STATUS_NOT_SUPPORTED, StatusCode::SWSS_RC_UNIMPLEMENTED}, + {SAI_STATUS_NO_MEMORY, StatusCode::SWSS_RC_NO_MEMORY}, + {SAI_STATUS_INSUFFICIENT_RESOURCES, StatusCode::SWSS_RC_FULL}, + {SAI_STATUS_INVALID_PARAMETER, StatusCode::SWSS_RC_INVALID_PARAM}, + {SAI_STATUS_ITEM_ALREADY_EXISTS, StatusCode::SWSS_RC_EXISTS}, + {SAI_STATUS_ITEM_NOT_FOUND, StatusCode::SWSS_RC_NOT_FOUND}, + {SAI_STATUS_TABLE_FULL, StatusCode::SWSS_RC_FULL}, + {SAI_STATUS_NOT_IMPLEMENTED, StatusCode::SWSS_RC_UNIMPLEMENTED}, + {SAI_STATUS_OBJECT_IN_USE, StatusCode::SWSS_RC_IN_USE}, + }; + + StatusCode status_; + std::stringstream stream_; + // Whether the ReturnCode is generated from a SAI status code or not. + bool is_sai_; +}; + +inline bool operator==(const StatusCode& lhs, const ReturnCode& rhs) { + return lhs == rhs.code(); +} + +inline bool operator!=(const StatusCode& lhs, const ReturnCode& rhs) { + return lhs != rhs.code(); +} + +template +class ReturnCodeOr { + public: + using value_type = T; + + // Value Constructors. + ReturnCodeOr(const T& value) + : return_code_(ReturnCode()), value_(std::unique_ptr(new T(value))) {} + ReturnCodeOr(T&& value) + : return_code_(ReturnCode()), + value_(std::unique_ptr(new T(std::move(value)))) {} + + // ReturnCode constructors. + ReturnCodeOr(const ReturnCode& return_code) : return_code_(return_code) { + assert(!return_code.ok()); + } + + // ReturnCode accessors. + bool ok() const { return return_code_.ok(); } + const ReturnCode& status() const { return return_code_; } + + // Value accessors. + const T& value() const& { + assert(return_code_.ok()); + return *value_; + } + T& value() & { + assert(return_code_.ok()); + return *value_; + } + const T&& value() const&& { + assert(return_code_.ok()); + return std::move(*value_); + } + T&& value() && { + assert(return_code_.ok()); + return std::move(*value_); + } + + const T& operator*() const& { return value(); } + T& operator*() & { return value(); } + const T&& operator*() const&& { return value(); } + T&& operator*() && { return value(); } + + const T* operator->() const { return value_.get(); } + T* operator->() { return value_.get(); } + + private: + ReturnCode return_code_; + std::unique_ptr value_; +}; diff --git a/orchagent/vrforch.cpp b/orchagent/vrforch.cpp index 19ca5c0fd88..44d2b5a016d 100644 --- a/orchagent/vrforch.cpp +++ b/orchagent/vrforch.cpp @@ -6,9 +6,11 @@ #include #include "sai.h" +#include "sai_serialize.h" #include "macaddress.h" #include "orch.h" #include "request_parser.h" +#include "return_code.h" #include "vrforch.h" #include "vxlanorch.h" #include "directory.h" @@ -93,12 +95,15 @@ bool VRFOrch::addOperation(const Request& request) attrs.data()); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to create virtual router name: %s, rv: %d", vrf_name.c_str(), status); - task_process_status handle_status = handleSaiCreateStatus(SAI_API_VIRTUAL_ROUTER, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + ReturnCode rc = ReturnCode(status) + << "Failed to create virtual router name: " + << vrf_name << ", rv: " << sai_serialize_status(status); + SWSS_LOG_ERROR("%s", rc.message().c_str()); + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), rc); + handleSaiCreateStatus(SAI_API_VIRTUAL_ROUTER, status); + // Remove from orchagent queue when there is SAI error + return true; } vrf_table_[vrf_name].vrf_id = router_id; @@ -127,12 +132,15 @@ bool VRFOrch::addOperation(const Request& request) sai_status_t status = sai_virtual_router_api->set_virtual_router_attribute(router_id, &attr); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to update virtual router attribute. vrf name: %s, rv: %d", vrf_name.c_str(), status); - task_process_status handle_status = handleSaiSetStatus(SAI_API_VIRTUAL_ROUTER, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + ReturnCode rc = ReturnCode(status) + << "Failed to update virtual router attribute. vrf name: " + << vrf_name << ", rv: " << sai_serialize_status(status); + SWSS_LOG_ERROR("%s", rc.message().c_str()); + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), rc); + handleSaiSetStatus(SAI_API_VIRTUAL_ROUTER, status); + // Remove from orchagent queue when there is SAI error + return true; } } @@ -146,6 +154,8 @@ bool VRFOrch::addOperation(const Request& request) SWSS_LOG_NOTICE("VRF '%s' was updated", vrf_name.c_str()); } + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), ReturnCode()); return true; } @@ -157,7 +167,11 @@ bool VRFOrch::delOperation(const Request& request) const std::string& vrf_name = request.getKeyString(0); if (vrf_table_.find(vrf_name) == std::end(vrf_table_)) { - SWSS_LOG_ERROR("VRF '%s' doesn't exist", vrf_name.c_str()); + ReturnCode rc = ReturnCode(StatusCode::SWSS_RC_NOT_FOUND) + << "VRF '" << vrf_name << "' doesn't exist"; + SWSS_LOG_ERROR("%s", rc.message().c_str()); + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), rc); return true; } @@ -168,12 +182,15 @@ bool VRFOrch::delOperation(const Request& request) sai_status_t status = sai_virtual_router_api->remove_virtual_router(router_id); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to remove virtual router name: %s, rv:%d", vrf_name.c_str(), status); - task_process_status handle_status = handleSaiRemoveStatus(SAI_API_VIRTUAL_ROUTER, status); - if (handle_status != task_success) - { - return parseHandleSaiStatusFailure(handle_status); - } + ReturnCode rc = ReturnCode(status) + << "Failed to remove virtual router name: " + << vrf_name << ", rv:" << sai_serialize_status(status); + SWSS_LOG_ERROR("%s", rc.message().c_str()); + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), rc); + handleSaiRemoveStatus(SAI_API_VIRTUAL_ROUTER, status); + // Remove from orchagent queue when there is SAI error + return true; } vrf_table_.erase(vrf_name); @@ -187,6 +204,8 @@ bool VRFOrch::delOperation(const Request& request) SWSS_LOG_NOTICE("VRF '%s' was removed", vrf_name.c_str()); + m_publisher.publish(request.getTableName(), request.getFullKey(), + request.getFullAttrFields(), ReturnCode()); return true; } diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index f1d02898f9e..a4ebe36711a 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -32,6 +32,7 @@ tests_SOURCES = aclorch_ut.cpp \ mock_hiredis.cpp \ mock_redisreply.cpp \ bulker_ut.cpp \ + fake_response_publisher.cpp \ $(top_srcdir)/lib/gearboxutils.cpp \ $(top_srcdir)/orchagent/orchdaemon.cpp \ $(top_srcdir)/orchagent/orch.cpp \ diff --git a/tests/mock_tests/database_config.json b/tests/mock_tests/database_config.json index 1b6343d20e7..83018486836 100644 --- a/tests/mock_tests/database_config.json +++ b/tests/mock_tests/database_config.json @@ -61,6 +61,11 @@ "id" : 12, "separator": "|", "instance" : "redis_chassis" + }, + "APPL_STATE_DB" : { + "id" : 14, + "separator": ":", + "instance" : "redis" } }, "VERSION" : "1.0" diff --git a/tests/mock_tests/fake_response_publisher.cpp b/tests/mock_tests/fake_response_publisher.cpp new file mode 100644 index 00000000000..e4ff2a4d63c --- /dev/null +++ b/tests/mock_tests/fake_response_publisher.cpp @@ -0,0 +1,23 @@ +#include +#include + +#include "response_publisher.h" + +ResponsePublisher::ResponsePublisher(const std::string& dbName) + : m_db(dbName, 0) {} + +void ResponsePublisher::publish( + const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, + const std::vector& state_attrs, bool replace) {} + +void ResponsePublisher::publish( + const std::string& table, const std::string& key, + const std::vector& intent_attrs, + const ReturnCode& status, bool replace) {} + +void ResponsePublisher::writeToDB( + const std::string& table, const std::string& key, + const std::vector& values, const std::string& op, + bool replace) {} diff --git a/tests/test_vrf.py b/tests/test_vrf.py index d595d5aad3b..ee8697811af 100644 --- a/tests/test_vrf.py +++ b/tests/test_vrf.py @@ -12,6 +12,9 @@ def setup_db(self, dvs): self.pdb = swsscommon.DBConnector(0, dvs.redis_sock, 0) self.adb = swsscommon.DBConnector(1, dvs.redis_sock, 0) self.cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0) + self.sdb = swsscommon.DBConnector(63, dvs.redis_sock, 0) + self.response_consumer = swsscommon.NotificationConsumer( + self.pdb, "APPL_DB_VRF_TABLE_RESPONSE_CHANNEL") def create_entry(self, tbl, key, pairs): fvs = swsscommon.FieldValuePairs(pairs) @@ -54,6 +57,24 @@ def is_vrf_attributes_correct(self, db, table, key, expected_attributes): (value, name, expected_attributes[name]) + def verify_response(self, consumer, key, attr_list, status, err_message = "SWSS_RC_SUCCESS"): + consumer.readData() + (op, data, fvs) = consumer.pop() + assert data == key + assert op == status + assert len(fvs) >= 1 + assert fvs[0][0] == "err_str" + assert fvs[0][1] == err_message + fvs = fvs[1:] + + attr_keys = {entry[0] for entry in fvs} + assert attr_keys == set(attr_list.keys()) + + for name, value in fvs: + assert attr_list[name] == value, "Wrong value %s for the attribute %s = %s" % \ + (value, name, attr_list[name]) + + def vrf_create(self, dvs, vrf_name, attributes, expected_attributes): # check that the vrf wasn't exist before assert self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") == 1, "The initial state is incorrect" @@ -82,6 +103,16 @@ def vrf_create(self, dvs, vrf_name, attributes, expected_attributes): exp_attr[attributes[an][0]] = attributes[an][1] self.is_vrf_attributes_correct(self.pdb, "VRF_TABLE", vrf_name, exp_attr) + # check application state database + tbl = swsscommon.Table(self.sdb, "VRF_TABLE") + intf_entries = tbl.getKeys() + assert len(intf_entries) == 1 + assert intf_entries[0] == vrf_name + self.is_vrf_attributes_correct(self.sdb, "VRF_TABLE", vrf_name, exp_attr) + + # check response channel + self.verify_response(self.response_consumer, vrf_name, exp_attr, "SWSS_RC_SUCCESS") + # check that the vrf entry was created assert self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") == 2, "The vrf wasn't created" @@ -113,6 +144,14 @@ def vrf_remove(self, dvs, vrf_name, state): intf_entries = tbl.getKeys() assert vrf_name not in intf_entries + # check application state database + tbl = swsscommon.Table(self.sdb, "VRF_TABLE") + intf_entries = tbl.getKeys() + assert vrf_name not in intf_entries + + # check response channel + self.verify_response(self.response_consumer, vrf_name, {}, "SWSS_RC_SUCCESS") + # check that the vrf entry was removed assert self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") == 1, "The vrf wasn't removed" @@ -127,6 +166,18 @@ def vrf_update(self, vrf_name, attributes, expected_attributes, state): # update the VRF entry in Config DB self.create_entry_tbl(self.cdb, "VRF", vrf_name, attributes) + # check application database + exp_attr = {} + for an in range(len(attributes)): + exp_attr[attributes[an][0]] = attributes[an][1] + self.is_vrf_attributes_correct(self.pdb, "VRF_TABLE", vrf_name, exp_attr) + + # check application state database + self.is_vrf_attributes_correct(self.sdb, "VRF_TABLE", vrf_name, exp_attr) + + # check response channel + self.verify_response(self.response_consumer, vrf_name, exp_attr, "SWSS_RC_SUCCESS") + # check correctness of the created attributes self.is_vrf_attributes_correct( self.adb, @@ -192,7 +243,6 @@ def test_VRFMgr_Comprehensive(self, dvs, testlog): state = self.vrf_create(dvs, vrf_name, req_attr, exp_attr) self.vrf_remove(dvs, vrf_name, state) - def test_VRFMgr(self, dvs, testlog): self.setup_db(dvs) @@ -238,7 +288,7 @@ def test_VRFMgr_Update(self, dvs, testlog): ) # try to update each attribute - req_attr = [] + req_attr = [('empty', 'empty')] exp_attr = {} for attr in attributes: req_res, exp_res = attr[2]() @@ -278,6 +328,10 @@ def create_entry_tbl(self, db, table, key, pairs): intf_entries_cnt = self.how_many_entries_exist(self.pdb, "VRF_TABLE") assert intf_entries_cnt == maximum_vrf_cnt + # check app_state_db + intf_entries_cnt = self.how_many_entries_exist(self.sdb, "VRF_TABLE") + assert intf_entries_cnt == maximum_vrf_cnt + # check asic_db current_entries_cnt = self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") assert (current_entries_cnt - initial_entries_cnt) == maximum_vrf_cnt @@ -297,6 +351,10 @@ def create_entry_tbl(self, db, table, key, pairs): intf_entries_cnt = self.how_many_entries_exist(self.pdb, "VRF_TABLE") assert intf_entries_cnt == 0 + # check app_state_db + intf_entries_cnt = self.how_many_entries_exist(self.sdb, "VRF_TABLE") + assert intf_entries_cnt == 0 + # check asic_db current_entries_cnt = self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") assert (current_entries_cnt - initial_entries_cnt) == 0 @@ -305,6 +363,47 @@ def create_entry_tbl(self, db, table, key, pairs): (exitcode, num) = dvs.runcmd(['sh', '-c', "ip link show | grep Vrf | wc -l"]) assert num.strip() == '0' + def test_VRFMgr_Error(self, dvs, testlog): + self.setup_db(dvs) + assert self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") == 1, "The initial state is incorrect" + + # create VRF with invalid attribute in APPL DB + tbl = swsscommon.ProducerStateTable(self.pdb, "VRF_TABLE") + vrf_name = "Vrf-invalid" + attributes = [ + ('v4', 'true'), + ('v6', 'false'), + ('ttl_action', 'trap'), + ('ip_opt_action', 'trap'), + ('l3_mc_action', 'drop'), + ('invalid', 'true'), + ] + exp_attr = {} + for an in range(len(attributes)): + exp_attr[attributes[an][0]] = attributes[an][1] + tbl.set(vrf_name, attributes) + + # check application database + tbl = swsscommon.Table(self.pdb, "VRF_TABLE") + intf_entries = tbl.getKeys() + assert len(intf_entries) == 1 + assert intf_entries[0] == vrf_name + self.is_vrf_attributes_correct(self.pdb, "VRF_TABLE", vrf_name, exp_attr) + + # check application state database + tbl = swsscommon.Table(self.sdb, "VRF_TABLE") + intf_entries = tbl.getKeys() + assert len(intf_entries) == 0 + + # check response channel + self.verify_response(self.response_consumer, vrf_name, exp_attr, "SWSS_RC_INVALID_PARAM", "[OrchAgent] Parse error: Unknown attribute name: invalid") + + # check ASIC DB + assert self.how_many_entries_exist(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER") == 1, "The vrf shouldn't be created" + + # clean up APPL DB + self.delete_entry_tbl(self.pdb, "VRF_TABLE", vrf_name) + # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying