Skip to content

Commit bab7b93

Browse files
stepanblyschakqiluo-msft
stepanblyschak
authored and
qiluo-msft
committed
[portsorch] fix PortsOrch::allPortsReady() returns true when it should not (sonic-net#1103)
* [portsorch] fix PortsOrch::allPortsReady() returns true when it should not Warm start flow before the change: 1st iteration: - BufferOrch::doTask(): returns since PortInitDone hasn't arived yet - PortsOrch::doTask(): processes all PORT_TABLE untill PortInitDone flag m_pendingPortSet is empty yet and m_portInitDone is true so allPortsReady() will return true - AnyOrch::doTask(): check g_portsOrch->allPortsRead() 2nd iteration: - BufferOrch::doTask(): now buffers are applied This causes BufferOrch override PfcWdOrch's zero-buffer profile. The change swaps BufferOrch and PortsOrch in m_orchList, because 1st BufferOrch iteration will always skip processing and eliminates possibility of having m_pendingPortSet not filled with ports after m_initDone is set to true. * remove extra newline * [pfcwdorch] fix PfcWdSwOrch::doTask() starts WD action when WD wasn't started in warm boot It appeared that pfc watchdog relied on a buggy behaviour of PortsOrch::allPortsReady(). In fixed PortsOrch::allPortsReady() you'll see that watchdog action is trying to start before watchdog was started, because allPortsReady() in PfcWdOrch::doTask() returned false. Before the fix watchdog was started before, because allPortsReady() lied that ports are ready when they were not. * [portsorch] populate m_pendingPortSet in PortsOrch::bake() * [portsorch] optimize to 3 iterations instead of 4 * Revert "[portsorch] optimize to 3 iterations instead of 4" * revert change of order in m_orchList * [mock_tests] fix tests build * [mock_test] create unittest for PortsOrch::allPortsReady cold/warm flows * [orchdaemon] fix removed sfloworch * [mock_tests] make mock_tests run on "make check"
1 parent 5ab3f6b commit bab7b93

14 files changed

+689
-43
lines changed

‎orchagent/pfcwdorch.cpp

+5
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,11 @@ void PfcWdSwOrch<DropHandler, ForwardHandler>::doTask(Consumer& consumer)
759759
{
760760
PfcWdOrch<DropHandler, ForwardHandler>::doTask(consumer);
761761

762+
if (!gPortsOrch->allPortsReady())
763+
{
764+
return;
765+
}
766+
762767
if ((consumer.getDbId() == APPL_DB) && (consumer.getTableName() == APP_PFC_WD_TABLE_NAME))
763768
{
764769
auto it = consumer.m_toSync.begin();

‎orchagent/portsorch.cpp

+10
Original file line numberDiff line numberDiff line change
@@ -1471,6 +1471,16 @@ bool PortsOrch::bake()
14711471
return false;
14721472
}
14731473

1474+
for (const auto& alias: keys)
1475+
{
1476+
if (alias == "PortConfigDone" || alias == "PortInitDone")
1477+
{
1478+
continue;
1479+
}
1480+
1481+
m_pendingPortSet.emplace(alias);
1482+
}
1483+
14741484
addExistingData(m_portTable.get());
14751485
addExistingData(APP_LAG_TABLE_NAME);
14761486
addExistingData(APP_LAG_MEMBER_TABLE_NAME);

‎tests/Makefile.am

+2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ CFLAGS_SAI = -I /usr/include/sai
22

33
TESTS = tests
44

5+
SUBDIRS = mock_tests
6+
57
noinst_PROGRAMS = tests
68

79
if DEBUG

‎tests/mock_tests/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
tests

‎tests/mock_tests/Makefile.am

+39-31
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
CFLAGS_SAI = -I /usr/include/sai
22

3-
bin_PROGRAMS = tests
3+
TESTS = tests
4+
5+
noinst_PROGRAMS = tests
46

57
LDADD_SAI = -lsaimeta -lsaimetadata -lsaivs -lsairedis
68

@@ -13,42 +15,48 @@ endif
1315
CFLAGS_GTEST =
1416
LDADD_GTEST = -L/usr/src/gtest
1517

16-
tests_SOURCES = swssnet_ut.cpp request_parser_ut.cpp aclorch_ut.cpp saispy_ut.cpp \
18+
tests_SOURCES = aclorch_ut.cpp \
19+
portsorch_ut.cpp \
20+
saispy_ut.cpp \
21+
ut_saihelper.cpp \
1722
mock_orchagent_main.cpp \
1823
mock_dbconnector.cpp \
1924
mock_consumerstatetable.cpp \
25+
mock_table.cpp \
2026
mock_hiredis.cpp \
2127
mock_redisreply.cpp \
22-
../orchagent/orchdaemon.cpp \
23-
../orchagent/orch.cpp \
24-
../orchagent/notifications.cpp \
25-
../orchagent/routeorch.cpp \
26-
../orchagent/neighorch.cpp \
27-
../orchagent/intfsorch.cpp \
28-
../orchagent/portsorch.cpp \
29-
../orchagent/copporch.cpp \
30-
../orchagent/tunneldecaporch.cpp \
31-
../orchagent/qosorch.cpp \
32-
../orchagent/bufferorch.cpp \
33-
../orchagent/mirrororch.cpp \
34-
../orchagent/fdborch.cpp \
35-
../orchagent/aclorch.cpp \
36-
../orchagent/saihelper.cpp \
37-
../orchagent/switchorch.cpp \
38-
../orchagent/pfcwdorch.cpp \
39-
../orchagent/pfcactionhandler.cpp \
40-
../orchagent/policerorch.cpp \
41-
../orchagent/crmorch.cpp \
42-
../orchagent/request_parser.cpp \
43-
../orchagent/vrforch.cpp \
44-
../orchagent/countercheckorch.cpp \
45-
../orchagent/vxlanorch.cpp \
46-
../orchagent/vnetorch.cpp \
47-
../orchagent/dtelorch.cpp \
48-
../orchagent/flexcounterorch.cpp \
49-
../orchagent/watermarkorch.cpp
28+
$(top_srcdir)/orchagent/orchdaemon.cpp \
29+
$(top_srcdir)/orchagent/orch.cpp \
30+
$(top_srcdir)/orchagent/notifications.cpp \
31+
$(top_srcdir)/orchagent/routeorch.cpp \
32+
$(top_srcdir)/orchagent/neighorch.cpp \
33+
$(top_srcdir)/orchagent/intfsorch.cpp \
34+
$(top_srcdir)/orchagent/portsorch.cpp \
35+
$(top_srcdir)/orchagent/copporch.cpp \
36+
$(top_srcdir)/orchagent/tunneldecaporch.cpp \
37+
$(top_srcdir)/orchagent/qosorch.cpp \
38+
$(top_srcdir)/orchagent/bufferorch.cpp \
39+
$(top_srcdir)/orchagent/mirrororch.cpp \
40+
$(top_srcdir)/orchagent/fdborch.cpp \
41+
$(top_srcdir)/orchagent/aclorch.cpp \
42+
$(top_srcdir)/orchagent/saihelper.cpp \
43+
$(top_srcdir)/orchagent/switchorch.cpp \
44+
$(top_srcdir)/orchagent/pfcwdorch.cpp \
45+
$(top_srcdir)/orchagent/pfcactionhandler.cpp \
46+
$(top_srcdir)/orchagent/policerorch.cpp \
47+
$(top_srcdir)/orchagent/crmorch.cpp \
48+
$(top_srcdir)/orchagent/request_parser.cpp \
49+
$(top_srcdir)/orchagent/vrforch.cpp \
50+
$(top_srcdir)/orchagent/countercheckorch.cpp \
51+
$(top_srcdir)/orchagent/vxlanorch.cpp \
52+
$(top_srcdir)/orchagent/vnetorch.cpp \
53+
$(top_srcdir)/orchagent/dtelorch.cpp \
54+
$(top_srcdir)/orchagent/flexcounterorch.cpp \
55+
$(top_srcdir)/orchagent/watermarkorch.cpp \
56+
$(top_srcdir)/orchagent/chassisorch.cpp \
57+
$(top_srcdir)/orchagent/sfloworch.cpp
5058

5159
tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
52-
tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I../orchagent
60+
tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I$(top_srcdir)/orchagent
5361
tests_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis -lpthread \
5462
-lswsscommon -lswsscommon -lgtest -lgtest_main

‎tests/mock_tests/aclorch_ut.cpp

+9-9
Original file line numberDiff line numberDiff line change
@@ -643,16 +643,16 @@ namespace aclorch_test
643643
// leakage check
644644
bool validateResourceCountWithLowerLayerDb(const AclOrch *aclOrch)
645645
{
646-
// TODO: Using the need to include "sai_vs_state.h". That will need the include path from `configure`
647-
// Do this later ...
646+
// TODO: Using the need to include "sai_vs_state.h". That will need the include path from `configure`
647+
// Do this later ...
648648
#if WITH_SAI == LIBVS
649-
// {
650-
// auto& aclTableHash = g_switch_state_map.at(gSwitchId)->objectHash.at(SAI_OBJECT_TYPE_ACL_TABLE);
651-
//
652-
// return aclTableHash.size() == Portal::AclOrchInternal::getAclTables(aclOrch).size();
653-
// }
654-
//
655-
// TODO: add rule check
649+
// {
650+
// auto& aclTableHash = g_switch_state_map.at(gSwitchId)->objectHash.at(SAI_OBJECT_TYPE_ACL_TABLE);
651+
//
652+
// return aclTableHash.size() == Portal::AclOrchInternal::getAclTables(aclOrch).size();
653+
// }
654+
//
655+
// TODO: add rule check
656656
#endif
657657

658658
return true;

‎tests/mock_tests/mock_consumerstatetable.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,4 @@ namespace swss
77
TableName_KeySet(tableName)
88
{
99
}
10-
}
10+
}

‎tests/mock_tests/mock_dbconnector.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,6 @@ namespace swss
4040

4141
int DBConnector::getDbId() const
4242
{
43-
return 12345;
43+
return m_dbId;
4444
}
45-
}
45+
}

‎tests/mock_tests/mock_orchagent_main.h

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
#pragma once
2+
3+
#include "orch.h"
4+
#include "switchorch.h"
5+
#include "crmorch.h"
6+
#include "portsorch.h"
7+
#include "routeorch.h"
8+
#include "intfsorch.h"
9+
#include "neighorch.h"
10+
#include "fdborch.h"
11+
#include "mirrororch.h"
12+
#include "bufferorch.h"
13+
#include "vrforch.h"
14+
#include "vnetorch.h"
15+
#include "vxlanorch.h"
16+
#include "policerorch.h"
17+
18+
extern int gBatchSize;
19+
extern bool gSwssRecord;
20+
extern bool gSairedisRecord;
21+
extern bool gLogRotate;
22+
extern ofstream gRecordOfs;
23+
extern string gRecordFile;
24+
25+
extern MacAddress gMacAddress;
26+
extern MacAddress gVxlanMacAddress;
27+
28+
extern sai_object_id_t gSwitchId;
29+
extern sai_object_id_t gVirtualRouterId;
30+
extern sai_object_id_t gUnderlayIfId;
31+
32+
extern SwitchOrch *gSwitchOrch;
33+
extern CrmOrch *gCrmOrch;
34+
extern PortsOrch *gPortsOrch;
35+
extern RouteOrch *gRouteOrch;
36+
extern IntfsOrch *gIntfsOrch;
37+
extern NeighOrch *gNeighOrch;
38+
extern FdbOrch *gFdbOrch;
39+
extern MirrorOrch *gMirrorOrch;
40+
extern BufferOrch *gBufferOrch;
41+
extern VRFOrch *gVrfOrch;
42+
43+
extern sai_acl_api_t *sai_acl_api;
44+
extern sai_switch_api_t *sai_switch_api;
45+
extern sai_virtual_router_api_t *sai_virtual_router_api;
46+
extern sai_port_api_t *sai_port_api;
47+
extern sai_lag_api_t *sai_lag_api;
48+
extern sai_vlan_api_t *sai_vlan_api;
49+
extern sai_bridge_api_t *sai_bridge_api;
50+
extern sai_router_interface_api_t *sai_router_intfs_api;
51+
extern sai_route_api_t *sai_route_api;
52+
extern sai_neighbor_api_t *sai_neighbor_api;
53+
extern sai_tunnel_api_t *sai_tunnel_api;
54+
extern sai_next_hop_api_t *sai_next_hop_api;
55+
extern sai_hostif_api_t *sai_hostif_api;
56+
extern sai_buffer_api_t *sai_buffer_api;

‎tests/mock_tests/mock_table.cpp

+74
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
#include "table.h"
2+
3+
using TableDataT = std::map<std::string, std::vector<swss::FieldValueTuple>>;
4+
using TablesT = std::map<std::string, TableDataT>;
5+
6+
namespace testing_db
7+
{
8+
9+
TableDataT gTableData;
10+
TablesT gTables;
11+
std::map<int, TablesT> gDB;
12+
13+
void reset()
14+
{
15+
gDB.clear();
16+
}
17+
}
18+
19+
namespace swss
20+
{
21+
22+
using namespace testing_db;
23+
24+
bool Table::get(const std::string &key, std::vector<FieldValueTuple> &ovalues)
25+
{
26+
auto table = gDB[getDbId()][getTableName()];
27+
if (table.find(key) == table.end())
28+
{
29+
return false;
30+
}
31+
32+
ovalues = table[key];
33+
return true;
34+
}
35+
36+
bool Table::hget(const std::string &key, const std::string &field, std::string &value)
37+
{
38+
auto table = gDB[getDbId()][getTableName()];
39+
if (table.find(key) == table.end())
40+
{
41+
return false;
42+
}
43+
44+
for (const auto &it : table[key])
45+
{
46+
if (it.first == field)
47+
{
48+
value = it.second;
49+
return true;
50+
}
51+
}
52+
53+
return false;
54+
}
55+
56+
void Table::set(const std::string &key,
57+
const std::vector<FieldValueTuple> &values,
58+
const std::string &op,
59+
const std::string &prefix)
60+
{
61+
auto &table = gDB[getDbId()][getTableName()];
62+
table[key] = values;
63+
}
64+
65+
void Table::getKeys(std::vector<std::string> &keys)
66+
{
67+
keys.clear();
68+
auto table = gDB[getDbId()][getTableName()];
69+
for (const auto &it : table)
70+
{
71+
keys.push_back(it.first);
72+
}
73+
}
74+
}

‎tests/mock_tests/mock_table.h

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
#include "table.h"
2+
3+
namespace testing_db
4+
{
5+
void reset();
6+
}

0 commit comments

Comments
 (0)