Add support for IP interface loopback action by liorghub · Pull Request #2307 · sonic-net/sonic-swss

liorghub · 2022-06-02T12:57:37Z

What I did
Add support for IP interface loopback action.

Why I did it
New feature in SONiC.

How I verified it
New VS tests were added.

Details if related

liorghub · 2022-06-06T07:05:03Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-06-06T07:05:04Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-06-06T07:05:14Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny

IMO, the code can be simplified. Please let me know if you prefer an offline discussion.

prsunny · 2022-06-08T23:55:07Z

orchagent/port.h

    }

    std::string         m_alias;
+    loopback_action_e   m_loopback_action;


IMO, this is not a config that we apply frequently. I don't see a reason why this needs to be cached. If user sets the action, we can just go ahead with setting the value. This is slightly overcomplicating.

@prsunny I tried to follow your approach to remove from cache but I found out that we do need it in cache, I will explain why. The cache is used not only for skipping multiple user configuration but also for skipping set flow after create flow. There are 2 flows for setting loopback action, set and create. Set flow is done when we set loopback action on an existing IP interface (func setIntfLoopbackAction). Create flow is done when loopback action set is part of the IP interface creation, this happens in init (func setIntf). Both functions I mentioned are being called one after the other and the way to skip set flow after create flow took place, is using the cache. This is the approach taken for all IP interface attributes. I did however made the code less complicated by using string instead of enum for the action type ("drop" or "forward").

I tend to disagree. Understand your point on create and set. But what is the concern if the same value is set again. For mtu and admin state, it is done currently for sub-port and those values are anyways part of port class. Loopback setting is rare and I read its mostly as setting proxy_arp etc, for which there is no cache. This is a single event and lets get this simplified.

prsunny · 2022-06-08T23:55:39Z

orchagent/intfsorch.cpp

    attr.value.oid = vrf_id;
    attrs.push_back(attr);

+    if(port.m_loopback_action != LOOPBACK_ACTION_NONE)


space after if. Also suggest pass this as a param instead of retrieving from port

prsunny · 2022-06-08T23:57:14Z

orchagent/intfsorch.cpp

+                    if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction))
+                    {
+                        port.m_loopback_action = loopbackAction;
+                        if(setIntfLoopbackAction(port))


space after if. Please pass the action param as a string to this function.

prsunny · 2022-06-09T00:01:46Z

orchagent/intfsorch.cpp

    return true;
 }

+bool IntfsOrch::setIntfLoopbackAction(const Port &port)


if you pass loopback action as a string to this function, no need for getIntfLoopbackActionStr

prsunny · 2022-06-09T00:05:36Z

tests/test_interface.py

+
+        # set interface loopback action in config database
+        action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"}
+        action = random.choice(list(action_map.keys()))


we dont prefer to have randomness in test. Please have coverage for both actions.

lgtm-com · 2022-06-14T06:10:50Z

This pull request introduces 1 alert when merging abe328d into bf4d890 - view on LGTM.com

new alerts:

1 for Unused import

liorghub · 2022-06-15T10:44:30Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-06-15T10:44:32Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-06-15T10:44:42Z

Azure Pipelines successfully started running 1 pipeline(s).

liorghub · 2022-06-16T13:52:14Z

Test p4rt/test_p4rt_acl.py::TestP4RTAcl::test_AclRuleAddWithoutTableDefinitionFails ERROR failed, trying to rerun.

liorghub · 2022-06-16T13:52:32Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-06-16T13:52:34Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-06-16T13:52:43Z

Azure Pipelines successfully started running 1 pipeline(s).

liorghub · 2022-06-16T17:48:19Z

Test p4rt/test_p4rt_acl.py::TestP4RTAcl::test_AclRuleAddWithoutTableDefinitionFails ERROR failed again, trying to rerun.

liorghub · 2022-06-16T17:48:35Z

/azpw run Azure.sonic-swss

mssonicbld · 2022-06-16T17:48:37Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2022-06-16T17:48:46Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2022-06-22T20:25:28Z

orchagent/intfsorch.cpp

 };

+// Translation of loopback action from sonic to sai type
+const unordered_map<string, sai_packet_action_t> IntfsOrch::m_sai_loopback_action_map =


You can have it a local map variable.. Also please follow the existing naming convention in the file. No need to have it as part of IntfsOrch class

prsunny · 2022-06-22T20:35:31Z

orchagent/port.h

    }

    std::string         m_alias;
+    loopback_action_e   m_loopback_action;


I tend to disagree. Understand your point on create and set. But what is the concern if the same value is set again. For mtu and admin state, it is done currently for sub-port and those values are anyways part of port class. Loopback setting is rare and I read its mostly as setting proxy_arp etc, for which there is no cache. This is a single event and lets get this simplified.

prsunny · 2022-06-23T17:16:26Z

/azp run

azure-pipelines · 2022-06-23T17:16:36Z

Azure Pipelines successfully started running 1 pipeline(s).

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

Update sonic-swss submodule pointer to include the following: * Port configuration incremental update support ([sonic-net#2305](sonic-net/sonic-swss#2305)) * [VS Test] Skip failing subport tests ([sonic-net#2370](sonic-net/sonic-swss#2370)) * [teammgr]: Waiting MACsec ready before doLagMemberTask ([sonic-net#2286](sonic-net/sonic-swss#2286)) * [vnetorch] [vxlanorch] fix a set of memory usage issues ([sonic-net#2352](sonic-net/sonic-swss#2352)) * [tests] [asan] add graceful stop flag ([sonic-net#2347](sonic-net/sonic-swss#2347)) * [asan] suppress the static variable leaks ([sonic-net#2354](sonic-net/sonic-swss#2354)) * Add support for IP interface loopback action ([sonic-net#2307](sonic-net/sonic-swss#2307)) * [orchagent]: srv6orch support for uSID ([sonic-net#2335](sonic-net/sonic-swss#2335)) Signed-off-by: dprital <drorp@nvidia.com>

Update sonic-swss submodule pointer to include the following: * Port configuration incremental update support ([#2305](sonic-net/sonic-swss#2305)) * [VS Test] Skip failing subport tests ([#2370](sonic-net/sonic-swss#2370)) * [teammgr]: Waiting MACsec ready before doLagMemberTask ([#2286](sonic-net/sonic-swss#2286)) * [vnetorch] [vxlanorch] fix a set of memory usage issues ([#2352](sonic-net/sonic-swss#2352)) * [tests] [asan] add graceful stop flag ([#2347](sonic-net/sonic-swss#2347)) * [asan] suppress the static variable leaks ([#2354](sonic-net/sonic-swss#2354)) * Add support for IP interface loopback action ([#2307](sonic-net/sonic-swss#2307)) * [orchagent]: srv6orch support for uSID ([#2335](sonic-net/sonic-swss#2335)) Signed-off-by: dprital <drorp@nvidia.com>

This reverts commit 5043701. Revert "[VS Test] Skip failing subport tests (#2370)" This reverts commit 7175245.

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

This reverts commit 5043701.

…et#2307)"" This reverts commit 0bf15c4.

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

liorghub added 7 commits May 31, 2022 03:55

Add IP interface loopback action support

ea5a175

Add VS test for loopback action

5dc7668

Remove debug prints leftovers

bfb61a2

Remove more debug prints leftovers

f871125

Remove unneeded function oveloading

25a6cae

Add newlines in setIntf signature

bc7e20a

Fix review comments

ac63f6f

liorghub requested a review from prsunny as a code owner June 2, 2022 12:57

liorghub mentioned this pull request Jun 2, 2022

IP interface loopback action HLD sonic-net/SONiC#1006

Merged

dprital added the Request for 202205 Branch label Jun 8, 2022

prsunny reviewed Jun 9, 2022

View reviewed changes

Fix review comments

abe328d

Remove unused import

5e571ec

prsunny reviewed Jun 22, 2022

View reviewed changes

Fix review comments

4849030

prsunny approved these changes Jun 23, 2022

View reviewed changes

prsunny merged commit 5043701 into sonic-net:master Jun 27, 2022

yxieca pushed a commit that referenced this pull request Jun 28, 2022

Add support for IP interface loopback action (#2307)

665bbbb

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

yxieca added the Included in 202205 Branch label Jun 28, 2022

dprital mentioned this pull request Jul 7, 2022

[submodule] Advance sonic-swss pointer sonic-net/sonic-buildimage#11380

Merged

7 tasks

prabhataravind added a commit that referenced this pull request Jul 20, 2022

Revert "Add support for IP interface loopback action (#2307)"

40e2d4b

This reverts commit 5043701. Revert "[VS Test] Skip failing subport tests (#2370)" This reverts commit 7175245.

preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022

Add support for IP interface loopback action (sonic-net#2307)

56f569f

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

liorghub added a commit to liorghub/sonic-swss that referenced this pull request Aug 17, 2022

Revert "Add support for IP interface loopback action (sonic-net#2307)"

0bf15c4

This reverts commit 5043701.

liorghub added a commit to liorghub/sonic-swss that referenced this pull request Aug 18, 2022

Revert "Revert "Add support for IP interface loopback action (sonic-n…

705966d

…et#2307)"" This reverts commit 0bf15c4.

Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025

Add support for IP interface loopback action (sonic-net#2307)

55b30fc

* Add IP interface loopback action support Co-authored-by: liora <liora@nvidia.com>

Conversation

liorghub commented Jun 2, 2022

Uh oh!

liorghub commented Jun 6, 2022

Uh oh!

mssonicbld commented Jun 6, 2022

Uh oh!

azure-pipelines bot commented Jun 6, 2022

Uh oh!

prsunny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liorghub Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 14, 2022

Uh oh!

liorghub commented Jun 15, 2022

Uh oh!

mssonicbld commented Jun 15, 2022

Uh oh!

azure-pipelines bot commented Jun 15, 2022

Uh oh!

liorghub commented Jun 16, 2022

Uh oh!

liorghub commented Jun 16, 2022

Uh oh!

mssonicbld commented Jun 16, 2022

Uh oh!

azure-pipelines bot commented Jun 16, 2022

Uh oh!

liorghub commented Jun 16, 2022

Uh oh!

liorghub commented Jun 16, 2022

Uh oh!

mssonicbld commented Jun 16, 2022

Uh oh!

azure-pipelines bot commented Jun 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsunny commented Jun 23, 2022

Uh oh!

azure-pipelines bot commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liorghub Jun 14, 2022 •

edited

Loading