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

Sflow orchagent changes #1012

Merged
merged 10 commits into from
Oct 28, 2019
Merged

Sflow orchagent changes #1012

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9974a14
Sflow orchagent changes
Jul 11, 2019
1714af3
Addressing review comments
Sep 6, 2019
5d20b8e
Addressig review comments
Oct 4, 2019
0a784d8
Fixing build
Oct 4, 2019
d4a2b52
Fixing build
Oct 4, 2019
7674af7
Addressing code review comments
Oct 11, 2019
d31ccd1
Renaming variables
Oct 12, 2019
082232d
Merge branch 'master' into sfloworch
lguohan Oct 14, 2019
b34b48e
Change admin_state
GarrickHe Oct 17, 2019
ef110c3
Merge branch 'master' of https://github.com/Azure/sonic-swss into sfl…
Oct 22, 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
9 changes: 7 additions & 2 deletions cfgmgr/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ CFLAGS_SAI = -I /usr/include/sai
LIBNL_CFLAGS = -I/usr/include/libnl3
LIBNL_LIBS = -lnl-genl-3 -lnl-route-3 -lnl-3

bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd
bin_PROGRAMS = vlanmgrd teammgrd portmgrd intfmgrd buffermgrd vrfmgrd nbrmgrd vxlanmgrd sflowmgrd

if DEBUG
DBGFLAGS = -ggdb -DDEBUG
Expand Down Expand Up @@ -49,4 +49,9 @@ nbrmgrd_LDADD = -lswsscommon $(LIBNL_LIBS)
vxlanmgrd_SOURCES = vxlanmgrd.cpp vxlanmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h
vxlanmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
vxlanmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
vxlanmgrd_LDADD = -lswsscommon
vxlanmgrd_LDADD = -lswsscommon

sflowmgrd_SOURCES = sflowmgrd.cpp sflowmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h
sflowmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
sflowmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
sflowmgrd_LDADD = -lswsscommon
370 changes: 370 additions & 0 deletions cfgmgr/sflowmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,370 @@
#include "logger.h"
#include "dbconnector.h"
#include "producerstatetable.h"
#include "tokenize.h"
#include "ipprefix.h"
#include "sflowmgr.h"
#include "exec.h"
#include "shellcmd.h"

using namespace std;
using namespace swss;

map<string,string> sflowSpeedRateInitMap =
{
{SFLOW_SAMPLE_RATE_KEY_400G, SFLOW_SAMPLE_RATE_VALUE_400G},
{SFLOW_SAMPLE_RATE_KEY_100G, SFLOW_SAMPLE_RATE_VALUE_100G},
{SFLOW_SAMPLE_RATE_KEY_50G, SFLOW_SAMPLE_RATE_VALUE_50G},
{SFLOW_SAMPLE_RATE_KEY_40G, SFLOW_SAMPLE_RATE_VALUE_40G},
{SFLOW_SAMPLE_RATE_KEY_25G, SFLOW_SAMPLE_RATE_VALUE_25G},
{SFLOW_SAMPLE_RATE_KEY_10G, SFLOW_SAMPLE_RATE_VALUE_10G},
{SFLOW_SAMPLE_RATE_KEY_1G, SFLOW_SAMPLE_RATE_VALUE_1G}
};

SflowMgr::SflowMgr(DBConnector *cfgDb, DBConnector *appDb, const vector<string> &tableNames) :
Orch(cfgDb, tableNames),
m_cfgSflowTable(cfgDb, CFG_SFLOW_TABLE_NAME),
m_cfgSflowSessionTable(cfgDb, CFG_SFLOW_SESSION_TABLE_NAME),
m_appSflowTable(appDb, APP_SFLOW_TABLE_NAME),
m_appSflowSessionTable(appDb, APP_SFLOW_SESSION_TABLE_NAME)
{
intf_all_conf = true;
gEnable = false;
}

void SflowMgr::sflowHandleService(bool enable)
{
stringstream cmd;
string res;

SWSS_LOG_ENTER();

if (enable)
{
cmd << "service hsflowd restart";
}
else
{
cmd << "service hsflowd stop";
}

int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
else
{
SWSS_LOG_INFO("Command '%s' succeeded", cmd.str().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.

You may want to put is as NOTICE stating "Starting hsflowd service"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}

}

void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
{
auto it = consumer.m_toSync.begin();

while (it != consumer.m_toSync.end())
{
KeyOpFieldsValuesTuple t = it->second;

string key = kfvKey(t);
string op = kfvOp(t);
auto values = kfvFieldsValues(t);

if (op == SET_COMMAND)
{
SflowLocalPortInfo port_info;
bool new_port = false;

auto sflowPortConf = m_sflowPortLocalConfMap.find(key);
if (sflowPortConf == m_sflowPortLocalConfMap.end())
{
new_port = true;
port_info.local_conf = false;
port_info.speed = SFLOW_ERROR_SPEED_STR;
port_info.local_rate = "";
port_info.local_admin = "";
m_sflowPortLocalConfMap[key] = port_info;
}
for (auto i : values)
{
if (fvField(i) == "speed")
{
m_sflowPortLocalConfMap[key].speed = fvValue(i);
}
}

if (new_port)
{
if (gEnable && intf_all_conf)
{
vector<FieldValueTuple> fvs;
sflowGetGlobalFvs(fvs, m_sflowPortLocalConfMap[key].speed);
m_appSflowSessionTable.set(key, fvs);
}
}
}
else if (op == DEL_COMMAND)
{
auto sflowPortConf = m_sflowPortLocalConfMap.find(key);
if (sflowPortConf != m_sflowPortLocalConfMap.end())
{
bool local_cfg = m_sflowPortLocalConfMap[key].local_conf;

m_sflowPortLocalConfMap.erase(key);
if ((intf_all_conf && gEnable) || local_cfg)
{
m_appSflowSessionTable.del(key);
}
}
}
it = consumer.m_toSync.erase(it);
}
}

void SflowMgr::sflowHandleSessionAll(bool enable)
{
for (auto it: m_sflowPortLocalConfMap)
{
if (!it.second.local_conf)
{
vector<FieldValueTuple> fvs;
sflowGetGlobalFvs(fvs, it.second.speed);
if (enable)
{
m_appSflowSessionTable.set(it.first, fvs);
}
else
{
m_appSflowSessionTable.del(it.first);
}
}
}
}

void SflowMgr::sflowHandleSessionLocal(bool enable)
{
for (auto it: m_sflowPortLocalConfMap)
{
if (it.second.local_conf)
{
vector<FieldValueTuple> fvs;
sflowGetLocalFvs(fvs, it.second);
if (enable)
{
m_appSflowSessionTable.set(it.first, fvs);
}
else
{
m_appSflowSessionTable.del(it.first);
}
}
}
}

void SflowMgr::sflowGetGlobalFvs(vector<FieldValueTuple> &fvs, string speed)
{
string rate;
FieldValueTuple fv1("admin_state", "enable");
fvs.push_back(fv1);

if (speed != SFLOW_ERROR_SPEED_STR)
{
rate = sflowSpeedRateInitMap[speed];
}
else
{
rate = SFLOW_ERROR_SPEED_STR;
}
FieldValueTuple fv2("sample_rate",rate);
fvs.push_back(fv2);
}

void SflowMgr::sflowGetLocalFvs(vector<FieldValueTuple> &fvs, SflowLocalPortInfo &local_info)
{
if (local_info.local_admin.length() > 0)
{
FieldValueTuple fv1("admin_state", local_info.local_admin);
fvs.push_back(fv1);
}

FieldValueTuple fv2("sample_rate", local_info.local_rate);
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not sure why only local_admin.length() is checked?. isn't it required for local_rate also?. Another option is when you create the local_info, you can populate the global values for admin and rate and change when localInfo is updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Prince,
While it is not required for the local_admin to be externally configured, the default can be derived in orchagent from "sflow status" using config sflow enable. So only when admin is configured externally for a port that overrides global setting, this field needs to be filled.
For rate however, the manager must necessarily fill the information at all cases. If externally configured the rate string is used as such and if not the rate is derived from the speed of the interface. Thus the rate string will always exists while admin is only when configured which is the reason why the check is not done for rate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have modified the logic so that both admin as well as rate are published and both information would be present in app-db even if user CLI provides only one. The other would be derived and published in appDb so that both orchagent and CLI would have clear picture of the configuration rather than having assumptions

fvs.push_back(fv2);
}

void SflowMgr::sflowUpdateLocalPortInfo(string alias, vector<FieldValueTuple> &fvs)
{
for (auto i : fvs)
{
if (fvField(i) == "admin_state")
{
m_sflowPortLocalConfMap[alias].local_admin = fvValue(i);
}
else if (fvField(i) == "sample_rate")
{
m_sflowPortLocalConfMap[alias].local_rate = fvValue(i);
}
}
}

void SflowMgr::sflowCheckAndFillRate(string alias, vector<FieldValueTuple> &fvs)
{
string rate;

for (auto i : fvs)
{
if (fvField(i) == "sample_rate")
{
/* Rate exists already. */
return;
}
}
string speed = m_sflowPortLocalConfMap[alias].speed;

if (speed != SFLOW_ERROR_SPEED_STR)
{
rate = sflowSpeedRateInitMap[speed];
}
else
{
rate = SFLOW_ERROR_SPEED_STR;
}
m_sflowPortLocalConfMap[alias].local_rate = rate;
FieldValueTuple fv("sample_rate",rate);
fvs.push_back(fv);
}

void SflowMgr::doTask(Consumer &consumer)
{
SWSS_LOG_ENTER();

auto table = consumer.getTableName();

if (table == CFG_PORT_TABLE_NAME)
{
sflowUpdatePortInfo(consumer);
return;
}

auto it = consumer.m_toSync.begin();
while (it != consumer.m_toSync.end())
prsunny marked this conversation as resolved.
Show resolved Hide resolved
{
KeyOpFieldsValuesTuple t = it->second;

string key = kfvKey(t);
string op = kfvOp(t);
auto values = kfvFieldsValues(t);

if (op == SET_COMMAND)
{
if (table == CFG_SFLOW_TABLE_NAME)
{
for (auto i : values)
{
if (fvField(i) == "admin_state")
{
bool enable = false;
if (fvValue(i) == "enable")
{
prsunny marked this conversation as resolved.
Show resolved Hide resolved
enable = true;
}
if (enable == gEnable)
{
break;
}
gEnable = enable;
sflowHandleService(enable);
if (intf_all_conf)
{
sflowHandleSessionAll(enable);
}
sflowHandleSessionLocal(enable);
prsunny marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +291 to +293
Copy link
Contributor

Choose a reason for hiding this comment

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

Typical practice for admin_state toggle is just to propagate the field-value tuple change to APPL_DB. Here, when looking into these two function internals, admin_state: down triggers deleting the entire key, say Ethernet1, in APPL_DB SFLOW_SESSION table instead of an update to field admin_state under the key. This is consistent with the HLD. But quite curious about the design thought behind it.

Copy link
Collaborator Author

@dgsudharsan dgsudharsan Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@wendani : Sflow has the following commands

  1. config sflow enable ------------> Enable/disable feature as a whole. When this is disabled, it means all sflow configurations need to be removed (Feature level disable).
  2. config sflow interface all enable/disable ----> Global level sflow command to control the enabling/disabling sflow globally. By default when sflow feature is enabled, all ports will have flow enabled. The main usage of this command is in 'disable' configuration. When users need to enable sflow on only one interface or very few interfaces, without global interface disable command, the user has to configure individual interface disable on the rest of the interfaces except for the interfaces of interest. Thus to avoid this, users can have sflow global interface all disable and then do individual sflow enable on interested interfaces which would override the global setting.
  3. config sflow interface Ethernetx enable/disable ------> Local interface level command which would override global interface all command when configured on specific interfaces

}
}
m_appSflowTable.set(key, values);
}
else if (table == CFG_SFLOW_SESSION_TABLE_NAME)
{
if (key == "all")
{
for (auto i : values)
{
if (fvField(i) == "admin_state")
{
bool enable = false;

if (fvValue(i) == "enable")
{
enable = true;
}
if ((enable != intf_all_conf) && (gEnable))
{
sflowHandleSessionAll(enable);
}
intf_all_conf = enable;
}
}
}
else
{
auto sflowPortConf = m_sflowPortLocalConfMap.find(key);

if (sflowPortConf == m_sflowPortLocalConfMap.end())
{
it++;
continue;
}
if ((m_sflowPortLocalConfMap[key].local_rate == "") ||
(m_sflowPortLocalConfMap[key].local_rate == SFLOW_ERROR_SPEED_STR))
{
sflowCheckAndFillRate(key,values);
}
sflowUpdateLocalPortInfo(key,values);
m_sflowPortLocalConfMap[key].local_conf = true;
m_appSflowSessionTable.set(key, values);
}
}
}
else if (op == DEL_COMMAND)
{
if (table == CFG_SFLOW_TABLE_NAME)
{
if (gEnable)
{
sflowHandleService(false);
sflowHandleSessionAll(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also call sflowHandleSessionLocal(false); here to disable individually configured port(s); otherwise redis-cli -n 4 del "SFLOW|global" command will not remove APPL_DB SFLOW_SESSION_TABLE:EthernetX for individually configured port EthernetX.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. This needs to be fixed.

}
gEnable = false;
m_appSflowTable.del(key);
}
else if (table == CFG_SFLOW_SESSION_TABLE_NAME)
{
if (key == "all")
{
if (!intf_all_conf)
{
sflowHandleSessionAll(true);
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 with true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When DEL is issued for "config sflow interface all" command and it is currently not enabled, it means we are removing "config sflow interface disable all" CLI. Once it is removed, the default state is all interfaces are enabled and thus the API is called with true

Copy link
Contributor

Choose a reason for hiding this comment

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

Should feed m_gEnable instead of true. Consider the following sequence of CLIs:

$ sudo config sflow interface disable all

m_intfAllConf is false

$ redis-cli -n 4 del "SFLOW|global"

m_gEnable is false

$ redis-cli -n 4 del "SFLOW_SESSION|all"

APPL_DB SFLOW_SESSION_TABLE:EthernetX will show up for non-individually configured port EthernetX under the condition that m_gEnable is false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed that APP_DB would be populated but there is a check in orchagent which will ensure that ports are not programmed when sflow feature is disabled. Hence there will not be any functional issue.
I saw another scenario too where similarly appDB would be populated but it wouldn't be pushed to ASIC DB. When feature is disabled and when config sflow interface Ethernetx sample-rate/enable command is executed we can find them present in APP_DB.
These scenarios need to be cleaned up.

}
intf_all_conf = true;
}
else
{
m_appSflowSessionTable.del(key);
m_sflowPortLocalConfMap[key].local_conf = false;
m_sflowPortLocalConfMap[key].local_rate = "";
m_sflowPortLocalConfMap[key].local_admin = "";

/* If Global configured, set global session on port after local config is deleted */
if (intf_all_conf)
{
vector<FieldValueTuple> fvs;
sflowGetGlobalFvs(fvs, m_sflowPortLocalConfMap[key].speed);
m_appSflowSessionTable.set(key,fvs);
}
}
}
}
it = consumer.m_toSync.erase(it);
}
}
Loading