From 99d251f2da4c0e78da33d67b13239e2c1e183b4f Mon Sep 17 00:00:00 2001 From: Andriy Yurkiv <70649192+ayurkiv-nvda@users.noreply.github.com> Date: Wed, 24 Mar 2021 18:53:10 +0200 Subject: [PATCH] Enable PFCWD only on ports where PFC is enabled (#1508) - What I did Prevent errors in the log when PFCWD starts on ports that PFC was not previously enabled. Example of such errors in the log: Nov 13 17:27:22.878468 arc-switch1038 ERR swss#orchagent: :- createEntry: Failed to start PFC Watchdog on port Ethernet100 Nov 13 17:27:23.878620 arc-switch1038 NOTICE swss#orchagent: :- registerInWdDb: No lossless TC found on port Ethernet100 Add unit test to cover the new flow. - How I did it Before enabling PFCWD on a port, verify if PFC was previously enabled. If not, reply with an error to the user and as a warning in the log. In the case where the command to enable PFCWD is done for all ports, the configuration will be applied for only those that has PCF enabled and for the rest the configuration will be skipped. In this case the output of the requested command will be only with a list of skipped ports. - How to verify it 'pfcwd start EthernetX 600' where EthernetX is a port that has not configured PFC (usually admin state 'down' by default) --- pfcwd/main.py | 39 ++++++++----- tests/mock_tables/asic0/config_db.json | 21 +++++++ tests/mock_tables/asic1/config_db.json | 20 +++++++ tests/mock_tables/config_db.json | 9 +++ tests/pfcwd_input/pfcwd_test_vectors.py | 24 ++++++-- tests/pfcwd_test.py | 78 +++++++++++++++++++++++++ 6 files changed, 169 insertions(+), 22 deletions(-) diff --git a/pfcwd/main.py b/pfcwd/main.py index bc5211191b9a..9b038316ca78 100644 --- a/pfcwd/main.py +++ b/pfcwd/main.py @@ -11,6 +11,11 @@ from tabulate import tabulate from utilities_common import multi_asic as multi_asic_util from utilities_common import constants +from sonic_py_common import logger + +SYSLOG_IDENTIFIER = "config" + +log = logger.Logger(SYSLOG_IDENTIFIER) # mock the redis for unit test purposes # try: @@ -53,7 +58,7 @@ CONFIG_HEADER = ('PORT',) + list(zip(*CONFIG_DESCRIPTION))[0] CONFIG_DB_PFC_WD_TABLE_NAME = 'PFC_WD' - +PORT_QOS_MAP = "PORT_QOS_MAP" # Main entrypoint @click.group() @@ -242,6 +247,20 @@ def start(self, action, restoration_time, ports, detection_time): exit() self.start_cmd(action, restoration_time, ports, detection_time) + + def verify_pfc_enable_status_per_port(self, port, pfcwd_info): + pfc_status = self.config_db.get_entry(PORT_QOS_MAP, port).get('pfc_enable') + if pfc_status is None: + log.log_warning("SKIPPED: PFC is not enabled on port: {}".format(port), also_print_to_console=True) + return + + self.config_db.mod_entry( + CONFIG_DB_PFC_WD_TABLE_NAME, port, None + ) + self.config_db.mod_entry( + CONFIG_DB_PFC_WD_TABLE_NAME, port, pfcwd_info + ) + @multi_asic_util.run_on_multi_asic def start_cmd(self, action, restoration_time, ports, detection_time): if os.geteuid() != 0: @@ -272,21 +291,11 @@ def start_cmd(self, action, restoration_time, ports, detection_time): for port in ports: if port == "all": for p in all_ports: - self.config_db.mod_entry( - CONFIG_DB_PFC_WD_TABLE_NAME, p, None - ) - self.config_db.mod_entry( - CONFIG_DB_PFC_WD_TABLE_NAME, p, pfcwd_info - ) + self.verify_pfc_enable_status_per_port(p, pfcwd_info) else: if port not in all_ports: continue - self.config_db.mod_entry( - CONFIG_DB_PFC_WD_TABLE_NAME, port, None - ) - self.config_db.mod_entry( - CONFIG_DB_PFC_WD_TABLE_NAME, port, pfcwd_info - ) + self.verify_pfc_enable_status_per_port(port, pfcwd_info) @multi_asic_util.run_on_multi_asic def interval(self, poll_interval): @@ -375,9 +384,7 @@ def start_default(self): } for port in active_ports: - self.config_db.set_entry( - CONFIG_DB_PFC_WD_TABLE_NAME, port, pfcwd_info - ) + self.verify_pfc_enable_status_per_port(port, pfcwd_info) pfcwd_info = {} pfcwd_info['POLL_INTERVAL'] = DEFAULT_POLL_INTERVAL * multiply diff --git a/tests/mock_tables/asic0/config_db.json b/tests/mock_tables/asic0/config_db.json index 14ee76aaa33d..0643baf57f9d 100644 --- a/tests/mock_tables/asic0/config_db.json +++ b/tests/mock_tables/asic0/config_db.json @@ -108,6 +108,27 @@ "BIG_RED_SWITCH": "enable", "POLL_INTERVAL": "199" }, + "PORT_QOS_MAP|Ethernet0": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet4": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet8": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP0": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP4": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP256": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP260": { + "pfc_enable": "3,4" + }, "CRM|Config": { "acl_table_threshold_type": "percentage", "nexthop_group_threshold_type": "percentage", diff --git a/tests/mock_tables/asic1/config_db.json b/tests/mock_tables/asic1/config_db.json index 4d515ba2d3f2..5aab92c45a67 100644 --- a/tests/mock_tables/asic1/config_db.json +++ b/tests/mock_tables/asic1/config_db.json @@ -77,6 +77,26 @@ "BIG_RED_SWITCH": "enable", "POLL_INTERVAL": "199" }, + "PORT_QOS_MAP|Ethernet0": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet4": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet8": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP0": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP4": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP256": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet-BP260": { + }, "CRM|Config": { "acl_table_threshold_type": "percentage", "nexthop_group_threshold_type": "percentage", diff --git a/tests/mock_tables/config_db.json b/tests/mock_tables/config_db.json index 14dec0192b3c..051c49004d6e 100644 --- a/tests/mock_tables/config_db.json +++ b/tests/mock_tables/config_db.json @@ -683,6 +683,10 @@ "peer_switch": "sonic-switch", "type": "ToRRouter" }, + "DEVICE_NEIGHBOR|Ethernet0": { + "name": "Servers", + "port": "eth0" + }, "DEVICE_NEIGHBOR|Ethernet4": { "name": "Servers0", "port": "eth0" @@ -1450,6 +1454,11 @@ "PORT_QOS_MAP|Ethernet0": { "pfc_enable": "3,4" }, + "PORT_QOS_MAP|Ethernet4": { + "pfc_enable": "3,4" + }, + "PORT_QOS_MAP|Ethernet8": { + }, "DEFAULT_LOSSLESS_BUFFER_PARAMETER|AZURE": { "default_dynamic_th": "0", "over_subscribe_ratio": "2" diff --git a/tests/pfcwd_input/pfcwd_test_vectors.py b/tests/pfcwd_input/pfcwd_test_vectors.py index 9b6b7488a214..127bfaac2b64 100644 --- a/tests/pfcwd_input/pfcwd_test_vectors.py +++ b/tests/pfcwd_input/pfcwd_test_vectors.py @@ -22,7 +22,7 @@ --------- -------- ---------------- ------------------ Ethernet0 forward 302 301 Ethernet4 forward 302 301 -Ethernet8 forward 302 301 +Ethernet8 drop 600 600 """ pfcwd_show_start_action_alert_output = """\ @@ -31,7 +31,7 @@ --------- -------- ---------------- ------------------ Ethernet0 alert 502 501 Ethernet4 alert 502 501 -Ethernet8 alert 502 501 +Ethernet8 drop 600 600 """ pfcwd_show_start_action_drop_output = """\ @@ -40,7 +40,16 @@ --------- -------- ---------------- ------------------ Ethernet0 drop 602 601 Ethernet4 drop 602 601 -Ethernet8 drop 602 601 +Ethernet8 drop 600 600 +""" + +pfcwd_show_start_default = """\ +Changed polling interval to 200ms + PORT ACTION DETECTION TIME RESTORATION TIME +--------- -------- ---------------- ------------------ +Ethernet0 drop 200 200 +Ethernet4 drop 200 200 +Ethernet8 drop 600 600 """ pfcwd_show_start_config_output_fail = """\ @@ -94,6 +103,9 @@ ------- -------- ------------------------- ------------ ------------ ----------------- ----------------- """ +pfc_is_not_enabled = "SKIPPED: PFC is not enabled on port: Ethernet8\n" +pfc_is_not_enabled_masic = "SKIPPED: PFC is not enabled on port: Ethernet-BP260\n" + testData = { 'pfcwd_show_config' : [ {'cmd' : ['show', 'config'], 'args': [], @@ -290,7 +302,7 @@ Ethernet-BP0 drop 302 301 Ethernet-BP4 drop 302 301 Ethernet-BP256 drop 302 301 -Ethernet-BP260 drop 302 301 +Ethernet-BP260 drop 200 200 """ show_pfc_config_start_action_alert_masic = """\ @@ -305,7 +317,7 @@ Ethernet-BP0 alert 402 401 Ethernet-BP4 alert 402 401 Ethernet-BP256 alert 402 401 -Ethernet-BP260 alert 402 401 +Ethernet-BP260 drop 200 200 """ show_pfc_config_start_action_forward_masic = """\ @@ -320,7 +332,7 @@ Ethernet-BP0 forward 702 701 Ethernet-BP4 forward 702 701 Ethernet-BP256 forward 702 701 -Ethernet-BP260 forward 702 701 +Ethernet-BP260 drop 200 200 """ show_pfc_config_start_fail = """\ diff --git a/tests/pfcwd_test.py b/tests/pfcwd_test.py index b8d19edd5728..9a97ff7b3362 100644 --- a/tests/pfcwd_test.py +++ b/tests/pfcwd_test.py @@ -131,6 +131,7 @@ def test_pfcwd_start_actions(self, mock_os): print(result.output) assert result.output == pfcwd_show_config_output + # always skip Ethernet8 because 'pfc_enable' not configured for this port mock_os.geteuid.return_value = 0 result = runner.invoke( pfcwd.cli.commands["start"], @@ -192,6 +193,53 @@ def test_pfcwd_start_actions(self, mock_os): assert result.exit_code == 0 assert result.output == pfcwd_show_start_action_drop_output + result = runner.invoke( + pfcwd.cli.commands["start_default"], + [], + obj=db + ) + + assert result.exit_code == 0 + + result = runner.invoke( + pfcwd.cli.commands["show"].commands["config"], + obj=db + ) + + print(result.output) + assert result.exit_code == 0 + assert result.output == pfcwd_show_start_default + + + @patch('pfcwd.main.os') + def test_pfcwd_pfc_not_enabled(self, mock_os): + import pfcwd.main as pfcwd + runner = CliRunner() + db = Db() + + # get initial config + result = runner.invoke( + pfcwd.cli.commands["show"].commands["config"], + obj=db + ) + print(result.output) + assert result.output == pfcwd_show_config_output + + mock_os.geteuid.return_value = 0 + + result = runner.invoke( + pfcwd.cli.commands["start"], + [ + "--action", "drop", "--restoration-time", "601", + "Ethernet8", "602" + ], + obj=db + ) + print(result.output) + assert result.exit_code == 0 + assert pfc_is_not_enabled == result.output + + def test_pfcwd_start_ports_invalid(self): # pfcwd start --action drop --restoration-time 200 Ethernet0 200 import pfcwd.main as pfcwd @@ -322,6 +370,7 @@ def test_pfcwd_start_actions_masic(self, mock_os): print(result.output) assert result.output == show_pfc_config_all + # always skip Ethernet-BP260 because 'pfc_enable' not configured for this port mock_os.geteuid.return_value = 0 result = runner.invoke( pfcwd.cli.commands["start"], @@ -411,6 +460,35 @@ def test_pfcwd_start_ports_masic_invalid(self): # same as original config assert result.output == show_pfc_config_all + @patch('pfcwd.main.os') + def test_pfcwd_pfc_not_enabled_masic(self, mock_os): + import pfcwd.main as pfcwd + runner = CliRunner() + db = Db() + + mock_os.geteuid.return_value = 0 + result = runner.invoke( + pfcwd.cli.commands["start"], + [ + "--action", "drop", "--restoration-time", "601", + "Ethernet-BP260", "602" + ], + obj=db + ) + + assert result.exit_code == 0 + assert pfc_is_not_enabled_masic == result.output + + result = runner.invoke( + pfcwd.cli.commands["show"].commands["config"], + obj=db + ) + + print(result.output) + assert result.exit_code == 0 + # same as original config + assert result.output == show_pfc_config_all + @classmethod def teardown_class(cls): print("TEARDOWN")