Skip to content

Commit 5641bd1

Browse files
dakotac-aristadakotac-arista
authored
[202503] [Backport] Fix Stale queue counter mappings in COUNTERS_DB post port breakout (#196)
PR: sonic-net/sonic-swss#3982 Also, included missing mock test function from master. As identified in msft-202412: #175 Signed-off-by: Dakota Crozier <[email protected]>
1 parent c12e5f2 commit 5641bd1

File tree

4 files changed

+153
-14
lines changed

4 files changed

+153
-14
lines changed

‎orchagent/high_frequency_telemetry/counternameupdater.cpp‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,15 @@ void CounterNameMapUpdater::setCounterNameMap(const std::vector<swss::FieldValue
4040
{
4141
SWSS_LOG_ENTER();
4242

43-
if (gHFTOrch)
43+
for (const auto& map : counter_name_maps)
4444
{
45-
for (const auto& map : counter_name_maps)
45+
const std::string& counter_name = fvField(map);
46+
sai_object_id_t oid = SAI_NULL_OBJECT_ID;
47+
if (!fvValue(map).empty())
4648
{
47-
const std::string& counter_name = fvField(map);
48-
sai_object_id_t oid = SAI_NULL_OBJECT_ID;
49-
if (!fvValue(map).empty())
50-
{
51-
sai_deserialize_object_id(fvValue(map), oid);
52-
}
53-
setCounterNameMap(counter_name, oid);
49+
sai_deserialize_object_id(fvValue(map), oid);
5450
}
51+
setCounterNameMap(counter_name, oid);
5552
}
5653
}
5754

‎orchagent/portsorch.cpp‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3850,10 +3850,38 @@ bool PortsOrch::initPort(const PortConfig &port)
38503850
m_recircPortRole[alias] = role;
38513851
}
38523852

3853-
// We have to test the size of m_queue_ids here since it isn't initialized on some platforms (like DPU)
3854-
if (p.m_host_tx_queue_configured && p.m_queue_ids.size() > p.m_host_tx_queue)
3853+
// If queue-related flex counters are already enabled, generate queue maps
3854+
// for the newly added port so that usecases like dynamic port breakout works.
3855+
bool queueFcEnabled = flex_counters_orch->getQueueCountersState() ||
3856+
flex_counters_orch->getQueueWatermarkCountersState() ||
3857+
flex_counters_orch->getWredQueueCountersState();
3858+
if (queueFcEnabled && !p.m_queue_ids.empty())
38553859
{
3856-
createPortBufferQueueCounters(p, to_string(p.m_host_tx_queue), false);
3860+
auto queuesStateVector = flex_counters_orch->getQueueConfigurations();
3861+
3862+
auto maxQueueNumber = getNumberOfPortSupportedQueueCounters(p.m_alias);
3863+
FlexCounterQueueStates flexCounterQueueState(maxQueueNumber);
3864+
3865+
if (queuesStateVector.count(p.m_alias))
3866+
{
3867+
flexCounterQueueState = queuesStateVector.at(p.m_alias);
3868+
}
3869+
else if (queuesStateVector.count(createAllAvailableBuffersStr))
3870+
{
3871+
if (maxQueueNumber > 0)
3872+
{
3873+
flexCounterQueueState.enableQueueCounters(0, maxQueueNumber - 1);
3874+
}
3875+
}
3876+
else
3877+
{
3878+
if (p.m_host_tx_queue_configured && p.m_host_tx_queue < maxQueueNumber)
3879+
{
3880+
flexCounterQueueState.enableQueueCounter(p.m_host_tx_queue);
3881+
}
3882+
}
3883+
3884+
generateQueueMapPerPort(p, flexCounterQueueState, false);
38573885
}
38583886

38593887
SWSS_LOG_NOTICE("Initialized port %s", alias.c_str());
@@ -3886,9 +3914,12 @@ void PortsOrch::deInitPort(string alias, sai_object_id_t port_id)
38863914
return;
38873915
}
38883916

3889-
if (p.m_host_tx_queue_configured && p.m_queue_ids.size() > p.m_host_tx_queue)
3917+
if (!p.m_queue_ids.empty())
38903918
{
3891-
removePortBufferQueueCounters(p, to_string(p.m_host_tx_queue), false);
3919+
auto skip_host_tx_queue = p.m_host_tx_queue_configured && (p.m_queue_ids.size() > p.m_host_tx_queue);
3920+
// Remove all queue counters and mappings for this port to avoid stale entries
3921+
std::string range = "0-" + to_string(p.m_queue_ids.size() - 1);
3922+
removePortBufferQueueCounters(p, range, skip_host_tx_queue);
38923923
}
38933924

38943925
/* remove port from flex_counter_table for updating counters */

‎tests/mock_tests/mock_table.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,15 @@ namespace swss
9999
}
100100
}
101101

102+
void Table::hset(const std::string &key, const std::string &field, const std::string &value,
103+
const std::string& op, const std::string& prefix)
104+
{
105+
FieldValueTuple fvp(field, value);
106+
std::vector<FieldValueTuple> attrs = { fvp };
107+
108+
Table::set(key, attrs, op, prefix);
109+
}
110+
102111
void Table::getKeys(std::vector<std::string> &keys)
103112
{
104113
keys.clear();
@@ -117,6 +126,35 @@ namespace swss
117126
}
118127
}
119128

129+
void Table::hdel(const std::string &key, const std::string &field, const std::string &op, const std::string &prefix)
130+
{
131+
auto &table = gDB[m_pipe->getDbId()][getTableName()];
132+
auto key_iter = table.find(key);
133+
if (key_iter == table.end())
134+
{
135+
return;
136+
}
137+
138+
auto &attrs = key_iter->second;
139+
std::vector<FieldValueTuple> new_attrs;
140+
for (const auto &attr : attrs)
141+
{
142+
if (attr.first != field)
143+
{
144+
new_attrs.push_back(attr);
145+
}
146+
}
147+
148+
if (new_attrs.empty())
149+
{
150+
table.erase(key);
151+
}
152+
else
153+
{
154+
table[key] = new_attrs;
155+
}
156+
}
157+
120158
void ProducerStateTable::set(const std::string &key,
121159
const std::vector<FieldValueTuple> &values,
122160
const std::string &op,

‎tests/mock_tests/portsorch_ut.cpp‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,6 +1332,79 @@ namespace portsorch_test
13321332
_unhook_sai_queue_api();
13331333
}
13341334

1335+
TEST_F(PortsOrchTest, PortDeleteQueueCountersCleanup)
1336+
{
1337+
Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
1338+
std::deque<KeyOpFieldsValuesTuple> entries;
1339+
1340+
auto &ports = defaultPortList;
1341+
ASSERT_TRUE(!ports.empty());
1342+
1343+
// Create ports
1344+
for (const auto &it : ports)
1345+
{
1346+
portTable.set(it.first, it.second);
1347+
}
1348+
1349+
portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } });
1350+
portTable.set("PortInitDone", { { "lanes", "0" } });
1351+
1352+
gPortsOrch->addExistingData(&portTable);
1353+
static_cast<Orch *>(gPortsOrch)->doTask();
1354+
1355+
Port port;
1356+
auto testQueueIdx = 0;
1357+
string portName = "Ethernet0";
1358+
ASSERT_TRUE(gPortsOrch->getPort(portName, port));
1359+
ASSERT_NE(port.m_port_id, SAI_NULL_OBJECT_ID);
1360+
ASSERT_GT(port.m_queue_ids.size(), 0u);
1361+
1362+
// Enable Queue flex counters
1363+
Table flexCounterCfg = Table(m_config_db.get(), CFG_FLEX_COUNTER_TABLE_NAME);
1364+
const std::vector<FieldValueTuple> enable({ {FLEX_COUNTER_STATUS_FIELD, "enable"} });
1365+
flexCounterCfg.set("QUEUE_WATERMARK", enable);
1366+
flexCounterCfg.set("QUEUE", enable);
1367+
1368+
auto flexCounterOrch = gDirectory.get<FlexCounterOrch*>();
1369+
flexCounterOrch->addExistingData(&flexCounterCfg);
1370+
static_cast<Orch *>(flexCounterOrch)->doTask();
1371+
1372+
sai_object_id_t targetQueueOid = port.m_queue_ids[testQueueIdx];
1373+
1374+
// Delete the port
1375+
entries.push_back({portName, "DEL", { { } }});
1376+
auto consumer = dynamic_cast<Consumer *>(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME));
1377+
consumer->addToSync(entries);
1378+
static_cast<Orch *>(gPortsOrch)->doTask();
1379+
entries.clear();
1380+
1381+
Table countersQueueNameMap(m_counters_db.get(), "COUNTERS_QUEUE_NAME_MAP");
1382+
Table countersQueuePortMap(m_counters_db.get(), "COUNTERS_QUEUE_PORT_MAP");
1383+
1384+
// Verify specific alias:idx field is gone from name maps
1385+
string dummy;
1386+
ASSERT_FALSE(countersQueueNameMap.hget("", portName + ":" + to_string(testQueueIdx), dummy));
1387+
ASSERT_FALSE(countersQueuePortMap.hget("", sai_serialize_object_id(targetQueueOid), dummy));
1388+
1389+
// Re-add the same port
1390+
auto it = ports.find(portName);
1391+
entries.push_back({portName, "SET", it->second});
1392+
consumer->addToSync(entries);
1393+
static_cast<Orch *>(gPortsOrch)->doTask();
1394+
entries.clear();
1395+
1396+
// Fetch the re-added port and verify COUNTERS_DB entries exist for the queue index
1397+
Port readded;
1398+
ASSERT_TRUE(gPortsOrch->getPort(portName, readded));
1399+
ASSERT_GT(readded.m_queue_ids.size(), 0u);
1400+
sai_object_id_t readdedQ1 = readded.m_queue_ids[testQueueIdx];
1401+
1402+
// Name-map should contain alias:idx as a field
1403+
ASSERT_TRUE(countersQueueNameMap.hget("", portName + ":" + to_string(testQueueIdx), dummy));
1404+
// Port-map should contain queue OID -> port OID mapping as a field
1405+
ASSERT_TRUE(countersQueuePortMap.hget("", sai_serialize_object_id(readdedQ1), dummy));
1406+
}
1407+
13351408
TEST_F(PortsOrchTest, PortPTConfigDefaultTimestampTemplate)
13361409
{
13371410
auto portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);

0 commit comments

Comments
 (0)