[proxy_arp] Implement proxy ARP feature #1285

volodymyrsamotiy · 2020-04-30T15:59:54Z

Signed-off-by: Volodymyr Samotiy volodymyrs@mellanox.com

What I did
Implemented ARP proxy feature.

Why I did it
Feature request according to HLD https://github.com/Azure/SONiC/blob/master/doc/arp/Proxy%20Arp.md.

How I verified it

Verified that it is possible to enable and disable proxy ARP on non VNET interfaces
- Verified kernel configuration via proc FS
- Verified VLAN flood SAI configuration for VLAN RIF
Verified that proxy ARP is enabled by default on VNET interfaces
Verified that ARP packets are propagated correctly when proxy ARP is enabled

Details if related
N/A

lgtm-com · 2020-04-30T16:22:29Z

This pull request introduces 1 alert and fixes 10 when merging 1e4a62e into 3829053 - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

8 for Unused local variable
2 for Unused import

Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

prsunny · 2020-05-08T21:28:07Z

can you please fix lgtm alerts?

* Update VNET virtual switch test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

* Remove redundant attribute check from VNET virtual switch test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

* Remove redundant code in VNET virtual test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

volodymyrsamotiy · 2020-05-09T20:57:07Z

retest this please

volodymyrsamotiy · 2020-05-12T08:47:44Z

retest this please

prsunny · 2020-05-21T00:53:04Z

orchagent/intfsorch.cpp

+{
+    SWSS_LOG_ENTER();
+
+    if (proxy_arp != "enabled" || proxy_arp != "disabled")


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

prsunny · 2020-05-21T01:08:09Z

cfgmgr/intfmgr.cpp

@@ -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))


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

prsunny · 2020-05-21T01:09:29Z

cfgmgr/intfmgr.cpp

+    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());
+


Suggest to remove extra line to avoid file getting bigger

prsunny · 2020-05-21T01:25:32Z

orchagent/intfsorch.cpp

+        }
+        else
+        {
+            port.m_vlan_info.vlan_flood_type = SAI_VLAN_FLOOD_CONTROL_TYPE_ALL;


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

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;

prsunny · 2020-05-21T01:26:33Z

orchagent/intfsorch.cpp

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


Pls remove this extra line. Just a suggestion

prsunny · 2020-05-21T01:29:02Z

orchagent/port.h

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


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

* Fix review comments Sined-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

* Remove extra line Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

volodymyrsamotiy · 2020-05-22T16:54:06Z

fixed review comments

prsunny · 2020-05-22T20:28:24Z

cfgmgr/intfmgr.cpp

@@ -420,7 +453,23 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
            FieldValueTuple fvTuple("mac_addr", MacAddress().to_string());
            data.push_back(fvTuple);
        }
-
+
+


Please remove this extra line

prsunny · 2020-05-22T20:28:30Z

cfgmgr/intfmgr.cpp

@@ -272,6 +272,33 @@ void IntfMgr::removeSubIntfState(const string &alias)
    }
 }

+bool IntfMgr::setIntfProxyArp(const string &alias, const string &vrf_name, const string &proxy_arp)


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

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

abdosi · 2020-05-28T01:15:30Z

@volodymyrsamotiy please create PR for 201911 branch. Chery-pick has conflict

@prsunny

fixes sonic-net#1285

volodymyrsamotiy force-pushed the proxy_arp branch 5 times, most recently from de6a68c to 1215973 Compare May 4, 2020 12:32

[proxy_arp] Implement proxy ARP feature

931c8e7

Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

volodymyrsamotiy force-pushed the proxy_arp branch from 1215973 to 931c8e7 Compare May 4, 2020 12:46

Volodymyr Samotiy added 3 commits May 9, 2020 17:18

[proxy_arp] Implement proxy ARP feature

33a8ef0

* Update VNET virtual switch test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

[proxy_arp] Implement proxy ARP feature

05f28d7

* Remove redundant attribute check from VNET virtual switch test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

[proxy_arp] Implement proxy ARP feature

2d6354f

* Remove redundant code in VNET virtual test Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

prsunny reviewed May 21, 2020

View reviewed changes

liat-grozovik added the request_for_201911_branch label May 21, 2020

Volodymyr Samotiy added 3 commits May 22, 2020 19:41

[proxy_arp] Implement proxy ARP feature

f649457

* Fix review comments Sined-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

[proxy_arp] Implement proxy ARP feature

3c47041

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

[proxy_arp] Implement proxy ARP feature

e0ac724

* Remove extra line Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

prsunny reviewed May 22, 2020

View reviewed changes

[proxy_arp] Implement proxy ARP feature

dfcdabd

* Fix review comments Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>

prsunny approved these changes May 26, 2020

View reviewed changes

prsunny merged commit 4798584 into sonic-net:master May 26, 2020

abdosi removed the request_for_201911_branch label May 28, 2020

volodymyrsamotiy mentioned this pull request May 28, 2020

[proxy_arp] Implement proxy ARP feature #1302

Merged

prsunny mentioned this pull request May 29, 2020

Enable proxy_arp setting for Vnet interfaces sonic-net/sonic-restapi#41

Merged

volodymyrsamotiy mentioned this pull request Jul 23, 2020

[VNET] proxy ARP not enabled by default sonic-net/sonic-buildimage#4236

Closed

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Fix python3 issue in ecnconfig (sonic-net#1286)

fab84fb

fixes sonic-net#1285

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proxy_arp] Implement proxy ARP feature #1285

[proxy_arp] Implement proxy ARP feature #1285

volodymyrsamotiy commented Apr 30, 2020

lgtm-com bot commented Apr 30, 2020

prsunny commented May 8, 2020

volodymyrsamotiy commented May 9, 2020

volodymyrsamotiy commented May 12, 2020

prsunny May 21, 2020

prsunny May 21, 2020

prsunny May 21, 2020

prsunny May 21, 2020

shikenghua Sep 14, 2020

prsunny May 21, 2020

prsunny May 21, 2020

volodymyrsamotiy commented May 22, 2020

prsunny May 22, 2020

prsunny May 22, 2020

abdosi commented May 28, 2020

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

		}

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

[proxy_arp] Implement proxy ARP feature #1285

[proxy_arp] Implement proxy ARP feature #1285

Conversation

volodymyrsamotiy commented Apr 30, 2020

lgtm-com bot commented Apr 30, 2020

prsunny commented May 8, 2020

volodymyrsamotiy commented May 9, 2020

volodymyrsamotiy commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyrsamotiy commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdosi commented May 28, 2020