Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
176 changes: 176 additions & 0 deletions cfgorch/cfgorch.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
#include "cfgorch.h"
#include "logger.h"
#include "subscriberstatetable.h"

using namespace swss;

CfgOrch::CfgOrch(DBConnector *db, string tableName) :
m_db(db)
{
Consumer consumer(new SubscriberStateTable(m_db, tableName));
m_consumerMap.insert(ConsumerMapPair(tableName, consumer));
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2017

Choose a reason for hiding this comment

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

insert [](start = 18, length = 6)

Use emplace to prevent object copy.

}

CfgOrch::CfgOrch(DBConnector *db, vector<string> &tableNames) :
m_db(db)
{
for(auto it : tableNames)
{
Consumer consumer(new SubscriberStateTable(m_db, it));
m_consumerMap.insert(ConsumerMapPair(it, consumer));
}
}

CfgOrch::~CfgOrch()
{
for(auto &it : m_consumerMap)
delete it.second.m_consumer;
}

vector<Selectable *> CfgOrch::getSelectables()
{
vector<Selectable *> selectables;
for(auto it : m_consumerMap) {
selectables.push_back(it.second.m_consumer);
}
return selectables;
}

bool CfgOrch::hasSelectable(TableConsumable *selectable) const
{
for(auto it : m_consumerMap) {
if (it.second.m_consumer == selectable) {
return true;
}
}
return false;
}

bool CfgOrch::syncCfgDB(string tableName, Table &tableConsumer)
{
SWSS_LOG_ENTER();

auto consumer_it = m_consumerMap.find(tableName);
if (consumer_it == m_consumerMap.end())
{
SWSS_LOG_ERROR("Unrecognized tableName:%s\n", tableName.c_str());
return false;
}
Consumer& consumer = consumer_it->second;

vector<KeyOpFieldsValuesTuple> tuples;

tableConsumer.getTableContent(tuples);
for (auto tuple : tuples)
{
string key = kfvKey(tuple);
/* Directly put it into consumer.m_toSync map */
if (consumer.m_toSync.find(key) == consumer.m_toSync.end())
{
consumer.m_toSync[key] = make_tuple(key, SET_COMMAND, kfvFieldsValues(tuple));
SWSS_LOG_DEBUG("%s", (dumpTuple(consumer, tuple)).c_str());
}
/*
* Syncing from DB directly, don't expect duplicate keys.
* Or there is pending task from consumber state pipe, in this case just skip it.
*/
else
{
SWSS_LOG_WARN("Duplicate key %s found in tableName:%s\n", key.c_str(), tableName.c_str());
continue;
}
doTask(consumer);
}
return true;
}

bool CfgOrch::execute(string tableName)
{
SWSS_LOG_ENTER();

auto consumer_it = m_consumerMap.find(tableName);
if (consumer_it == m_consumerMap.end())
{
SWSS_LOG_ERROR("Unrecognized tableName:%s\n", tableName.c_str());
return false;
}
Consumer& consumer = consumer_it->second;

int data_popped = 0;
while (1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 [](start = 11, length = 1)

true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, will change it.

{
KeyOpFieldsValuesTuple new_data;
consumer.m_consumer->pop(new_data);

string key = kfvKey(new_data);
string op = kfvOp(new_data);
/*
* Done with all new data. Or
* possible nothing popped, ie. the oparation is already merged with other operations
*/
if (op.empty())
{
SWSS_LOG_DEBUG("Number of kfv data popped: %d\n", data_popped);
break;
}
data_popped++;
SWSS_LOG_DEBUG("%s", (dumpTuple(consumer, new_data)).c_str());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

( [](start = 29, length = 1)

You could remove this pair.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok.


/* If a new task comes or if a DEL task comes, we directly put it into consumer.m_toSync map */
if (consumer.m_toSync.find(key) == consumer.m_toSync.end() || op == DEL_COMMAND)
{
consumer.m_toSync[key] = new_data;
}
/* If an old task is still there, we combine the old task with new task */
else
{
KeyOpFieldsValuesTuple existing_data = consumer.m_toSync[key];

auto new_values = kfvFieldsValues(new_data);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

auto [](start = 12, length = 4)

Many places, you could use "auto &" to reduce copying cost.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, will check how to sync with orch class too.

auto existing_values = kfvFieldsValues(existing_data);

for (auto it : new_values)
{
string field = fvField(it);
string value = fvValue(it);

auto iu = existing_values.begin();
while (iu != existing_values.end())
{
string ofield = fvField(*iu);
if (field == ofield)
iu = existing_values.erase(iu);
else
iu++;
}
existing_values.push_back(FieldValueTuple(field, value));
}
consumer.m_toSync[key] = KeyOpFieldsValuesTuple(key, op, existing_values);
}
}
if (!consumer.m_toSync.empty())
doTask(consumer);

return true;
}

void CfgOrch::doTask()
{
for(auto &it : m_consumerMap)
{
if (!it.second.m_toSync.empty())
doTask(it.second);
}
}

string CfgOrch::dumpTuple(Consumer &consumer, KeyOpFieldsValuesTuple &tuple)
{
string s = consumer.m_consumer->getTableName() + ":" + kfvKey(tuple)
+ "|" + kfvOp(tuple);
for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++)
{
s += "|" + fvField(*i) + ":" + fvValue(*i);
}

return s;
}
48 changes: 48 additions & 0 deletions cfgorch/cfgorch.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#pragma once

#include <map>
#include <memory>
#include "dbconnector.h"
#include "table.h"
#include "consumertable.h"

using namespace std;
using namespace swss;
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2017

Choose a reason for hiding this comment

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

using namespace should not be used in header file.
https://stackoverflow.com/questions/5849457/using-namespace-in-c-headers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, will remove them.


#define DEFAULT_KEY_SEPARATOR ":"
#define CONFIGDB_KEY_SEPARATOR "|"

typedef map<string, KeyOpFieldsValuesTuple> SyncMap;
struct Consumer {
Consumer(TableConsumable* consumer) : m_consumer(consumer) { }
TableConsumable* m_consumer;
/* Store the latest 'golden' status */
SyncMap m_toSync;
};
typedef pair<string, Consumer> ConsumerMapPair;
typedef map<string, Consumer> ConsumerMap;

class CfgOrch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CfgOrch [](start = 6, length = 7)

inherit from Orch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Orch looks too heavy for config orchestration, but cfgorch does have major overlap with orch. Will look into it further to see how to reduce the duplication.

{
public:
CfgOrch(DBConnector *db, string tableName);
CfgOrch(DBConnector *db, vector<string> &tableNames);
virtual ~CfgOrch();

vector<Selectable*> getSelectables();
bool hasSelectable(TableConsumable* s) const;

bool execute(string tableName);
/* Iterate all consumers in m_consumerMap and run doTask(Consumer) */
void doTask();

protected:
DBConnector *m_db;
ConsumerMap m_consumerMap;

/* Run doTask against a specific consumer */
virtual void doTask(Consumer &consumer) = 0;
string dumpTuple(Consumer &consumer, KeyOpFieldsValuesTuple &tuple);
bool syncCfgDB(string tableName, Table &tableConsumer);
};

1 change: 1 addition & 0 deletions debian/rules
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ include /usr/share/dpkg/default.mk

override_dh_auto_configure:
dh_auto_configure --
dh_auto_configure -- --enable-gtest
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--enable-gtest [](start = 22, length = 14)

It will not work in sonic-slave. The tests require redis service running.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure how sonic prepares unit test environment in Jenkins build. I install the debian package in sonic-slave and start redis service with "redis-server &" commond.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2017

Choose a reason for hiding this comment

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

You got it clear. So just keep original version, and manually test it with

./configure --enable-gtest
make
tests/tests


override_dh_auto_install:
dh_auto_install --destdir=debian/swss
Expand Down
11 changes: 6 additions & 5 deletions tests/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
CFLAGS_SAI = -I /usr/include/sai
INCLUDES = -I ../orchagent
INCLUDES = -I ../orchagent -I $(top_srcdir)/cfgorch

bin_PROGRAMS = tests

Expand All @@ -9,12 +9,13 @@ else
DBGFLAGS = -g -DNDEBUG
endif

CFLAGS_GTEST =
LDADD_GTEST =
CFLAGS_GTEST = -I $(top_srcdir)/../sonic-swss-common/googletest/googletest/include
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$(top_srcdir)/.. [](start = 18, length = 16)

We should not refer any files outside the $(top_srcdir)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We only have google unit test environment in swss-common. What is your suggestion on how to build and run unit test in swss?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, the question is do we want to build unit test binary automatically like that in swss-common? Or it is ok just put unit test code there, whoever is interested in running the unit test can figure out how to build and run it themselves?

The other option is to make gtest common library like swss-common.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

agree that we shall not include files outside this repository. I think the option 1 in the link Qi provided is preferred.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Oct 23, 2017

Choose a reason for hiding this comment

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

We plan to remove googletest from sonic-swss-common repo, and use installed Google Test DEB package near future.
It's ok to build the test if easy. I guess you will need to install Google Test DEB package in the sonic-slave docker.

LDADD_GTEST = $(top_srcdir)/../sonic-swss-common/googletest/build/googlemock/gtest/libgtest_main.a \
$(top_srcdir)/../sonic-swss-common/googletest/build/googlemock/gtest/libgtest.a

tests_SOURCES = swssnet_ut.cpp
tests_SOURCES = swssnet_ut.cpp cfgorch_ut.cpp $(top_srcdir)/cfgorch/cfgorch.cpp

tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
tests_LDADD = $(LDADD_GTEST) -lnl-genl-3 -lhiredis -lhiredis -lpthread \
-lswsscommon -lswsscommon -lgtest -lgtest_main
-lswsscommon -lswsscommon
Loading