From 54e18478af1a6cabdac12d99587302d39c145bf7 Mon Sep 17 00:00:00 2001 From: Vivek R Date: Mon, 16 May 2022 16:17:46 -0700 Subject: [PATCH] [IntfMgrd] Retry adding ipv6 prefix by setting disabled_ipv6 flag (#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. --- cfgmgr/intfmgr.cpp | 26 +++- cfgmgr/intfmgr.h | 1 + tests/mock_tests/Makefile.am | 23 +++- .../intfmgrd/add_ipv6_prefix_ut.cpp | 117 ++++++++++++++++++ tests/mock_tests/mock_table.cpp | 43 +++++++ 5 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 93281dbcd96e..91ed762ef971 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -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); + } } } @@ -1139,3 +1153,13 @@ void IntfMgr::doPortTableTask(const string& key, vector 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; +} diff --git a/cfgmgr/intfmgr.h b/cfgmgr/intfmgr.h index 683e208c0e9a..65fd0512007d 100644 --- a/cfgmgr/intfmgr.h +++ b/cfgmgr/intfmgr.h @@ -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}; }; diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am index 7a25eb92b4a5..0bc676e6d3a8 100644 --- a/tests/mock_tests/Makefile.am +++ b/tests/mock_tests/Makefile.am @@ -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 @@ -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 diff --git a/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp b/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp new file mode 100644 index 000000000000..f07fa9ccbd88 --- /dev/null +++ b/tests/mock_tests/intfmgrd/add_ipv6_prefix_ut.cpp @@ -0,0 +1,117 @@ +#include "gtest/gtest.h" +#include +#include +#include +#include +#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 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 m_config_db; + std::shared_ptr m_app_db; + std::shared_ptr m_state_db; + std::vector cfg_intf_tables; + + virtual void SetUp() override + { + testing_db::reset(); + m_config_db = std::make_shared("CONFIG_DB", 0); + m_app_db = std::make_shared("APPL_DB", 0); + m_state_db = std::make_shared("STATE_DB", 0); + + swss::WarmStart::initialize("intfmgrd", "swss"); + + std::vector 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 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& keys = {"Ethernet0", "2001::8/64"}; + const std::vector 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 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& keys = {"Ethernet0", "2001::8/64"}; + const std::vector 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); + } +} diff --git a/tests/mock_tests/mock_table.cpp b/tests/mock_tests/mock_table.cpp index 29011c30a0e6..080fce775b1b 100644 --- a/tests/mock_tests/mock_table.cpp +++ b/tests/mock_tests/mock_table.cpp @@ -1,4 +1,6 @@ #include "table.h" +#include "producerstatetable.h" +#include using TableDataT = std::map>; using TablesT = std::map; @@ -71,4 +73,45 @@ namespace swss keys.push_back(it.first); } } + + void ProducerStateTable::set(const std::string &key, + const std::vector &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 new_values(values); + std::set 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); + } + }