[orchagent]: Extend the SRv6Orch to support the programming of the L3Adj #2902

cscarpitta · 2023-09-13T16:47:54Z

What I did
Extended the SRv6Orch to support the programming of the L3Adj associated with SRv6 uA, End.X, uDX4, uDX6, End.DX4, and End.DX6 behaviors.
HLD: sonic-net/SONiC#1472

Why I did it
The current implementation of SRv6Orch does not process the L3Adj.

How I verified it
Added new test cases to test_srv6.py

Extend the SRv6Orch to support the programming of the L3Adj associated with SRv6 uA, End.X, uDX4, uDX6, End.DX4, and End.DX6 behaviors. Signed-off-by: Carmine Scarpitta <[email protected]>

Add unit tests for several SRv6 behaviors: uA, uDX4, uDX6, End.X, End.DX4, End.DX6 Signed-off-by: Carmine Scarpitta <[email protected]>

Signed-off-by: Carmine Scarpitta <[email protected]>

ahsalam · 2023-10-14T09:14:59Z

@prsunny @kperumalbfn could you please help and review the code of this PR?
This feature is tracked for 202311 release and documented by this HLD sonic-net/SONiC#1472
@zhangyanzhao

prsunny · 2023-10-24T21:29:36Z

@prsunny @kperumalbfn could you please help and review the code of this PR? This feature is tracked for 202311 release and documented by this HLD sonic-net/SONiC#1472 @zhangyanzhao

@kperumalbfn to review/signoff

shuaishang · 2023-10-25T08:38:33Z

orchagent/srv6orch.cpp

+        SWSS_LOG_INFO("Neighbor ADD event: %s, installing pending SRv6 SIDs",
+                        update.entry.ip_address.to_string().c_str());
+
+        auto it = m_pendingSRv6MySIDEntries.find(NextHopKey(update.entry.ip_address.to_string()));


The update.entry should contains ip_address + alias，you can't lookup m_pendingSRv6MySIDEntries with only ip_address.
Two neighbors may has same ip_address and different alias.

@shuaishang I changed the code to use ip_address + alias as key when performing the lookup in m_pendingSRv6MySIDEntries.

kperumalbfn · 2023-10-24T22:56:53Z

orchagent/srv6orch.cpp

+    if (mySidNextHopRequired(end_behavior))
+    {
+        sai_object_id_t next_hop_id;
+        nexthop = NextHopKey(adj); 


should we return error if this adj is ECMP as this is not yet supported?

@kperumalbfn I added a check to return an error if the adj is ECMP: 6bb9fe2

kperumalbfn · 2023-10-25T16:43:52Z

tests/test_srv6.py

+            elif fv[0] == "SAI_MY_SID_ENTRY_ATTR_ENDPOINT_BEHAVIOR_FLAVOR":
+                assert fv[1] == "SAI_MY_SID_ENTRY_ENDPOINT_BEHAVIOR_FLAVOR_PSP_AND_USD"
+
+        # create MySID END.DX6


can you add a testcase that tests neighbor add/delete/update after adding mysid entry that covers m_pendingSRv6MySIDEntries update and delete and iterate over all the mysid entries for the neighbor update.

@kperumalbfn I added the test case as requested: 2e9485c

kperumalbfn · 2023-10-25T16:52:21Z

orchagent/srv6orch.cpp

+             * when the neighbor comes back
+             */
+            auto pending_mysid_entry = make_tuple(my_sid_string, dt_vrf, adj, end_action);
+            m_pendingSRv6MySIDEntries[NextHopKey(update.entry.ip_address.to_string())].insert(pending_mysid_entry);


NextHopKey with ip_address and interface?? Check all the other places where m_pendingSRv6MySIDEntries dictionary is accessed.

@kperumalbfn Fixed. I changed the code to use ip_address + alias as key when performing the lookup in m_pendingSRv6MySIDEntries.

eddieruan-alibaba · 2023-10-25T06:36:07Z

orchagent/srv6orch.cpp

+            next_hop_id = m_neighOrch->getNextHopId(nexthop);
+            if(next_hop_id == SAI_NULL_OBJECT_ID)
+            {
+              SWSS_LOG_ERROR("Failed to get nexthop for adjacency %s", adj.c_str());


What's the reason that you don't add it to pending list?

Would it be better to let it retry?

@eddieruan-alibaba Agree. I changed the code to add the SID to the pending list: b4c557f

When installing a SID associated with a L3Adj, check if the L3Adj is ECMP. If the L3Adj is ECMP, returns an error, because ECMP is not yet supported. Signed-off-by: Carmine Scarpitta <[email protected]>

Signed-off-by: Carmine Scarpitta <[email protected]>

If SID installation fails due to nexthop not available, add the SID to the pending SIDs list and retry when the nexthop becomes available Signed-off-by: Carmine Scarpitta <[email protected]>

Signed-off-by: Carmine Scarpitta <[email protected]>

Verify that the MySID is installed as soon as the associated neighbor (L3 Adjacency) is ready. Signed-off-by: Carmine Scarpitta <[email protected]>

cscarpitta · 2023-10-27T00:17:48Z

@kperumalbfn @eddieruan-alibaba @shuaishang Many thanks for the review. All comments have been addressed.

kperumalbfn

LGTM

ahsalam · 2023-11-13T16:19:03Z

Hi @prsunny, the PR has been approved by @kperumalbfn . It is ready for merge, could you please merge it ?
The HLD has been already merged.

cscarpitta · 2023-11-16T20:52:48Z

Hi @prsunny This PR has been reviewed and approved. It is ready for merge. Could you please merge the PR?

cscarpitta · 2023-11-21T07:46:36Z

/azp run

azure-pipelines · 2023-11-21T07:46:41Z

Commenter does not have sufficient privileges for PR 2902 in repo sonic-net/sonic-swss

cscarpitta · 2023-11-21T11:26:52Z

/azpw run

mssonicbld · 2023-11-21T11:26:56Z

/AzurePipelines run

azure-pipelines · 2023-11-21T11:27:06Z

Azure Pipelines successfully started running 1 pipeline(s).

[orchagent]: Extend SRv6Orch to support L3Adj

12dce9e

Extend the SRv6Orch to support the programming of the L3Adj associated with SRv6 uA, End.X, uDX4, uDX6, End.DX4, and End.DX6 behaviors. Signed-off-by: Carmine Scarpitta <[email protected]>

cscarpitta requested a review from prsunny as a code owner September 13, 2023 16:47

cscarpitta mentioned this pull request Sep 13, 2023

Add SRv6 SID L3Adj HLD sonic-net/SONiC#1472

Merged

[tests]: Add unit tests for SRv6 uA, uDX*, End.X, End.DX*

81eb9ef

Add unit tests for several SRv6 behaviors: uA, uDX4, uDX6, End.X, End.DX4, End.DX6 Signed-off-by: Carmine Scarpitta <[email protected]>

cscarpitta force-pushed the srv6_sid_l3adj branch from dd936e5 to 81eb9ef Compare September 15, 2023 09:36

prsunny requested a review from kperumalbfn September 18, 2023 21:42

cscarpitta added 3 commits October 3, 2023 02:55

Merge remote-tracking branch 'origin/master' into srv6_sid_l3adj

c5027ce

[orchagent]: Handle neighbor "up" / "down" events in SRv6Orch

80c2c07

Signed-off-by: Carmine Scarpitta <[email protected]>

[orchagent] Fix SRv6 test failures

eb5bb8a

Signed-off-by: Carmine Scarpitta <[email protected]>

Merge branch 'master' into srv6_sid_l3adj

cde7563

shuaishang reviewed Oct 25, 2023

View reviewed changes

Perform MySID lookup based on IP address + alias

e69f788

kperumalbfn reviewed Oct 25, 2023

View reviewed changes

eddieruan-alibaba reviewed Oct 25, 2023

View reviewed changes

cscarpitta and others added 7 commits October 27, 2023 01:43

[orchagent] Check if SRv6 SID L3Adj is ECMP

6bb9fe2

When installing a SID associated with a L3Adj, check if the L3Adj is ECMP. If the L3Adj is ECMP, returns an error, because ECMP is not yet supported. Signed-off-by: Carmine Scarpitta <[email protected]>

[orchagent] Perform MySID lookup based on IP address + alias

161f4b1

Signed-off-by: Carmine Scarpitta <[email protected]>

[orchagent] Minor fixes in SRv6Orch

48f5e56

Signed-off-by: Carmine Scarpitta <[email protected]>

[orchagent] Postpone SRv6 SID installation when nexthop is not ready

b4c557f

If SID installation fails due to nexthop not available, add the SID to the pending SIDs list and retry when the nexthop becomes available Signed-off-by: Carmine Scarpitta <[email protected]>

[tests]: Minor fixes in test_srv6.py

7e7dcff

Signed-off-by: Carmine Scarpitta <[email protected]>

[tests]: Add test cases to verify SID L3Adj

2e9485c

Verify that the MySID is installed as soon as the associated neighbor (L3 Adjacency) is ready. Signed-off-by: Carmine Scarpitta <[email protected]>

Merge branch 'master' into srv6_sid_l3adj

4efe27b

Merge branch 'master' into srv6_sid_l3adj

b342ef3

kperumalbfn approved these changes Nov 2, 2023

View reviewed changes

Merge branch 'master' into srv6_sid_l3adj

393da08

Merge branch 'master' into srv6_sid_l3adj

6b4b8c0

prsunny approved these changes Nov 22, 2023

View reviewed changes

prsunny merged commit b8841ec into sonic-net:master Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[orchagent]: Extend the SRv6Orch to support the programming of the L3Adj #2902

[orchagent]: Extend the SRv6Orch to support the programming of the L3Adj #2902

cscarpitta commented Sep 13, 2023 •

edited

Loading

ahsalam commented Oct 14, 2023

prsunny commented Oct 24, 2023

shuaishang Oct 25, 2023

cscarpitta Oct 27, 2023 •

edited

Loading

kperumalbfn Oct 24, 2023

cscarpitta Oct 27, 2023

kperumalbfn Oct 25, 2023 •

edited

Loading

cscarpitta Oct 27, 2023

kperumalbfn Oct 25, 2023

cscarpitta Oct 27, 2023 •

edited

Loading

eddieruan-alibaba Oct 25, 2023

cscarpitta Oct 27, 2023

cscarpitta commented Oct 27, 2023

kperumalbfn left a comment

ahsalam commented Nov 13, 2023 •

edited

Loading

cscarpitta commented Nov 16, 2023

cscarpitta commented Nov 21, 2023

azure-pipelines bot commented Nov 21, 2023

cscarpitta commented Nov 21, 2023

mssonicbld commented Nov 21, 2023

azure-pipelines bot commented Nov 21, 2023

[orchagent]: Extend the SRv6Orch to support the programming of the L3Adj #2902

[orchagent]: Extend the SRv6Orch to support the programming of the L3Adj #2902

Conversation

cscarpitta commented Sep 13, 2023 • edited Loading

ahsalam commented Oct 14, 2023

prsunny commented Oct 24, 2023

shuaishang Oct 25, 2023

Choose a reason for hiding this comment

cscarpitta Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

kperumalbfn Oct 24, 2023

Choose a reason for hiding this comment

cscarpitta Oct 27, 2023

Choose a reason for hiding this comment

kperumalbfn Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

cscarpitta Oct 27, 2023

Choose a reason for hiding this comment

kperumalbfn Oct 25, 2023

Choose a reason for hiding this comment

cscarpitta Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

eddieruan-alibaba Oct 25, 2023

Choose a reason for hiding this comment

cscarpitta Oct 27, 2023

Choose a reason for hiding this comment

cscarpitta commented Oct 27, 2023

kperumalbfn left a comment

Choose a reason for hiding this comment

ahsalam commented Nov 13, 2023 • edited Loading

cscarpitta commented Nov 16, 2023

cscarpitta commented Nov 21, 2023

azure-pipelines bot commented Nov 21, 2023

cscarpitta commented Nov 21, 2023

mssonicbld commented Nov 21, 2023

azure-pipelines bot commented Nov 21, 2023

cscarpitta commented Sep 13, 2023 •

edited

Loading

cscarpitta Oct 27, 2023 •

edited

Loading

kperumalbfn Oct 25, 2023 •

edited

Loading

cscarpitta Oct 27, 2023 •

edited

Loading

ahsalam commented Nov 13, 2023 •

edited

Loading