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

[intfmgrd]: Support loopback #742

Merged
merged 2 commits into from
Jan 16, 2019
Merged

[intfmgrd]: Support loopback #742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44cb337
[intfmgrd]: Support loopback
marian-pritsak Jan 9, 2019
f74ba0a
[vs]: Add virtual switch test for loopback interfaces
Jan 11, 2019
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
45 changes: 38 additions & 7 deletions cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ using namespace swss;

#define VLAN_PREFIX "Vlan"
#define LAG_PREFIX "PortChannel"
#define LOOPBACK_PREFIX "Loopback"
#define VNET_PREFIX "Vnet"

IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) :
Expand Down Expand Up @@ -98,6 +99,10 @@ bool IntfMgr::isIntfStateOk(const string &alias)
SWSS_LOG_DEBUG("Port %s is ready", alias.c_str());
return true;
}
else if (!alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX))
{
return true;
}

return false;
}
Expand All @@ -110,6 +115,7 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,

string alias(keys[0]);
string vrf_name = "";
bool is_lo = !alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX);

for (auto idx : data)
{
Expand All @@ -135,13 +141,29 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
return false;
}

setIntfVrf(alias, vrf_name);
m_appIntfTableProducer.set(alias, data);
// Set Interface VRF except for lo
if (!is_lo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you excluding the loopbacks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ip address for lo is configured in /etc/network/interfaces

Copy link
Contributor

@nikos-github nikos-github Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I follow your response. It is valid for a loopback to be in a vrf. Why are you excluding the loopbacks?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikos-github , your comment is valid, but currently we do not support to have loopback in the vrf. We can add it in the future PR.

{
setIntfVrf(alias, vrf_name);
m_appIntfTableProducer.set(alias, data);
}
else
{
m_appIntfTableProducer.set("lo", data);
}
}
else if (op == DEL_COMMAND)
{
setIntfVrf(alias, "");
m_appIntfTableProducer.del(alias);
// Set Interface VRF except for lo
if (!is_lo)
{
setIntfVrf(alias, "");
m_appIntfTableProducer.del(alias);
}
else
{
m_appIntfTableProducer.del("lo");
}
}
else
{
Expand All @@ -159,7 +181,8 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys,

string alias(keys[0]);
IpPrefix ip_prefix(keys[1]);
string appKey = keys[0] + ":" + keys[1];
bool is_lo = !alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX);
string appKey = (is_lo ? "lo" : keys[0]) + ":" + keys[1];

if (op == SET_COMMAND)
{
Expand All @@ -173,7 +196,11 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys,
return false;
}

setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());
// Set Interface IP except for lo
if (!is_lo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain how your code is dealing with multiple IP addresses on the loopback which is a valid config and must continue to work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the issue with multiple IPs?
It's just different keys in APP DB

Copy link
Contributor

Choose a reason for hiding this comment

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

Today the loopback in sonic has more than one address. So with your changes post #635, will all the loopback addresses make it to the asic and correctly? I'm just asking you to explain and also test.

{
setIntfIp(alias, "add", ip_prefix.to_string(), ip_prefix.isV4());
}

std::vector<FieldValueTuple> fvVector;
FieldValueTuple f("family", ip_prefix.isV4() ? IPV4_NAME : IPV6_NAME);
Expand All @@ -186,7 +213,11 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys,
}
else if (op == DEL_COMMAND)
{
setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4());
// Set Interface IP except for lo
Copy link
Contributor

Choose a reason for hiding this comment

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

This set of changes require unit tests.

if (!is_lo)
{
setIntfIp(alias, "del", ip_prefix.to_string(), ip_prefix.isV4());
}
m_appIntfTableProducer.del(appKey);
m_stateIntfTable.del(keys[0] + state_db_key_delimiter + keys[1]);
}
Expand Down
1 change: 1 addition & 0 deletions cfgmgr/intfmgrd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ int main(int argc, char **argv)
CFG_INTF_TABLE_NAME,
CFG_LAG_INTF_TABLE_NAME,
CFG_VLAN_INTF_TABLE_NAME,
CFG_LOOPBACK_INTERFACE_TABLE_NAME,
};

DBConnector cfgDb(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
Expand Down
74 changes: 74 additions & 0 deletions tests/test_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,3 +387,77 @@ def test_InterfaceSetMtu(self, dvs, testlog):

# remove port channel
self.remove_port_channel(dvs, "PortChannel002")

class TestLoopbackRouterInterface(object):
def setup_db(self, dvs):
self.pdb = swsscommon.DBConnector(0, dvs.redis_sock, 0)
self.adb = swsscommon.DBConnector(1, dvs.redis_sock, 0)
self.cdb = swsscommon.DBConnector(4, dvs.redis_sock, 0)

def createLoIntf(self, interface, ip):
tbl = swsscommon.Table(self.cdb, "LOOPBACK_INTERFACE")
fvs = swsscommon.FieldValuePairs([("NULL", "NULL")])
tbl.set(interface + "|" + ip, fvs)
time.sleep(1)

def removeLoIntf(self, interface, ip):
tbl = swsscommon.Table(self.cdb, "LOOPBACK_INTERFACE")
tbl._del(interface + "|" + ip);
time.sleep(1)

def test_InterfacesCreateRemove(self, dvs, testlog):
self.setup_db(dvs)

# Create loopback interfaces
self.createLoIntf("Loopback0", "10.1.0.1/32")
self.createLoIntf("Loopback1", "10.1.0.2/32")

# Check configuration database
tbl = swsscommon.Table(self.cdb, "LOOPBACK_INTERFACE")
intf_entries = tbl.getKeys()

assert len(intf_entries) == 2
assert "Loopback0|10.1.0.1/32" in intf_entries
assert "Loopback1|10.1.0.2/32" in intf_entries

# Check application database
tbl = swsscommon.Table(self.pdb, "INTF_TABLE:lo")
intf_entries = tbl.getKeys()

assert len(intf_entries) == 2
assert "10.1.0.1/32" in intf_entries
assert "10.1.0.2/32" in intf_entries

# Check ASIC database
tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY")
for key in tbl.getKeys():
route = json.loads(key)
if route["dest"] == "10.1.0.1/32":
lo0_ip2me_found = True
if route["dest"] == "10.1.0.2/32":
lo1_ip2me_found = True

assert lo0_ip2me_found and lo1_ip2me_found

# Remove lopback interfaces
self.removeLoIntf("Loopback0", "10.1.0.1/32")
self.removeLoIntf("Loopback1", "10.1.0.2/32")

# Check configuration database
tbl = swsscommon.Table(self.cdb, "LOOPBACK_INTERFACE")
intf_entries = tbl.getKeys()
assert len(intf_entries) == 0

# Check application database
tbl = swsscommon.Table(self.pdb, "INTF_TABLE:lo")
intf_entries = tbl.getKeys()
assert len(intf_entries) == 0

# Check ASIC database
tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTE_ENTRY")
for key in tbl.getKeys():
route = json.loads(key)
if route["dest"] == "10.1.0.1/32":
assert False
if route["dest"] == "10.1.0.2/32":
assert False