Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[intfsorch] add RIF flex counter group #765

Merged
merged 5 commits into from
Feb 20, 2019
Merged

[intfsorch] add RIF flex counter group #765

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ea60b6
[intfsorch] add RIF flex counter group
Jan 22, 2019
21bbf46
[intfsorch] rm unused commented out code
Jan 23, 2019
9bd8dda
Merge branch 'master' into rif
mykolaf Jan 28, 2019
c141395
lower log level, rm unused code
Feb 13, 2019
7fe7e5a
fix log message
Feb 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions orchagent/flexcounterorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
extern sai_port_api_t *sai_port_api;

extern PortsOrch *gPortsOrch;
extern IntfsOrch *gIntfsOrch;

unordered_map<string, string> flexCounterGroupMap =
{
Expand All @@ -18,6 +19,7 @@ unordered_map<string, string> flexCounterGroupMap =
{"PFCWD", PFC_WD_FLEX_COUNTER_GROUP},
{"QUEUE_WATERMARK", QUEUE_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP},
{"PG_WATERMARK", PG_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP},
{"RIF", RIF_STAT_COUNTER_FLEX_COUNTER_GROUP},
};


Expand Down Expand Up @@ -78,6 +80,7 @@ void FlexCounterOrch::doTask(Consumer &consumer)
// The queue maps will be generated as soon as counters are enabled
gPortsOrch->generateQueueMap();
gPortsOrch->generatePriorityGroupMap();
gIntfsOrch->generateInterfaceMap();

vector<FieldValueTuple> fieldValues;
fieldValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, value);
Expand Down
145 changes: 145 additions & 0 deletions orchagent/intfsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <map>
#include <net/if.h>

#include "sai_serialize.h"
#include "intfsorch.h"
#include "ipprefix.h"
#include "logger.h"
Expand All @@ -30,10 +31,49 @@ extern BufferOrch *gBufferOrch;

const int intfsorch_pri = 35;

#define RIF_FLEX_STAT_COUNTER_POLL_MSECS "1000"
#define UPDATE_MAPS_SEC 1

static const vector<sai_router_interface_stat_t> rifStatIds =
{
SAI_ROUTER_INTERFACE_STAT_IN_PACKETS,
SAI_ROUTER_INTERFACE_STAT_IN_OCTETS,
SAI_ROUTER_INTERFACE_STAT_IN_ERROR_PACKETS,
SAI_ROUTER_INTERFACE_STAT_IN_ERROR_OCTETS,
SAI_ROUTER_INTERFACE_STAT_OUT_PACKETS,
SAI_ROUTER_INTERFACE_STAT_OUT_OCTETS,
SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_PACKETS,
SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS,
};

IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch) :
Orch(db, tableName, intfsorch_pri), m_vrfOrch(vrf_orch)
{
SWSS_LOG_ENTER();

/* Initialize DB connectors */
m_counter_db = shared_ptr<DBConnector>(new DBConnector(COUNTERS_DB, DBConnector::DEFAULT_UNIXSOCKET, 0));
m_flex_db = shared_ptr<DBConnector>(new DBConnector(FLEX_COUNTER_DB, DBConnector::DEFAULT_UNIXSOCKET, 0));
// m_state_db = shared_ptr<DBConnector>(new DBConnector(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented code

m_asic_db = shared_ptr<DBConnector>(new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0));
/* Initialize COUNTER_DB tables */
m_rifNameTable = unique_ptr<Table>(new Table(m_counter_db.get(), COUNTERS_RIF_NAME_MAP));
m_rifTypeTable = unique_ptr<Table>(new Table(m_counter_db.get(), COUNTERS_RIF_TYPE_MAP));

// m_stateInterfaceTable = unique_ptr<Table>(new Table(m_state_db.get(), STATE_INTERFACE_TABLE_NAME));
m_vidToRidTable = unique_ptr<Table>(new Table(m_asic_db.get(), "VIDTORID"));
auto intervT = timespec { .tv_sec = UPDATE_MAPS_SEC , .tv_nsec = 0 };
m_updateMapsTimer = new SelectableTimer(intervT);
auto executorT = new ExecutableTimer(m_updateMapsTimer, this, "UPDATE_MAPS_TIMER");
Orch::addExecutor(executorT);
/* Initialize FLEX_COUNTER_DB tables */
m_flexCounterTable = unique_ptr<ProducerTable>(new ProducerTable(m_flex_db.get(), FLEX_COUNTER_TABLE));
m_flexCounterGroupTable = unique_ptr<ProducerTable>(new ProducerTable(m_flex_db.get(), FLEX_COUNTER_GROUP_TABLE));

vector<FieldValueTuple> fieldValues;
fieldValues.emplace_back(POLL_INTERVAL_FIELD, RIF_FLEX_STAT_COUNTER_POLL_MSECS);
fieldValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ);
m_flexCounterGroupTable->set(RIF_STAT_COUNTER_FLEX_COUNTER_GROUP, fieldValues);
}

sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias)
Expand Down Expand Up @@ -431,6 +471,7 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
port.m_vr_id = vrf_id;

gPortsOrch->setPort(port.m_alias, port);
m_rifsToAdd.push_back(port);

SWSS_LOG_NOTICE("Create router interface %s MTU %u", port.m_alias.c_str(), port.m_mtu);

Expand All @@ -447,6 +488,9 @@ bool IntfsOrch::removeRouterIntfs(Port &port)
return false;
}

const auto id = sai_serialize_object_id(port.m_rif_id);
removeRifFromFlexCounter(id, port.m_alias);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called directly whereas addRifToFlexCounter is based on timer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is due to FC logic.
The Flex counter will try to determine the object's type based on the vid. The VIDTOIRID is not instantly available on create.
For the delete flow it's the opposite.

If there will be some issues with this logic, I believe the FC flow should be changed. Right now it's tricky to dynamically add/remove objects that need to be polled.

Copy link

@tieguoevan tieguoevan May 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to ensure deleting flex counter is before deleting rif in syncd? If deleting rif happened before deleting flex counter, rid cannot be found in the VIDTOIRID.


sai_status_t status = sai_router_intfs_api->remove_router_interface(port.m_rif_id);
if (status != SAI_STATUS_SUCCESS)
{
Expand Down Expand Up @@ -652,3 +696,104 @@ void IntfsOrch::removeDirectedBroadcast(const Port &port, const IpAddress &ip_ad

SWSS_LOG_NOTICE("Remove broadcast route ip:%s", ip_addr.to_string().c_str());
}

void IntfsOrch::addRifToFlexCounter(const string &id, const string &name, const string &type)
{
SWSS_LOG_ENTER();
/* update RIF maps in COUNTERS_DB */
vector<FieldValueTuple> rifNameVector;
vector<FieldValueTuple> rifTypeVector;

rifNameVector.emplace_back(name, id);
rifTypeVector.emplace_back(id, type);

m_rifNameTable->set("", rifNameVector);
m_rifTypeTable->set("", rifTypeVector);

/* update RIF in FLEX_COUNTER_DB */
string key = getRifFlexCounterTableKey(id);


std::ostringstream counters_stream;
for (const auto& it: rifStatIds)
{
counters_stream << sai_serialize_router_interface_stat(it) << comma;
}

/* check the state of intf, if registering the intf to FC will result in runtime error */
vector<FieldValueTuple> fvt;
vector<FieldValueTuple> fieldValues;
fieldValues.emplace_back(RIF_COUNTER_ID_LIST, counters_stream.str());

m_flexCounterTable->set(key, fieldValues);
SWSS_LOG_DEBUG("Registered interface %s to Flex counter", name.c_str());
}

void IntfsOrch::removeRifFromFlexCounter(const string &id, const string &name)
{
SWSS_LOG_ENTER();
/* remove it from COUNTERS_DB maps */
m_rifNameTable->hdel("", name);
m_rifTypeTable->hdel("", id);

/* remove it from FLEX_COUNTER_DB */
string key = getRifFlexCounterTableKey(id);

vector<FieldValueTuple> fieldValues;
fieldValues.emplace_back(RIF_COUNTER_ID_LIST, "");

m_flexCounterTable->set(key, fieldValues);
SWSS_LOG_DEBUG("Unregistered interface %s from Flex counter", name.c_str());
}

string IntfsOrch::getRifFlexCounterTableKey(string key)
{
return string(RIF_STAT_COUNTER_FLEX_COUNTER_GROUP) + ":" + key;
}

void IntfsOrch::generateInterfaceMap()
{
m_updateMapsTimer->start();
}

void IntfsOrch::doTask(SelectableTimer &timer)
{
SWSS_LOG_ENTER();

SWSS_LOG_NOTICE("Registering %ld new intfs, deleting %ld ", m_rifsToAdd.size(), m_rifsToRemove.size());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to change to INFO/DEBUG level

string value;
for (auto it = m_rifsToAdd.begin(); it != m_rifsToAdd.end(); ++it)
{
SWSS_LOG_NOTICE("Registering begin -> %s ", it->m_alias.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have a separate for-loop for this log? With the log in below for-loop, this appears to be redundant.

}
for (auto it = m_rifsToAdd.begin(); it != m_rifsToAdd.end(); )
{
const auto id = sai_serialize_object_id(it->m_rif_id);
SWSS_LOG_NOTICE("Registering %s, id %s", it->m_alias.c_str(), id.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to change this to INFO level

std::string type;
switch(it->m_type)
{
case Port::PHY:
case Port::LAG:
type = "SAI_ROUTER_INTERFACE_TYPE_PORT";
break;
case Port::VLAN:
type = "SAI_ROUTER_INTERFACE_TYPE_VLAN";
break;
default:
SWSS_LOG_ERROR("Unsupported port type: %d", it->m_type);
type = "";
break;
}
if (m_vidToRidTable->hget("", id, value))
{
SWSS_LOG_NOTICE("Registering %s it is ready", it->m_alias.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest INFO level

addRifToFlexCounter(id, it->m_alias, type);
it = m_rifsToAdd.erase(it);
}
else
{
++it;
}
}
}
25 changes: 25 additions & 0 deletions orchagent/intfsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "orch.h"
#include "portsorch.h"
#include "vrforch.h"
#include "timer.h"

#include "ipaddresses.h"
#include "ipprefix.h"
Expand All @@ -15,6 +16,8 @@
extern sai_object_id_t gVirtualRouterId;
extern MacAddress gMacAddress;

#define RIF_STAT_COUNTER_FLEX_COUNTER_GROUP "RIF_STAT_COUNTER"

struct IntfsEntry
{
std::set<IpPrefix> ip_addresses;
Expand All @@ -35,10 +38,32 @@ class IntfsOrch : public Orch

bool setRouterIntfsMtu(Port &port);
std::set<IpPrefix> getSubnetRoutes();

void generateInterfaceMap();
void addRifToFlexCounter(const string&, const string&, const string&);
void removeRifFromFlexCounter(const string&, const string&);

private:

SelectableTimer* m_updateMapsTimer = nullptr;
std::vector<Port> m_rifsToAdd;
std::vector<Port> m_rifsToRemove;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this updated any where. Is it missed?


VRFOrch *m_vrfOrch;
IntfsTable m_syncdIntfses;
void doTask(Consumer &consumer);
void doTask(SelectableTimer &timer);

shared_ptr<DBConnector> m_counter_db;
shared_ptr<DBConnector> m_flex_db;
shared_ptr<DBConnector> m_asic_db;
unique_ptr<Table> m_rifNameTable;
unique_ptr<Table> m_rifTypeTable;
unique_ptr<Table> m_vidToRidTable;
unique_ptr<ProducerTable> m_flexCounterTable;
unique_ptr<ProducerTable> m_flexCounterGroupTable;

std::string getRifFlexCounterTableKey(std::string s);

int getRouterIntfsRefCount(const string&);

Expand Down