Skip to content

Commit c2de7fc

Browse files
stephenxsstephenxs
authored
[QosOrch] The notifications cannot be drained in QosOrch in case the first one needs to retry (sonic-net#2206)
What I did Bugfix: The notifications cannot be drained in QosOrch in case the first one needs to retry Why I did it Bugfix How I verified it Mock test and manual test. Details if related This is because both doTask and the table handlers take consumer as the argument. In doTask, each notification is iterated in the main loop and passed to table handlers. However, consumer is passed to table handlers, which makes it unable to access the rest notifications following the first one. In case the first one needs to retry and remains in m_toSync, the rest notifications do not have opportunity to run. Signed-off-by: Stephen Sun <[email protected]>
1 parent 5575935 commit c2de7fc

File tree

3 files changed

+133
-46
lines changed

3 files changed

+133
-46
lines changed

‎orchagent/qosorch.cpp

+25-31
Original file line numberDiff line numberDiff line change
@@ -98,13 +98,11 @@ map<string, string> qos_to_ref_table_map = {
9898
#define DSCP_MAX_VAL 63
9999
#define EXP_MAX_VAL 7
100100

101-
task_process_status QosMapHandler::processWorkItem(Consumer& consumer)
101+
task_process_status QosMapHandler::processWorkItem(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
102102
{
103103
SWSS_LOG_ENTER();
104104

105105
sai_object_id_t sai_object = SAI_NULL_OBJECT_ID;
106-
auto it = consumer.m_toSync.begin();
107-
KeyOpFieldsValuesTuple tuple = it->second;
108106
string qos_object_name = kfvKey(tuple);
109107
string qos_map_type_name = consumer.getTableName();
110108
string op = kfvOp(tuple);
@@ -321,11 +319,11 @@ bool DscpToTcMapHandler::removeQosItem(sai_object_id_t sai_object)
321319
return true;
322320
}
323321

324-
task_process_status QosOrch::handleDscpToTcTable(Consumer& consumer)
322+
task_process_status QosOrch::handleDscpToTcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
325323
{
326324
SWSS_LOG_ENTER();
327325
DscpToTcMapHandler dscp_tc_handler;
328-
return dscp_tc_handler.processWorkItem(consumer);
326+
return dscp_tc_handler.processWorkItem(consumer, tuple);
329327
}
330328

331329
bool MplsTcToTcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -376,11 +374,11 @@ sai_object_id_t MplsTcToTcMapHandler::addQosItem(const vector<sai_attribute_t> &
376374
return sai_object;
377375
}
378376

379-
task_process_status QosOrch::handleMplsTcToTcTable(Consumer& consumer)
377+
task_process_status QosOrch::handleMplsTcToTcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
380378
{
381379
SWSS_LOG_ENTER();
382380
MplsTcToTcMapHandler mpls_tc_to_tc_handler;
383-
return mpls_tc_to_tc_handler.processWorkItem(consumer);
381+
return mpls_tc_to_tc_handler.processWorkItem(consumer, tuple);
384382
}
385383

386384
bool Dot1pToTcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -445,11 +443,11 @@ sai_object_id_t Dot1pToTcMapHandler::addQosItem(const vector<sai_attribute_t> &a
445443
return object_id;
446444
}
447445

448-
task_process_status QosOrch::handleDot1pToTcTable(Consumer &consumer)
446+
task_process_status QosOrch::handleDot1pToTcTable(Consumer &consumer, KeyOpFieldsValuesTuple &tuple)
449447
{
450448
SWSS_LOG_ENTER();
451449
Dot1pToTcMapHandler dot1p_tc_handler;
452-
return dot1p_tc_handler.processWorkItem(consumer);
450+
return dot1p_tc_handler.processWorkItem(consumer, tuple);
453451
}
454452

455453
bool TcToQueueMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -498,11 +496,11 @@ sai_object_id_t TcToQueueMapHandler::addQosItem(const vector<sai_attribute_t> &a
498496
return sai_object;
499497
}
500498

501-
task_process_status QosOrch::handleTcToQueueTable(Consumer& consumer)
499+
task_process_status QosOrch::handleTcToQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
502500
{
503501
SWSS_LOG_ENTER();
504502
TcToQueueMapHandler tc_queue_handler;
505-
return tc_queue_handler.processWorkItem(consumer);
503+
return tc_queue_handler.processWorkItem(consumer, tuple);
506504
}
507505

508506
void WredMapHandler::freeAttribResources(vector<sai_attribute_t> &attributes)
@@ -719,11 +717,11 @@ bool WredMapHandler::removeQosItem(sai_object_id_t sai_object)
719717
return true;
720718
}
721719

722-
task_process_status QosOrch::handleWredProfileTable(Consumer& consumer)
720+
task_process_status QosOrch::handleWredProfileTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
723721
{
724722
SWSS_LOG_ENTER();
725723
WredMapHandler wred_handler;
726-
return wred_handler.processWorkItem(consumer);
724+
return wred_handler.processWorkItem(consumer, tuple);
727725
}
728726

729727
bool TcToPgHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -772,11 +770,11 @@ sai_object_id_t TcToPgHandler::addQosItem(const vector<sai_attribute_t> &attribu
772770

773771
}
774772

775-
task_process_status QosOrch::handleTcToPgTable(Consumer& consumer)
773+
task_process_status QosOrch::handleTcToPgTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
776774
{
777775
SWSS_LOG_ENTER();
778776
TcToPgHandler tc_to_pg_handler;
779-
return tc_to_pg_handler.processWorkItem(consumer);
777+
return tc_to_pg_handler.processWorkItem(consumer, tuple);
780778
}
781779

782780
bool PfcPrioToPgHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -826,11 +824,11 @@ sai_object_id_t PfcPrioToPgHandler::addQosItem(const vector<sai_attribute_t> &at
826824

827825
}
828826

829-
task_process_status QosOrch::handlePfcPrioToPgTable(Consumer& consumer)
827+
task_process_status QosOrch::handlePfcPrioToPgTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
830828
{
831829
SWSS_LOG_ENTER();
832830
PfcPrioToPgHandler pfc_prio_to_pg_handler;
833-
return pfc_prio_to_pg_handler.processWorkItem(consumer);
831+
return pfc_prio_to_pg_handler.processWorkItem(consumer, tuple);
834832
}
835833

836834
bool PfcToQueueHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes)
@@ -967,11 +965,11 @@ sai_object_id_t DscpToFcMapHandler::addQosItem(const vector<sai_attribute_t> &at
967965
return sai_object;
968966
}
969967

970-
task_process_status QosOrch::handleDscpToFcTable(Consumer& consumer)
968+
task_process_status QosOrch::handleDscpToFcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
971969
{
972970
SWSS_LOG_ENTER();
973971
DscpToFcMapHandler dscp_fc_handler;
974-
return dscp_fc_handler.processWorkItem(consumer);
972+
return dscp_fc_handler.processWorkItem(consumer, tuple);
975973
}
976974

977975
bool ExpToFcMapHandler::convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple,
@@ -1058,18 +1056,18 @@ sai_object_id_t ExpToFcMapHandler::addQosItem(const vector<sai_attribute_t> &att
10581056
return sai_object;
10591057
}
10601058

1061-
task_process_status QosOrch::handleExpToFcTable(Consumer& consumer)
1059+
task_process_status QosOrch::handleExpToFcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
10621060
{
10631061
SWSS_LOG_ENTER();
10641062
ExpToFcMapHandler exp_fc_handler;
1065-
return exp_fc_handler.processWorkItem(consumer);
1063+
return exp_fc_handler.processWorkItem(consumer, tuple);
10661064
}
10671065

1068-
task_process_status QosOrch::handlePfcToQueueTable(Consumer& consumer)
1066+
task_process_status QosOrch::handlePfcToQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
10691067
{
10701068
SWSS_LOG_ENTER();
10711069
PfcToQueueHandler pfc_to_queue_handler;
1072-
return pfc_to_queue_handler.processWorkItem(consumer);
1070+
return pfc_to_queue_handler.processWorkItem(consumer, tuple);
10731071
}
10741072

10751073
QosOrch::QosOrch(DBConnector *db, vector<string> &tableNames) : Orch(db, tableNames)
@@ -1104,14 +1102,13 @@ void QosOrch::initTableHandlers()
11041102
m_qos_handler_map.insert(qos_handler_pair(CFG_PFC_PRIORITY_TO_QUEUE_MAP_TABLE_NAME, &QosOrch::handlePfcToQueueTable));
11051103
}
11061104

1107-
task_process_status QosOrch::handleSchedulerTable(Consumer& consumer)
1105+
task_process_status QosOrch::handleSchedulerTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
11081106
{
11091107
SWSS_LOG_ENTER();
11101108

11111109
sai_status_t sai_status;
11121110
sai_object_id_t sai_object = SAI_NULL_OBJECT_ID;
11131111

1114-
KeyOpFieldsValuesTuple tuple = consumer.m_toSync.begin()->second;
11151112
string qos_map_type_name = CFG_SCHEDULER_TABLE_NAME;
11161113
string qos_object_name = kfvKey(tuple);
11171114
string op = kfvOp(tuple);
@@ -1457,11 +1454,9 @@ bool QosOrch::applyWredProfileToQueue(Port &port, size_t queue_ind, sai_object_i
14571454
return true;
14581455
}
14591456

1460-
task_process_status QosOrch::handleQueueTable(Consumer& consumer)
1457+
task_process_status QosOrch::handleQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
14611458
{
14621459
SWSS_LOG_ENTER();
1463-
auto it = consumer.m_toSync.begin();
1464-
KeyOpFieldsValuesTuple tuple = it->second;
14651460
Port port;
14661461
bool result;
14671462
string key = kfvKey(tuple);
@@ -1690,11 +1685,10 @@ task_process_status QosOrch::ResolveMapAndApplyToPort(
16901685
return task_process_status::task_success;
16911686
}
16921687

1693-
task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer)
1688+
task_process_status QosOrch::handlePortQosMapTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple)
16941689
{
16951690
SWSS_LOG_ENTER();
16961691

1697-
KeyOpFieldsValuesTuple tuple = consumer.m_toSync.begin()->second;
16981692
string key = kfvKey(tuple);
16991693
string op = kfvOp(tuple);
17001694
vector<string> port_names = tokenize(key, list_item_delimiter);
@@ -1898,7 +1892,7 @@ void QosOrch::doTask(Consumer &consumer)
18981892
continue;
18991893
}
19001894

1901-
auto task_status = (this->*(m_qos_handler_map[qos_map_type_name]))(consumer);
1895+
auto task_status = (this->*(m_qos_handler_map[qos_map_type_name]))(consumer, it->second);
19021896
switch(task_status)
19031897
{
19041898
case task_process_status::task_success :

‎orchagent/qosorch.h

+15-15
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ const string ecn_all = "ecn_all";
5959
class QosMapHandler
6060
{
6161
public:
62-
task_process_status processWorkItem(Consumer& consumer);
62+
task_process_status processWorkItem(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
6363
virtual bool convertFieldValuesToAttributes(KeyOpFieldsValuesTuple &tuple, vector<sai_attribute_t> &attributes) = 0;
6464
virtual void freeAttribResources(vector<sai_attribute_t> &attributes);
6565
virtual bool modifyQosItem(sai_object_id_t, vector<sai_attribute_t> &attributes);
@@ -158,25 +158,25 @@ class QosOrch : public Orch
158158
void doTask() override;
159159
virtual void doTask(Consumer& consumer);
160160

161-
typedef task_process_status (QosOrch::*qos_table_handler)(Consumer& consumer);
161+
typedef task_process_status (QosOrch::*qos_table_handler)(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
162162
typedef map<string, qos_table_handler> qos_table_handler_map;
163163
typedef pair<string, qos_table_handler> qos_handler_pair;
164164

165165
void initTableHandlers();
166166

167-
task_process_status handleDscpToTcTable(Consumer& consumer);
168-
task_process_status handleMplsTcToTcTable(Consumer& consumer);
169-
task_process_status handleDot1pToTcTable(Consumer& consumer);
170-
task_process_status handlePfcPrioToPgTable(Consumer& consumer);
171-
task_process_status handlePfcToQueueTable(Consumer& consumer);
172-
task_process_status handlePortQosMapTable(Consumer& consumer);
173-
task_process_status handleTcToPgTable(Consumer& consumer);
174-
task_process_status handleTcToQueueTable(Consumer& consumer);
175-
task_process_status handleSchedulerTable(Consumer& consumer);
176-
task_process_status handleQueueTable(Consumer& consumer);
177-
task_process_status handleWredProfileTable(Consumer& consumer);
178-
task_process_status handleDscpToFcTable(Consumer& consumer);
179-
task_process_status handleExpToFcTable(Consumer& consumer);
167+
task_process_status handleDscpToTcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
168+
task_process_status handleMplsTcToTcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
169+
task_process_status handleDot1pToTcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
170+
task_process_status handlePfcPrioToPgTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
171+
task_process_status handlePfcToQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
172+
task_process_status handlePortQosMapTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
173+
task_process_status handleTcToPgTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
174+
task_process_status handleTcToQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
175+
task_process_status handleSchedulerTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
176+
task_process_status handleQueueTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
177+
task_process_status handleWredProfileTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
178+
task_process_status handleDscpToFcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
179+
task_process_status handleExpToFcTable(Consumer& consumer, KeyOpFieldsValuesTuple &tuple);
180180

181181
sai_object_id_t getSchedulerGroup(const Port &port, const sai_object_id_t queue_id);
182182

‎tests/mock_tests/qosorch_ut.cpp

+93
Original file line numberDiff line numberDiff line change
@@ -786,4 +786,97 @@ namespace qosorch_test
786786
static_cast<Orch *>(gQosOrch)->doTask();
787787
ASSERT_EQ((*QosOrch::getTypeMap()[CFG_DSCP_TO_TC_MAP_TABLE_NAME])["AZURE"].m_saiObjectId, switch_dscp_to_tc_map_id);
788788
}
789+
790+
TEST_F(QosOrchTest, QosOrchTestRetryFirstItem)
791+
{
792+
// There was a bug in QosOrch that the 2nd notifications and after can not be handled, eg the 1st one needs to be retried
793+
// This is to verify the bug has been fixed
794+
vector<string> ts;
795+
std::deque<KeyOpFieldsValuesTuple> entries;
796+
797+
// Try adding dscp_to_tc_map AZURE.1 and AZURE to PORT_QOS_MAP table
798+
// The object AZURE.1 does not exist so the first item can not be handled and remain in m_toSync.
799+
entries.push_back({"Ethernet0", "SET",
800+
{
801+
{"dscp_to_tc_map", "AZURE.1"}
802+
}});
803+
entries.push_back({"Ethernet4", "SET",
804+
{
805+
{"dscp_to_tc_map", "AZURE"}
806+
}});
807+
auto portQosMapConsumer = dynamic_cast<Consumer *>(gQosOrch->getExecutor(CFG_PORT_QOS_MAP_TABLE_NAME));
808+
portQosMapConsumer->addToSync(entries);
809+
entries.clear();
810+
static_cast<Orch *>(gQosOrch)->doTask();
811+
// The 2nd notification should be handled. Make sure by checking reference
812+
CheckDependency(CFG_PORT_QOS_MAP_TABLE_NAME, "Ethernet4", "dscp_to_tc_map", CFG_DSCP_TO_TC_MAP_TABLE_NAME, "AZURE");
813+
// Make sure there is one item left
814+
portQosMapConsumer->dumpPendingTasks(ts);
815+
ASSERT_EQ(ts[0], "PORT_QOS_MAP|Ethernet0|SET|dscp_to_tc_map:AZURE.1");
816+
ASSERT_EQ(ts.size(), 1);
817+
ts.clear();
818+
819+
// Try adding scheduler.0 and scheduler.2 to QUEUE table
820+
entries.push_back({"Ethernet0|0", "SET",
821+
{
822+
{"scheduler", "scheduler.2"}
823+
}});
824+
entries.push_back({"Ethernet0|1", "SET",
825+
{
826+
{"scheduler", "scheduler.0"}
827+
}});
828+
auto queueConsumer = dynamic_cast<Consumer *>(gQosOrch->getExecutor(CFG_QUEUE_TABLE_NAME));
829+
queueConsumer->addToSync(entries);
830+
entries.clear();
831+
static_cast<Orch *>(gQosOrch)->doTask();
832+
// The 2nd notification should be handled. Make sure by checking reference
833+
CheckDependency(CFG_QUEUE_TABLE_NAME, "Ethernet0|1", "scheduler", CFG_SCHEDULER_TABLE_NAME, "scheduler.0");
834+
// Make sure there is one item left
835+
queueConsumer->dumpPendingTasks(ts);
836+
ASSERT_EQ(ts[0], "QUEUE|Ethernet0|0|SET|scheduler:scheduler.2");
837+
ASSERT_EQ(ts.size(), 1);
838+
ts.clear();
839+
840+
// Try removing AZURE and adding AZURE.1 to DSCP_TO_TC_MAP table
841+
entries.push_back({"AZURE", "DEL", {{}}});
842+
entries.push_back({"AZURE.1", "SET",
843+
{
844+
{"1", "1"}
845+
}});
846+
auto consumer = dynamic_cast<Consumer *>(gQosOrch->getExecutor(CFG_DSCP_TO_TC_MAP_TABLE_NAME));
847+
consumer->addToSync(entries);
848+
entries.clear();
849+
static_cast<Orch *>(gQosOrch)->doTask();
850+
// The 2nd notification should be handled. Make sure by checking reference
851+
CheckDependency(CFG_PORT_QOS_MAP_TABLE_NAME, "Ethernet0", "dscp_to_tc_map", CFG_DSCP_TO_TC_MAP_TABLE_NAME, "AZURE.1");
852+
// The pending item in PORT_QOS_MAP table should also be handled since the dependency is met
853+
portQosMapConsumer->dumpPendingTasks(ts);
854+
ASSERT_TRUE(ts.empty());
855+
consumer->dumpPendingTasks(ts);
856+
ASSERT_EQ(ts[0], "DSCP_TO_TC_MAP|AZURE|DEL|:");
857+
ASSERT_EQ(ts.size(), 1);
858+
ts.clear();
859+
860+
entries.push_back({"scheduler.0", "DEL", {{}}});
861+
entries.push_back({"scheduler.2", "SET",
862+
{
863+
{"type", "DWRR"},
864+
{"weight", "15"}
865+
}});
866+
consumer = dynamic_cast<Consumer *>(gQosOrch->getExecutor(CFG_SCHEDULER_TABLE_NAME));
867+
consumer->addToSync(entries);
868+
entries.clear();
869+
static_cast<Orch *>(gQosOrch)->doTask();
870+
// We need a second call to "doTask" because scheduler table is handled after queue table
871+
static_cast<Orch *>(gQosOrch)->doTask();
872+
// The 2nd notification should be handled. Make sure by checking reference
873+
CheckDependency(CFG_QUEUE_TABLE_NAME, "Ethernet0|0", "scheduler", CFG_SCHEDULER_TABLE_NAME, "scheduler.2");
874+
// The pending item in QUEUE table should also be handled since the dependency is met
875+
queueConsumer->dumpPendingTasks(ts);
876+
ASSERT_TRUE(ts.empty());
877+
consumer->dumpPendingTasks(ts);
878+
ASSERT_EQ(ts[0], "SCHEDULER|scheduler.0|DEL|:");
879+
ASSERT_EQ(ts.size(), 1);
880+
ts.clear();
881+
}
789882
}

0 commit comments

Comments
 (0)