Skip to content

Commit

Permalink
[IntfMgrd] Retry adding ipv6 prefix by setting disabled_ipv6 flag (so…
Browse files Browse the repository at this point in the history
…nic-net#2267)

*intfmgrd sets the flag by itself and retries setting the ip address if the ipv6 assignment fails for the first time
*There might be a race condition b/w intfmgrd and Mellanox SDK where the SDK created Linux Netdev iface but still doesn't yet set disable_ipv6 flag to 0. If intfmgrd tries to assign ip to the iface, the attempt fails.
  • Loading branch information
vivekrnv authored May 16, 2022
1 parent 11829a0 commit 54e1847
Show file tree
Hide file tree
Showing 5 changed files with 207 additions and 3 deletions.
26 changes: 25 additions & 1 deletion cfgmgr/intfmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,21 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd,
int ret = swss::exec(cmd.str(), res);
if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
if (!ipPrefix.isV4() && opCmd == "add")
{
SWSS_LOG_NOTICE("Failed to assign IPv6 on interface %s with return code %d, trying to enable IPv6 and retry", alias.c_str(), ret);
if (!enableIpv6Flag(alias))
{
SWSS_LOG_ERROR("Failed to enable IPv6 on interface %s", alias.c_str());
return;
}
ret = swss::exec(cmd.str(), res);
}

if (ret)
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
}
}
}

Expand Down Expand Up @@ -1139,3 +1153,13 @@ void IntfMgr::doPortTableTask(const string& key, vector<FieldValueTuple> data, s
}
}
}

bool IntfMgr::enableIpv6Flag(const string &alias)
{
stringstream cmd;
string temp_res;
cmd << "sysctl -w net.ipv6.conf." << shellquote(alias) << ".disable_ipv6=0";
int ret = swss::exec(cmd.str(), temp_res);
SWSS_LOG_INFO("disable_ipv6 flag is set to 0 for iface: %s, cmd: %s, ret: %d", alias.c_str(), cmd.str().c_str(), ret);
return (ret == 0) ? true : false;
}
1 change: 1 addition & 0 deletions cfgmgr/intfmgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class IntfMgr : public Orch

void updateSubIntfAdminStatus(const std::string &alias, const std::string &admin);
void updateSubIntfMtu(const std::string &alias, const std::string &mtu);
bool enableIpv6Flag(const std::string&);

bool m_replayDone {false};
};
Expand Down
23 changes: 21 additions & 2 deletions tests/mock_tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ INCLUDES = -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I $(top_srcdir)/lib

CFLAGS_SAI = -I /usr/include/sai

TESTS = tests
TESTS = tests tests_intfmgrd

noinst_PROGRAMS = tests
noinst_PROGRAMS = tests tests_intfmgrd

LDADD_SAI = -lsaimeta -lsaimetadata -lsaivs -lsairedis

Expand Down Expand Up @@ -115,3 +115,22 @@ tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAG
tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I$(top_srcdir)/orchagent
tests_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis -lpthread \
-lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3

## intfmgrd unit tests

tests_intfmgrd_SOURCES = intfmgrd/add_ipv6_prefix_ut.cpp \
$(top_srcdir)/cfgmgr/intfmgr.cpp \
$(top_srcdir)/orchagent/orch.cpp \
$(top_srcdir)/orchagent/request_parser.cpp \
$(top_srcdir)/lib/subintf.cpp \
mock_orchagent_main.cpp \
mock_dbconnector.cpp \
mock_table.cpp \
mock_hiredis.cpp \
fake_response_publisher.cpp \
mock_redisreply.cpp

tests_intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
tests_intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) -I $(top_srcdir)/cfgmgr -I $(top_srcdir)/orchagent/
tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \
-lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread
117 changes: 117 additions & 0 deletions tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
#include "gtest/gtest.h"
#include <iostream>
#include <fstream>
#include <unistd.h>
#include <sys/stat.h>
#include "../mock_table.h"
#include "warm_restart.h"
#define private public
#include "intfmgr.h"
#undef private

/* Override this pointer for custom behavior */
int (*callback)(const std::string &cmd, std::string &stdout) = nullptr;
std::vector<std::string> mockCallArgs;

namespace swss {
int exec(const std::string &cmd, std::string &stdout)
{
mockCallArgs.push_back(cmd);
return callback(cmd, stdout);
}
}

bool Ethernet0IPv6Set = false;

int cb(const std::string &cmd, std::string &stdout){
if (cmd == "sysctl -w net.ipv6.conf.\"Ethernet0\".disable_ipv6=0") Ethernet0IPv6Set = true;
else if (cmd.find("/sbin/ip -6 address \"add\"") == 0) {
return Ethernet0IPv6Set ? 0 : 2;
}
else {
return 0;
}
return 0;
}

// Test Fixture
namespace add_ipv6_prefix_ut
{
struct IntfMgrTest : public ::testing::Test
{
std::shared_ptr<swss::DBConnector> m_config_db;
std::shared_ptr<swss::DBConnector> m_app_db;
std::shared_ptr<swss::DBConnector> m_state_db;
std::vector<std::string> cfg_intf_tables;

virtual void SetUp() override
{
testing_db::reset();
m_config_db = std::make_shared<swss::DBConnector>("CONFIG_DB", 0);
m_app_db = std::make_shared<swss::DBConnector>("APPL_DB", 0);
m_state_db = std::make_shared<swss::DBConnector>("STATE_DB", 0);

swss::WarmStart::initialize("intfmgrd", "swss");

std::vector<std::string> tables = {
CFG_INTF_TABLE_NAME,
CFG_LAG_INTF_TABLE_NAME,
CFG_VLAN_INTF_TABLE_NAME,
CFG_LOOPBACK_INTERFACE_TABLE_NAME,
CFG_VLAN_SUB_INTF_TABLE_NAME,
CFG_VOQ_INBAND_INTERFACE_TABLE_NAME,
};
cfg_intf_tables = tables;
mockCallArgs.clear();
callback = cb;
}
};

TEST_F(IntfMgrTest, testSettingIpv6Flag){
Ethernet0IPv6Set = false;
swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables);
/* Set portStateTable */
std::vector<swss::FieldValueTuple> values;
values.emplace_back("state", "ok");
intfmgr.m_statePortTable.set("Ethernet0", values, "SET", "");
/* Set m_stateIntfTable */
values.clear();
values.emplace_back("vrf", "");
intfmgr.m_stateIntfTable.set("Ethernet0", values, "SET", "");
/* Set Ipv6 prefix */
const std::vector<std::string>& keys = {"Ethernet0", "2001::8/64"};
const std::vector<swss::FieldValueTuple> data;
intfmgr.doIntfAddrTask(keys, data, "SET");
int ip_cmd_called = 0;
for (auto cmd : mockCallArgs){
if (cmd.find("/sbin/ip -6 address \"add\"") == 0){
ip_cmd_called++;
}
}
ASSERT_EQ(ip_cmd_called, 2);
}

TEST_F(IntfMgrTest, testNoSettingIpv6Flag){
Ethernet0IPv6Set = true; // Assuming it is already set by SDK
swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables);
/* Set portStateTable */
std::vector<swss::FieldValueTuple> values;
values.emplace_back("state", "ok");
intfmgr.m_statePortTable.set("Ethernet0", values, "SET", "");
/* Set m_stateIntfTable */
values.clear();
values.emplace_back("vrf", "");
intfmgr.m_stateIntfTable.set("Ethernet0", values, "SET", "");
/* Set Ipv6 prefix */
const std::vector<std::string>& keys = {"Ethernet0", "2001::8/64"};
const std::vector<swss::FieldValueTuple> data;
intfmgr.doIntfAddrTask(keys, data, "SET");
int ip_cmd_called = 0;
for (auto cmd : mockCallArgs){
if (cmd.find("/sbin/ip -6 address \"add\"") == 0){
ip_cmd_called++;
}
}
ASSERT_EQ(ip_cmd_called, 1);
}
}
43 changes: 43 additions & 0 deletions tests/mock_tests/mock_table.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#include "table.h"
#include "producerstatetable.h"
#include <set>

using TableDataT = std::map<std::string, std::vector<swss::FieldValueTuple>>;
using TablesT = std::map<std::string, TableDataT>;
Expand Down Expand Up @@ -71,4 +73,45 @@ namespace swss
keys.push_back(it.first);
}
}

void ProducerStateTable::set(const std::string &key,
const std::vector<FieldValueTuple> &values,
const std::string &op,
const std::string &prefix)
{
auto &table = gDB[m_pipe->getDbId()][getTableName()];
auto iter = table.find(key);
if (iter == table.end())
{
table[key] = values;
}
else
{
std::vector<FieldValueTuple> new_values(values);
std::set<std::string> field_set;
for (auto &value : values)
{
field_set.insert(fvField(value));
}
for (auto &value : iter->second)
{
auto &field = fvField(value);
if (field_set.find(field) != field_set.end())
{
continue;
}
new_values.push_back(value);
}
iter->second.swap(new_values);
}
}

void ProducerStateTable::del(const std::string &key,
const std::string &op,
const std::string &prefix)
{
auto &table = gDB[m_pipe->getDbId()][getTableName()];
table.erase(key);
}

}

0 comments on commit 54e1847

Please sign in to comment.