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.

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] Retry adding ipv6 prefix to iface if it fails because of disabled_ipv6 flag set to 1 #9

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 24 additions & 3 deletions cfgmgr/intfmgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,21 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd,
(cmd << IP_CMD << " -6 address " << shellquote(opCmd) << " " << shellquote(ipPrefixStr) << " dev " << shellquote(alias) << metric);
}

int ret = swss::exec(cmd.str(), res);
if (ret)
int failed = swss::exec(cmd.str(), res);
if (failed && !ipPrefix.isV4() && opCmd == "add")
{
SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret);
SWSS_LOG_NOTICE("Failed to assign IPv6 on interface %s with return code %d, trying to enable IPv6 and retry", alias.c_str(), failed);
if (!enableIpv6Flag(alias))
{
SWSS_LOG_ERROR("Failed to enable IPv6 on interface %s", alias.c_str());
return;
}
failed = swss::exec(cmd.str(), res);
}

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

Expand Down Expand Up @@ -1139,3 +1150,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 @@ -113,3 +113,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);
}
Comment on lines +17 to +21

Choose a reason for hiding this comment

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

If my understanding is correct, this mock works due to how dynamic linker resolves symbols. Since swss::exec is defined inside tests binary it is getting used instead of swss::exec from libswsscommon.so. That means every test, not just add_ipv6_prefix_ut tests will use this version of swss::exec, however, they won't have an API to provide mock implementation. Is my undestanding correct?

Copy link
Owner Author

Choose a reason for hiding this comment

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

tests_intfmgrd will be compiled into a different binary so the other orchgent tests are not affected.

}

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

}