Skip to content
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

[portsorch]: Bring the physical ports up are only after buffer configuration was applied #515

Merged
merged 4 commits into from
Jun 8, 2018
Merged

[portsorch]: Bring the physical ports up are only after buffer configuration was applied #515

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
294f1c3
Don't up ports, until buffer configuration is applied
pavel-shirshov Jun 6, 2018
42e9063
Set MTU first, then set port state to UP
pavel-shirshov Jun 6, 2018
519f17c
Introduce the test
pavel-shirshov Jun 6, 2018
c759f34
Use logical operator && for boolean values
pavel-shirshov Jun 7, 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
78 changes: 76 additions & 2 deletions orchagent/bufferorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ BufferOrch::BufferOrch(DBConnector *db, vector<string> &tableNames) : Orch(db, t
{
SWSS_LOG_ENTER();
initTableHandlers();
initBufferReadyLists(db);
};

void BufferOrch::initTableHandlers()
Expand All @@ -42,6 +43,59 @@ void BufferOrch::initTableHandlers()
m_bufferHandlerMap.insert(buffer_handler_pair(CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME, &BufferOrch::processEgressBufferProfileList));
}

void BufferOrch::initBufferReadyLists(DBConnector *db)
{
SWSS_LOG_ENTER();

Table pg_table(db, CFG_BUFFER_PG_TABLE_NAME);
initBufferReadyList(pg_table);

Table queue_table(db, CFG_BUFFER_QUEUE_TABLE_NAME);
initBufferReadyList(queue_table);
}

void BufferOrch::initBufferReadyList(Table& table)
{
SWSS_LOG_ENTER();

std::vector<std::string> keys;
table.getKeys(keys);

for (const auto& key: keys)
{
m_ready_list[key] = false;

auto tokens = tokenize(key, config_db_key_delimiter);
if (tokens.size() != 2)
{
SWSS_LOG_ERROR("Wrong format of a table '%s' key '%s'. Skip it", table.getTableName().c_str(), key.c_str());
continue;
}

auto port_names = tokenize(tokens[0], list_item_delimiter);

for(const auto& port_name: port_names)
{
m_port_ready_list_ref[port_name].push_back(key);
}
}
}

bool BufferOrch::isPortReady(const std::string& port_name) const
{
SWSS_LOG_ENTER();

const auto& list_of_keys = m_port_ready_list_ref.at(port_name);

bool result = true;
for (const auto& key: list_of_keys)
{
result &= m_ready_list.at(key);
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

&= [](start = 15, length = 2)

Better not use bitwise operator on bool. #Closed

}

return result;
}

task_process_status BufferOrch::processBufferPool(Consumer &consumer)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -315,7 +369,7 @@ task_process_status BufferOrch::processQueue(Consumer &consumer)
auto it = consumer.m_toSync.begin();
KeyOpFieldsValuesTuple tuple = it->second;
sai_object_id_t sai_buffer_profile;
string key = kfvKey(tuple);
const string key = kfvKey(tuple);
string op = kfvOp(tuple);
vector<string> tokens;
sai_uint32_t range_low, range_high;
Expand Down Expand Up @@ -374,6 +428,16 @@ task_process_status BufferOrch::processQueue(Consumer &consumer)
}
}
}

if (m_ready_list.find(key) != m_ready_list.end())
{
m_ready_list[key] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

m_ready_list [](start = 8, length = 12)

search the key twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you please elaborate on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

m_ready_list.find()
m_ready_list[]

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (m_ready_list.find(key) != m_ready_list.end()) 
{ 
    m_ready_list[key] = true;

vs

auto ready_list_it = m_ready_list.find(key);
if (ready_list_it != m_ready_list.end()) 
{ 
    *ready_list_it = true;

I think first case is more readable, although it might be slightly slower in case the compiler will not optimize it.

That code is not on he hot path, so I preferred to use better readability here.

}
else
{
SWSS_LOG_ERROR("Queue profile '%s' was inserted after BufferOrch init", key.c_str());
}

return task_process_status::task_success;
}

Expand All @@ -386,7 +450,7 @@ task_process_status BufferOrch::processPriorityGroup(Consumer &consumer)
auto it = consumer.m_toSync.begin();
KeyOpFieldsValuesTuple tuple = it->second;
sai_object_id_t sai_buffer_profile;
string key = kfvKey(tuple);
const string key = kfvKey(tuple);
string op = kfvOp(tuple);
vector<string> tokens;
sai_uint32_t range_low, range_high;
Expand Down Expand Up @@ -451,6 +515,16 @@ task_process_status BufferOrch::processPriorityGroup(Consumer &consumer)
}
}
}

if (m_ready_list.find(key) != m_ready_list.end())
{
m_ready_list[key] = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

m_ready_list [](start = 8, length = 12)

the same

}
else
{
SWSS_LOG_ERROR("PG profile '%s' was inserted after BufferOrch init", key.c_str());
}

return task_process_status::task_success;
}

Expand Down
7 changes: 7 additions & 0 deletions orchagent/bufferorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#ifndef SWSS_BUFFORCH_H
#define SWSS_BUFFORCH_H

#include <string>
#include <map>
#include <unordered_map>
#include "orch.h"
#include "portsorch.h"

Expand All @@ -26,6 +28,7 @@ class BufferOrch : public Orch
{
public:
BufferOrch(DBConnector *db, vector<string> &tableNames);
bool isPortReady(const std::string& port_name) const;
static type_map m_buffer_type_maps;
private:
typedef task_process_status (BufferOrch::*buffer_table_handler)(Consumer& consumer);
Expand All @@ -34,6 +37,8 @@ class BufferOrch : public Orch

virtual void doTask(Consumer& consumer);
void initTableHandlers();
void initBufferReadyLists(DBConnector *db);
void initBufferReadyList(Table& table);
task_process_status processBufferPool(Consumer &consumer);
task_process_status processBufferProfile(Consumer &consumer);
task_process_status processQueue(Consumer &consumer);
Expand All @@ -42,6 +47,8 @@ class BufferOrch : public Orch
task_process_status processEgressBufferProfileList(Consumer &consumer);

buffer_table_handler_map m_bufferHandlerMap;
std::unordered_map<std::string, bool> m_ready_list;
std::unordered_map<std::string, std::vector<std::string>> m_port_ready_list_ref;
};
#endif /* SWSS_BUFFORCH_H */

5 changes: 3 additions & 2 deletions orchagent/orchdaemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ NeighOrch *gNeighOrch;
RouteOrch *gRouteOrch;
AclOrch *gAclOrch;
CrmOrch *gCrmOrch;
BufferOrch *gBufferOrch;

OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb, DBConnector *stateDb) :
m_applDb(applDb),
Expand Down Expand Up @@ -90,7 +91,7 @@ bool OrchDaemon::init()
CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME
};
BufferOrch *buffer_orch = new BufferOrch(m_configDb, buffer_tables);
gBufferOrch = new BufferOrch(m_configDb, buffer_tables);

TableConnector appDbMirrorSession(m_applDb, APP_MIRROR_SESSION_TABLE_NAME);
TableConnector confDbMirrorSession(m_configDb, CFG_MIRROR_SESSION_TABLE_NAME);
Expand All @@ -109,7 +110,7 @@ bool OrchDaemon::init()

gAclOrch = new AclOrch(acl_table_connectors, gPortsOrch, mirror_orch, gNeighOrch, gRouteOrch);

m_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, buffer_orch, mirror_orch, gAclOrch, gFdbOrch, vrf_orch };
m_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, gBufferOrch, mirror_orch, gAclOrch, gFdbOrch, vrf_orch };
m_select = new Select();


Expand Down
29 changes: 19 additions & 10 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "sai_serialize.h"
#include "crmorch.h"
#include "countercheckorch.h"
#include "bufferorch.h"
#include "notifier.h"

extern sai_switch_api_t *sai_switch_api;
Expand All @@ -31,6 +32,7 @@ extern sai_queue_api_t *sai_queue_api;
extern sai_object_id_t gSwitchId;
extern NeighOrch *gNeighOrch;
extern CrmOrch *gCrmOrch;
extern BufferOrch *gBufferOrch;

#define VLAN_PREFIX "Vlan"
#define DEFAULT_VLAN_ID 1
Expand Down Expand Up @@ -1280,6 +1282,13 @@ void PortsOrch::doPortTask(Consumer &consumer)
continue;
}

if (alias != "PortConfigDone" && !gBufferOrch->isPortReady(alias))
{
// buffer configuration hasn't been applied yet. save it for future retry
it++;
continue;
}

Port p;
if (!getPort(alias, p) && alias != "PortConfigDone")
{
Expand Down Expand Up @@ -1332,31 +1341,31 @@ void PortsOrch::doPortTask(Consumer &consumer)
}
}

if (admin_status != "")
if (mtu != 0)
{
if (setPortAdminStatus(p.m_port_id, admin_status == "up"))
if (setPortMtu(p.m_port_id, mtu))
{
SWSS_LOG_NOTICE("Set port %s admin status to %s", alias.c_str(), admin_status.c_str());
p.m_mtu = mtu;
m_portList[alias] = p;
SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
}
else
{
SWSS_LOG_ERROR("Failed to set port %s admin status to %s", alias.c_str(), admin_status.c_str());
SWSS_LOG_ERROR("Failed to set port %s MTU to %u", alias.c_str(), mtu);
it++;
continue;
}
}

if (mtu != 0)
if (admin_status != "")
{
if (setPortMtu(p.m_port_id, mtu))
if (setPortAdminStatus(p.m_port_id, admin_status == "up"))
{
p.m_mtu = mtu;
m_portList[alias] = p;
SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu);
SWSS_LOG_NOTICE("Set port %s admin status to %s", alias.c_str(), admin_status.c_str());
}
else
{
SWSS_LOG_ERROR("Failed to set port %s MTU to %u", alias.c_str(), mtu);
SWSS_LOG_ERROR("Failed to set port %s admin status to %s", alias.c_str(), admin_status.c_str());
it++;
continue;
}
Expand Down
33 changes: 33 additions & 0 deletions tests/test_port_buffer_rel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from swsscommon import swsscommon
import time

# The test check that the ports will be up, when the admin state is UP by conf db.

def test_PortsAreUpAfterBuffers(dvs):
num_ports = 32
asic_db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)

conf_port_table = swsscommon.Table(conf_db, "PORT")
asic_port_table = swsscommon.Table(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_PORT")

# enable all ports
fvs = swsscommon.FieldValuePairs([("admin_status", "up")])
for i in range(0, num_ports):
conf_port_table.set("Ethernet%d" % (i*4), fvs)

time.sleep(5)

# check that the ports are enabled in ASIC
asic_port_records = asic_port_table.getKeys()
assert len(asic_port_records) == (num_ports + 1), "Number of port records doesn't match number of ports" # +CPU port
num_set = 0
for k in asic_port_records:
status, fvs = asic_port_table.get(k)
assert status, "Got an error when get a key"
for fv in fvs:
if fv[0] == "SAI_PORT_ATTR_ADMIN_STATE":
assert fv[1] == "true", "The port isn't UP as expected"
num_set += 1
# make sure that state is set for all "num_ports" ports
assert num_set == num_ports, "Not all ports are up"