Skip to content

Commit

Permalink
Fix review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@liorghub
liorghub committed Jun 14, 2022
1 parent ac63f6f commit abe328d
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 90 deletions.
108 changes: 46 additions & 62 deletions orchagent/intfsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,11 @@ static const vector<sai_router_interface_stat_t> rifStatIds =
SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS,
};

// Translation of loopback action from string to sai type
const unordered_map<string, loopback_action_e> IntfsOrch::m_loopback_action_map =
{
{"drop", LOOPBACK_ACTION_DROP},
{"forward", LOOPBACK_ACTION_FORWARD},
};

// Translation of loopback action from sonic to sai type
const unordered_map<loopback_action_e, sai_packet_action_t> IntfsOrch::m_sai_loopback_action_map =
const unordered_map<string, sai_packet_action_t> IntfsOrch::m_sai_loopback_action_map =
{
{LOOPBACK_ACTION_DROP, SAI_PACKET_ACTION_DROP},
{LOOPBACK_ACTION_FORWARD, SAI_PACKET_ACTION_FORWARD},
{"drop", SAI_PACKET_ACTION_DROP},
{"forward", SAI_PACKET_ACTION_FORWARD},
};

IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb) :
Expand Down Expand Up @@ -430,19 +423,24 @@ bool IntfsOrch::setIntfProxyArp(const string &alias, const string &proxy_arp)
return true;
}

bool IntfsOrch::setIntfLoopbackAction(const Port &port)
bool IntfsOrch::setIntfLoopbackAction(const Port &port, string actionStr)
{
sai_attribute_t attr;
attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION;
attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action);
sai_packet_action_t action;

if (!getSaiLoopbackAction(actionStr, action))
{
return false;
}

string action_str = getIntfLoopbackActionStr(port.m_loopback_action);
attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION;
attr.value.s32 = action;

sai_status_t status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Loopback action [%s] set failed, interface [%s], rc [%d]",
action_str.c_str(), port.m_alias.c_str(), status);
actionStr.c_str(), port.m_alias.c_str(), status);

task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status);
if (handle_status != task_success)
Expand All @@ -452,40 +450,10 @@ bool IntfsOrch::setIntfLoopbackAction(const Port &port)
}

SWSS_LOG_NOTICE("Loopback action [%s] set success, interface [%s]",
action_str.c_str(), port.m_alias.c_str());
actionStr.c_str(), port.m_alias.c_str());
return true;
}

bool IntfsOrch::getIntfLoopbackAction(const std::string &actionStr, loopback_action_e &action)
{
auto it = m_loopback_action_map.find(actionStr);
if (it != m_loopback_action_map.end())
{
action = m_loopback_action_map.at(actionStr);
return true;
}
else
{
SWSS_LOG_WARN("Unsupported loopback action [%s]", actionStr.c_str());
return false;
}
}

string IntfsOrch::getIntfLoopbackActionStr(loopback_action_e action)
{
for (auto it = m_loopback_action_map.begin(); it != m_loopback_action_map.end(); ++it)
{
if (it->second == action)
{
return it->first;
}
}

SWSS_LOG_WARN("Failed to fetch action as string for action [%u]", action);
return string();
}


set<IpPrefix> IntfsOrch:: getSubnetRoutes()
{
SWSS_LOG_ENTER();
Expand All @@ -504,19 +472,18 @@ set<IpPrefix> IntfsOrch:: getSubnetRoutes()
}

bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix,
const bool adminUp, const uint32_t mtu, loopback_action_e loopbackAction)
const bool adminUp, const uint32_t mtu, string loopbackAction)

{
SWSS_LOG_ENTER();

Port port;
gPortsOrch->getPort(alias, port);
port.m_loopback_action = loopbackAction;

auto it_intfs = m_syncdIntfses.find(alias);
if (it_intfs == m_syncdIntfses.end())
{
if (!ip_prefix && addRouterIntfs(vrf_id, port))
if (!ip_prefix && addRouterIntfs(vrf_id, port, loopbackAction))
{
gPortsOrch->increasePortRefCount(alias);
IntfsEntry intfs_entry;
Expand Down Expand Up @@ -738,7 +705,7 @@ void IntfsOrch::doTask(Consumer &consumer)
string inband_type = "";
bool mpls = false;
string vlan = "";
loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE;
string loopbackAction = "";

for (auto idx : data)
{
Expand Down Expand Up @@ -833,10 +800,7 @@ void IntfsOrch::doTask(Consumer &consumer)
}
else if (field == "loopback_action")
{
if(!getIntfLoopbackAction(value, loopbackAction))
{
continue;
}
loopbackAction = value;
}
}

Expand Down Expand Up @@ -990,11 +954,11 @@ void IntfsOrch::doTask(Consumer &consumer)
}

/* Set loopback action */
if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction))
if ((!loopbackAction.empty()) && (port.m_loopback_action != loopbackAction))
{
port.m_loopback_action = loopbackAction;
if(setIntfLoopbackAction(port))
if (setIntfLoopbackAction(port, loopbackAction))
{
port.m_loopback_action = loopbackAction;
gPortsOrch->setPort(alias, port);
}
}
Expand Down Expand Up @@ -1139,7 +1103,22 @@ void IntfsOrch::doTask(Consumer &consumer)
}
}

bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
bool IntfsOrch::getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action)
{
auto it = m_sai_loopback_action_map.find(actionStr);
if (it != m_sai_loopback_action_map.end())
{
action = m_sai_loopback_action_map.at(actionStr);
return true;
}
else
{
SWSS_LOG_WARN("Unsupported loopback action [%s]", actionStr.c_str());
return false;
}
}

bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackActionStr)
{
SWSS_LOG_ENTER();

Expand All @@ -1159,11 +1138,15 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
attr.value.oid = vrf_id;
attrs.push_back(attr);

if(port.m_loopback_action != LOOPBACK_ACTION_NONE)
if (!loopbackActionStr.empty())
{
attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION;
attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action);
attrs.push_back(attr);
sai_packet_action_t loopbackAction;
if (getSaiLoopbackAction(loopbackActionStr, loopbackAction))
{
attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION;
attr.value.s32 = loopbackAction;
attrs.push_back(attr);
}
}

attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS;
Expand Down Expand Up @@ -1276,6 +1259,7 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port)
}

port.m_vr_id = vrf_id;
port.m_loopback_action = loopbackActionStr;

gPortsOrch->setPort(port.m_alias, port);
m_rifsToAdd.push_back(port);
Expand Down
12 changes: 5 additions & 7 deletions orchagent/intfsorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,9 @@ class IntfsOrch : public Orch
void addRifToFlexCounter(const string&, const string&, const string&);
void removeRifFromFlexCounter(const string&, const string&);

bool setIntfLoopbackAction(const Port &port);
bool getIntfLoopbackAction(const std::string &action_str, loopback_action_e &action);
string getIntfLoopbackActionStr(loopback_action_e action);
bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE);
bool setIntfLoopbackAction(const Port &port, string actionStr);
bool getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action);
bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, string loopbackAction = "");
bool removeIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr);

void addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix);
Expand Down Expand Up @@ -91,12 +90,11 @@ class IntfsOrch : public Orch
unique_ptr<Table> m_vidToRidTable;
unique_ptr<ProducerTable> m_flexCounterTable;
unique_ptr<ProducerTable> m_flexCounterGroupTable;
static const unordered_map<string, loopback_action_e> m_loopback_action_map;
static const unordered_map<loopback_action_e, sai_packet_action_t> m_sai_loopback_action_map;
static const unordered_map<string, sai_packet_action_t> m_sai_loopback_action_map;

std::string getRifFlexCounterTableKey(std::string s);

bool addRouterIntfs(sai_object_id_t vrf_id, Port &port);
bool addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackAction);
bool removeRouterIntfs(Port &port);

void addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix);
Expand Down
9 changes: 1 addition & 8 deletions orchagent/port.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,6 @@ struct SystemLagInfo
int32_t spa_id = 0;
};

typedef enum loopback_action
{
LOOPBACK_ACTION_NONE,
LOOPBACK_ACTION_DROP,
LOOPBACK_ACTION_FORWARD,
} loopback_action_e;

class Port
{
public:
Expand Down Expand Up @@ -114,7 +107,6 @@ class Port
}

std::string m_alias;
loopback_action_e m_loopback_action;
Type m_type;
int m_index = 0; // PHY_PORT: index
uint32_t m_mtu = DEFAULT_MTU;
Expand Down Expand Up @@ -158,6 +150,7 @@ class Port
sai_port_interface_type_t m_interface_type;
std::vector<uint32_t> m_adv_interface_types;
bool m_mpls = false;
std::string m_loopback_action;

/*
* Following two bit vectors are used to lock
Expand Down
45 changes: 32 additions & 13 deletions tests/test_interface.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2213,13 +2213,11 @@ def set_loopback_action(self, interface, action):
tbl.set(interface, fvs)
time.sleep(1)

def loopback_action_test(self, iface):
def loopback_action_test(self, iface, action):
# create interface
self.create_l3_intf(iface, "")

# set interface loopback action in config database
action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"}
action = random.choice(list(action_map.keys()))
# set interface loopback action in config db
self.set_loopback_action(iface, action)

# check application database
Expand All @@ -2234,10 +2232,11 @@ def loopback_action_test(self, iface):
assert fv[1] == action
assert action_found == True

# check asic database
# check asic db
tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE")
intf_entries = tbl.getKeys()

action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"}
action_found = False
for key in intf_entries:
(status, fvs) = tbl.get(key)
Expand All @@ -2252,25 +2251,45 @@ def loopback_action_test(self, iface):
# remove interface
self.remove_l3_intf(iface)

def test_interfaceLoopbackAction(self, dvs, testlog):
def test_interfaceLoopbackActionDrop(self, dvs, testlog):
self.setup_db(dvs)
self.loopback_action_test("Ethernet8", "drop")

def test_interfaceLoopbackActionForward(self, dvs, testlog):
self.setup_db(dvs)
self.loopback_action_test("Ethernet8")
self.loopback_action_test("Ethernet8", "forward")

def test_subInterfaceLoopbackAction(self, dvs, testlog):
def test_subInterfaceLoopbackActionDrop(self, dvs, testlog):
self.setup_db(dvs)
self.loopback_action_test("Ethernet8.1")
self.loopback_action_test("Ethernet8.1", "drop")

def test_subInterfaceLoopbackActionForward(self, dvs, testlog):
self.setup_db(dvs)
self.loopback_action_test("Ethernet8.1", "forward")

def test_vlanInterfaceLoopbackAction(self, dvs, testlog):
def test_vlanInterfaceLoopbackActionDrop(self, dvs, testlog):
self.setup_db(dvs)
self.create_vlan("10")
self.loopback_action_test("Vlan10")
self.loopback_action_test("Vlan10", "drop")
self.remove_vlan("10")

def test_vlanInterfaceLoopbackActionForward(self, dvs, testlog):
self.setup_db(dvs)
self.create_vlan("20")
self.loopback_action_test("Vlan20", "forward")
self.remove_vlan("20")

def test_portChannelInterfaceLoopbackAction(self, dvs, testlog):
def test_portChannelInterfaceLoopbackActionDrop(self, dvs, testlog):
self.setup_db(dvs)
self.create_port_channel("PortChannel009")
self.loopback_action_test("PortChannel009")
self.loopback_action_test("PortChannel009", "drop")
self.remove_port_channel("PortChannel009")

def test_portChannelInterfaceLoopbackActionForward(self, dvs, testlog):
self.setup_db(dvs)
self.create_port_channel("PortChannel010")
self.loopback_action_test("PortChannel010", "forward")
self.remove_port_channel("PortChannel010")

# Add Dummy always-pass test at end as workaroud
# for issue when Flaky fail on final test it invokes module tear-down before retrying
Expand Down

0 comments on commit abe328d

Please sign in to comment.