Skip to content

Commit

Permalink
[everflow] Add retry mechanism for mirror sessions and policers
Browse files Browse the repository at this point in the history 
Signed-off-by: Danny Allen <daall@microsoft.com>
  • Loading branch information
@daall
daall committed Oct 27, 2020
1 parent 495816d commit b628f35
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 27 deletions.
23 changes: 16 additions & 7 deletions orchagent/aclorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1083,12 +1083,6 @@ bool AclRuleMirror::validateAddAction(string attr_name, string attr_value)

m_sessionName = attr_value;

if (!m_pMirrorOrch->sessionExists(m_sessionName))
{
SWSS_LOG_ERROR("Mirror rule reference mirror session that does not exists %s", m_sessionName.c_str());
return false;
}

// insert placeholder value, we'll set the session oid in AclRuleMirror::create()
m_actions[action] = sai_attribute_value_t{};

Expand Down Expand Up @@ -1178,6 +1172,12 @@ bool AclRuleMirror::create()
sai_object_id_t oid = SAI_NULL_OBJECT_ID;
bool state = false;

if (!m_pMirrorOrch->sessionExists(m_sessionName))
{
SWSS_LOG_ERROR("Mirror rule references mirror session \"%s\" that does not exist yet", m_sessionName.c_str());
return false;
}

if (!m_pMirrorOrch->getSessionStatus(m_sessionName, state))
{
SWSS_LOG_THROW("Failed to get mirror session state for session %s", m_sessionName.c_str());
Expand Down Expand Up @@ -3112,7 +3112,16 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
}


newRule = AclRule::makeShared(type, this, m_mirrorOrch, m_dTelOrch, rule_id, table_id, t);
try
{
newRule = AclRule::makeShared(type, this, m_mirrorOrch, m_dTelOrch, rule_id, table_id, t);
}
catch (exception &e)
{
SWSS_LOG_ERROR("Error while creating ACL rule %s: %s", rule_id.c_str(), e.what());
it = consumer.m_toSync.erase(it);
return;
}

for (const auto& itr : kfvFieldsValues(t))
{
Expand Down
42 changes: 23 additions & 19 deletions orchagent/mirrororch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ bool MirrorOrch::validateSrcPortList(const string& srcPortList)
return true;
}

void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& data)
task_process_status MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& data)
{
SWSS_LOG_ENTER();

Expand All @@ -349,7 +349,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
{
SWSS_LOG_NOTICE("Failed to create session, session %s already exists",
key.c_str());
return;
return task_process_status::task_invalid_entry;
}

string platform = getenv("platform") ? getenv("platform") : "";
Expand All @@ -364,7 +364,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
if (!entry.srcIp.isV4())
{
SWSS_LOG_ERROR("Unsupported version of sessions %s source IP address", key.c_str());
return;
return task_process_status::task_failed;
}
}
else if (fvField(i) == MIRROR_SESSION_DST_IP)
Expand All @@ -373,7 +373,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
if (!entry.dstIp.isV4())
{
SWSS_LOG_ERROR("Unsupported version of sessions %s destination IP address", key.c_str());
return;
return task_process_status::task_failed;
}
}
else if (fvField(i) == MIRROR_SESSION_GRE_TYPE)
Expand All @@ -398,7 +398,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
{
SWSS_LOG_ERROR("Failed to get policer %s",
fvValue(i).c_str());
return;
return task_process_status::task_need_retry;
}

m_policerOrch->increaseRefCount(fvValue(i));
Expand All @@ -409,7 +409,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
if (!validateSrcPortList(fvValue(i)))
{
SWSS_LOG_ERROR("Failed to get valid source port list %s", fvValue(i).c_str());
return;
return task_process_status::task_failed;
}
entry.src_port = fvValue(i);
}
Expand All @@ -418,7 +418,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
if (!validateDstPort(fvValue(i)))
{
SWSS_LOG_ERROR("Failed to get valid destination port %s", fvValue(i).c_str());
return;
return task_process_status::task_failed;
}
entry.dst_port = fvValue(i);
}
Expand All @@ -428,7 +428,7 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
|| fvValue(i) == MIRROR_BOTH_DIRECTION))
{
SWSS_LOG_ERROR("Failed to get valid direction %s", fvValue(i).c_str());
return;
return task_process_status::task_failed;
}
entry.direction = fvValue(i);
}
Expand All @@ -439,18 +439,18 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
else
{
SWSS_LOG_ERROR("Failed to parse session %s configuration. Unknown attribute %s", key.c_str(), fvField(i).c_str());
return;
return task_process_status::task_failed;
}
}
catch (const exception& e)
{
SWSS_LOG_ERROR("Failed to parse session %s attribute %s error: %s.", key.c_str(), fvField(i).c_str(), e.what());
return;
return task_process_status::task_failed;
}
catch (...)
{
SWSS_LOG_ERROR("Failed to parse session %s attribute %s. Unknown error has been occurred", key.c_str(), fvField(i).c_str());
return;
return task_process_status::task_failed;
}
}

Expand All @@ -470,6 +470,8 @@ void MirrorOrch::createEntry(const string& key, const vector<FieldValueTuple>& d
// Attach the destination IP to the routeOrch
m_routeOrch->attach(this, entry.dstIp);
}

return task_process_status::task_success;
}

task_process_status MirrorOrch::deleteEntry(const string& name)
Expand Down Expand Up @@ -1429,26 +1431,28 @@ void MirrorOrch::doTask(Consumer& consumer)

string key = kfvKey(t);
string op = kfvOp(t);
task_process_status task_status = task_process_status::task_failed;

if (op == SET_COMMAND)
{
createEntry(key, kfvFieldsValues(t));
task_status = createEntry(key, kfvFieldsValues(t));
}
else if (op == DEL_COMMAND)
{
auto task_status = deleteEntry(key);
// Specifically retry the task when asked
if (task_status == task_process_status::task_need_retry)
{
it++;
continue;
}
task_status = deleteEntry(key);
}
else
{
SWSS_LOG_ERROR("Unknown operation type %s", op.c_str());
}

// Specifically retry the task when asked
if (task_status == task_process_status::task_need_retry)
{
it++;
continue;
}

consumer.m_toSync.erase(it++);
}
}
2 changes: 1 addition & 1 deletion orchagent/mirrororch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class MirrorOrch : public Orch, public Observer, public Subject

bool m_freeze = false;

void createEntry(const string&, const vector<FieldValueTuple>&);
task_process_status createEntry(const string&, const vector<FieldValueTuple>&);
task_process_status deleteEntry(const string&);

bool activateSession(const string&, MirrorEntry&);
Expand Down
11 changes: 11 additions & 0 deletions orchagent/orchdaemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,17 @@ bool OrchDaemon::warmRestoreAndSyncUp()
o->postBake();
}

/*
* MIRROR ACL rules depend on mirror sessions, which won't be up after warm
* reboot until after the postBake step has finished. So, we need to give
* AclOrch one more round to resolve any rules that are waiting for mirror
* sessions.
*
* TODO: Implement a more generic mechanism for handling these types of
* hard dependencies.
*/
gAclOrch->Orch::doTask();

/*
* At this point, all the pre-existing data should have been processed properly, and
* orchagent should be in exact same state of pre-shutdown.
Expand Down

0 comments on commit b628f35

Please sign in to comment.