From d566e15a86b4064a2abdb0b2b3edf3ef56bdb666 Mon Sep 17 00:00:00 2001 From: arista-nwolfe <94405414+arista-nwolfe@users.noreply.github.com> Date: Mon, 5 Feb 2024 13:17:12 -0500 Subject: [PATCH 1/8] Allow L4 port range egress ACL rules on DNX (#3014) Allow L4 port range egress ACL rules on DNX (#3014) --- orchagent/aclorch.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp index 707462799f..6906744cc2 100644 --- a/orchagent/aclorch.cpp +++ b/orchagent/aclorch.cpp @@ -2202,9 +2202,10 @@ bool AclTable::addStageMandatoryRangeFields() SWSS_LOG_ENTER(); string platform = getenv("platform") ? getenv("platform") : ""; + string sub_platform = getenv("sub_platform") ? getenv("sub_platform") : ""; auto match = SAI_ACL_TABLE_ATTR_FIELD_ACL_RANGE_TYPE; - if ((platform == BRCM_PLATFORM_SUBSTRING) && + if ((platform == BRCM_PLATFORM_SUBSTRING) && (sub_platform != BRCM_DNX_PLATFORM_SUBSTRING) && (stage == ACL_STAGE_EGRESS)) { return false; From 4d470592533c0cd81fb7d64719d9e69304d743e0 Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Tue, 6 Feb 2024 08:34:19 -0800 Subject: [PATCH 2/8] Fix memory leak and object copying bugs in orchagent (#3017) * Update orchagent for Bookworm There are two types of compiler errors. The first is GCC saying that in a for-loop, if the variable used for the objects is const, then a reference to the original object in the list can be used, instead of making a copy. The second is that two variables could potentially be used uninitialized. This appears to be a false positive to me, but I assigned it to 0 regardless. --- orchagent/fabricportsorch.cpp | 4 ++-- orchagent/fdborch.cpp | 4 ++-- orchagent/fdborch.h | 1 + orchagent/mirrororch.cpp | 4 ++-- orchagent/portsorch.cpp | 8 ++++---- orchagent/portsorch.h | 1 + tests/mock_tests/aclorch_ut.cpp | 2 ++ 7 files changed, 14 insertions(+), 10 deletions(-) diff --git a/orchagent/fabricportsorch.cpp b/orchagent/fabricportsorch.cpp index d521e02b1a..798a62988c 100644 --- a/orchagent/fabricportsorch.cpp +++ b/orchagent/fabricportsorch.cpp @@ -269,8 +269,8 @@ void FabricPortsOrch::updateFabricPortState() string key = FABRIC_PORT_PREFIX + to_string(lane); std::vector values; - uint32_t remote_peer; - uint32_t remote_port; + uint32_t remote_peer = 0; + uint32_t remote_port = 0; attr.id = SAI_PORT_ATTR_FABRIC_ATTACHED; status = sai_port_api->get_port_attribute(port, 1, &attr); diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 3861e823c9..03c854fee3 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -42,8 +42,8 @@ FdbOrch::FdbOrch(DBConnector* applDbConnector, vector app Orch::addExecutor(flushNotifier); /* Add FDB notifications support from ASIC */ - DBConnector *notificationsDb = new DBConnector("ASIC_DB", 0); - m_fdbNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); + m_notificationsDb = make_shared("ASIC_DB", 0); + m_fdbNotificationConsumer = new swss::NotificationConsumer(m_notificationsDb.get(), "NOTIFICATIONS"); auto fdbNotifier = new Notifier(m_fdbNotificationConsumer, this, "FDB_NOTIFICATIONS"); Orch::addExecutor(fdbNotifier); } diff --git a/orchagent/fdborch.h b/orchagent/fdborch.h index 09bc6dcc69..9e71bc8c6b 100644 --- a/orchagent/fdborch.h +++ b/orchagent/fdborch.h @@ -113,6 +113,7 @@ class FdbOrch: public Orch, public Subject, public Observer Table m_mclagFdbStateTable; NotificationConsumer* m_flushNotificationsConsumer; NotificationConsumer* m_fdbNotificationConsumer; + shared_ptr m_notificationsDb; void doTask(Consumer& consumer); void doTask(NotificationConsumer& consumer); diff --git a/orchagent/mirrororch.cpp b/orchagent/mirrororch.cpp index 5647d48879..d2981330e4 100644 --- a/orchagent/mirrororch.cpp +++ b/orchagent/mirrororch.cpp @@ -329,7 +329,7 @@ bool MirrorOrch::validateSrcPortList(const string& srcPortList) vector portv; int portCount = 0; m_portsOrch->getLagMember(port, portv); - for (const auto p : portv) + for (const auto &p : portv) { if (checkPortExistsInSrcPortList(p.m_alias, srcPortList)) { @@ -828,7 +828,7 @@ bool MirrorOrch::setUnsetPortMirror(Port port, { vector portv; m_portsOrch->getLagMember(port, portv); - for (const auto p : portv) + for (const auto &p : portv) { if (p.m_type != Port::PHY) { diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index abfa9d5ee9..9f9e22339f 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -628,14 +628,14 @@ PortsOrch::PortsOrch(DBConnector *db, DBConnector *stateDb, vector("ASIC_DB", 0); + m_portStatusNotificationConsumer = new swss::NotificationConsumer(m_notificationsDb.get(), "NOTIFICATIONS"); auto portStatusNotificatier = new Notifier(m_portStatusNotificationConsumer, this, "PORT_STATUS_NOTIFICATIONS"); Orch::addExecutor(portStatusNotificatier); if (m_cmisModuleAsicSyncSupported) { - m_portHostTxReadyNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS"); + m_portHostTxReadyNotificationConsumer = new swss::NotificationConsumer(m_notificationsDb.get(), "NOTIFICATIONS"); auto portHostTxReadyNotificatier = new Notifier(m_portHostTxReadyNotificationConsumer, this, "PORT_HOST_TX_NOTIFICATIONS"); Orch::addExecutor(portHostTxReadyNotificatier); } @@ -2335,7 +2335,7 @@ bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) return false; } - for (const auto p: portv) + for (const auto &p: portv) { sai_attribute_t attr; attr.id = SAI_HOSTIF_ATTR_VLAN_TAG; diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index cc71c056b4..4d069ccfc5 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -263,6 +263,7 @@ class PortsOrch : public Orch, public Subject shared_ptr m_counter_db; shared_ptr m_flex_db; shared_ptr m_state_db; + shared_ptr m_notificationsDb; FlexCounterManager port_stat_manager; FlexCounterManager port_buffer_drop_stat_manager; diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp index 8a05e9188e..8005199935 100644 --- a/tests/mock_tests/aclorch_ut.cpp +++ b/tests/mock_tests/aclorch_ut.cpp @@ -459,6 +459,8 @@ namespace aclorch_test gMirrorOrch = nullptr; delete gRouteOrch; gRouteOrch = nullptr; + delete gFlowCounterRouteOrch; + gFlowCounterRouteOrch = nullptr; delete gSrv6Orch; gSrv6Orch = nullptr; delete gNeighOrch; From 77d56e6e16c0136a34f792ca2aa37de79cdd4b0d Mon Sep 17 00:00:00 2001 From: saksarav-nokia Date: Tue, 6 Feb 2024 12:44:59 -0500 Subject: [PATCH 3/8] Fix the Orchagent crash seen during Port channel OC test cases. (#3042) The function addNeighbor adds the remote system neighbor against the remote system port and increment the reference count for remote system port's RIF. However when it adds the nextHop in addNextHop function , it adds it against Inband port with RIF-ID of remote system port, but increases the RIF reference count of Inband port instead of remote system port.When the neighbor is removed in removeNeighbor, it decreases the ref count of remote system port for RIF. But when it removes the nexthop in removeNextHop, it decreases the ref count for remote system port. So if the remote system port has both ipv4 and ipv6 configured, then the ref count is incremented by 2 for remote system port's RIF (ipv4 and ipv4 nbr) and incremented by 2 (ipv4 and ipv6 nexthop) for Inband Port's RIF. But the ref count is decremented 4 times for remote system port's RIF. So sometimes, as soon as the ipv4 or ipv6 is delted, the orchagent tries to delete the remote system port's RIF, but since SAI meta layer has different ref count, it returns failure and orchagent crashes. Signed-off-by: saksarav --- orchagent/neighorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp index 47bcca3c32..efbe7ff481 100644 --- a/orchagent/neighorch.cpp +++ b/orchagent/neighorch.cpp @@ -298,7 +298,7 @@ bool NeighOrch::addNextHop(const NextHopKey &nh) next_hop_entry.nh_flags = 0; m_syncdNextHops[nexthop] = next_hop_entry; - m_intfsOrch->increaseRouterIntfsRefCount(nexthop.alias); + m_intfsOrch->increaseRouterIntfsRefCount(nh.alias); if (nexthop.isMplsNextHop()) { From 5fd896f69ba996439ecef9442d17e8b99bfa37ae Mon Sep 17 00:00:00 2001 From: vdahiya12 <67608553+vdahiya12@users.noreply.github.com> Date: Tue, 6 Feb 2024 17:36:45 -0800 Subject: [PATCH 4/8] [PortOrch] Add FEC codeword errors in port stats (#3029) [PortOrch] Add FEC codeword errors in port stats (#3029) --- orchagent/portsorch.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 9f9e22339f..0ce38850d0 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -215,7 +215,23 @@ const vector port_stat_ids = SAI_PORT_STAT_IP_IN_RECEIVES, SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES, SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES, - SAI_PORT_STAT_IF_IN_FEC_SYMBOL_ERRORS + SAI_PORT_STAT_IF_IN_FEC_SYMBOL_ERRORS, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S0, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S1, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S2, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S3, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S4, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S5, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S6, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S7, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S8, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S9, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S10, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S11, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S12, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S13, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S14, + SAI_PORT_STAT_IF_IN_FEC_CODEWORD_ERRORS_S15 }; const vector gbport_stat_ids = From b18cbac601bac8d19a575c97409de7070055e91c Mon Sep 17 00:00:00 2001 From: xumia <59720581+xumia@users.noreply.github.com> Date: Wed, 7 Feb 2024 09:42:21 +0800 Subject: [PATCH 5/8] [Ci] Fix the test script naming issue (#3021) * [Ci] Fix the test failure not detected issue * [Ci] Fix the test script naming issue (#3021) --- .azure-pipelines/test-docker-sonic-vs-template.yml | 3 ++- tests/{run-tests.py => run-tests.sh} | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) rename tests/{run-tests.py => run-tests.sh} (92%) diff --git a/.azure-pipelines/test-docker-sonic-vs-template.yml b/.azure-pipelines/test-docker-sonic-vs-template.yml index b7443bbd15..dbddd64077 100644 --- a/.azure-pipelines/test-docker-sonic-vs-template.yml +++ b/.azure-pipelines/test-docker-sonic-vs-template.yml @@ -48,6 +48,7 @@ jobs: variables: DIFF_COVER_CHECK_THRESHOLD: 80 DIFF_COVER_ENABLE: 'true' + DIFF_COVER_COVERAGE_FILES: Cobertura.xml pool: sonic-common-test @@ -142,7 +143,7 @@ jobs: # Run the tests in parallel and retry retry=3 IMAGE_NAME=docker-sonic-vs:$(Build.DefinitionName).$(Build.BuildNumber).asan-${{ parameters.asan }} - echo $all_tests | xargs -n 1 | xargs -P 8 -I TEST_MODULE sudo ./run-tests.py "$IMAGE_NAME" "$params" "TEST_MODULE" 3 + echo $all_tests | xargs -n 1 | xargs -P 8 -I TEST_MODULE sudo ./run-tests.sh "$IMAGE_NAME" "$params" "TEST_MODULE" 3 rm -rf $(Build.ArtifactStagingDirectory)/download displayName: "Run vs tests" diff --git a/tests/run-tests.py b/tests/run-tests.sh similarity index 92% rename from tests/run-tests.py rename to tests/run-tests.sh index a30fb1e1a8..b9cdadf783 100755 --- a/tests/run-tests.py +++ b/tests/run-tests.sh @@ -4,7 +4,7 @@ PY_TEST_PARAMS="$2" TESTS="$3" RETRY=$4 -[ -z "RETRY" ] && RETRY=1 +[ -z "$RETRY" ] && RETRY=1 JUNITXML=$(echo "$TESTS" | cut -d "." -f1)_tr.xml set -x From 3bd0144485751f5820e36220b84305d739ee74de Mon Sep 17 00:00:00 2001 From: siqbal1986 Date: Wed, 7 Feb 2024 13:24:37 -0800 Subject: [PATCH 6/8] Bfd support for TSA state. (#2926) * Bfd session bringdown when entering TSA state. * VS tests to verify the logic. --- orchagent/bfdorch.cpp | 187 +++++++++++++++++++++++++++++++++++++-- orchagent/bfdorch.h | 16 ++++ orchagent/orchdaemon.cpp | 10 ++- orchagent/vnetorch.cpp | 4 + tests/test_bfd.py | 142 +++++++++++++++++++++++++++++ tests/test_vnet.py | 93 +++++++++++++++++++ 6 files changed, 443 insertions(+), 9 deletions(-) diff --git a/orchagent/bfdorch.cpp b/orchagent/bfdorch.cpp index 25c6c20cf2..6c435cdddb 100644 --- a/orchagent/bfdorch.cpp +++ b/orchagent/bfdorch.cpp @@ -84,7 +84,12 @@ BfdOrch::~BfdOrch(void) void BfdOrch::doTask(Consumer &consumer) { SWSS_LOG_ENTER(); - + BgpGlobalStateOrch* bgp_global_state_orch = gDirectory.get(); + bool tsa_enabled = false; + if (bgp_global_state_orch) + { + tsa_enabled = bgp_global_state_orch->getTsaState(); + } auto it = consumer.m_toSync.begin(); while (it != consumer.m_toSync.end()) { @@ -96,18 +101,66 @@ void BfdOrch::doTask(Consumer &consumer) if (op == SET_COMMAND) { - if (!create_bfd_session(key, data)) + bool tsa_shutdown_enabled = false; + for (auto i : data) { - it++; - continue; + auto value = fvValue(i); + //shutdown_bfd_during_tsa parameter is used by the BFD session creator to ensure that the the + //specified session gets removed when the device goes into TSA state. + //if this parameter is not specified or set to false for a session, the + // corrosponding BFD session would be maintained even in TSA state. + if (fvField(i) == "shutdown_bfd_during_tsa" && value == "true" ) + { + tsa_shutdown_enabled = true; + break; + } + } + if (tsa_shutdown_enabled) + { + bfd_session_cache[key] = data; + if (!tsa_enabled) + { + if (!create_bfd_session(key, data)) + { + it++; + continue; + } + } + else + { + notify_session_state_down(key); + } + } + else + { + if (!create_bfd_session(key, data)) + { + it++; + continue; + } } } else if (op == DEL_COMMAND) { - if (!remove_bfd_session(key)) + if (bfd_session_cache.find(key) != bfd_session_cache.end() ) { - it++; - continue; + bfd_session_cache.erase(key); + if (!tsa_enabled) + { + if (!remove_bfd_session(key)) + { + it++; + continue; + } + } + } + else + { + if (!remove_bfd_session(key)) + { + it++; + continue; + } } } else @@ -298,6 +351,12 @@ bool BfdOrch::create_bfd_session(const string& key, const vector(value); } + else if (fvField(i) == "shutdown_bfd_during_tsa") + { + //since we are handling shutdown_bfd_during_tsa in the caller function, we need to ignore it here. + //failure to ignore this parameter would cause error log. + continue; + } else SWSS_LOG_ERROR("Unsupported BFD attribute %s\n", fvField(i).c_str()); } @@ -551,3 +610,117 @@ uint32_t BfdOrch::bfd_src_port(void) return (port++); } +void BfdOrch::notify_session_state_down(const string& key) +{ + SWSS_LOG_ENTER(); + size_t found_vrf = key.find(delimiter); + if (found_vrf == string::npos) + { + SWSS_LOG_ERROR("Failed to parse key %s, no vrf is given", key.c_str()); + return; + } + + size_t found_ifname = key.find(delimiter, found_vrf + 1); + if (found_ifname == string::npos) + { + SWSS_LOG_ERROR("Failed to parse key %s, no ifname is given", key.c_str()); + return; + } + string vrf_name = key.substr(0, found_vrf); + string alias = key.substr(found_vrf + 1, found_ifname - found_vrf - 1); + IpAddress peer_address(key.substr(found_ifname + 1)); + BfdUpdate update; + update.peer = get_state_db_key(vrf_name, alias, peer_address); + update.state = SAI_BFD_SESSION_STATE_DOWN; + notify(SUBJECT_TYPE_BFD_SESSION_STATE_CHANGE, static_cast(&update)); +} + +void BfdOrch::handleTsaStateChange(bool tsaState) +{ + SWSS_LOG_ENTER(); + for (auto it : bfd_session_cache) + { + if (tsaState == true) + { + if (bfd_session_map.find(it.first) != bfd_session_map.end()) + { + notify_session_state_down(it.first); + remove_bfd_session(it.first); + } + } + else + { + if (bfd_session_map.find(it.first) == bfd_session_map.end()) + { + create_bfd_session(it.first, it.second); + } + } + } +} + +BgpGlobalStateOrch::BgpGlobalStateOrch(DBConnector *db, string tableName): + Orch(db, tableName) +{ + SWSS_LOG_ENTER(); + tsa_enabled = false; +} + +BgpGlobalStateOrch::~BgpGlobalStateOrch(void) +{ + SWSS_LOG_ENTER(); +} + +bool BgpGlobalStateOrch::getTsaState() +{ + SWSS_LOG_ENTER(); + return tsa_enabled; +} +void BgpGlobalStateOrch::doTask(Consumer &consumer) +{ + SWSS_LOG_ENTER(); + + auto it = consumer.m_toSync.begin(); + while (it != consumer.m_toSync.end()) + { + KeyOpFieldsValuesTuple t = it->second; + + string key = kfvKey(t); + string op = kfvOp(t); + auto data = kfvFieldsValues(t); + + if (op == SET_COMMAND) + { + for (auto i : data) + { + auto value = fvValue(i); + auto type = fvField(i); + SWSS_LOG_INFO("SET on key %s, data T %s, V %s\n", key.c_str(), type.c_str(), value.c_str()); + if (type == "tsa_enabled") + { + bool state = true ? value == "true" : false; + if (tsa_enabled != state) + { + SWSS_LOG_NOTICE("BgpGlobalStateOrch TSA state Changed to %d from %d.\n", int(state), int(tsa_enabled)); + tsa_enabled = state; + + BfdOrch* bfd_orch = gDirectory.get(); + if (bfd_orch) + { + bfd_orch->handleTsaStateChange(state); + } + } + } + } + } + else if (op == DEL_COMMAND) + { + SWSS_LOG_ERROR("DEL on key %s is not expected.\n", key.c_str()); + } + else + { + SWSS_LOG_ERROR("Unknown operation type %s\n", op.c_str()); + } + it = consumer.m_toSync.erase(it); + } +} + diff --git a/orchagent/bfdorch.h b/orchagent/bfdorch.h index 4a0cb9edfb..31e0e4c930 100644 --- a/orchagent/bfdorch.h +++ b/orchagent/bfdorch.h @@ -17,6 +17,7 @@ class BfdOrch: public Orch, public Subject void doTask(swss::NotificationConsumer &consumer); BfdOrch(swss::DBConnector *db, std::string tableName, TableConnector stateDbBfdSessionTable); virtual ~BfdOrch(void); + void handleTsaStateChange(bool tsaState); private: bool create_bfd_session(const std::string& key, const std::vector& data); @@ -26,6 +27,7 @@ class BfdOrch: public Orch, public Subject uint32_t bfd_gen_id(void); uint32_t bfd_src_port(void); + void notify_session_state_down(const std::string& key); bool register_bfd_state_change_notification(void); void update_port_number(std::vector &attrs); sai_status_t retry_create_bfd_session(sai_object_id_t &bfd_session_id, vector attrs); @@ -37,6 +39,20 @@ class BfdOrch: public Orch, public Subject swss::NotificationConsumer* m_bfdStateNotificationConsumer; bool register_state_change_notif; + std::map> bfd_session_cache; + }; +class BgpGlobalStateOrch : public Orch +{ +public: + void doTask(Consumer &consumer); + BgpGlobalStateOrch(swss::DBConnector *db, std::string tableName); + virtual ~BgpGlobalStateOrch(void); + bool getTsaState(); + +private: + bool tsa_enabled; + +}; #endif /* SWSS_BFDORCH_H */ diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index 8861d49ca0..a2e7f86ad9 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -145,9 +145,15 @@ bool OrchDaemon::init() TableConnector stateDbFdb(m_stateDb, STATE_FDB_TABLE_NAME); TableConnector stateMclagDbFdb(m_stateDb, STATE_MCLAG_REMOTE_FDB_TABLE_NAME); gFdbOrch = new FdbOrch(m_applDb, app_fdb_tables, stateDbFdb, stateMclagDbFdb, gPortsOrch); + TableConnector stateDbBfdSessionTable(m_stateDb, STATE_BFD_SESSION_TABLE_NAME); - gBfdOrch = new BfdOrch(m_applDb, APP_BFD_SESSION_TABLE_NAME, stateDbBfdSessionTable); + BgpGlobalStateOrch* bgp_global_state_orch; + bgp_global_state_orch = new BgpGlobalStateOrch(m_configDb, CFG_BGP_DEVICE_GLOBAL_TABLE_NAME); + gDirectory.set(bgp_global_state_orch); + + gBfdOrch = new BfdOrch(m_applDb, APP_BFD_SESSION_TABLE_NAME, stateDbBfdSessionTable); + gDirectory.set(gBfdOrch); static const vector route_pattern_tables = { CFG_FLOW_COUNTER_ROUTE_PATTERN_TABLE_NAME, }; @@ -399,7 +405,7 @@ bool OrchDaemon::init() * when iterating ConsumerMap. This is ensured implicitly by the order of keys in ordered map. * For cases when Orch has to process tables in specific order, like PortsOrch during warm start, it has to override Orch::doTask() */ - m_orchList = { gSwitchOrch, gCrmOrch, gPortsOrch, gBufferOrch, gFlowCounterRouteOrch, gIntfsOrch, gNeighOrch, gNhgMapOrch, gNhgOrch, gCbfNhgOrch, gRouteOrch, gCoppOrch, gQosOrch, wm_orch, gPolicerOrch, tunnel_decap_orch, sflow_orch, gDebugCounterOrch, gMacsecOrch, gBfdOrch, gSrv6Orch, mux_orch, mux_cb_orch, gMonitorOrch}; + m_orchList = { gSwitchOrch, gCrmOrch, gPortsOrch, gBufferOrch, gFlowCounterRouteOrch, gIntfsOrch, gNeighOrch, gNhgMapOrch, gNhgOrch, gCbfNhgOrch, gRouteOrch, gCoppOrch, gQosOrch, wm_orch, gPolicerOrch, tunnel_decap_orch, sflow_orch, gDebugCounterOrch, gMacsecOrch, bgp_global_state_orch, gBfdOrch, gSrv6Orch, mux_orch, mux_cb_orch, gMonitorOrch}; bool initialize_dtel = false; if (platform == BFN_PLATFORM_SUBSTRING || platform == VS_PLATFORM_SUBSTRING) diff --git a/orchagent/vnetorch.cpp b/orchagent/vnetorch.cpp index 4b4e91b978..b976c728a7 100644 --- a/orchagent/vnetorch.cpp +++ b/orchagent/vnetorch.cpp @@ -1817,6 +1817,10 @@ void VNetRouteOrch::createBfdSession(const string& vnet, const NextHopKey& endpo FieldValueTuple fvTuple("local_addr", src_ip.to_string()); data.push_back(fvTuple); data.emplace_back("multihop", "true"); + // The BFD sessions established by the Vnet routes with monitoring need to be brought down + // when the device goes into TSA. The following parameter ensures that these session are + // brought down while transitioning to TSA and brought back up when transitioning to TSB. + data.emplace_back("shutdown_bfd_during_tsa", "true"); bfd_session_producer_.set(key, data); bfd_sessions_[monitor_addr].bfd_state = SAI_BFD_SESSION_STATE_DOWN; } diff --git a/tests/test_bfd.py b/tests/test_bfd.py index 5add329278..5cd18bbe05 100644 --- a/tests/test_bfd.py +++ b/tests/test_bfd.py @@ -9,6 +9,7 @@ def setup_db(self, dvs): self.pdb = dvs.get_app_db() self.adb = dvs.get_asic_db() self.sdb = dvs.get_state_db() + self.cdb = dvs.get_config_db() def get_exist_bfd_session(self): return set(self.adb.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION")) @@ -43,6 +44,22 @@ def update_bfd_session_state(self, dvs, session, state): ntf_data = "[{\"bfd_session_id\":\""+session+"\",\"session_state\":\""+bfd_sai_state[state]+"\"}]" ntf.send("bfd_session_state_change", ntf_data, fvp) + def update_bgp_global_dev_state(self, state): + tbl = swsscommon.Table(self.cdb.db_connection, "BGP_DEVICE_GLOBAL") + fvs = swsscommon.FieldValuePairs(list(state.items())) + key = "STATE" + tbl.set(key, fvs) + time.sleep(1) + + def set_tsa(self): + state = {"tsa_enabled": "true"} + self.update_bgp_global_dev_state(state) + + def clear_tsa(self): + state = {"tsa_enabled": "false"} + self.update_bgp_global_dev_state(state) + + def test_addRemoveBfdSession(self, dvs): self.setup_db(dvs) @@ -476,6 +493,131 @@ def test_multipleBfdSessions(self, dvs): self.remove_bfd_session(key4) self.adb.wait_for_deleted_entry("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", session4) + def test_addRemoveBfdSession_with_tsa_case1(self, dvs): + # This is a test for BFD caching mechanism. + # This test sets up a BFD session with shutdown_bfd_during_tsa=true and checks state DB for session creation. + # Then TSA is applied and removal of the session is verified in app db. This is followed by TSB and finally the + # reinstated session is verified. + self.setup_db(dvs) + + bfdSessions = self.get_exist_bfd_session() + + # Create BFD session + fieldValues = {"local_addr": "10.0.0.1", "type": "demand_active", "shutdown_bfd_during_tsa": "true"} + self.create_bfd_session("default:default:10.0.0.2", fieldValues) + self.adb.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", len(bfdSessions) + 1) + + # Checked created BFD session in ASIC_DB + createdSessions = self.get_exist_bfd_session() - bfdSessions + assert len(createdSessions) == 1 + + session = createdSessions.pop() + expected_adb_values = { + "SAI_BFD_SESSION_ATTR_SRC_IP_ADDRESS": "10.0.0.1", + "SAI_BFD_SESSION_ATTR_DST_IP_ADDRESS": "10.0.0.2", + "SAI_BFD_SESSION_ATTR_TYPE": "SAI_BFD_SESSION_TYPE_DEMAND_ACTIVE", + "SAI_BFD_SESSION_ATTR_TOS": "192", + "SAI_BFD_SESSION_ATTR_IPHDR_VERSION": "4" + } + self.check_asic_bfd_session_value(session, expected_adb_values) + + # Check STATE_DB entry related to the BFD session + expected_sdb_values = {"state": "Down", "type": "demand_active", "local_addr" : "10.0.0.1", "tx_interval" :"1000", + "rx_interval" : "1000", "multiplier" : "10", "multihop": "false", "local_discriminator" : "12"} + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + # Send BFD session state notification to update BFD session state + self.update_bfd_session_state(dvs, session, "Up") + time.sleep(2) + bfdSessions = self.get_exist_bfd_session() + # Confirm BFD session state in STATE_DB is updated as expected. + expected_sdb_values["state"] = "Up" + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + #set TSA + self.set_tsa() + time.sleep(2) + + #ensure the session is removed. + self.adb.wait_for_deleted_entry("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", session) + + #set TSB + self.clear_tsa() + time.sleep(2) + createdSessions = self.get_exist_bfd_session() - bfdSessions + session = createdSessions.pop() + expected_sdb_values["local_discriminator"] = "13" + self.update_bfd_session_state(dvs, session, "Up") + time.sleep(2) + # bfd session should come back + expected_sdb_values["state"] = "Up" + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + # Remove the BFD session + + self.remove_bfd_session("default:default:10.0.0.2") + self.adb.wait_for_deleted_entry("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", session) + + + def test_addRemoveBfdSession_with_tsa_case2(self, dvs): + # This is a test for BFD caching mechanism. + # This test sets up a BFD session with shutdown_bfd_during_tsa=true and checks state DB for session creation. + # Then TSA is applied and removal of the session is verified from app db. At this point the session is removed. + # This isfollowed by TSB. Since the session configuration has been removed during TSB, the BFD session should not + # start up. + self.setup_db(dvs) + + bfdSessions = self.get_exist_bfd_session() + + # Create BFD session + fieldValues = {"local_addr": "10.0.0.1", "type": "demand_active"} + self.create_bfd_session("default:default:10.0.0.2", fieldValues) + self.adb.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", len(bfdSessions) + 1) + + # Checked created BFD session in ASIC_DB + createdSessions = self.get_exist_bfd_session() - bfdSessions + assert len(createdSessions) == 1 + + session = createdSessions.pop() + expected_adb_values = { + "SAI_BFD_SESSION_ATTR_SRC_IP_ADDRESS": "10.0.0.1", + "SAI_BFD_SESSION_ATTR_DST_IP_ADDRESS": "10.0.0.2", + "SAI_BFD_SESSION_ATTR_TYPE": "SAI_BFD_SESSION_TYPE_DEMAND_ACTIVE", + "SAI_BFD_SESSION_ATTR_TOS": "192", + "SAI_BFD_SESSION_ATTR_IPHDR_VERSION": "4" + } + self.check_asic_bfd_session_value(session, expected_adb_values) + + # Check STATE_DB entry related to the BFD session + expected_sdb_values = {"state": "Down", "type": "demand_active", "local_addr" : "10.0.0.1", "tx_interval" :"1000", + "rx_interval" : "1000", "multiplier" : "10", "multihop": "false", "local_discriminator" : "14"} + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + # Send BFD session state notification to update BFD session state + self.update_bfd_session_state(dvs, session, "Up") + time.sleep(2) + # Confirm BFD session state in STATE_DB is updated as expected. + expected_sdb_values["state"] = "Up" + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + #set TSA + self.set_tsa() + time.sleep(2) + + #ensure the session is still present. + self.adb.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", len(bfdSessions) + 1) + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + #set TSB + self.clear_tsa() + time.sleep(2) + + self.check_state_bfd_session_value("default|default|10.0.0.2", expected_sdb_values) + + # Remove the BFD session + self.remove_bfd_session("default:default:10.0.0.2") + self.adb.wait_for_deleted_entry("ASIC_STATE:SAI_OBJECT_TYPE_BFD_SESSION", session) + def test_bfd_state_db_clear(self, dvs): self.setup_db(dvs) diff --git a/tests/test_vnet.py b/tests/test_vnet.py index 3b1ef6efd9..4873c072ed 100644 --- a/tests/test_vnet.py +++ b/tests/test_vnet.py @@ -545,6 +545,21 @@ def check_syslog(dvs, marker, err_log): def_vr_id = 0 switch_mac = None +def update_bgp_global_dev_state(dvs, state): + config_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0) + create_entry_tbl( + config_db, + "BGP_DEVICE_GLOBAL",'|',"STATE", + [ + ("tsa_enabled", state), + ] + ) + +def set_tsa(dvs): + update_bgp_global_dev_state(dvs, "true") + +def clear_tsa(dvs): + update_bgp_global_dev_state(dvs, "false") class VnetVxlanVrfTunnel(object): @@ -3428,6 +3443,84 @@ def test_vnet_orch_24(self, dvs, testlog): # delete vxlan tunnel delete_vxlan_tunnel(dvs, tunnel_name) + ''' + Test 25 - Test for BFD TSA and TSB behaviour within overlay tunnel routes. + ''' + def test_vnet_orch_25(self, dvs, testlog): + # This test creates a vnet route with BFD monitoring.This followd by application of TSA and absence of BFD sessions + # is verified. Following the removal of TSA the Vnet route is verified to be up. + vnet_obj = self.get_vnet_obj() + tunnel_name = 'tunnel_25' + + vnet_obj.fetch_exist_entries(dvs) + + create_vxlan_tunnel(dvs, tunnel_name, '9.9.9.9') + create_vnet_entry(dvs, 'Vnet25', tunnel_name, '10025', "") + + vnet_obj.check_vnet_entry(dvs, 'Vnet25') + vnet_obj.check_vxlan_tunnel_entry(dvs, tunnel_name, 'Vnet25', '10025') + + vnet_obj.check_vxlan_tunnel(dvs, tunnel_name, '9.9.9.9') + + vnet_obj.fetch_exist_entries(dvs) + create_vnet_routes(dvs, "125.100.1.1/32", 'Vnet25', '9.0.0.1,9.0.0.2,9.0.0.3', ep_monitor='9.1.0.1,9.1.0.2,9.1.0.3') + + # default bfd status is down, route should not be programmed in this status + vnet_obj.check_del_vnet_routes(dvs, 'Vnet25', ["125.100.1.1/32"]) + check_state_db_routes(dvs, 'Vnet25', "125.100.1.1/32", []) + check_remove_routes_advertisement(dvs, "125.100.1.1/32") + + # Route should be properly configured when all bfd session states go up + update_bfd_session_state(dvs, '9.1.0.1', 'Up') + update_bfd_session_state(dvs, '9.1.0.2', 'Up') + update_bfd_session_state(dvs, '9.1.0.3', 'Up') + time.sleep(2) + + # make sure the route is up. + route1, nhg1_1 = vnet_obj.check_vnet_ecmp_routes(dvs, 'Vnet25', ['9.0.0.1', '9.0.0.2', '9.0.0.3'], tunnel_name) + check_state_db_routes(dvs, 'Vnet25', "125.100.1.1/32", ['9.0.0.1', '9.0.0.2', '9.0.0.3']) + # The default Vnet setting does not advertise prefix + check_remove_routes_advertisement(dvs, "125.100.1.1/32") + + # tsa would remove all bfd sessions down. + set_tsa(dvs) + time.sleep(2) + + # Route should be removed. + vnet_obj.check_del_vnet_routes(dvs, 'Vnet25', ["125.100.1.1/32"]) + check_state_db_routes(dvs, 'Vnet25', "125.100.1.1/32", []) + check_remove_routes_advertisement(dvs, "125.100.1.1/32") + + #clearing TSA should bring the route back. + clear_tsa(dvs) + time.sleep(2) + + update_bfd_session_state(dvs, '9.1.0.1', 'Up') + update_bfd_session_state(dvs, '9.1.0.2', 'Up') + update_bfd_session_state(dvs, '9.1.0.3', 'Up') + time.sleep(2) + + route1, nhg1_1 = vnet_obj.check_vnet_ecmp_routes(dvs, 'Vnet25', ['9.0.0.1', '9.0.0.2', '9.0.0.3'], tunnel_name, route_ids=route1, nhg=nhg1_1) + check_state_db_routes(dvs, 'Vnet25', "125.100.1.1/32", ['9.0.0.1', '9.0.0.2', '9.0.0.3']) + # The default Vnet setting does not advertise prefix + check_remove_routes_advertisement(dvs, "125.100.1.1/32") + + # Remove tunnel route + delete_vnet_routes(dvs, "125.100.1.1/32", 'Vnet25') + vnet_obj.check_del_vnet_routes(dvs, 'Vnet25', ["125.100.1.1/32"]) + check_remove_state_db_routes(dvs, 'Vnet25', "125.100.1.1/32") + check_remove_routes_advertisement(dvs, "125.100.1.1/32") + + # Check the corresponding nexthop group is removed + vnet_obj.fetch_exist_entries(dvs) + assert nhg1_1 not in vnet_obj.nhgs + # Check the BFD session specific to the endpoint group is removed while others exist + check_del_bfd_session(dvs, ['9.1.0.1', '9.1.0.2', '9.1.0.3']) + + delete_vnet_entry(dvs, 'Vnet25') + vnet_obj.check_del_vnet_entry(dvs, 'Vnet25') + delete_vxlan_tunnel(dvs, tunnel_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 def test_nonflaky_dummy(): From b3b6a8384aa2cfc2e6cb1c08312af1cbeac5c0c5 Mon Sep 17 00:00:00 2001 From: Nikola Dancejic <26731235+Ndancejic@users.noreply.github.com> Date: Wed, 7 Feb 2024 18:40:23 -0800 Subject: [PATCH 7/8] [test_mux] Multi-mux-nh full test coverage (#3028) * [test_mux] Multi-mux-nh full test coverage --- tests/test_mux.py | 104 +++++++++++++++++++++++++++++----------------- 1 file changed, 67 insertions(+), 37 deletions(-) diff --git a/tests/test_mux.py b/tests/test_mux.py index 462cfb00f4..207ec6741b 100644 --- a/tests/test_mux.py +++ b/tests/test_mux.py @@ -518,22 +518,26 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route): " " + self.SERV1_IPV4 + "\"" ) - def multi_nexthop_check(self, asicdb, dvs_route, route, nexthops, mux_states): + def multi_nexthop_check(self, asicdb, dvs_route, route, nexthops, mux_states, non_mux_nexthop = None): if isinstance(route, list): route_copy = route.copy() else: route_copy = [route] + for r in route_copy: + if non_mux_nexthop != None: + self.check_route_nexthop(dvs_route, asicdb, r, non_mux_nexthop) + continue for i,state in enumerate(mux_states): # Find first active mux port, and check that route points to that neighbor if state == ACTIVE: self.check_route_nexthop(dvs_route, asicdb, r, nexthops[i]) - return + break else: # If no active mux port, check that route points to tunnel self.check_route_nexthop(dvs_route, asicdb, r, tunnel_nh_id, True) - def multi_nexthop_test_create(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops): + def multi_nexthop_test_create(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops, non_mux_nexthop = None): ''' Tests the creation of a route with multiple nexthops in various combinations of initial mux state ''' @@ -547,8 +551,11 @@ def multi_nexthop_test_create(self, appdb, asicdb, dvs, dvs_route, route, mux_po self.set_mux_state(appdb, port, states[i]) # Add route - self.add_route(dvs, route, nexthops) - self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, states) + if non_mux_nexthop != None: + self.add_route(dvs, route, nexthops + [non_mux_nexthop]) + else: + self.add_route(dvs, route, nexthops) + self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, states, non_mux_nexthop) self.del_route(dvs, route) @@ -575,7 +582,7 @@ def multi_nexthop_test_fdb(self, appdb, asicdb, dvs, dvs_route, route, mux_ports # Reset fdb self.add_neighbor(dvs, nexthop, macs[i]) - def multi_nexthop_test_toggle(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops): + def multi_nexthop_test_toggle(self, appdb, asicdb, dvs_route, route, mux_ports, nexthops, non_mux_nexthop=None): ''' Tests toggling mux state for a route with multiple nexthops ''' @@ -585,41 +592,44 @@ def multi_nexthop_test_toggle(self, appdb, asicdb, dvs, dvs_route, route, mux_po for states in init_mux_states: print("Testing state change in states: %s, for nexthops: %s" % (str(states), str(nexthops))) for i,port in enumerate(mux_ports): + if nexthops[i] == non_mux_nexthop: + continue self.set_mux_state(appdb, port, states[i]) for toggle_index,toggle_port in enumerate(mux_ports): + if nexthops[toggle_index] == non_mux_nexthop: + continue new_states = states.copy() print("Toggling %s from %s" % (toggle_port, states[toggle_index])) if states[toggle_index] == ACTIVE: - print("setting %s to %s" % (toggle_port, STANDBY)) new_states[toggle_index] = STANDBY self.set_mux_state(appdb, toggle_port, STANDBY) - self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states) + self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states, non_mux_nexthop) - print("setting %s to %s" % (toggle_port, ACTIVE)) new_states[toggle_index] = ACTIVE self.set_mux_state(appdb, toggle_port, ACTIVE) - self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states) + self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states, non_mux_nexthop) else: - print("setting %s to %s" % (toggle_port, ACTIVE)) new_states[toggle_index] = ACTIVE self.set_mux_state(appdb, toggle_port, ACTIVE) - self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states) + self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states, non_mux_nexthop) - print("setting %s to %s" % (toggle_port, STANDBY)) new_states[toggle_index] = STANDBY self.set_mux_state(appdb, toggle_port, STANDBY) - self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states) + self.multi_nexthop_check(asicdb, dvs_route, route, nexthops, new_states, non_mux_nexthop) # Set everything back to active for i,port in enumerate(mux_ports): - self.set_mux_state(appdb, port, ACTIVE) + if nexthops[i] == non_mux_nexthop: + continue + self.set_mux_state(appdb, port, ACTIVE) - def multi_nexthop_test_route_update_keep_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops, new_nexthop, new_mux_port): + def multi_nexthop_test_route_update_keep_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops, new_nexthop, new_mux_port, nh_is_mux=True): ''' Tests route update for a route with multiple nexthops with same number of nexthops + - nh_is_mux: is True if new nexthop is a mux nexthop, False if not ''' # Add route self.add_route(dvs, route, nexthops) @@ -629,22 +639,28 @@ def multi_nexthop_test_route_update_keep_size(self, appdb, asicdb, dvs, dvs_rout new_nexthops = nexthops.copy() new_muxports = mux_ports.copy() - print("Triggering route update %s to replace: %s" % (str(new_nexthops), str(nexthop))) + print("Triggering route update %s to replace: %s with: %s" % (str(new_nexthops), str(nexthop), str(new_nexthop))) new_nexthops[i] = new_nexthop new_muxports[i] = new_mux_port - new_nexthops.sort() - new_muxports.sort() + if nh_is_mux: + # We need to sort the nexthops to match the way they will pe processed + new_nexthops.sort() + new_muxports.sort() self.add_route(dvs, route, new_nexthops) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, route, new_muxports, new_nexthops) + + if nh_is_mux: + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, new_muxports, new_nexthops) + else: + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, new_muxports, new_nexthops, non_mux_nexthop=new_nexthop) # Reset route self.add_route(dvs, route, nexthops) self.del_route(dvs, route) - def multi_nexthop_test_route_update_increase_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops): + def multi_nexthop_test_route_update_increase_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops, non_mux_nexthop=None): ''' Tests route update for a route with multiple nexthops increasing number of nexthops over time ''' @@ -652,21 +668,31 @@ def multi_nexthop_test_route_update_increase_size(self, appdb, asicdb, dvs, dvs_ for i,nexthop in enumerate(nexthops): print("Triggering route update to add: %s. new route %s -> %s" % (str(nexthop), route, nexthops[:i+1])) self.add_route(dvs, route, nexthops[:i+1]) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, route, mux_ports[:i+1], nexthops[:i+1]) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, mux_ports[:i+1], nexthops[:i+1]) + + # Add non_mux_nexthop to route list + if non_mux_nexthop != None: + print("Triggering route update to add non_mux: %s. new route %s -> %s" % (str(non_mux_nexthop), route, nexthops + [non_mux_nexthop])) + self.add_route(dvs, route, nexthops + [non_mux_nexthop]) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, mux_ports + [None], nexthops + [non_mux_nexthop], non_mux_nexthop=non_mux_nexthop) self.del_route(dvs, route) - def multi_nexthop_test_route_update_decrease_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops): + def multi_nexthop_test_route_update_decrease_size(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops, non_mux_nexthop=None): ''' Tests route update for a route with multiple nexthops increasing number of nexthops over time ''' print("Test route update for route with multiple mux nexthops") + + if non_mux_nexthop != None: + print("Triggering route update to add non_mux: %s. new route %s -> %s" % (str(non_mux_nexthop), route, [non_mux_nexthop] + nexthops)) + self.add_route(dvs, route, [non_mux_nexthop] + nexthops) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, [None] + mux_ports, [non_mux_nexthop] + nexthops, non_mux_nexthop=non_mux_nexthop) + for i,nexthop in enumerate(nexthops): - if i == len(nexthops)-1: - break print("Triggering route update to remove: %s. new route %s -> %s" % (str(nexthop), route, nexthops[i:])) self.add_route(dvs, route, nexthops[i:]) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, mux_ports[i:], nexthops[i:]) self.del_route(dvs, route) @@ -678,7 +704,7 @@ def multi_nexthop_test_neighbor_add(self, appdb, asicdb, dvs, dvs_route, route, for i,nexthop in enumerate(nexthops): print("Triggering neighbor add for %s" % (nexthop)) self.add_neighbor(dvs, nexthop, macs[i]) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, mux_ports, nexthops) def multi_nexthop_test_neighbor_del(self, appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops): ''' @@ -688,7 +714,7 @@ def multi_nexthop_test_neighbor_del(self, appdb, asicdb, dvs, dvs_route, route, for nexthop in nexthops: print("Triggering neighbor del for %s" % (nexthop)) self.add_neighbor(dvs, nexthop, "00:00:00:00:00:00") - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, route, mux_ports, nexthops) + self.multi_nexthop_test_toggle(appdb, asicdb, dvs_route, route, mux_ports, nexthops) def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_mac, asicdb): ''' @@ -705,8 +731,8 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_ ipv6_nexthops = [self.SERV1_IPV6, self.SERV2_IPV6] new_ipv4_nexthop = self.SERV3_IPV4 new_ipv6_nexthop = self.SERV3_IPV6 - non_mux_ipv4 = ["11.11.11.11", "12.12.12.12"] - non_mux_ipv6 = ["2222::100", "2222::101"] + non_mux_ipv4 = "11.11.11.11" + non_mux_ipv6 = "2222::100" non_mux_mac = "00:aa:aa:aa:aa:aa" mux_ports = ["Ethernet0", "Ethernet4"] new_mux_port = "Ethernet8" @@ -717,6 +743,8 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_ self.add_neighbor(dvs, new_ipv4_nexthop, new_mac) self.add_neighbor(dvs, new_ipv6_nexthop, new_mac) + self.add_neighbor(dvs, non_mux_ipv4, non_mux_mac) + self.add_neighbor(dvs, non_mux_ipv6, non_mux_mac) for port in mux_ports: self.set_mux_state(appdb, port, ACTIVE) @@ -726,12 +754,16 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_ # These tests create route: self.multi_nexthop_test_create(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops) self.multi_nexthop_test_create(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops) + self.multi_nexthop_test_create(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, non_mux_ipv4) + self.multi_nexthop_test_create(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, non_mux_ipv6) self.multi_nexthop_test_route_update_keep_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, new_ipv4_nexthop, new_mux_port) self.multi_nexthop_test_route_update_keep_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, new_ipv6_nexthop, new_mux_port) - self.multi_nexthop_test_route_update_increase_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops) - self.multi_nexthop_test_route_update_increase_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops) - self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops) - self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops) + self.multi_nexthop_test_route_update_keep_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, non_mux_ipv4, None, nh_is_mux=False) + self.multi_nexthop_test_route_update_keep_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, non_mux_ipv6, None, nh_is_mux=False) + self.multi_nexthop_test_route_update_increase_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, non_mux_nexthop=non_mux_ipv4) + self.multi_nexthop_test_route_update_increase_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, non_mux_nexthop=non_mux_ipv6) + self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv4, mux_ports, ipv4_nexthops, non_mux_nexthop=non_mux_ipv4) + self.multi_nexthop_test_route_update_decrease_size(appdb, asicdb, dvs, dvs_route, route_ipv6, mux_ports, ipv6_nexthops, non_mux_nexthop=non_mux_ipv6) # # These tests do not create route, so create beforehand: self.add_route(dvs, route_ipv4, ipv4_nexthops) @@ -741,8 +773,6 @@ def create_and_test_multi_nexthop_routes(self, dvs, dvs_route, appdb, macs, new_ self.multi_nexthop_test_fdb(appdb, asicdb, dvs, dvs_route, [route_ipv4, route_B_ipv4], mux_ports, ipv4_nexthops, macs) self.multi_nexthop_test_fdb(appdb, asicdb, dvs, dvs_route, [route_ipv6, route_B_ipv6], mux_ports, ipv6_nexthops, macs) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, [route_ipv4, route_B_ipv4], mux_ports, ipv4_nexthops) - self.multi_nexthop_test_toggle(appdb, asicdb, dvs, dvs_route, [route_ipv6, route_B_ipv6], mux_ports, ipv6_nexthops) self.multi_nexthop_test_neighbor_add(appdb, asicdb, dvs, dvs_route, [route_ipv4, route_B_ipv4], mux_ports, ipv4_nexthops, macs) self.multi_nexthop_test_neighbor_add(appdb, asicdb, dvs, dvs_route, [route_ipv6, route_B_ipv6], mux_ports, ipv6_nexthops, macs) self.multi_nexthop_test_neighbor_del(appdb, asicdb, dvs, dvs_route, [route_ipv4, route_B_ipv4], mux_ports, ipv4_nexthops) @@ -1408,7 +1438,7 @@ def test_NH(self, dvs, dvs_route, intf_fdb_map, setup, setup_mux_cable, self.create_and_test_NH_routes(appdb, asicdb, dvs, dvs_route, mac) - def test_multi_nexthop(self, dvs, dvs_route, intf_fdb_map, neighbor_cleanup, testlog): + def test_multi_nexthop(self, dvs, dvs_route, intf_fdb_map, neighbor_cleanup, testlog, setup): appdb = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0) asicdb = dvs.get_asic_db() macs = [intf_fdb_map["Ethernet0"], intf_fdb_map["Ethernet4"]] From 1221eae41788176b5de0db3e869006ff22d2d4ff Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Tue, 13 Feb 2024 17:20:54 -0800 Subject: [PATCH 8/8] Explicitly initialize two local variables to 0 (#3046) GCC 12 (in Debian Bookworm) claims that on armhf targets, the `lower` local variable in `Orch::generateIdListFromMap` could potentially be used uninitialized a few lines later when it's being converted to a string. I don't see how that could be the case here, since if the string is being printed, the only code path where this is possible requires `lower` to have been set to something. To silence the warning, set both `lower` and `upper` to 0 explicitly. --- orchagent/orch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 1e33d7c5eb..d1cbdb89c8 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -697,7 +697,7 @@ set Orch::generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId { unsigned long currentIdMask = 1; bool started = false, needGenerateMap = false; - sai_uint32_t lower, upper; + sai_uint32_t lower = 0, upper = 0; set idStringList; for (sai_uint32_t id = 0; id <= maxId; id ++) {