Skip to content

Warm reboot: Orchagent state restore #554

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

Merged
lguohan merged 12 commits into from
Aug 16, 2018
Merged

Warm reboot: Orchagent state restore #554

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4b34c1b
Orchagent state restore
jipanyang Jul 30, 2018
17398e8
Adpat to the new warm reboot schema
jipanyang Aug 2, 2018
e2d262b
Set Executor name at constructor phase
jipanyang Aug 9, 2018
e4973fc
Use common Consumer::dumpTuple() for Orch::recordTuple() and Orch::du…
jipanyang Aug 9, 2018
f6f2555
Change the validation of pending task after restore to NOTICE level f…
jipanyang Aug 11, 2018
7c55562
Merge remote-tracking branch 'upstream/master' into warm_reboot_colla…
jipanyang Aug 11, 2018
e6bdeef
Remove unused function call, and update comment format
jipanyang Aug 12, 2018
a4f5742
Move orchagent state restore and sync up out of main select loop.
jipanyang Aug 13, 2018
8f1700e
Remove the extra round of data draining processing which is not needed.
jipanyang Aug 13, 2018
4324763
Address review comments
jipanyang Aug 14, 2018
dc13ecc
Merge remote-tracking branch 'upstream/master' into warm_reboot_colla…
jipanyang Aug 14, 2018
3e034d4
Merge branch 'master' into warm_reboot_collab_4_orchagent
lguohan Aug 15, 2018
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
8 changes: 5 additions & 3 deletions orchagent/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
INCLUDES = -I $(top_srcdir)
INCLUDES = -I $(top_srcdir) -I $(top_srcdir)/warmrestart

CFLAGS_SAI = -I /usr/include/sai

Expand Down Expand Up @@ -66,12 +66,14 @@ orchagent_SOURCES = \
switchorch.h \
swssnet.h \
tunneldecaporch.h \
crmorch.h
crmorch.h \
request_parser.h \
vrforch.h \
dtelorch.h \
countercheckorch.h \
flexcounterorch.h
flexcounterorch.h \
$(top_srcdir)/warmrestart/warm_restart.cpp \
$(top_srcdir)/warmrestart/warm_restart.h

orchagent_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
orchagent_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
Expand Down
14 changes: 7 additions & 7 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a
sai_object_id_t session_oid;

if (!m_pDTelOrch ||
(attr_name != ACTION_DTEL_FLOW_OP &&
(attr_name != ACTION_DTEL_FLOW_OP &&
attr_name != ACTION_DTEL_INT_SESSION &&
attr_name != ACTION_DTEL_FLOW_SAMPLE_PERCENT &&
attr_name != ACTION_DTEL_REPORT_ALL_PACKETS))
Expand Down Expand Up @@ -1356,7 +1356,7 @@ bool AclRuleDTelFlowWatchListEntry::validateAddAction(string attr_name, string a
value.aclaction.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false;
value.aclaction.enable = (attr_value == DTEL_ENABLED) ? true : false;
}

m_actions[aclDTelActionLookup[attr_name]] = value;

return true;
Expand Down Expand Up @@ -1514,7 +1514,7 @@ bool AclRuleDTelDropWatchListEntry::validateAddAction(string attr_name, string a

value.aclaction.parameter.booldata = (attr_value == DTEL_ENABLED) ? true : false;
value.aclaction.enable = (attr_value == DTEL_ENABLED) ? true : false;

m_actions[aclDTelActionLookup[attr_name]] = value;

return true;
Expand Down Expand Up @@ -1693,8 +1693,8 @@ void AclOrch::init(vector<TableConnector>& connectors, PortsOrch *portOrch, Mirr
// initialized before thread start.
auto interv = timespec { .tv_sec = COUNTERS_READ_INTERVAL, .tv_nsec = 0 };
auto timer = new SelectableTimer(interv);
auto executor = new ExecutableTimer(timer, this);
Orch::addExecutor("", executor);
auto executor = new ExecutableTimer(timer, this, "ACL_POLL_TIMER");
Orch::addExecutor(executor);
timer->start();
}

Expand Down Expand Up @@ -2320,7 +2320,7 @@ sai_status_t AclOrch::bindAclTable(sai_object_id_t table_oid, AclTable &aclTable
sai_status_t status = SAI_STATUS_SUCCESS;

SWSS_LOG_INFO("%s table %s to ports", bind ? "Bind" : "Unbind", aclTable.id.c_str());

if (aclTable.ports.empty())
{
if (bind)
Expand Down Expand Up @@ -2431,7 +2431,7 @@ sai_status_t AclOrch::createDTelWatchListTables()
SWSS_LOG_INFO("Successfully created ACL table %s, oid: %lX", flowWLTable.description.c_str(), table_oid);

/* Create Drop watchlist ACL table */

table_attrs.clear();

dropWLTable.id = TABLE_TYPE_DTEL_DROP_WATCHLIST;
Expand Down
4 changes: 2 additions & 2 deletions orchagent/countercheckorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ CounterCheckOrch::CounterCheckOrch(DBConnector *db, vector<string> &tableNames):

auto interv = timespec { .tv_sec = COUNTER_CHECK_POLL_TIMEOUT_SEC, .tv_nsec = 0 };
auto timer = new SelectableTimer(interv);
auto executor = new ExecutableTimer(timer, this);
Orch::addExecutor("MC_COUNTERS_POLL", executor);
auto executor = new ExecutableTimer(timer, this, "MC_COUNTERS_POLL");
Orch::addExecutor(executor);
timer->start();
}

Expand Down
9 changes: 6 additions & 3 deletions orchagent/crmorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,11 @@ CrmOrch::CrmOrch(DBConnector *db, string tableName):
m_resourcesMap.emplace(res.first, CrmResourceEntry(res.second, CRM_THRESHOLD_TYPE_DEFAULT, CRM_THRESHOLD_LOW_DEFAULT, CRM_THRESHOLD_HIGH_DEFAULT));
}

auto executor = new ExecutableTimer(m_timer.get(), this);
Orch::addExecutor("CRM_COUNTERS_POLL", executor);
// The CRM stats needs to be populated again
m_countersCrmTable->del(CRM_COUNTERS_TABLE_KEY);
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

m_countersCrmTable->del(CRM_COUNTERS_TABLE_KEY); [](start = 4, length = 48)

If it is applied to cold start, let's do it in another PR. #Closed

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

For cold start, no data exists for CRM stats table, so it has no effect.


auto executor = new ExecutableTimer(m_timer.get(), this, "CRM_COUNTERS_POLL");
Orch::addExecutor(executor);
m_timer->start();
}

Expand Down Expand Up @@ -333,7 +336,7 @@ void CrmOrch::decCrmAclUsedCounter(CrmResourceType resource, sai_acl_stage_t sta
{
m_resourcesMap.at(resource).countersMap[getCrmAclKey(stage, point)].usedCounter--;

// Remove ACL table related counters
// Remove ACL table related counters
if (resource == CrmResourceType::CRM_ACL_TABLE)
{
auto & cntMap = m_resourcesMap.at(CrmResourceType::CRM_ACL_TABLE).countersMap;
Expand Down
16 changes: 12 additions & 4 deletions orchagent/fdborch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ FdbOrch::FdbOrch(DBConnector *db, string tableName, PortsOrch *port) :
{
m_portsOrch->attach(this);
m_flushNotificationsConsumer = new NotificationConsumer(db, "FLUSHFDBREQUEST");
auto flushNotifier = new Notifier(m_flushNotificationsConsumer, this);
Orch::addExecutor("", flushNotifier);
auto flushNotifier = new Notifier(m_flushNotificationsConsumer, this, "FLUSHFDBREQUEST");
Orch::addExecutor(flushNotifier);

/* Add FDB notifications support from ASIC */
DBConnector *notificationsDb = new DBConnector(ASIC_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
m_fdbNotificationConsumer = new swss::NotificationConsumer(notificationsDb, "NOTIFICATIONS");
auto fdbNotifier = new Notifier(m_fdbNotificationConsumer, this);
Orch::addExecutor("FDB_NOTIFICATIONS", fdbNotifier);
auto fdbNotifier = new Notifier(m_fdbNotificationConsumer, this, "FDB_NOTIFICATIONS");
Orch::addExecutor(fdbNotifier);
}

void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id)
Expand All @@ -53,6 +53,14 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj
return;
}

// we already have such entries
if (m_entries.count(update.entry) != 0)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

count [](start = 22, length = 5)

find(). Actually the if-else below is checking duplication. #Closed

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

ok.

{
SWSS_LOG_INFO("FdbOrch notification: mac %s is already in bv_id 0x%lx",
update.entry.mac.to_string().c_str(), entry->bv_id);
break;
}

update.add = true;

{
Expand Down
12 changes: 11 additions & 1 deletion orchagent/intfsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,16 @@ void IntfsOrch::doTask(Consumer &consumer)
{
if (alias == "lo")
{
addIp2MeRoute(ip_prefix);
// set request for lo may come after warm start restore
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is it related to warm start? If not, let's do it in another PR. #Closed

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

It is related to warm start. It is noticed that sometime IP data for lo comes after warm start.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 14, 2018

Choose a reason for hiding this comment

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

The logic is also applied to cold start. Are you fixing a bug also exists in cold start? if yes, we prefer a separate PR just for it.

In reply to: 209787586 [](ancestors = 209787586)

auto it_intfs = m_syncdIntfses.find(alias);
if (it_intfs == m_syncdIntfses.end())
{
IntfsEntry intfs_entry;
intfs_entry.ref_count = 0;
m_syncdIntfses[alias] = intfs_entry;
addIp2MeRoute(ip_prefix);
}

it = consumer.m_toSync.erase(it);
continue;
}
Expand Down Expand Up @@ -171,6 +180,7 @@ void IntfsOrch::doTask(Consumer &consumer)
{
if (alias == "lo")
{
m_syncdIntfses.erase(alias);
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The same. #Closed

removeIp2MeRoute(ip_prefix);
it = consumer.m_toSync.erase(it);
continue;
Expand Down
31 changes: 21 additions & 10 deletions orchagent/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ extern "C" {
#include "saihelper.h"
#include "notifications.h"
#include <signal.h>
#include <warm_restart.h>
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

#include "warm_restart.h" #Closed

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

ok.


using namespace std;
using namespace swss;
Expand Down Expand Up @@ -77,11 +78,29 @@ void sighup_handler(int signo)
}
}

void syncd_apply_view()
{
SWSS_LOG_NOTICE("Notify syncd APPLY_VIEW");

sai_status_t status;
sai_attribute_t attr;
attr.id = SAI_REDIS_SWITCH_ATTR_NOTIFY_SYNCD;
attr.value.s32 = SAI_REDIS_NOTIFY_SYNCD_APPLY_VIEW;
status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);

if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to notify syncd APPLY_VIEW %d", status);
exit(EXIT_FAILURE);
}
}

int main(int argc, char **argv)
{
swss::Logger::linkToDbNative("orchagent");

SWSS_LOG_ENTER();
WarmStart::checkWarmStart("orchagent");

if (signal(SIGHUP, sighup_handler) == SIG_ERR)
{
Expand Down Expand Up @@ -261,17 +280,9 @@ int main(int argc, char **argv)

try
{
SWSS_LOG_NOTICE("Notify syncd APPLY_VIEW");

attr.id = SAI_REDIS_SWITCH_ATTR_NOTIFY_SYNCD;
attr.value.s32 = SAI_REDIS_NOTIFY_SYNCD_APPLY_VIEW;
status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);

if (status != SAI_STATUS_SUCCESS)
if (!WarmStart::isWarmStart())
{
SWSS_LOG_ERROR("Failed to notify syncd APPLY_VIEW %d", status);
delete orchDaemon;
exit(EXIT_FAILURE);
syncd_apply_view();
}

orchDaemon->start();
Expand Down
4 changes: 2 additions & 2 deletions orchagent/notifier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

class Notifier : public Executor {
public:
Notifier(NotificationConsumer *select, Orch *orch)
: Executor(select, orch)
Notifier(NotificationConsumer *select, Orch *orch, const string &name)
: Executor(select, orch, name)
{
}

Expand Down
64 changes: 46 additions & 18 deletions orchagent/orch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,30 @@ void Consumer::drain()
m_orch->doTask(*this);
}

string Consumer::dumpTuple(KeyOpFieldsValuesTuple &tuple)
{
string s = getTableName() + getConsumerTable()->getTableNameSeparator() + kfvKey(tuple)
+ "|" + kfvOp(tuple);
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

| [](start = 18, length = 1)

getTableNameSeparator() could be '|', do you want to change this '|'? #Closed

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

This a good question.
We had it fixed to ":" previously. Changed it to getConsumerTable()->getTableNameSeparator() upon comment. I think one of he benefits of doing this is we'll have the same key format as that in redis DB. Any other idea?

for (auto i = kfvFieldsValues(tuple).begin(); i != kfvFieldsValues(tuple).end(); i++)
{
s += "|" + fvField(*i) + ":" + fvValue(*i);
}

return s;
}

void Consumer::dumpToSyncTasks(vector<string> &ts)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

dumpToSyncTasks [](start = 15, length = 15)

dumpTuple() and dumpPendingTasks() are not well designed interfaces.

I agree the observability is important.

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

dumpTuple() is existing function name. It is refactored so the common function may be used in multiple places. Any specific issue/suggestion?

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 14, 2018

Choose a reason for hiding this comment

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

I will investigate and hopefully submit a PR

In reply to: 209787965 [](ancestors = 209787965)

{
for (auto &tm :m_toSync)
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 13, 2018

Choose a reason for hiding this comment

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

Add one more blank after ':'

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

ok

Copy link
Copy Markdown
Contributor Author

@jipanyang jipanyang Aug 13, 2018

Choose a reason for hiding this comment

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

ok

{
KeyOpFieldsValuesTuple& tuple = tm.second;

string s = dumpTuple(tuple);

ts.push_back(s);
}
}

size_t Orch::addExistingData(const string& tableName)
{
auto consumer = dynamic_cast<Consumer *>(getExecutor(tableName));
Expand Down Expand Up @@ -224,7 +248,7 @@ bool Orch::bake()
{
continue;
}

size_t refilled = consumer->refillToSync();
SWSS_LOG_NOTICE("Add warm input: %s, %zd", executorName.c_str(), refilled);
}
Expand Down Expand Up @@ -326,6 +350,21 @@ void Orch::doTask()
}
}

void Orch::dumpToSyncTasks(vector<string> &ts)
{
for(auto &it : m_consumerMap)
{
Consumer* consumer = dynamic_cast<Consumer *>(it.second.get());
if (consumer == NULL)
{
SWSS_LOG_DEBUG("Executor is not a Consumer");
continue;
}

consumer->dumpToSyncTasks(ts);
}
}

void Orch::logfileReopen()
{
gRecordOfs.close();
Expand All @@ -347,12 +386,7 @@ void Orch::logfileReopen()

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

gRecordOfs << getTimestamp() << "|" << s << endl;

Expand All @@ -366,13 +400,7 @@ void Orch::recordTuple(Consumer &consumer, KeyOpFieldsValuesTuple &tuple)

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

string s = consumer.dumpTuple(tuple);
return s;
}

Expand Down Expand Up @@ -461,18 +489,18 @@ void Orch::addConsumer(DBConnector *db, string tableName, int pri)
{
if (db->getDbId() == CONFIG_DB || db->getDbId() == STATE_DB)
{
addExecutor(tableName, new Consumer(new SubscriberStateTable(db, tableName, TableConsumable::DEFAULT_POP_BATCH_SIZE, pri), this));
addExecutor(new Consumer(new SubscriberStateTable(db, tableName, TableConsumable::DEFAULT_POP_BATCH_SIZE, pri), this, tableName));
}
else
{
addExecutor(tableName, new Consumer(new ConsumerStateTable(db, tableName, gBatchSize, pri), this));
addExecutor(new Consumer(new ConsumerStateTable(db, tableName, gBatchSize, pri), this, tableName));
}
}

void Orch::addExecutor(string executorName, Executor* executor)
void Orch::addExecutor(Executor* executor)
{
m_consumerMap.emplace(std::piecewise_construct,
std::forward_as_tuple(executorName),
std::forward_as_tuple(executor->getName()),
std::forward_as_tuple(executor));
}

Expand Down
Loading