diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 9eb23ca1..c7f7700f 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1844,7 +1844,7 @@ class PortPhyAttrContext : public AttrContextid : %d", rid, attr->id); - return; + return false; } const auto &attrLaneCountMap = outer_it->second; @@ -1871,7 +1871,7 @@ class PortPhyAttrContext : public AttrContextid, rid); - return; + return false; } auto portLaneCount = inner_it->second; @@ -1883,23 +1883,23 @@ class PortPhyAttrContext : public AttrContextrxSignalDetectData.resize(portLaneCount); attr->value.portlanelatchstatuslist.count = portLaneCount; attr->value.portlanelatchstatuslist.list = data->rxSignalDetectData.data(); - break; + return true; case SAI_PORT_ATTR_FEC_ALIGNMENT_LOCK: data->fecAlignmentLockData.resize(portLaneCount); attr->value.portlanelatchstatuslist.count = portLaneCount; attr->value.portlanelatchstatuslist.list = data->fecAlignmentLockData.data(); - break; + return true; case SAI_PORT_ATTR_RX_SNR: data->rxSnrData.resize(portLaneCount); attr->value.portsnrlist.count = portLaneCount; attr->value.portsnrlist.list = data->rxSnrData.data(); - break; + return true; default: SWSS_LOG_ERROR("PORT_PHY_ATTR: initAttrData: Unsupported attr-id : %d", attr->id); - break; + return false; } } @@ -1981,10 +1981,22 @@ class PortPhyAttrContext : public AttrContextremoveCounter(testPortOid); } + +/** + * Test that collectData() gracefully skips objects when initAttrData() fails. + * + * This simulates the Broadcom 7260CX3 scenario where SAI does not support + * SAI_PORT_ATTR_RX_SIGNAL_DETECT ΓÇö the lane count query returns NOT_SUPPORTED + * instead of BUFFER_OVERFLOW, so m_portLaneCountMap has no entry for the + * attribute, and initAttrData() returns false. + * + * Before the fix, collectData() would ignore the initAttrData() return value + * and call sai_get() anyway, which would fail and emit ERR syslog every 10s. + * After the fix, collectData() skips the object entirely. + * + * See: https://github.com/sonic-net/sonic-mgmt/issues/24023 + */ +TEST_F(TestPortPhyAttr, CollectDataSkipsWhenInitAttrDataFails) +{ + // Use a different port OID to avoid picking up stale Redis data + // from the previous test (CollectDataAndValidateCountersDB). + sai_object_id_t failPortOid = 0x1000000000099; + sai_object_id_t failPortRid = 0x1000000000099; + + int getCallCount = 0; + + // Mock SAI: return NOT_SUPPORTED for the lane count query + // This means m_portLaneCountMap will NOT have an entry for this RID/attr, + // so initAttrData() will return false when collectData() calls it. + sai->mock_get = [&getCallCount](sai_object_type_t object_type, + sai_object_id_t object_id, + uint32_t attr_count, + sai_attribute_t *attr_list) -> sai_status_t + { + if (object_type != SAI_OBJECT_TYPE_PORT) { + return SAI_STATUS_INVALID_PARAMETER; + } + + // Track calls after addObject's lane count query phase + getCallCount++; + + // Return NOT_SUPPORTED for the lane count query (attr_count == 1) + // This simulates Broadcom SAI on 7260CX3 not supporting RX_SIGNAL_DETECT + if (attr_count == 1) { + return SAI_STATUS_NOT_SUPPORTED; + } + + // If collectData() reaches sai_get with multiple attrs, that's a bug + // The fix should prevent this from happening + return SAI_STATUS_FAILURE; + }; + + vector portPhyAttrValues; + std::string attrIds = "SAI_PORT_ATTR_RX_SIGNAL_DETECT,SAI_PORT_ATTR_FEC_ALIGNMENT_LOCK,SAI_PORT_ATTR_RX_SNR"; + portPhyAttrValues.emplace_back(PORT_PHY_ATTR_ID_LIST, attrIds); + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + + // addObject will call updatePortLaneCountMap which queries SAI for lane count. + // Since SAI returns NOT_SUPPORTED, m_portLaneCountMap will be empty. + flexCounter->addCounter(failPortOid, failPortRid, portPhyAttrValues); + + int getCallsAfterAdd = getCallCount; + + vector pluginValues; + pluginValues.emplace_back(POLL_INTERVAL_FIELD, "1000"); + pluginValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + pluginValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + flexCounter->addCounterPlugin(pluginValues); + + usleep(1000 * 1050); // 1.05 seconds - one poll cycle + + // Verify that collectData did NOT make additional sai_get calls + // beyond what addObject's updatePortLaneCountMap already made. + // If initAttrData fails, collectData should skip the object entirely. + EXPECT_EQ(getCallCount, getCallsAfterAdd) + << "collectData should not call sai_get when initAttrData fails. " + << "Additional sai_get calls indicate the initAttrData return value was ignored."; + + // Verify no data was written to COUNTERS_DB + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, PORT_PHY_ATTR_TABLE, false); + + std::string expectedKey = toOid(failPortOid); + + std::string rxSignalDetectValue; + bool found = countersTable.hget(expectedKey, "phy_rx_signal_detect", rxSignalDetectValue); + EXPECT_FALSE(found) << "phy_rx_signal_detect should NOT be in COUNTERS_DB when initAttrData fails"; + + std::string fecAlignmentValue; + found = countersTable.hget(expectedKey, "pcs_fec_lane_alignment_lock", fecAlignmentValue); + EXPECT_FALSE(found) << "pcs_fec_lane_alignment_lock should NOT be in COUNTERS_DB when initAttrData fails"; + + std::string rxSnrValue; + found = countersTable.hget(expectedKey, "rx_snr", rxSnrValue); + EXPECT_FALSE(found) << "rx_snr should NOT be in COUNTERS_DB when initAttrData fails"; + + flexCounter->removeCounter(failPortOid); +}