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

[proxy_arp] Implement proxy ARP feature #1285

Merged
merged 8 commits into from
May 26, 2020
Merged

[proxy_arp] Implement proxy ARP feature #1285

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
931c8e7
[proxy_arp] Implement proxy ARP feature
Apr 30, 2020
33a8ef0
[proxy_arp] Implement proxy ARP feature
May 9, 2020
05f28d7
[proxy_arp] Implement proxy ARP feature
May 9, 2020
2d6354f
[proxy_arp] Implement proxy ARP feature
May 9, 2020
f649457
[proxy_arp] Implement proxy ARP feature
May 22, 2020
3c47041
[proxy_arp] Implement proxy ARP feature
May 22, 2020
e0ac724
[proxy_arp] Implement proxy ARP feature
May 22, 2020
dfcdabd
[proxy_arp] Implement proxy ARP feature
May 25, 2020
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
51 changes: 50 additions & 1 deletion cfgmgr/intfmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,34 @@ void IntfMgr::removeSubIntfState(const string &alias)
}
}

bool IntfMgr::setIntfProxyArp(const string &alias, const string &vrf_name, const string &proxy_arp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please remove this parameter as it is not used?

{
stringstream cmd;
string res;
string proxy_arp_pvlan;

if (proxy_arp == "enabled" || !vrf_name.compare(0, strlen(VNET_PREFIX), VNET_PREFIX))
{
proxy_arp_pvlan = "1";
}
else if (proxy_arp == "disabled" && vrf_name.compare(0, strlen(VNET_PREFIX), VNET_PREFIX))
{
proxy_arp_pvlan = "0";
}
else
{
SWSS_LOG_ERROR("Proxy ARP state is invalid: \"%s\"", proxy_arp.c_str());
return false;
}

cmd << ECHO_CMD << " " << proxy_arp_pvlan << " > /proc/sys/net/ipv4/conf/" << alias << "/proxy_arp_pvlan";
EXEC_WITH_ERROR_THROW(cmd.str(), res);

SWSS_LOG_INFO("Proxy ARP set to \"%s\" on interface \"%s\"", proxy_arp.c_str(), alias.c_str());

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to remove extra line to avoid file getting bigger

return true;
}

bool IntfMgr::isIntfStateOk(const string &alias)
{
vector<FieldValueTuple> temp;
Expand Down Expand Up @@ -346,6 +374,8 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
string mtu = "";
string adminStatus = "";
string nat_zone = "";
string proxy_arp = "";

for (auto idx : data)
{
const auto &field = fvField(idx);
Expand All @@ -363,6 +393,10 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
{
adminStatus = value;
}
else if (field == "proxy_arp")
{
proxy_arp = value;
}

if (field == "nat_zone")
{
Expand Down Expand Up @@ -420,7 +454,22 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
FieldValueTuple fvTuple("mac_addr", MacAddress().to_string());
data.push_back(fvTuple);
}


if (!proxy_arp.empty() || !vrf_name.compare(0, strlen(VNET_PREFIX), VNET_PREFIX))
Copy link
Collaborator

Choose a reason for hiding this comment

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

A small request for change : After some internal discussion, we don't want to have proxy_arp enabled explicitly for Vnets. Let that also be triggered by config_db entry. I'll make necessary modifications to requirement doc. Please remove this check on Vnets

{
if (!setIntfProxyArp(alias, vrf_name, proxy_arp))
{
SWSS_LOG_ERROR("Failed to set proxy ARP to \"%s\" state for the \"%s\" interface", proxy_arp.c_str(), alias.c_str());
return false;
}

if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
{
FieldValueTuple fvTuple("proxy_arp", proxy_arp);
data.push_back(fvTuple);
}
}

if (!subIntfAlias.empty())
{
if (m_subIntfList.find(subIntfAlias) == m_subIntfList.end())
Expand Down
2 changes: 2 additions & 0 deletions cfgmgr/intfmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ class IntfMgr : public Orch
void removeHostSubIntf(const std::string &subIntf);
void setSubIntfStateOk(const std::string &alias);
void removeSubIntfState(const std::string &alias);

bool setIntfProxyArp(const std::string &alias, const std::string &vrf_name, const std::string &proxy_arp);
};

}
Expand Down
91 changes: 91 additions & 0 deletions orchagent/intfsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ extern sai_router_interface_api_t* sai_router_intfs_api;
extern sai_route_api_t* sai_route_api;
extern sai_neighbor_api_t* sai_neighbor_api;
extern sai_switch_api_t* sai_switch_api;
extern sai_vlan_api_t* sai_vlan_api;

extern sai_object_id_t gSwitchId;
extern PortsOrch *gPortsOrch;
Expand Down Expand Up @@ -226,6 +227,81 @@ bool IntfsOrch::setRouterIntfsAdminStatus(const Port &port)
return true;
}

bool IntfsOrch::setIntfVlanFloodType(const Port &port)
{
SWSS_LOG_ENTER();

if (port.m_type != Port::VLAN)
{
SWSS_LOG_ERROR("VLAN flood type cannot be set for non VLAN interface \"%s\"", port.m_alias.c_str());
return false;
}

sai_attribute_t attr;
attr.id = SAI_VLAN_ATTR_BROADCAST_FLOOD_CONTROL_TYPE;
attr.value.s32 = port.m_vlan_info.vlan_flood_type;

sai_status_t status = sai_vlan_api->set_vlan_attribute(port.m_vlan_info.vlan_oid, &attr);
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to set flood type for VLAN %u, rv:%d", port.m_vlan_info.vlan_id, status);
return false;
}

return true;
}

bool IntfsOrch::setIntfProxyArp(const string &alias, const string &proxy_arp)
{
SWSS_LOG_ENTER();

if (proxy_arp != "enabled" || proxy_arp != "disabled")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this check. It wouldn't be added by IntfMgr if value is different

{
SWSS_LOG_ERROR("Proxy ARP state is invalid: \"%s\"", proxy_arp.c_str());
return false;
}

if (m_syncdIntfses.find(alias) == m_syncdIntfses.end())
{
SWSS_LOG_ERROR("Interface \"%s\" doesn't exist", alias.c_str());
return false;
}

if (m_syncdIntfses[alias].proxy_arp == (proxy_arp == "enabled" ? true : false))
{
SWSS_LOG_INFO("Proxy ARP is already set to \"%s\" on interface \"%s\"", proxy_arp.c_str(), alias.c_str());
return true;
}

Port port;
if (!gPortsOrch->getPort(alias, port))
{
SWSS_LOG_ERROR("Failed to get port info for the interface \"%s\"", alias.c_str());
return false;
}

if (port.m_type == Port::VLAN)
{
if (proxy_arp == "enabled")
{
port.m_vlan_info.vlan_flood_type = SAI_VLAN_FLOOD_CONTROL_TYPE_NONE;
}
else
{
port.m_vlan_info.vlan_flood_type = SAI_VLAN_FLOOD_CONTROL_TYPE_ALL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this updated back to port map. Please refer this after updating a field of port. In this case, i dont think you need to store this value. Just pass the flood_type to setIntfVlanFloodType

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @volodymyrsamotiy @prsunny, is it alright to influence the flooding behavior of all BUM packets when proxy_arp enabled? This flood_type attr seems not only applied to ARP packets.

typedef enum _sai_vlan_flood_control_type_t
{
    /**
     * @brief Flood on all vlan members
     *
     * When setting all to broadcast or unknown multicast flood, it also includes
     * flooding to the router. When setting all to unknown unicast flood, it does
     * not include flooding to the router
     */
    SAI_VLAN_FLOOD_CONTROL_TYPE_ALL,

    /** Disable flooding */
    SAI_VLAN_FLOOD_CONTROL_TYPE_NONE,

    /** Flood on the L2MC group */
    SAI_VLAN_FLOOD_CONTROL_TYPE_L2MC_GROUP,

    /**
     * @brief Flood on all vlan members and L2MC group
     *
     * Flood on all vlan members, without the router
     * In addition, flood on the supplied L2MC group
     */
    SAI_VLAN_FLOOD_CONTROL_TYPE_COMBINED

} sai_vlan_flood_control_type_t;

}

if (!setIntfVlanFloodType(port))
{
return false;
}
}

m_syncdIntfses[alias].proxy_arp = (proxy_arp == "enabled") ? true : false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls remove this extra line. Just a suggestion

return true;
}

set<IpPrefix> IntfsOrch:: getSubnetRoutes()
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -440,6 +516,7 @@ void IntfsOrch::doTask(Consumer &consumer)
uint32_t mtu;
bool adminUp;
uint32_t nat_zone_id = 0;
string proxy_arp = "";

for (auto idx : data)
{
Expand Down Expand Up @@ -515,6 +592,10 @@ void IntfsOrch::doTask(Consumer &consumer)
{
nat_zone = value;
}
else if (field == "proxy_arp")
{
proxy_arp = value;
}
}

if (alias == "eth0" || alias == "docker0")
Expand Down Expand Up @@ -666,6 +747,11 @@ void IntfsOrch::doTask(Consumer &consumer)
}
}

if (!proxy_arp.empty())
{
setIntfProxyArp(alias, proxy_arp);
}

it = consumer.m_toSync.erase(it);
}
else if (op == DEL_COMMAND)
Expand Down Expand Up @@ -724,6 +810,11 @@ void IntfsOrch::doTask(Consumer &consumer)
vnet_name = m_vnetInfses.at(alias);
}

if (m_syncdIntfses[alias].proxy_arp)
{
setIntfProxyArp(alias, "disabled");
}

if (!vnet_name.empty())
{
VNetOrch* vnet_orch = gDirectory.get<VNetOrch*>();
Expand Down
4 changes: 4 additions & 0 deletions orchagent/intfsorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ struct IntfsEntry
std::set<IpPrefix> ip_addresses;
int ref_count;
sai_object_id_t vrf_id;
bool proxy_arp;
};

typedef map<string, IntfsEntry> IntfsTable;
Expand Down Expand Up @@ -87,6 +88,9 @@ class IntfsOrch : public Orch

void addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix);
void removeDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix);

bool setIntfVlanFloodType(const Port &port);
bool setIntfProxyArp(const string &alias, const string &proxy_arp);
};

#endif /* SWSS_INTFSORCH_H */
5 changes: 3 additions & 2 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ typedef std::map<sai_vlan_id_t, VlanMemberEntry> vlan_members_t;

struct VlanInfo
{
sai_object_id_t vlan_oid = 0;
sai_vlan_id_t vlan_id = 0;
sai_object_id_t vlan_oid = 0;
sai_vlan_id_t vlan_id = 0;
sai_vlan_flood_control_type_t vlan_flood_type = SAI_VLAN_FLOOD_CONTROL_TYPE_ALL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to remove this attribute as it is a function handled in intforch and portorch doesn't necessarily be aware of this

};

class Port
Expand Down
Loading