Skip to content

swss: Fix sFlow sampling-rate and admin-state #1224

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

Closed
GarrickHe wants to merge 1 commit into from
Closed

swss: Fix sFlow sampling-rate and admin-state #1224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 32 additions & 13 deletions cfgmgr/sflowmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
if (sflowPortConf == m_sflowPortConfMap.end())
{
new_port = true;
port_info.local_conf = false;
port_info.local_rate_cfg = false;
port_info.local_admin_cfg = false;
port_info.speed = SFLOW_ERROR_SPEED_STR;
port_info.rate = "";
port_info.admin = "";
Expand Down Expand Up @@ -111,7 +112,8 @@ void SflowMgr::sflowUpdatePortInfo(Consumer &consumer)
auto sflowPortConf = m_sflowPortConfMap.find(key);
if (sflowPortConf != m_sflowPortConfMap.end())
{
bool local_cfg = m_sflowPortConfMap[key].local_conf;
bool local_cfg = m_sflowPortConfMap[key].local_rate_cfg ||
m_sflowPortConfMap[key].local_admin_cfg;

m_sflowPortConfMap.erase(key);
if ((m_intfAllConf && m_gEnable) || local_cfg)
Expand All @@ -128,18 +130,27 @@ void SflowMgr::sflowHandleSessionAll(bool enable)
{
for (auto it: m_sflowPortConfMap)
{
if (!it.second.local_conf)
Copy link
Contributor

@wendani wendani Jul 27, 2020

Choose a reason for hiding this comment

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

In configuring admin_state field of CONFIG_DB SFLOW|global table, both sflowHandleSessionAll and sflowHandleSessionLocal will be called if m_intfAllConf is true (default value). Before your change, it.second.local_conf, to my understanding, controls the mutual exclusion that an individual port is set in either sflowHandleSessionAll or sflowHandleSessionLocal, depending on whether it has its own configuration or not. Now you remove this variable, a port having its own config can be set twice to APPL_DB by both sflowHandleSessionAll and sflowHandleSessionLocal, calling m_appSflowSessionTable.set(it.first, fvs).

Copy link
Author

@GarrickHe GarrickHe Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@wangxin - Are you asking a question or suggesting a change?

I replaced port_info.local_conf with two variables port_info.local_rate_cfg and port_info.local_admin_cfg to differeniate between a local change to the port's admin-state and a local change to the port's sampling-rate. This change is needed to address the issue at hand. Without it we will hit a case where the user configured a port's sampling-rate, but the port will not obey the global admin-status (because there is a local config), as the unfixed UT demonstrates.

Now you remove this variable, a port having its own config can be set twice to APPL_DB by both sflowHandleSessionAll and sflowHandleSessionLocal, calling m_appSflowSessionTable.set(it.first, fvs).

Not sure what you mean here. If a port has a local config (either sample-rate, admin-status, or both), APPL_DB will be set when either sflowHandleSessionAll or sFlowHandleSessionLocal is called. This behavior is exactly the same as when there was only one variable port_info.local_config, because we are doing a OR (||) check:

https://github.com/Azure/sonic-swss/pull/1224/files#diff-e3e7c0a4d48317ec70d5339d6fd3c6d1R136
https://github.com/Azure/sonic-swss/pull/1224/files#diff-e3e7c0a4d48317ec70d5339d6fd3c6d1R162

Each of those function can configure ApplDB once as needed.

Copy link
Contributor

@wendani wendani Jul 27, 2020

Choose a reason for hiding this comment

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

As you said in reply "APPL_DB will be set when either sflowHandleSessionAll or sFlowHandleSessionLocal is called", APPL_DB will be set twice here https://github.com/Azure/sonic-swss/blob/master/cfgmgr/sflowmgr.cpp#L289

Copy link
Author

@GarrickHe GarrickHe Jul 27, 2020

Choose a reason for hiding this comment

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

Yes, once per function call and only if m_intfallConf evaluates to true. This is needed. I'm still not sure if you're trying to make a suggestion or asking a question...

if (enable)
{
vector<FieldValueTuple> fvs;
sflowGetGlobalInfo(fvs, it.second.speed);
if (enable)
if (it.second.local_rate_cfg || it.second.local_admin_cfg)
{
m_appSflowSessionTable.set(it.first, fvs);
sflowGetPortInfo(fvs, it.second);
/* Use global admin state if there is not a local one */
if (!it.second.local_admin_cfg) {
FieldValueTuple fv1("admin_state", "up");
fvs.push_back(fv1);
}
}
else
{
m_appSflowSessionTable.del(it.first);
sflowGetGlobalInfo(fvs, it.second.speed);
}
m_appSflowSessionTable.set(it.first, fvs);
}
else if (!it.second.local_admin_cfg)
{
m_appSflowSessionTable.del(it.first);
}
}
}
Expand All @@ -148,7 +159,7 @@ void SflowMgr::sflowHandleSessionLocal(bool enable)
{
for (auto it: m_sflowPortConfMap)
{
if (it.second.local_conf)
if (it.second.local_admin_cfg || it.second.local_rate_cfg)
{
vector<FieldValueTuple> fvs;
sflowGetPortInfo(fvs, it.second);
Expand Down Expand Up @@ -184,7 +195,7 @@ void SflowMgr::sflowGetGlobalInfo(vector<FieldValueTuple> &fvs, string speed)

void SflowMgr::sflowGetPortInfo(vector<FieldValueTuple> &fvs, SflowPortInfo &local_info)
{
if (local_info.admin.length() > 0)
if (local_info.local_admin_cfg)
{
FieldValueTuple fv1("admin_state", local_info.admin);
fvs.push_back(fv1);
Expand All @@ -206,17 +217,23 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &fv
{
rate_present = true;
m_sflowPortConfMap[alias].rate = fvValue(i);
m_sflowPortConfMap[alias].local_rate_cfg = true;
}
if (fvField(i) == "admin_state")
{
admin_present = true;
m_sflowPortConfMap[alias].admin = fvValue(i);
m_sflowPortConfMap[alias].local_admin_cfg = true;
}
}

if (!rate_present)
{
if (m_sflowPortConfMap[alias].rate == "")
/* Go back to default sample-rate if there is not existing rate OR
* if a local config has been done but the rate has been removed
*/
if (m_sflowPortConfMap[alias].rate == "" ||
m_sflowPortConfMap[alias].local_rate_cfg)
{
string speed = m_sflowPortConfMap[alias].speed;

Expand All @@ -230,6 +247,7 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &fv
}
m_sflowPortConfMap[alias].rate = rate;
}
m_sflowPortConfMap[alias].local_rate_cfg = false;
FieldValueTuple fv("sample_rate", m_sflowPortConfMap[alias].rate);
fvs.push_back(fv);
}
Expand All @@ -238,9 +256,10 @@ void SflowMgr::sflowCheckAndFillValues(string alias, vector<FieldValueTuple> &fv
{
if (m_sflowPortConfMap[alias].admin == "")
{
/* By default admin state is enable if not set explicitely */
/* By default admin state is enable if not set explicitly */
m_sflowPortConfMap[alias].admin = "up";
}
m_sflowPortConfMap[alias].local_admin_cfg = false;
FieldValueTuple fv("admin_state", m_sflowPortConfMap[alias].admin);
fvs.push_back(fv);
}
Expand Down Expand Up @@ -327,7 +346,6 @@ void SflowMgr::doTask(Consumer &consumer)
continue;
}
sflowCheckAndFillValues(key,values);
m_sflowPortConfMap[key].local_conf = true;
m_appSflowSessionTable.set(key, values);
}
}
Expand Down Expand Up @@ -357,7 +375,8 @@ void SflowMgr::doTask(Consumer &consumer)
else
{
m_appSflowSessionTable.del(key);
m_sflowPortConfMap[key].local_conf = false;
m_sflowPortConfMap[key].local_rate_cfg = false;
m_sflowPortConfMap[key].local_admin_cfg = false;
m_sflowPortConfMap[key].rate = "";
m_sflowPortConfMap[key].admin = "";

Expand Down
5 changes: 3 additions & 2 deletions cfgmgr/sflowmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ namespace swss {

struct SflowPortInfo
{
bool local_conf;
bool local_rate_cfg;
bool local_admin_cfg;
std::string speed;
std::string rate;
std::string admin;
Expand All @@ -50,7 +51,7 @@ class SflowMgr : public Orch
Table m_cfgSflowSessionTable;
ProducerStateTable m_appSflowTable;
ProducerStateTable m_appSflowSessionTable;
SflowPortConfMap m_sflowPortConfMap;
SflowPortConfMap m_sflowPortConfMap;
bool m_intfAllConf;
bool m_gEnable;

Expand Down