[dhcp_relay] Fix dhcp_relay restart error while add/del vlan (#2688)

Why I did
In device that doesn't have dhcp_relay service, restart dhcp_relay after add/del vlan would encounter failed

How I did it
Add support to check whether device is support dhcp_relay service.

How to verify it
1. Unit test
2. Build and install in device

Signed-off-by: Yaqiang Zhu <yaqiangzhu@microsoft.com>

config/vlan.py

@@ -8,6 +8,8 @@
 from .validated_config_db_connector import ValidatedConfigDBConnector
 
 ADHOC_VALIDATION = True
+DHCP_RELAY_TABLE = "DHCP_RELAY"
+DHCPV6_SERVERS = "dhcpv6_servers"
 
 #
 # 'vlan' group ('config vlan ...')
@@ -22,6 +24,11 @@ def set_dhcp_relay_table(table, config_db, vlan_name, value):
     config_db.set_entry(table, vlan_name, value)
 
 
+def is_dhcp_relay_running():
+    out, _ = clicommon.run_command("systemctl show dhcp_relay.service --property ActiveState --value", return_cmd=True)
+    return out.strip() == "active"
+
+
 @vlan.command('add')
 @click.argument('vid', metavar='<vid>', required=True, type=int)
 @clicommon.pass_db
@@ -46,22 +53,34 @@ def add_vlan(db, vid):
     # 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, None)
-    # We need to restart dhcp_relay service after dhcpv6_relay config change
-    dhcp_relay_util.handle_restart_dhcp_relay_service()
+
+def is_dhcpv6_relay_config_exist(db, vlan_name):
+    keys = db.cfgdb.get_keys(DHCP_RELAY_TABLE)
+    if len(keys) == 0 or vlan_name not in keys:
+        return False
+
+    table = db.cfgdb.get_entry("DHCP_RELAY", vlan_name)
+    dhcpv6_servers = table.get(DHCPV6_SERVERS, [])
+    if len(dhcpv6_servers) > 0:
+        return True
 
 
 @vlan.command('del')
 @click.argument('vid', metavar='<vid>', required=True, type=int)
+@click.option('--no_restart_dhcp_relay', is_flag=True, type=click.BOOL, required=False, default=False,
+              help="If no_restart_dhcp_relay is True, do not restart dhcp_relay while del vlan and \
+                  require dhcpv6 relay of this is empty")
 @clicommon.pass_db
-def del_vlan(db, vid):
+def del_vlan(db, vid, no_restart_dhcp_relay):
     """Delete VLAN"""
 
     log.log_info("'vlan del {}' executing...".format(vid))
 
     ctx = click.get_current_context()
     vlan = 'Vlan{}'.format(vid)
+    if no_restart_dhcp_relay:
+        if is_dhcpv6_relay_config_exist(db, vlan):
+            ctx.fail("Can't delete {} because related DHCPv6 Relay config is exist".format(vlan))
 
     config_db = ValidatedConfigDBConnector(db.cfgdb)
     if ADHOC_VALIDATION:
@@ -90,10 +109,12 @@ def del_vlan(db, vid):
     # 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()
+    if not no_restart_dhcp_relay and is_dhcpv6_relay_config_exist(db, vlan):
+        # 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
+        if is_dhcp_relay_running():
+            dhcp_relay_util.handle_restart_dhcp_relay_service()
 
 
 def restart_ndppd():

tests/conftest.py

@@ -361,9 +361,13 @@ def setup_fib_commands():
 @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)
+    origin_funcs = []
+    origin_funcs.append(config.vlan.dhcp_relay_util.restart_dhcp_relay_service)
+    origin_funcs.append(config.vlan.is_dhcp_relay_running)
+    config.vlan.dhcp_relay_util.restart_dhcp_relay_service = mock.MagicMock(return_value=0)
+    config.vlan.is_dhcp_relay_running = mock.MagicMock(return_value=True)
 
     yield
 
-    config.vlan.dhcp_relay_util.handle_restart_dhcp_relay_service = origin_func
+    config.vlan.dhcp_relay_util.restart_dhcp_relay_service = origin_funcs[0]
+    config.vlan.is_dhcp_relay_running = origin_funcs[1]

tests/vlan_test.py

@@ -597,7 +597,7 @@ def test_config_vlan_add_member_of_portchannel(self):
         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):
+    def test_config_add_del_vlan_dhcp_relay_with_empty_entry(self, ip_version, mock_restart_dhcp_relay_service):
         runner = CliRunner()
         db = Db()
 
@@ -611,11 +611,103 @@ def test_config_add_del_vlan_dhcp_relay(self, ip_version, mock_restart_dhcp_rela
         assert db.cfgdb.get_entry(IP_VERSION_PARAMS_MAP[ip_version]["table"], "Vlan1001") == exp_output
 
         # del vlan 1001
-        result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001"], obj=db)
+        with mock.patch("utilities_common.dhcp_relay_util.handle_restart_dhcp_relay_service") as mock_handle_restart:
+            result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+
+            assert result.exit_code == 0
+            assert "Vlan1001" not in db.cfgdb.get_keys(IP_VERSION_PARAMS_MAP[ip_version]["table"])
+            assert "Restart service dhcp_relay failed with error" not in result.output
+
+    @pytest.mark.parametrize("ip_version", ["ipv4", "ipv6"])
+    def test_config_add_del_vlan_dhcp_relay_with_non_empty_entry(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
+
+        exp_output = {"vlanid": "1001"} if ip_version == "ipv4" else {}
+        assert db.cfgdb.get_entry(IP_VERSION_PARAMS_MAP[ip_version]["table"], "Vlan1001") == exp_output
+        db.cfgdb.set_entry("DHCP_RELAY", "Vlan1001", {"dhcpv6_servers": ["fc02:2000::5"]})
+
+        # del vlan 1001
+        with mock.patch("utilities_common.dhcp_relay_util.handle_restart_dhcp_relay_service") as mock_handle_restart:
+            result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001"], obj=db)
+            print(result.exit_code)
+            print(result.output)
+
+            assert result.exit_code == 0
+            assert "Vlan1001" not in db.cfgdb.get_keys(IP_VERSION_PARAMS_MAP[ip_version]["table"])
+            mock_handle_restart.assert_called_once()
+            assert "Restart service dhcp_relay failed with error" not in result.output
+
+    @pytest.mark.parametrize("ip_version", ["ipv4", "ipv6"])
+    def test_config_add_del_vlan_with_dhcp_relay_not_running(self, ip_version):
+        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
+
+        exp_output = {"vlanid": "1001"} if ip_version == "ipv4" else {}
+        assert db.cfgdb.get_entry(IP_VERSION_PARAMS_MAP[ip_version]["table"], "Vlan1001") == exp_output
+
+        # del vlan 1001
+        with mock.patch("utilities_common.dhcp_relay_util.handle_restart_dhcp_relay_service") \
+             as mock_restart_dhcp_relay_service:
+            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"])
+            assert result.exit_code == 0
+            assert "Vlan1001" not in db.cfgdb.get_keys(IP_VERSION_PARAMS_MAP[ip_version]["table"])
+            assert mock_restart_dhcp_relay_service.call_count == 0
+            assert "Restarting DHCP relay service..." not in result.output
+            assert "Restart service dhcp_relay failed with error" not in result.output
+
+    def test_config_add_del_vlan_with_not_restart_dhcp_relay_ipv6(self):
+        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
+
+        db.cfgdb.set_entry("DHCP_RELAY", "Vlan1001", {"dhcpv6_servers": ["fc02:2000::5"]})
+
+        # del vlan 1001
+        with mock.patch("utilities_common.dhcp_relay_util.handle_restart_dhcp_relay_service") \
+             as mock_restart_dhcp_relay_service:
+            result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001", "--no_restart_dhcp_relay"],
+                                   obj=db)
+            print(result.exit_code)
+            print(result.output)
+
+            assert result.exit_code != 0
+            assert mock_restart_dhcp_relay_service.call_count == 0
+            assert "Can't delete Vlan1001 because related DHCPv6 Relay config is exist" in result.output
+
+        db.cfgdb.set_entry("DHCP_RELAY", "Vlan1001", None)
+        # del vlan 1001
+        with mock.patch("utilities_common.dhcp_relay_util.handle_restart_dhcp_relay_service") \
+             as mock_restart_dhcp_relay_service:
+            result = runner.invoke(config.config.commands["vlan"].commands["del"], ["1001", "--no_restart_dhcp_relay"],
+                                   obj=db)
+            print(result.exit_code)
+            print(result.output)
+
+            assert result.exit_code == 0
+            assert mock_restart_dhcp_relay_service.call_count == 0
 
     @pytest.mark.parametrize("ip_version", ["ipv6"])
     def test_config_add_exist_vlan_dhcp_relay(self, ip_version):