[vlan] Refresh dhcpv6_relay config while adding/deleting a vlan (#2660)

What I did Currently, add/del a vlan doesn't change related dhcpv6_relay config, which is incorrect. How I did it 1. Add dhcp_relay table init entry while adding vlan 2. Delete dhcp_relay related config while deleting vlan 3. Add unitest How to verify it 1. By unitest 2. install whl and run cli Signed-off-by: Yaqiang Zhu <yaqiangzhu@microsoft.com>
sonic-net · Feb 10, 2023 · 784a15c · 784a15c
1 parent 7e94c5f
commit 784a15c
Show file tree

Hide file tree

Showing 6 changed files with 118 additions and 24 deletions.
diff --git a/config/vlan.py b/config/vlan.py
@@ -1,5 +1,6 @@
 import click
 import utilities_common.cli as clicommon
+import utilities_common.dhcp_relay_util as dhcp_relay_util
 
 from jsonpatch import JsonPatchConflict
 from time import sleep
@@ -16,6 +17,11 @@ def vlan():
     """VLAN-related configuration tasks"""
     pass
 
+
+def set_dhcp_relay_table(table, config_db, vlan_name, value):
+    config_db.set_entry(table, vlan_name, value)
+
+
 @vlan.command('add')
 @click.argument('vid', metavar='<vid>', required=True, type=int)
 @clicommon.pass_db
@@ -24,22 +30,27 @@ def add_vlan(db, vid):
 
     ctx = click.get_current_context()
     vlan = 'Vlan{}'.format(vid)
-    
+
     config_db = ValidatedConfigDBConnector(db.cfgdb)
     if ADHOC_VALIDATION:
         if not clicommon.is_vlanid_in_range(vid):
             ctx.fail("Invalid VLAN ID {} (1-4094)".format(vid))
 
         if vid == 1:
             ctx.fail("{} is default VLAN".format(vlan)) # TODO: MISSING CONSTRAINT IN YANG MODEL
-    
+
         if clicommon.check_if_vlanid_exist(db.cfgdb, vlan): # TODO: MISSING CONSTRAINT IN YANG MODEL
             ctx.fail("{} already exists".format(vlan))
-
-    try:
-        config_db.set_entry('VLAN', vlan, {'vlanid': str(vid)})
-    except ValueError:
-        ctx.fail("Invalid VLAN ID {} (1-4094)".format(vid))
+        if clicommon.check_if_vlanid_exist(db.cfgdb, vlan, "DHCP_RELAY"):
+            ctx.fail("DHCPv6 relay config for {} already exists".format(vlan))
+    # set dhcpv4_relay table
+    set_dhcp_relay_table('VLAN', config_db, vlan, {'vlanid': str(vid)})
+
+    # set dhcpv6_relay table
+    set_dhcp_relay_table('DHCP_RELAY', config_db, vlan, {'vlanid': str(vid)})
+    # We need to restart dhcp_relay service after dhcpv6_relay config change
+    dhcp_relay_util.handle_restart_dhcp_relay_service()
+
 
 @vlan.command('del')
 @click.argument('vid', metavar='<vid>', required=True, type=int)
@@ -67,19 +78,23 @@ def del_vlan(db, vid):
                 ctx.fail("{} can not be removed. First remove IP addresses assigned to this VLAN".format(vlan))
 
         keys = [ (k, v) for k, v in db.cfgdb.get_table('VLAN_MEMBER') if k == 'Vlan{}'.format(vid) ]
-    
+
         if keys: # TODO: MISSING CONSTRAINT IN YANG MODEL
             ctx.fail("VLAN ID {} can not be removed. First remove all members assigned to this VLAN.".format(vid))
-        
+
         vxlan_table = db.cfgdb.get_table('VXLAN_TUNNEL_MAP')
         for vxmap_key, vxmap_data in vxlan_table.items():
             if vxmap_data['vlan'] == 'Vlan{}'.format(vid):
                 ctx.fail("vlan: {} can not be removed. First remove vxlan mapping '{}' assigned to VLAN".format(vid, '|'.join(vxmap_key)) )
-
-    try: 
-        config_db.set_entry('VLAN', 'Vlan{}'.format(vid), None)
-    except JsonPatchConflict:
-        ctx.fail("{} does not exist".format(vlan))
+
+    # set dhcpv4_relay table
+    set_dhcp_relay_table('VLAN', config_db, vlan, None)
+
+    # set dhcpv6_relay table
+    set_dhcp_relay_table('DHCP_RELAY', config_db, vlan, None)
+    # We need to restart dhcp_relay service after dhcpv6_relay config change
+    dhcp_relay_util.handle_restart_dhcp_relay_service()
+
 
 def restart_ndppd():
     verify_swss_running_cmd = "docker container inspect -f '{{.State.Status}}' swss"

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -20,6 +20,7 @@
     )
 from . import config_int_ip_common
 import utilities_common.constants as constants
+import config.main as config
 
 test_path = os.path.dirname(os.path.abspath(__file__))
 modules_path = os.path.dirname(test_path)
@@ -356,3 +357,13 @@ def setup_fib_commands():
     import show.main as show
     return show
 
+
+@pytest.fixture(scope='function')
+def mock_restart_dhcp_relay_service():
+    print("We are mocking restart dhcp_relay")
+    origin_func = config.vlan.dhcp_relay_util.handle_restart_dhcp_relay_service
+    config.vlan.dhcp_relay_util.handle_restart_dhcp_relay_service = mock.MagicMock(return_value=0)
+
+    yield
+
+    config.vlan.dhcp_relay_util.handle_restart_dhcp_relay_service = origin_func
diff --git a/tests/mclag_test.py b/tests/mclag_test.py
@@ -448,7 +448,7 @@ def test_mclag_add_member(self):
 
 
 
-    def test_mclag_add_unique_ip(self):
+    def test_mclag_add_unique_ip(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
         obj = {'db':db.cfgdb}
@@ -483,7 +483,7 @@ def test_mclag_add_unique_ip(self):
         keys = db.cfgdb.get_keys('MCLAG_UNIQUE_IP')
         assert MCLAG_UNIQUE_IP_VLAN not in keys, "unique ip not conifgured" 
 
-    def test_mclag_add_unique_ip_non_default_vrf(self):
+    def test_mclag_add_unique_ip_non_default_vrf(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
         obj = {'db':db.cfgdb}

diff --git a/tests/vlan_test.py b/tests/vlan_test.py
@@ -1,5 +1,6 @@
 import os
 import traceback
+import pytest
 from unittest import mock
 
 from click.testing import CliRunner
@@ -10,6 +11,18 @@
 from importlib import reload
 import utilities_common.bgp_util as bgp_util
 
+IP_VERSION_PARAMS_MAP = {
+    "ipv4": {
+        "table": "VLAN"
+    },
+    "ipv6": {
+        "table": "DHCP_RELAY"
+    }
+}
+DHCP_RELAY_TABLE_ENTRY = {
+    "vlanid": "1001"
+}
+
 show_vlan_brief_output="""\
 +-----------+-----------------+-----------------+----------------+-------------+
 |   VLAN ID | IP Address      | Ports           | Port Tagging   | Proxy ARP   |
@@ -143,6 +156,8 @@
 |      4000 |                 | PortChannel1001 | tagged         | disabled    |
 +-----------+-----------------+-----------------+----------------+-------------+
 """
+
+
 class TestVlan(object):
     _old_run_bgp_command = None
     @classmethod
@@ -319,7 +334,7 @@ def test_config_vlan_add_rif_portchannel_member(self):
         assert result.exit_code != 0
         assert "Error: PortChannel0001 is a router interface!" in result.output
 
-    def test_config_vlan_with_vxlanmap_del_vlan(self):
+    def test_config_vlan_with_vxlanmap_del_vlan(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
         obj = {'config_db': db.cfgdb}
@@ -343,7 +358,7 @@ def test_config_vlan_with_vxlanmap_del_vlan(self):
         assert result.exit_code != 0
         assert "Error: vlan: 1027 can not be removed. First remove vxlan mapping" in result.output
 
-    def test_config_vlan_del_vlan(self):
+    def test_config_vlan_del_vlan(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
         obj = {'config_db':db.cfgdb}
@@ -401,7 +416,7 @@ def test_config_vlan_del_nonexist_vlan_member(self):
         assert result.exit_code != 0
         assert "Error: Ethernet0 is not a member of Vlan1000" in result.output
 
-    def test_config_add_del_vlan_and_vlan_member(self):
+    def test_config_add_del_vlan_and_vlan_member(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
 
@@ -444,7 +459,7 @@ def test_config_add_del_vlan_and_vlan_member(self):
         assert result.exit_code == 0
         assert result.output == show_vlan_brief_output
 
-    def test_config_add_del_vlan_and_vlan_member_in_alias_mode(self):
+    def test_config_add_del_vlan_and_vlan_member_in_alias_mode(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
 
@@ -521,7 +536,7 @@ def test_config_vlan_proxy_arp_with_nonexist_vlan_intf(self):
             assert result.exit_code != 0
             assert "Interface Vlan1001 does not exist" in result.output
 
-    def test_config_vlan_proxy_arp_enable(self):
+    def test_config_vlan_proxy_arp_enable(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
 
@@ -533,7 +548,7 @@ def test_config_vlan_proxy_arp_enable(self):
         assert result.exit_code == 0 
         assert db.cfgdb.get_entry("VLAN_INTERFACE", "Vlan1000") == {"proxy_arp": "enabled"}
 
-    def test_config_vlan_proxy_arp_disable(self):
+    def test_config_vlan_proxy_arp_disable(self, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
 
@@ -584,6 +599,39 @@ def test_config_vlan_add_member_of_portchannel(self):
         assert result.exit_code != 0
         assert "Error: Ethernet32 is part of portchannel!" in result.output
 
+    @pytest.mark.parametrize("ip_version", ["ipv4", "ipv6"])
+    def test_config_add_del_vlan_dhcp_relay(self, ip_version, mock_restart_dhcp_relay_service):
+        runner = CliRunner()
+        db = Db()
+
+        # add vlan 1001
+        result = runner.invoke(config.config.commands["vlan"].commands["add"], ["1001"], obj=db)
+        print(result.exit_code)
+        print(result.output)
+        assert result.exit_code == 0
+
+        assert db.cfgdb.get_entry(IP_VERSION_PARAMS_MAP[ip_version]["table"], "Vlan1001") == DHCP_RELAY_TABLE_ENTRY
+
+        # del vlan 1001
+        result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001"], obj=db)
+        print(result.exit_code)
+        print(result.output)
+
+        assert "Vlan1001" not in db.cfgdb.get_keys(IP_VERSION_PARAMS_MAP[ip_version]["table"])
+
+    @pytest.mark.parametrize("ip_version", ["ipv6"])
+    def test_config_add_exist_vlan_dhcp_relay(self, ip_version):
+        runner = CliRunner()
+        db = Db()
+
+        db.cfgdb.set_entry("DHCP_RELAY", "Vlan1001", {"vlanid": "1001"})
+        # add vlan 1001
+        result = runner.invoke(config.config.commands["vlan"].commands["add"], ["1001"], obj=db)
+        print(result.exit_code)
+        print(result.output)
+        assert result.exit_code != 0
+        assert "DHCPv6 relay config for Vlan1001 already exists" in result.output
+
     @classmethod
     def teardown_class(cls):
         os.environ['UTILITIES_UNIT_TESTING'] = "0"

diff --git a/utilities_common/cli.py b/utilities_common/cli.py
@@ -251,10 +251,10 @@ def is_vlanid_in_range(vid):
 
     return False
 
-def check_if_vlanid_exist(config_db, vlan):
+def check_if_vlanid_exist(config_db, vlan, table_name='VLAN'):
     """Check if vlan id exits in the config db or ot"""
 
-    if len(config_db.get_entry('VLAN', vlan)) != 0:
+    if len(config_db.get_entry(table_name, vlan)) != 0:
         return True
 
     return False

diff --git a/utilities_common/dhcp_relay_util.py b/utilities_common/dhcp_relay_util.py
@@ -0,0 +1,20 @@
+import click
+import utilities_common.cli as clicommon
+
+
+def restart_dhcp_relay_service():
+    """
+    Restart dhcp_relay service
+    """
+    click.echo("Restarting DHCP relay service...")
+    clicommon.run_command("systemctl stop dhcp_relay", display_cmd=False)
+    clicommon.run_command("systemctl reset-failed dhcp_relay", display_cmd=False)
+    clicommon.run_command("systemctl start dhcp_relay", display_cmd=False)
+
+
+def handle_restart_dhcp_relay_service():
+    try:
+        restart_dhcp_relay_service()
+    except SystemExit as e:
+        ctx = click.get_current_context()
+        ctx.fail("Restart service dhcp_relay failed with error {}".format(e))