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

Jump to bottom

Add support for IP interface loopback action #2307

Merged
merged 10 commits into from
Jun 27, 2022
Merged

Add support for IP interface loopback action #2307

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ea5a175
Add IP interface loopback action support
liorghub May 31, 2022
5dc7668
Add VS test for loopback action
liorghub May 31, 2022
bfb61a2
Remove debug prints leftovers
liorghub May 31, 2022
f871125
Remove more debug prints leftovers
liorghub May 31, 2022
25a6cae
Remove unneeded function oveloading
liorghub May 31, 2022
bc7e20a
Add newlines in setIntf signature
liorghub May 31, 2022
ac63f6f
Fix review comments
liorghub Jun 1, 2022
abe328d
Fix review comments
liorghub Jun 14, 2022
5e571ec
Remove unused import
liorghub Jun 15, 2022
4849030
Fix review comments
liorghub Jun 23, 2022
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
12 changes: 12 additions & 0 deletions cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
string grat_arp = "";
string mpls = "";
string ipv6_link_local_mode = "";
string loopback_action = "";

for (auto idx : data)
{
Expand Down Expand Up @@ -750,6 +751,10 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
{
vlanId = value;
}
else if (field == "loopback_action")
{
loopback_action = value;
}
}

if (op == SET_COMMAND)
Expand Down Expand Up @@ -791,6 +796,13 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
data.push_back(fvTuple);
}

/* Set loopback action */
if (!loopback_action.empty())
{
FieldValueTuple fvTuple("loopback_action", loopback_action);
data.push_back(fvTuple);
}

/* Set mpls */
if (!setIntfMpls(alias, mpls))
{
Expand Down
103 changes: 101 additions & 2 deletions orchagent/intfsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,20 @@ 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 =
{
{LOOPBACK_ACTION_DROP, SAI_PACKET_ACTION_DROP},
{LOOPBACK_ACTION_FORWARD, SAI_PACKET_ACTION_FORWARD},
};

IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb) :
Orch(db, tableName, intfsorch_pri), m_vrfOrch(vrf_orch)
{
Expand Down Expand Up @@ -416,6 +430,62 @@ bool IntfsOrch::setIntfProxyArp(const string &alias, const string &proxy_arp)
return true;
}

bool IntfsOrch::setIntfLoopbackAction(const Port &port)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you pass loopback action as a string to this function, no need for getIntfLoopbackActionStr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
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);

string action_str = getIntfLoopbackActionStr(port.m_loopback_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);

task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status);
if (handle_status != task_success)
{
return parseHandleSaiStatusFailure(handle_status);
}
}

SWSS_LOG_NOTICE("Loopback action [%s] set success, interface [%s]",
action_str.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 @@ -433,12 +503,15 @@ set<IpPrefix> IntfsOrch:: getSubnetRoutes()
return subnet_routes;
}

bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu)
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)

{
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())
Expand Down Expand Up @@ -665,6 +738,7 @@ void IntfsOrch::doTask(Consumer &consumer)
string inband_type = "";
bool mpls = false;
string vlan = "";
loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE;

for (auto idx : data)
{
Expand Down Expand Up @@ -757,6 +831,13 @@ void IntfsOrch::doTask(Consumer &consumer)
{
vlan = value;
}
else if (field == "loopback_action")
{
if(!getIntfLoopbackAction(value, loopbackAction))
{
continue;
}
}
}

if (alias == "eth0" || alias == "docker0")
Expand Down Expand Up @@ -874,7 +955,8 @@ void IntfsOrch::doTask(Consumer &consumer)
{
adminUp = port.m_admin_state_up;
}
if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu))

if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu, loopbackAction))
{
it++;
continue;
Expand Down Expand Up @@ -906,6 +988,16 @@ void IntfsOrch::doTask(Consumer &consumer)
setRouterIntfsMpls(port);
gPortsOrch->setPort(alias, port);
}

/* Set loopback action */
if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction))
{
port.m_loopback_action = loopbackAction;
if(setIntfLoopbackAction(port))
Copy link
Collaborator

Choose a reason for hiding this comment

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

space after if. Please pass the action param as a string to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
gPortsOrch->setPort(alias, port);
}
}
}
}

Expand Down Expand Up @@ -1067,6 +1159,13 @@ 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

space after if. Also suggest pass this as a param instead of retrieving from port

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
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);
}

attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS;
if (port.m_mac)
{
Expand Down
7 changes: 6 additions & 1 deletion orchagent/intfsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ class IntfsOrch : public Orch
void addRifToFlexCounter(const string&, const string&, const string&);
void removeRifFromFlexCounter(const string&, const string&);

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);
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 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 @@ -88,6 +91,8 @@ 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;

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

Expand Down
8 changes: 8 additions & 0 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ 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 @@ -107,6 +114,7 @@ class Port
}

std::string m_alias;
loopback_action_e m_loopback_action;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, this is not a config that we apply frequently. I don't see a reason why this needs to be cached. If user sets the action, we can just go ahead with setting the value. This is slightly overcomplicating.

Copy link
Contributor Author

@liorghub liorghub Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@prsunny I tried to follow your approach to remove from cache but I found out that we do need it in cache, I will explain why. The cache is used not only for skipping multiple user configuration but also for skipping set flow after create flow. There are 2 flows for setting loopback action, set and create. Set flow is done when we set loopback action on an existing IP interface (func setIntfLoopbackAction). Create flow is done when loopback action set is part of the IP interface creation, this happens in init (func setIntf). Both functions I mentioned are being called one after the other and the way to skip set flow after create flow took place, is using the cache. This is the approach taken for all IP interface attributes. I did however made the code less complicated by using string instead of enum for the action type ("drop" or "forward").

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to disagree. Understand your point on create and set. But what is the concern if the same value is set again. For mtu and admin state, it is done currently for sub-port and those values are anyways part of port class. Loopback setting is rare and I read its mostly as setting proxy_arp etc, for which there is no cache. This is a single event and lets get this simplified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Type m_type;
int m_index = 0; // PHY_PORT: index
uint32_t m_mtu = DEFAULT_MTU;
Expand Down
78 changes: 78 additions & 0 deletions tests/test_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import time
import json
import pytest
import random

from swsscommon import swsscommon

VLAN_SUB_INTERFACE_SEPARATOR = '.'

class TestRouterInterface(object):
def setup_db(self, dvs):
self.pdb = swsscommon.DBConnector(0, dvs.redis_sock, 0)
Expand Down Expand Up @@ -2193,6 +2196,81 @@ def test_VLanInterfaceIpv6LinkLocalOnly(self, dvs, testlog):
# one loopback router interface
assert len(intf_entries) == 1

def set_loopback_action(self, interface, action):
if interface.startswith("PortChannel"):
tbl_name = "PORTCHANNEL_INTERFACE"
elif interface.startswith("Vlan"):
tbl_name = "VLAN_INTERFACE"
else:
sub_intf_sep_idx = interface.find(VLAN_SUB_INTERFACE_SEPARATOR)
if sub_intf_sep_idx != -1:
tbl_name = "VLAN_SUB_INTERFACE"
else:
tbl_name = "INTERFACE"

fvs = swsscommon.FieldValuePairs([("loopback_action", action)])
tbl = swsscommon.Table(self.cdb, tbl_name)
tbl.set(interface, fvs)
time.sleep(1)

def loopback_action_test(self, iface):
# 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()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we dont prefer to have randomness in test. Please have coverage for both actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

self.set_loopback_action(iface, action)

# check application database
tbl = swsscommon.Table(self.pdb, "INTF_TABLE")
(status, fvs) = tbl.get(iface)
assert status == True

action_found = False
for fv in fvs:
if fv[0] == "loopback_action":
action_found = True
assert fv[1] == action
assert action_found == True

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

action_found = False
for key in intf_entries:
(status, fvs) = tbl.get(key)
assert status == True

for fv in fvs:
if fv[0] == "SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION":
action_found = True
assert fv[1] == action_map[action]
assert action_found == True

# remove interface
self.remove_l3_intf(iface)

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

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

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

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

# 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