Add SAG implementation #1887

superchild · 2021-10-20T07:52:09Z

What I did

Add static anycast gateway related commands support.
Refer to SAG HLD#837

How I did it

Add new commands
- config static-anycast-gateway mac_address add/del
- config vlan static-anycast-gateway add/del
Add unit test cases

How to verify it

Executing unit tests

Signed-off-by: Jimi Chen jimi_chen@edge-core.com

config/main.py

config/vlan.py

show/main.py

superchild · 2021-11-15T07:50:43Z

@cbpaviz
Thanks for your comments, codes are modified according that.
Please help to check and review again.

superchild · 2021-12-06T03:19:34Z

/azpw run

mssonicbld · 2021-12-06T03:19:37Z

/AzurePipelines run

azure-pipelines · 2021-12-06T03:19:47Z

Azure Pipelines successfully started running 1 pipeline(s).

superchild · 2021-12-07T01:17:09Z

/azp run

azure-pipelines · 2021-12-07T01:17:14Z

Commenter does not have sufficient privileges for PR 1887 in repo Azure/sonic-utilities

superchild · 2021-12-07T01:17:40Z

/azpw run

mssonicbld · 2021-12-07T01:17:41Z

/AzurePipelines run

azure-pipelines · 2021-12-07T01:17:52Z

Azure Pipelines successfully started running 1 pipeline(s).

superchild · 2021-12-10T05:43:12Z

@cbpaviz Could you help to review again, I merged the code from master branch due to some confict.

superchild · 2021-12-15T00:55:09Z

/azpw run

mssonicbld · 2021-12-15T00:55:10Z

/AzurePipelines run

azure-pipelines · 2021-12-15T00:55:21Z

Azure Pipelines successfully started running 1 pipeline(s).

superchild · 2021-12-15T01:08:46Z

/azpw run

mssonicbld · 2021-12-15T01:08:47Z

/AzurePipelines run

azure-pipelines · 2021-12-15T01:08:58Z

Azure Pipelines successfully started running 1 pipeline(s).

yxieca · 2021-12-27T17:30:29Z

/azp run

azure-pipelines · 2021-12-27T17:30:39Z

Azure Pipelines successfully started running 1 pipeline(s).

superchild · 2022-05-06T13:43:40Z

@cbpaviz
Could you help to review again?
Thanks.

superchild · 2023-02-22T01:22:49Z

@cbpaviz Could you help to review again, I merged the code from master branch due to some conflict.

kulpatel · 2023-02-27T06:26:44Z

@superchild , why SAG config commands are different compare to EC config commands (available in their config guid). is it intentional ?

superchild · 2023-03-01T01:13:42Z

@kulpatel, this PR is discussed and reviewed by community, EC will also follow the same design after this merged.
The original design is slightly different from community's opinion.

Junchao-Mellanox · 2023-03-07T06:32:32Z

config/main.py

+@mac_address.command('del')
+@click.argument('mac_address', metavar='<mac_address>', required=True, type=str)
+@clicommon.pass_db
+def del_mac(db, mac_address):


Since only one mac address is allowed. mac_address is not a useful argument here.

This is removed in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:35:47Z

config/vlan.py

@@ -95,6 +95,12 @@ def del_vlan(db, vid):
    # We need to restart dhcp_relay service after dhcpv6_relay config change
    dhcp_relay_util.handle_restart_dhcp_relay_service()

+    keys = [ (k, v) for k, v in db.cfgdb.get_table('VLAN_MEMBER') if k == 'Vlan{}'.format(vid) ]


is this change related to this feature?

This is reverted in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:37:22Z

config/vlan.py

+    if not clicommon.is_valid_vlan_interface(db.cfgdb, vlan):
+        ctx.fail(f"Interface {vlan} does not exist")
+
+    db.cfgdb.mod_entry('VLAN_INTERFACE', vlan, {"static_anycast_gateway": "true"})


do we need check whether static_anycast_gateway has already been configured?

This is removed in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:38:15Z

config/main.py

+    sag_entry = db.cfgdb.get_entry('SAG', 'GLOBAL')
+    if sag_entry:
+        if sag_entry.get('gateway_mac').lower() == mac_address.lower():
+            db.cfgdb.mod_entry('SAG', 'GLOBAL', None)


do we need disable sag for each vlan?

This is handled in sonic-swss.

Junchao-Mellanox · 2023-03-07T06:40:47Z

config/vlan.py

+
+    log.log_info(f"'vlan static-anycast-gateway disable {vid}' executing...")
+
+    if not clicommon.is_vlanid_in_range(vid):


if this check is needed, maybe we should do the same for enable_vlan_sag; otherwise, please remove this check

This is removed in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:42:25Z

config/vlan.py

+    keys = [ (k, v) for k, v in db.cfgdb.get_table('VLAN_MEMBER') if k == 'Vlan{}'.format(vid) ]
+
+    if keys:
+        ctx.fail("VLAN ID {} can not be removed. First remove all members assigned to this VLAN.".format(vid))


Use f-string

Junchao-Mellanox · 2023-03-07T06:42:36Z

config/vlan.py

+    if keys:
+        ctx.fail("VLAN ID {} can not be removed. First remove all members assigned to this VLAN.".format(vid))
+
+    db.cfgdb.set_entry('VLAN', 'Vlan{}'.format(vid), None)


Use f-string

Junchao-Mellanox · 2023-03-07T06:44:40Z

show/main.py

+@clicommon.pass_db
+def sag(db):
+    """Show static anycast gateway information"""
+    sag_entry = db.cfgdb.get_entry('SAG', 'GLOBAL')


what if SAG is not configured?

This is changed in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:46:58Z

show/main.py

+    body = []
+    intf_dict = db.cfgdb.get_table('VLAN_INTERFACE')
+    if intf_dict:
+        for intf in intf_dict:


It would be more brief to use

for key,value in intf_dict.items(): if value.get('static_anycast_gateway') == 'true':

This is removed in the updated PR #2881

Junchao-Mellanox · 2023-03-07T06:53:29Z

show/vlan.py

+    cfg, _ = ctx
+    _, vlan_ip_data, _ = cfg
+
+    for key in vlan_ip_data:


why do we need a for loop here? how about this:

if vlan in vlan_ip_data:

This is removed in the updated PR #2881

superchild · 2023-06-16T03:50:53Z

@zhangyanzhao
Sorry for the wrong operation sync, this PR's commit history is gone.
I already reset the commit on my dev branch, could you please reopen this PR?
Or should I resend a new PR for it?

superchild mentioned this pull request Oct 20, 2021

Static Anycast Gateway HLD sonic-net/SONiC#837

Merged

superchild mentioned this pull request Oct 28, 2021

Add SAG SONiC Yang sonic-net/sonic-buildimage#9018

Open

5 tasks

cbpaviz reviewed Nov 13, 2021

View reviewed changes

config/main.py Outdated Show resolved Hide resolved

cbpaviz reviewed Nov 13, 2021

View reviewed changes

config/vlan.py Outdated Show resolved Hide resolved

cbpaviz reviewed Nov 13, 2021

View reviewed changes

show/main.py Outdated Show resolved Hide resolved

cbpaviz previously approved these changes Nov 16, 2021

View reviewed changes

superchild dismissed cbpaviz’s stale review via c9f131d December 3, 2021 07:12

cbpaviz previously approved these changes May 6, 2022

View reviewed changes

superchild dismissed cbpaviz’s stale review via 28a1396 February 22, 2023 01:11

Junchao-Mellanox suggested changes Mar 7, 2023

View reviewed changes

superchild closed this Jun 16, 2023

superchild force-pushed the sag-dev branch 2 times, most recently from 28a1396 to 3ba8241 Compare June 16, 2023 03:38

superchild mentioned this pull request Jun 19, 2023

Add SAG implementation #2881

Open


		log.log_info(f"'vlan static-anycast-gateway disable {vid}' executing...")

		if not clicommon.is_vlanid_in_range(vid):

Add SAG implementation #1887

Add SAG implementation #1887

Conversation

superchild commented Oct 20, 2021

What I did

How I did it

How to verify it

superchild commented Nov 15, 2021

superchild commented Dec 6, 2021

mssonicbld commented Dec 6, 2021

azure-pipelines bot commented Dec 6, 2021

superchild commented Dec 7, 2021

azure-pipelines bot commented Dec 7, 2021

superchild commented Dec 7, 2021

mssonicbld commented Dec 7, 2021

azure-pipelines bot commented Dec 7, 2021

superchild commented Dec 10, 2021

superchild commented Dec 15, 2021

mssonicbld commented Dec 15, 2021

azure-pipelines bot commented Dec 15, 2021

superchild commented Dec 15, 2021

mssonicbld commented Dec 15, 2021

azure-pipelines bot commented Dec 15, 2021

yxieca commented Dec 27, 2021

azure-pipelines bot commented Dec 27, 2021

superchild commented May 6, 2022

superchild commented Feb 22, 2023

kulpatel commented Feb 27, 2023

superchild commented Mar 1, 2023

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

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

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

superchild commented Jun 16, 2023