Skip to content

Commit

Permalink
[syncd] Comparison logic workaround for empty buffer profile (sonic-n…
Browse files Browse the repository at this point in the history 
…et#906)

* [syncd] WIP buffer profile GET

* Add object status serialize method

* Add buffer profile get test

* [syncd] Allow AsicView return dummy created object

* Update aspell

* [syncd] Transfer not processed objects to temp view

Check issue sonic-net/sonic-sairedis#899
for details.

* [tests] Add missing recording file for tests
  • Loading branch information
@kcudnik
kcudnik authored Sep 4, 2021
1 parent d8eb0ea commit 509b888
Show file tree
Hide file tree
Showing 11 changed files with 379 additions and 25 deletions.
8 changes: 5 additions & 3 deletions syncd/AsicView.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ std::vector<std::shared_ptr<SaiObj>> AsicView::getAllNotProcessedObjects() const
* @param[in] rid Real ID
* @param[in] vid Virtual ID
*/
void AsicView::createDummyExistingObject(
std::shared_ptr<SaiObj> AsicView::createDummyExistingObject(
_In_ sai_object_id_t rid,
_In_ sai_object_id_t vid)
{
Expand Down Expand Up @@ -540,6 +540,8 @@ void AsicView::createDummyExistingObject(

m_ridToVid[rid] = vid;
m_vidToRid[vid] = rid;

return o;
}

/**
Expand Down Expand Up @@ -1289,10 +1291,10 @@ void AsicView::checkObjectsStatus() const
{
const auto &o = *p.second;

SWSS_LOG_ERROR("object was not processed: %s %s, status: %d (ref: %d)",
SWSS_LOG_ERROR("object was not processed: %s %s, status: %s (ref: %d)",
o.m_str_object_type.c_str(),
o.m_str_object_id.c_str(),
o.getObjectStatus(),
ObjectStatus::sai_serialize_object_status(o.getObjectStatus()).c_str(),
o.isOidObject() ? getVidReferenceCount(o.getVid()): -1);

count++;
Expand Down
2 changes: 1 addition & 1 deletion syncd/AsicView.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ namespace syncd
* @param[in] rid Real ID
* @param[in] vid Virtual ID
*/
void createDummyExistingObject(
std::shared_ptr<SaiObj> createDummyExistingObject(
_In_ sai_object_id_t rid,
_In_ sai_object_id_t vid);

Expand Down
185 changes: 174 additions & 11 deletions syncd/ComparisonLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ void ComparisonLogic::compareViews()

applyViewTransition(current, temp);

transferNotProcessed(current, temp);

// TODO have a method to check for not processed objects
// and maybe add them to list on processing attributes
// and move note processed objects to temporary view as well
// we need to check oid attributes as well

SWSS_LOG_NOTICE("ASIC operations to execute: %zu", current.asicGetOperationsCount());

temp.checkObjectsStatus();
Expand Down Expand Up @@ -183,6 +190,8 @@ void ComparisonLogic::matchOids(
{
SWSS_LOG_ENTER();

auto coldBootDiscoveredVids = m_switch->getColdBootDiscoveredVids();

for (const auto &temporaryIt: temporaryView.m_oOids)
{
sai_object_id_t temporaryVid = temporaryIt.first;
Expand Down Expand Up @@ -211,6 +220,37 @@ void ComparisonLogic::matchOids(
currentIt->second->m_str_object_type.c_str(),
sai_serialize_object_id(rid).c_str(),
sai_serialize_object_id(vid).c_str());

if (coldBootDiscoveredVids.find(vid) == coldBootDiscoveredVids.end())
{
auto ot = currentIt->second->getObjectType();

switch (ot)
{
case SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP:
case SAI_OBJECT_TYPE_SCHEDULER_GROUP:
case SAI_OBJECT_TYPE_QUEUE:
case SAI_OBJECT_TYPE_PORT:
break;

default:

// Should we also add check if also not in removed?
// This is for case of only GET:
// 1. cold boot:
// - OA assigns buffer profile to Queue
// 2. warm boot:
// - OA do GET on Queue and got previous buffer profile
// - OA assigns new buffer profile on Queue
//
// Question: what should happen to old buffer profile? should it be removed?
// No, since OA still hold's the reference to that VID and may use it later.
SWSS_LOG_INFO("matched %s VID %s was not in cold boot, possible only GET?",
sai_serialize_object_id(vid).c_str(),
currentIt->second->m_str_object_type.c_str());
break;
}
}
}

SWSS_LOG_NOTICE("matched oids");
Expand Down Expand Up @@ -1125,8 +1165,8 @@ void ComparisonLogic::updateObjectStatus(
bool ComparisonLogic::performObjectSetTransition(
_In_ AsicView &currentView,
_In_ AsicView &temporaryView,
_In_ const std::shared_ptr<SaiObj> &currentBestMatch,
_In_ const std::shared_ptr<const SaiObj> &temporaryObj,
_In_ const std::shared_ptr<SaiObj> currentBestMatch,
_In_ std::shared_ptr<SaiObj> temporaryObj,
_In_ bool performTransition)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -1386,6 +1426,8 @@ bool ComparisonLogic::performObjectSetTransition(
return false;
}

const bool beginTempSizeZero = temporaryObj->getAllAttributes().size() == 0;

/*
* Current best match can have more attributes than temporary object.
* let see if we can bring them to default value if possible.
Expand Down Expand Up @@ -1530,14 +1572,38 @@ bool ComparisonLogic::performObjectSetTransition(
continue;
}

// SAI_QUEUE_ATTR_PARENT_SCHEDULER_NODE
// SAI_SCHEDULER_GROUP_ATTR_SCHEDULER_PROFILE_ID*
// SAI_SCHEDULER_GROUP_ATTR_PARENT_NODE
// SAI_BRIDGE_PORT_ATTR_BRIDGE_ID
//
// TODO matched by ID (MATCHED state) should always be updatable
// except those 4 above (at least for those above since they can have
// default value present after switch creation
// current best match is MATCHED

//auto vid = currentBestMatch->getVid();

// TODO don't transfer oid attributes, we don't know how to handle this yet
// If OA did GET on any attributes, snoop in syncd should catch that and write
// to database so we would have some attributes here.

if (beginTempSizeZero && !meta->isoidattribute)
{
SWSS_LOG_WARN("current attr is MoC|CaS and object is MATCHED: %s transferring %s:%s to temp object (was empty)",
currentBestMatch->m_str_object_id.c_str(),
meta->attridname,
currentAttr->getStrAttrValue().c_str());

std::shared_ptr<SaiAttr> transferedAttr = std::make_shared<SaiAttr>(
currentAttr->getStrAttrId(),
currentAttr->getStrAttrValue());

temporaryObj->setAttr(transferedAttr);

continue;
}

// SAI_QUEUE_ATTR_PARENT_SCHEDULER_NODE
// SAI_SCHEDULER_GROUP_ATTR_SCHEDULER_PROFILE_ID*
// SAI_SCHEDULER_GROUP_ATTR_PARENT_NODE
// SAI_BRIDGE_PORT_ATTR_BRIDGE_ID
//
// TODO matched by ID (MATCHED state) should always be updatable
// except those 4 above (at least for those above since they can have
// default value present after switch creation

// TODO SAI_SCHEDULER_GROUP_ATTR_SCHEDULER_PROFILE_ID is mandatory on create but also SET
// if attribute is set we and object is in MATCHED state then that means we are able to
Expand Down Expand Up @@ -1571,6 +1637,21 @@ bool ComparisonLogic::performObjectSetTransition(
meta->attridname,
currentAttr->getStrAttrValue().c_str());

// don't produce too much noise for queues
if (currentAttr->getStrAttrId() != "SAI_QUEUE_ATTR_TYPE")
{
SWSS_LOG_WARN("current attr is CREATE_ONLY and object is MATCHED: %s transferring %s:%s to temp object",
currentBestMatch->m_str_object_id.c_str(),
meta->attridname,
currentAttr->getStrAttrValue().c_str());
}

std::shared_ptr<SaiAttr> transferedAttr = std::make_shared<SaiAttr>(
currentAttr->getStrAttrId(),
currentAttr->getStrAttrValue());

temporaryObj->setAttr(transferedAttr);

continue;
}
}
Expand Down Expand Up @@ -1709,7 +1790,7 @@ bool ComparisonLogic::performObjectSetTransition(
void ComparisonLogic::processObjectForViewTransition(
_In_ AsicView &currentView,
_In_ AsicView &temporaryView,
_In_ const std::shared_ptr<SaiObj> &temporaryObj)
_Inout_ std::shared_ptr<SaiObj> temporaryObj)
{
SWSS_LOG_ENTER();

Expand Down Expand Up @@ -2273,6 +2354,10 @@ void ComparisonLogic::populateExistingObjects(

if (temporaryView.hasRid(rid))
{
SWSS_LOG_INFO("temporary view has existing %s RID %s",
sai_serialize_object_type(m_vendorSai->objectTypeQuery(rid)).c_str(),
sai_serialize_object_id(rid).c_str());

continue;
}

Expand All @@ -2295,6 +2380,11 @@ void ComparisonLogic::populateExistingObjects(
* from current view as well.
*/

SWSS_LOG_INFO("object was removed in init view: %s RID %s VID %s",
sai_serialize_object_type(m_vendorSai->objectTypeQuery(rid)).c_str(),
sai_serialize_object_id(rid).c_str(),
sai_serialize_object_id(vid).c_str());

continue;
}

Expand Down Expand Up @@ -2730,6 +2820,79 @@ void ComparisonLogic::createPreMatchMap(
count);
}

void ComparisonLogic::transferNotProcessed(
_In_ AsicView& current,
_In_ AsicView& temp)
{
SWSS_LOG_ENTER();

SWSS_LOG_NOTICE("calling transferNotProcessed");

/*
* It may happen that after performing view transition, some objects will
* not be processed. This may happen is scenario where buffer pool is
* assigned to buffer profile, and then buffer profile is assigned to
* queue. If OA will query only for oid for that buffer profile, then
* buffer pool will not be processed. Normally if it would be removed by
* comparison logic. More on this issue can be found on github:
* https://github.com/Azure/sonic-sairedis/issues/899
*
* So what we will do, we will transfer all not processed objects to
* temporary view with the same RID and VID. If nothing will happen to them,
* they will stay there until next warm boot where they will be removed.
*/

/*
* We need a loop (or recursion) since not processed objects may have oid
* attributes as well.
*/

while (current.getAllNotProcessedObjects().size())
{
SWSS_LOG_WARN("we have %zu not processed objects on current view, moving to temp view", current.getAllNotProcessedObjects().size());

for (const auto& obj: current.getAllNotProcessedObjects())
{
auto vid = obj->getVid();

auto rid = current.m_vidToRid.at(vid);

/*
* We should have only oid objects here, since all non oid objects
* are leafs in graph and has been removed.
*/

auto tmp = temp.createDummyExistingObject(rid, vid);

/*
* Move both objects to matched state since match oids was already
* called, and here we created some new objects that should be matched.
*/

current.m_oOids.at(vid)->setObjectStatus(SAI_OBJECT_STATUS_FINAL);
temp.m_oOids.at(vid)->setObjectStatus(SAI_OBJECT_STATUS_FINAL);

SWSS_LOG_WARN("moved %s VID %s RID %s to temporary view, and marked FINAL",
obj->m_str_object_type.c_str(),
obj->m_str_object_id.c_str(),
sai_serialize_object_id(rid).c_str());

for (auto& kvp: obj->getAllAttributes())
{
auto& sh = kvp.second;

auto attr = std::make_shared<SaiAttr>(sh->getStrAttrId(), sh->getStrAttrValue());

tmp->setAttr(attr);

SWSS_LOG_WARN(" * with attr: %s: %s",
sh->getStrAttrId().c_str(),
sh->getStrAttrValue().c_str());
}

}
}
}

void ComparisonLogic::applyViewTransition(
_In_ AsicView &current,
Expand Down
10 changes: 7 additions & 3 deletions syncd/ComparisonLogic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ namespace syncd
_In_ AsicView& current,
_In_ AsicView& temp);

void transferNotProcessed(
_In_ AsicView& current,
_In_ AsicView& temp);

void checkInternalObjects(
_In_ const AsicView& cv,
_In_ const AsicView& tv);
Expand Down Expand Up @@ -136,8 +140,8 @@ namespace syncd
bool performObjectSetTransition(
_In_ AsicView& currentView,
_In_ AsicView& temporaryView,
_In_ const std::shared_ptr<SaiObj>& currentBestMatch,
_In_ const std::shared_ptr<const SaiObj>& temporaryObj,
_In_ const std::shared_ptr<SaiObj> currentBestMatch,
_In_ const std::shared_ptr<SaiObj> temporaryObj,
_In_ bool performTransition);

void breakBeforeMake(
Expand All @@ -154,7 +158,7 @@ namespace syncd
void processObjectForViewTransition(
_In_ AsicView& currentView,
_In_ AsicView& temporaryView,
_In_ const std::shared_ptr<SaiObj>& temporaryObj);
_Inout_ std::shared_ptr<SaiObj> temporaryObj);

void checkSwitch(
_In_ const AsicView& currentView,
Expand Down
24 changes: 24 additions & 0 deletions syncd/SaiObj.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,30 @@

using namespace syncd;

std::string ObjectStatus::sai_serialize_object_status(
_In_ sai_object_status_t os)
{
SWSS_LOG_ENTER();

switch (os)
{
case SAI_OBJECT_STATUS_NOT_PROCESSED:
return "not-processed";

case SAI_OBJECT_STATUS_MATCHED:
return "matched";

case SAI_OBJECT_STATUS_REMOVED:
return "removed";

case SAI_OBJECT_STATUS_FINAL:
return "final";

default:
SWSS_LOG_THROW("unknown object status: %d", os);
}
}

SaiObj::SaiObj():
m_createdObject(false),
m_objectStatus(SAI_OBJECT_STATUS_NOT_PROCESSED)
Expand Down
13 changes: 13 additions & 0 deletions syncd/SaiObj.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,19 @@ namespace syncd

} sai_object_status_t;

class ObjectStatus
{
private:

ObjectStatus() = delete;
~ObjectStatus() = delete;

public:

static std::string sai_serialize_object_status(
_In_ sai_object_status_t os);
};

class SaiObj
{
private:
Expand Down
Loading

0 comments on commit 509b888

Please sign in to comment.