[IntfMgrd] Retry adding ipv6 prefix to iface if it fails because of disabled_ipv6 flag set to 1 #9
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| #include <iostream> | ||
| #include <string.h> | ||
| #include <fstream> | ||
| #include "logger.h" | ||
| #include "dbconnector.h" | ||
| #include "producerstatetable.h" | ||
|
|
@@ -28,6 +30,31 @@ using namespace swss; | |
| #define LOOPBACK_DEFAULT_MTU_STR "65536" | ||
| #define DEFAULT_MTU_STR 9100 | ||
|
|
||
| const string WHITESPACE = " \n\r\t\f\v"; | ||
|
|
||
| inline string rtrim(const string &s) | ||
| { | ||
| size_t end = s.find_last_not_of(WHITESPACE); | ||
| return (end == std::string::npos) ? "" : s.substr(0, end + 1); | ||
| } | ||
|
|
||
| inline bool enableIpv6Flag(const string &alias) | ||
| { | ||
| stringstream cmd; | ||
| string temp_res; | ||
| cmd << "sysctl -w " << "net.ipv6.conf." << shellquote(alias) << ".disable_ipv6=0"; | ||
| return (swss::exec(cmd.str(), temp_res) == 0) ? true : false; | ||
|
There was a problem hiding this comment. Will it be more optimal to write to a file using ifstream instead of spawning a bash shell There was a problem hiding this comment. Link Using bash shell is seemingly the norm in the code. Moreover the chances of this race happening are low already, so i think it's okay to use the standard approach |
||
| } | ||
|
|
||
| inline bool inferIPV6Capability() | ||
| { | ||
| string cmd1 = "cat /proc/sys/net/ipv6/conf/all/disable_ipv6"; | ||
| string cmd2 = "cat /proc/sys/net/ipv6/conf/default/disable_ipv6"; | ||
| string status1, status2; | ||
| if (swss::exec(cmd1, status1) || swss::exec(cmd2, status2)) return false; | ||
| return (rtrim(status1) == "0" && rtrim(status2) == "0") ? true : false; | ||
| } | ||
|
|
||
| IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, const vector<string> &tableNames) : | ||
| Orch(cfgDb, tableNames), | ||
| m_cfgIntfTable(cfgDb, CFG_INTF_TABLE_NAME), | ||
|
|
@@ -74,10 +101,12 @@ IntfMgr::IntfMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c | |
| { | ||
| mySwitchType = swtype; | ||
| } | ||
|
|
||
| g_ipv6Flag = inferIPV6Capability(); | ||
|
There was a problem hiding this comment. Why do we care about global configuration? |
||
| } | ||
|
|
||
| void IntfMgr::setIntfIp(const string &alias, const string &opCmd, | ||
| const IpPrefix &ipPrefix, bool retryV6) | ||
|
There was a problem hiding this comment. I don't see any retry logic, why this flag is called retryV6? There was a problem hiding this comment. I'm recursively calling this function again (and also enabling ipv6 on the iface) if this boolean is set. Will rename the argument. |
||
| { | ||
| stringstream cmd; | ||
| string res; | ||
|
|
@@ -115,6 +144,16 @@ void IntfMgr::setIntfIp(const string &alias, const string &opCmd, | |
| int ret = swss::exec(cmd.str(), res); | ||
| if (ret) | ||
| { | ||
| /* | ||
| ipv6 setting on the iface might still haven't been enabled by SDK. | ||
|
There was a problem hiding this comment. This comment refers to Nvidia SDK. It is better not to mention that in common code. |
||
| Retry adding Ipv6 Prefix after enabling the setting | ||
| */ | ||
| if (retryV6 && enableIpv6Flag(alias)) | ||
|
There was a problem hiding this comment. I don't think you need a flag because you can check whether opCmd is "add" & ipPrefix.isV6() and enable ipv6 on interface There was a problem hiding this comment. From the meeting with MSFT, they've suggested in trying setting the IP prefix first and then set the disable_ipv6 flag to zero if it fails, since this is relatively a rare condition |
||
| { | ||
| SWSS_LOG_INFO("Setting disable_ipv6 flag to 0 for iface: %s", alias.c_str()); | ||
|
There was a problem hiding this comment. The log does not reflect what this code block is doing There was a problem hiding this comment. Moved to the relevant method |
||
| return setIntfIp(alias, opCmd, ipPrefix, false); | ||
| } | ||
|
|
||
| SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret); | ||
| } | ||
| } | ||
|
|
@@ -1012,7 +1051,18 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys, | |
| return false; | ||
| } | ||
|
|
||
| /* | ||
| Even though network device is created at this point by the SDK, | ||
| ipv6 setting on the iface might still be disabled | ||
| Thus don't proceed setting an Ipv6 addr unless this setting is set by SDK | ||
| */ | ||
| if (!ip_prefix.isV4() && !g_ipv6Flag) | ||
| { | ||
| SWSS_LOG_ERROR("Tried to set ipv6 address %s to the iface %s, disable_ipv6 flag is set", ip_prefix.to_string().c_str(), alias.c_str()); | ||
| return true; | ||
| } | ||
|
There was a problem hiding this comment. Why do we care about global IPv6 setting? We have a per interface check inside setIntfIp |
||
|
|
||
| setIntfIp(alias, "add", ip_prefix, true); | ||
|
|
||
| std::vector<FieldValueTuple> fvVector; | ||
| FieldValueTuple f("family", ip_prefix.isV4() ? IPV4_NAME : IPV6_NAME); | ||
|
|
@@ -1029,7 +1079,7 @@ bool IntfMgr::doIntfAddrTask(const vector<string>& keys, | |
| } | ||
| else if (op == DEL_COMMAND) | ||
| { | ||
| setIntfIp(alias, "del", ip_prefix, false); | ||
|
|
||
| // Don't send ipv4 link local config to AppDB and Orchagent | ||
| if ((ip_prefix.isV4() == false) || (ip_prefix.getIp().getAddrScope() != IpAddress::AddrScope::LINK_SCOPE)) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| #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
There was a problem hiding this comment. 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? There was a problem hiding this comment. tests_intfmgrd will be compiled into a different binary so the other orchgent tests are not affected. |
||
| } | ||
|
|
||
|
|
||
| bool Ethernet0IPv6Set = false; | ||
| std::string globIpv6stdout = "0\n"; | ||
|
|
||
| int cb(const std::string &cmd, std::string &stdout){ | ||
| if (cmd == "cat /proc/sys/net/ipv6/conf/all/disable_ipv6" || | ||
| cmd == "cat /proc/sys/net/ipv6/conf/default/disable_ipv6") stdout = globIpv6stdout; | ||
| else if (cmd == "sysctl -w net.ipv6.conf.\"Ethernet0\".disable_ipv6=0") Ethernet0IPv6Set = false; | ||
| 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, testGlobalV6Enabled){ | ||
| swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); | ||
| ASSERT_EQ(intfmgr.g_ipv6Flag, true); | ||
| } | ||
|
|
||
| TEST_F(IntfMgrTest, testGlobalV6Disabled){ | ||
| globIpv6stdout = "1\n"; | ||
| swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables); | ||
| ASSERT_EQ(intfmgr.g_ipv6Flag, false); | ||
| } | ||
|
|
||
| TEST_F(IntfMgrTest, testSettingIpv6Flag){ | ||
| globIpv6stdout = "0\n"; | ||
| 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", ""); | ||
| 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){ | ||
| globIpv6stdout = "0\n"; | ||
| 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", ""); | ||
| 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); | ||
| } | ||
|
|
||
| TEST_F(IntfMgrTest, testIPV6GloballyDisabled){ | ||
| globIpv6stdout = "1\n"; // Ipv6 Disabled Globally | ||
| Ethernet0IPv6Set = false; // 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", ""); | ||
| 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, 0); // ip is not set; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like added headers aren't used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed