[SRv6]: SRv6 VPN SID by shuaishang · Pull Request #2793 · sonic-net/sonic-swss

shuaishang · 2023-05-25T08:51:13Z

What I did
HLD: SRv6 VPN HLD for 202305 release

Why I did it
This PR is to support SRv6 VPN functions

It depends on SAI 1.12. opencomputeproject/SAI #1744

How I verified it
pytest test_srv6.py
PS: to reduce the PR size, i created another PR #2766 for vs test.

Details if related

zhangyanzhao · 2023-05-25T15:52:24Z

added more reviewers

shuaishang · 2023-06-09T06:27:08Z

Reviewed in Routing WG weekly meeting 06082023:
https://lists.sonicfoundation.dev/g/sonic-wg-routing/wiki/34083

kperumalbfn · 2023-06-02T22:41:18Z

                    if (fvField(i) == "seg_src")
                        srv6_source = fvValue(i);

+                    if (fvField(i) == "vpn_sid" && fvValue(i) != "")


Could you update these new fields in doc/swss-schema.md

kperumalbfn · 2023-06-02T22:43:35Z

                        srv6_source = fvValue(i);

+                    if (fvField(i) == "vpn_sid" && fvValue(i) != "")
+                    {


Does this code change handle SRV6 policy referenced in HLD? There is no reference to SRv6_POLICY_TABLE table?

This PR only includes SRv6 VPN feature, no SRv6 Policy.

kperumalbfn · 2023-06-12T23:13:42Z

    NextHopKey(const std::string &str) :
-        vni(0), mac_address()
+        vni(0), mac_address(),
+        srv6_vpn_sid(""),


can we move the vpn_sid to the end of NH key without affecting the non-VPN NH key?

It will not affect non-VPN NH key, however I moved srv6_vpn_sid to end of NH key.

prsunny · 2023-06-13T00:05:05Z

/azp run

azure-pipelines · 2023-06-13T00:05:18Z

Azure Pipelines successfully started running 1 pipeline(s).

kperumalbfn · 2023-06-14T22:11:33Z

+uint32_t Srv6Orch::getAggId(const NextHopGroupKey &nhg)
+{
+    SWSS_LOG_ENTER();
+    static uint32_t g_agg_id = 1;


instead of having static g_agg_id inside a member function, can we move this to srv6Orch object?

It will be better to use some sort of index allocator instead of looping over the indices to determine the free index. Index allocator will be better for performance.

Fixed, changed to member "m_srv6_agg_id".

Since there is no "index allocator" library in orch and we don't want to introduce dependency by new library, we write this simple function to get the id. And in our system level test, there is no performance issue. Is it ok to keep this function?

kperumalbfn · 2023-06-14T22:20:39Z

+            {
+                if (!deleteSrv6Vpn(sr_nh, getAggId(it_nhg)))
+                {
+                    SWSS_LOG_ERROR("Failed to delete SRV6 vpn %s", sr_nh.to_string(false, true).c_str());


We can also include prefix aggId in the error log for better debugging

Fixed, add it in "createSrv6Vpn/deleteSrv6Vpn" error log.

kperumalbfn · 2023-06-15T18:09:55Z

+void Srv6Orch::increasePrefixAggIdRefCount(const NextHopGroupKey &nhg)
+{
+    SWSS_LOG_ENTER();
+    string k = nhg.get_srv6_vpn_key();


When the set of NHs are same, but the order of NHs in NHG key are different, does it have a different key? If so, we will use different prefix_agg_id. Could you please clarify this case?

It should be treated as same key, I fixed it.
Appreciated for your review comments~

…string.

shuaishang · 2023-07-31T06:59:11Z

@prsunny @kperumalbfn
Any more comments? Could this PR be approved?

kperumalbfn · 2023-08-08T19:55:20Z

+    tunnel_map_entry_attr.value.u32 = prefix_agg_id;
+    tunnel_map_entry_attrs.push_back(tunnel_map_entry_attr);
+
+    IpAddress vpn_sid(nh.srv6_vpn_sid);


Do we have any sonic-utility CLI to dump these map entries? We have VLAN-->VNI, VRF-->VNI map entries under 'show vxlan'. We could have one more for SRV6 tunnel map entries.

sure, I can add the CLI in sonic-utility after this PR merged.

kperumalbfn · 2023-08-08T19:57:16Z

@prsunny @kperumalbfn Any more comments? Could this PR be approved?

@shuaishang Please check the sanity failures and update the branch. Added one comment, pls check.

shuaishang · 2023-08-09T02:53:28Z

/azp run

azure-pipelines · 2023-08-09T02:53:33Z

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

lguohan · 2023-08-22T08:26:34Z

there isn't any unit test code for such pr? @prsunny , what is our guidance for the unit test coverage for such pr?

lguohan

please add unit test coverage to be 90%

kperumalbfn · 2023-08-22T15:32:24Z

there isn't any unit test code for such pr? @prsunny , what is our guidance for the unit test coverage for such pr?

sonic-swss tests are in a separate PR - #2766

@shuaishang It will be better to merge these two PRs.

prsunny · 2023-09-07T19:04:22Z

there isn't any unit test code for such pr? @prsunny , what is our guidance for the unit test coverage for such pr?

sonic-swss tests are in a separate PR - #2766

@shuaishang It will be better to merge these two PRs.

Agree, the test and PR must be together and in same PR.

shuaishang · 2023-09-08T01:47:42Z

@kperumalbfn @prsunny
Done. Merged the unit tests from #2766 into this PR.

shuaishang · 2023-09-11T07:03:16Z

@kperumalbfn @prsunny Done. Merged the unit tests from #2766 into this PR.

@kperumalbfn @prsunny could you please help to check? any more comments?

shuaishang · 2023-10-07T07:23:21Z

@kperumalbfn Any more comments?

kperumalbfn · 2023-10-09T19:41:40Z

@shuaishang Any plan for SRV6 sonic-mgmt tests? If so, please add those PRs.

shuaishang · 2023-10-10T09:23:09Z

@shuaishang Any plan for SRV6 sonic-mgmt tests? If so, please add those PRs.

@kperumalbfn No plan yet since the SONiC VS (linux) can't support SRv6 VPN packet yet..

kperumalbfn

LGTM

srv6 vpn

8e3e7bf

shuaishang requested a review from prsunny as a code owner May 25, 2023 08:51

shuaishang mentioned this pull request May 25, 2023

[202305][SRv6]: SRv6 VPN SID #2765

Closed

eddieruan-alibaba mentioned this pull request May 25, 2023

SRv6 VPN HLD for 202305 release sonic-net/SONiC#1252

Merged

zhangyanzhao requested review from srj102 and svshah-intel May 25, 2023 15:51

prsunny changed the title ~~[202305][SRv6]: SRv6 VPN SID~~ [SRv6]: SRv6 VPN SID May 27, 2023

prsunny requested review from kperumalbfn and stephenxs May 27, 2023 00:17

kperumalbfn reviewed Jun 12, 2023

View reviewed changes

kperumalbfn reviewed Jun 14, 2023

View reviewed changes

kperumalbfn reviewed Jun 15, 2023

View reviewed changes

shuaishang and others added 4 commits June 25, 2023 12:07

addressing review comment

c2b80de

Merge branch 'sonic-net:master' into srv6_vpn

3d95cad

addressing review comment

e74bc24

The key of Srv6PrefixAggIdTable should be NextHopGroupKey instead of …

4da302a

…string.

kperumalbfn reviewed Aug 8, 2023

View reviewed changes

kperumalbfn previously approved these changes Aug 11, 2023

View reviewed changes

lguohan requested changes Aug 22, 2023

View reviewed changes

shuaishang and others added 2 commits September 8, 2023 09:40

Merge branch 'sonic-net:master' into srv6_vpn

4307304

swss UTs for srv6 vpn functions

728cec5

shuaishang dismissed kperumalbfn’s stale review via 728cec5 September 8, 2023 01:45

kperumalbfn approved these changes Oct 16, 2023

View reviewed changes

Merge branch 'master' into srv6_vpn

ad303ea

shuaishang closed this Nov 19, 2024

Conversation

shuaishang commented May 25, 2023

Uh oh!

zhangyanzhao commented May 25, 2023

Uh oh!

shuaishang commented Jun 9, 2023

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!

prsunny commented Jun 13, 2023

Uh oh!

azure-pipelines bot commented Jun 13, 2023

Uh oh!

kperumalbfn Jun 14, 2023 • 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!

shuaishang commented Jul 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kperumalbfn commented Aug 8, 2023

Uh oh!

shuaishang commented Aug 9, 2023

Uh oh!

azure-pipelines bot commented Aug 9, 2023

Uh oh!

lguohan commented Aug 22, 2023

Uh oh!

lguohan left a comment

Choose a reason for hiding this comment

Uh oh!

kperumalbfn commented Aug 22, 2023

Uh oh!

prsunny commented Sep 7, 2023

Uh oh!

shuaishang commented Sep 8, 2023

Uh oh!

shuaishang commented Sep 11, 2023

Uh oh!

shuaishang commented Oct 7, 2023

Uh oh!

kperumalbfn commented Oct 9, 2023

Uh oh!

shuaishang commented Oct 10, 2023

Uh oh!

kperumalbfn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kperumalbfn Jun 14, 2023 •

edited

Loading