-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add port flap count and last flap timestamp to APPL_DB #3052
Changes from 3 commits
edd4ed7
b51da11
d303988
28687eb
8ccf70d
da88994
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3095,6 +3095,38 @@ bool PortsOrch::removeVlanHostIntf(Port vl) | |
return true; | ||
} | ||
|
||
void PortsOrch::updateDbPortFlapCount(Port& port, sai_port_oper_status_t pstatus) | ||
{ | ||
SWSS_LOG_ENTER(); | ||
|
||
++port.m_flap_count; | ||
prgeor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
vector<FieldValueTuple> tuples; | ||
FieldValueTuple tuple("flap_count", std::to_string(port.m_flap_count)); | ||
tuples.push_back(tuple); | ||
m_portTable->set(port.m_alias, tuples); | ||
prgeor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (pstatus == SAI_PORT_OPER_STATUS_DOWN) | ||
{ | ||
vector<FieldValueTuple> tuples; | ||
auto now = std::chrono::system_clock::now(); | ||
std::time_t now_c = std::chrono::system_clock::to_time_t(now); | ||
FieldValueTuple tuple("last_down_time", std::ctime(&now_c)); | ||
tuples.push_back(tuple); | ||
m_portTable->set(port.m_alias, tuples); | ||
} | ||
|
||
if (pstatus == SAI_PORT_OPER_STATUS_UP) | ||
{ | ||
vector<FieldValueTuple> tuples; | ||
auto now = std::chrono::system_clock::now(); | ||
std::time_t now_c = std::chrono::system_clock::to_time_t(now); | ||
FieldValueTuple tuple("last_up_time", std::ctime(&now_c)); | ||
tuples.push_back(tuple); | ||
m_portTable->set(port.m_alias, tuples); | ||
} | ||
} | ||
|
||
|
||
void PortsOrch::updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const | ||
{ | ||
SWSS_LOG_ENTER(); | ||
|
@@ -5303,7 +5335,7 @@ bool PortsOrch::initializePort(Port &port) | |
/* Check warm start states */ | ||
vector<FieldValueTuple> tuples; | ||
bool exist = m_portTable->get(port.m_alias, tuples); | ||
string operStatus; | ||
string operStatus, flapCount = "0"; | ||
if (exist) | ||
{ | ||
for (auto i : tuples) | ||
|
@@ -5312,9 +5344,14 @@ bool PortsOrch::initializePort(Port &port) | |
{ | ||
operStatus = fvValue(i); | ||
} | ||
|
||
if (fvField(i) == "flap_count") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to handle reconciliation during warm/fast reboot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dgsudharsan yes |
||
{ | ||
flapCount = fvValue(i); | ||
} | ||
} | ||
} | ||
SWSS_LOG_DEBUG("initializePort %s with oper %s", port.m_alias.c_str(), operStatus.c_str()); | ||
SWSS_LOG_INFO("Port %s with oper %s flap_count=%s", port.m_alias.c_str(), operStatus.c_str(), flapCount.c_str()); | ||
|
||
/** | ||
* Create database port oper status as DOWN if attr missing | ||
|
@@ -5335,6 +5372,20 @@ bool PortsOrch::initializePort(Port &port) | |
port.m_oper_status = SAI_PORT_OPER_STATUS_DOWN; | ||
} | ||
|
||
// initalize port flap count | ||
if (!flapCount.empty()) | ||
{ | ||
try | ||
{ | ||
port.m_flap_count = to_uint<std::uint32_t>(flapCount); | ||
} | ||
catch (const std::exception &e) | ||
{ | ||
SWSS_LOG_ERROR("Failed to get port (%s) flap_count: %s", port.m_alias.c_str(), e.what()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to throw an unwanted error during WB from old versions which doesn't have this field in DB. Please change to DEBUG as this is not a real error scenario There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prsunny if the value is not there in the DB, flapCount default value = "0" (see line 5317 above) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prgeor if we hit the exception, is it ok to still put the value to the DB? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nazariig fixed |
||
} | ||
m_portTable->hset(port.m_alias, "flap_count", flapCount); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prgeor is this a typo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nazariig the intention is to write uint64_t string value to the DB |
||
} | ||
|
||
/* initialize port admin status */ | ||
if (!getPortAdminStatus(port.m_port_id, port.m_admin_state_up)) | ||
{ | ||
|
@@ -7568,6 +7619,7 @@ void PortsOrch::updatePortOperStatus(Port &port, sai_port_oper_status_t status) | |
if (port.m_type == Port::PHY) | ||
{ | ||
updateDbPortOperStatus(port, status); | ||
updateDbPortFlapCount(port, status); | ||
updateGearboxPortOperStatus(port); | ||
|
||
/* Refresh the port states and reschedule the poller tasks */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,6 +147,8 @@ class PortsOrch : public Orch, public Subject | |
|
||
bool setHostIntfsOperStatus(const Port& port, bool up) const; | ||
void updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const; | ||
void updateDbPortFlapCount(Port& port, sai_port_oper_status_t pstatus); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prgeor please remove extra line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nazariig removed |
||
|
||
bool createVlanHostIntf(Port& vl, string hostif_name); | ||
bool removeVlanHostIntf(Port vl); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -499,6 +499,73 @@ namespace portsorch_test | |
} | ||
|
||
}; | ||
|
||
/* | ||
* Test port flap count | ||
*/ | ||
TEST_F(PortsOrchTest, PortFlapCount) | ||
{ | ||
Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); | ||
|
||
// Get SAI default ports to populate DB | ||
auto ports = ut_helper::getInitialSaiPorts(); | ||
|
||
// Populate port table with SAI ports | ||
for (const auto &it : ports) | ||
{ | ||
portTable.set(it.first, it.second); | ||
} | ||
|
||
// Set PortConfigDone, PortInitDone | ||
portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); | ||
portTable.set("PortInitDone", { { "lanes", "0" } }); | ||
|
||
// refill consumer | ||
gPortsOrch->addExistingData(&portTable); | ||
// Apply configuration : create ports | ||
static_cast<Orch *>(gPortsOrch)->doTask(); | ||
|
||
// Get first port, expect the oper status is not UP | ||
Port port; | ||
gPortsOrch->getPort("Ethernet0", port); | ||
ASSERT_TRUE(port.m_oper_status != SAI_PORT_OPER_STATUS_UP); | ||
ASSERT_TRUE(port.m_flap_count == 0); | ||
|
||
auto exec = static_cast<Notifier *>(gPortsOrch->getExecutor("PORT_STATUS_NOTIFICATIONS")); | ||
auto consumer = exec->getNotificationConsumer(); | ||
|
||
// mock a redis reply for notification, it notifies that Ehernet0 is going to up | ||
for (uint32_t count=0; count < 5; count++) { | ||
sai_port_oper_status_t oper_status = (count % 2 == 0) ? SAI_PORT_OPER_STATUS_UP : SAI_PORT_OPER_STATUS_DOWN; | ||
mockReply = (redisReply *)calloc(sizeof(redisReply), 1); | ||
mockReply->type = REDIS_REPLY_ARRAY; | ||
mockReply->elements = 3; // REDIS_PUBLISH_MESSAGE_ELEMNTS | ||
mockReply->element = (redisReply **)calloc(sizeof(redisReply *), mockReply->elements); | ||
mockReply->element[2] = (redisReply *)calloc(sizeof(redisReply), 1); | ||
mockReply->element[2]->type = REDIS_REPLY_STRING; | ||
sai_port_oper_status_notification_t port_oper_status; | ||
port_oper_status.port_state = oper_status; | ||
port_oper_status.port_id = port.m_port_id; | ||
std::string data = sai_serialize_port_oper_status_ntf(1, &port_oper_status); | ||
std::vector<FieldValueTuple> notifyValues; | ||
FieldValueTuple opdata("port_state_change", data); | ||
notifyValues.push_back(opdata); | ||
std::string msg = swss::JSon::buildJson(notifyValues); | ||
mockReply->element[2]->str = (char*)calloc(1, msg.length() + 1); | ||
memcpy(mockReply->element[2]->str, msg.c_str(), msg.length()); | ||
|
||
// trigger the notification | ||
consumer->readData(); | ||
gPortsOrch->doTask(*consumer); | ||
mockReply = nullptr; | ||
|
||
gPortsOrch->getPort("Ethernet0", port); | ||
ASSERT_TRUE(port.m_oper_status == oper_status); | ||
ASSERT_TRUE(port.m_flap_count == count+1); | ||
} | ||
|
||
cleanupPorts(gPortsOrch); | ||
} | ||
|
||
TEST_F(PortsOrchTest, PortBulkCreateRemove) | ||
{ | ||
|
@@ -1956,6 +2023,7 @@ namespace portsorch_test | |
|
||
gPortsOrch->getPort("Ethernet0", port); | ||
ASSERT_TRUE(port.m_oper_status == SAI_PORT_OPER_STATUS_UP); | ||
ASSERT_TRUE(port.m_flap_count == 1); | ||
|
||
std::vector<FieldValueTuple> values; | ||
portTable.get("Ethernet0", values); | ||
|
@@ -2102,4 +2170,4 @@ namespace portsorch_test | |
ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created | ||
} | ||
|
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @prgeor please add a new line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prgeor why not uint64? Do we expect an overflow? Should be reflected in the log? Any reaction from the SWSS is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nazariig done