Add SAG implementation by superchild · Pull Request #1887 · sonic-net/sonic-utilities

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

    # 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

Closed

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 requested 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):

Conversation

superchild commented Oct 20, 2021

What I did

How I did it

How to verify it

Uh oh!

Uh oh!

Uh oh!

Uh oh!

superchild commented Nov 15, 2021

Uh oh!

superchild commented Dec 6, 2021

Uh oh!

mssonicbld commented Dec 6, 2021

Uh oh!

azure-pipelines bot commented Dec 6, 2021

Uh oh!

superchild commented Dec 7, 2021

Uh oh!

azure-pipelines bot commented Dec 7, 2021

Uh oh!

superchild commented Dec 7, 2021

Uh oh!

mssonicbld commented Dec 7, 2021

Uh oh!

azure-pipelines bot commented Dec 7, 2021

Uh oh!

superchild commented Dec 10, 2021

Uh oh!

superchild commented Dec 15, 2021

Uh oh!

mssonicbld commented Dec 15, 2021

Uh oh!

azure-pipelines bot commented Dec 15, 2021

Uh oh!

superchild commented Dec 15, 2021

Uh oh!

mssonicbld commented Dec 15, 2021

Uh oh!

azure-pipelines bot commented Dec 15, 2021

Uh oh!

yxieca commented Dec 27, 2021

Uh oh!

azure-pipelines bot commented Dec 27, 2021

Uh oh!

superchild commented May 6, 2022

Uh oh!

superchild commented Feb 22, 2023

Uh oh!

kulpatel commented Feb 27, 2023

Uh oh!

superchild commented Mar 1, 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!

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