From 8aa5aba2c9cbfb1df1f9f524385734b96f1d5e36 Mon Sep 17 00:00:00 2001 From: mazora Date: Mon, 19 Feb 2024 13:20:22 +0200 Subject: [PATCH 1/4] [buffermgr] Moved switch statement outside of if-statmement in Buffermgr::doTask The switch statement which would normally erase buffer events was moved to be inside the if-statement which would only enter if the event is a SET event. This was introduced in commit e5329c39. This would cause an infinite loop, since non-set events would never be erased. The switch statement has now been moved to occur outside the if, allowing for non-set commands to be processed. Signed-off-by: mazora --- cfgmgr/buffermgr.cpp | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/cfgmgr/buffermgr.cpp b/cfgmgr/buffermgr.cpp index ba247197c1..32f71d8280 100644 --- a/cfgmgr/buffermgr.cpp +++ b/cfgmgr/buffermgr.cpp @@ -549,24 +549,23 @@ void BufferMgr::doTask(Consumer &consumer) task_status = doSpeedUpdateTask(port); } } - - switch (task_status) - { - case task_process_status::task_failed: - SWSS_LOG_ERROR("Failed to process table update"); - return; - case task_process_status::task_need_retry: - SWSS_LOG_INFO("Unable to process table update. Will retry..."); - ++it; - break; - case task_process_status::task_invalid_entry: - SWSS_LOG_ERROR("Failed to process invalid entry, drop it"); - it = consumer.m_toSync.erase(it); - break; - default: - it = consumer.m_toSync.erase(it); - break; - } + } + switch (task_status) + { + case task_process_status::task_failed: + SWSS_LOG_ERROR("Failed to process table update"); + return; + case task_process_status::task_need_retry: + SWSS_LOG_INFO("Unable to process table update. Will retry..."); + ++it; + break; + case task_process_status::task_invalid_entry: + SWSS_LOG_ERROR("Failed to process invalid entry, drop it"); + it = consumer.m_toSync.erase(it); + break; + default: + it = consumer.m_toSync.erase(it); + break; } } } From 47573f8876ed174ccb0b3e902a54b3919c348d43 Mon Sep 17 00:00:00 2001 From: mazora Date: Sun, 10 Mar 2024 15:39:24 +0200 Subject: [PATCH 2/4] Add Test for Code Coverage Adding test to increase code coverage. Since this test is a "negative" test, meaning that it should not be a valid configurations, I only validate if the test succeeds based off of syslogs. Added imports for type hinting. --- tests/test_buffer_traditional.py | 112 +++++++++++++++++++++++++++++-- 1 file changed, 108 insertions(+), 4 deletions(-) diff --git a/tests/test_buffer_traditional.py b/tests/test_buffer_traditional.py index 21371cb05a..5ae823fd5b 100644 --- a/tests/test_buffer_traditional.py +++ b/tests/test_buffer_traditional.py @@ -3,14 +3,18 @@ class TestBuffer(object): + from conftest import DockerVirtualSwitch lossless_pgs = [] INTF = "Ethernet0" def setup_db(self, dvs): - self.app_db = dvs.get_app_db() - self.asic_db = dvs.get_asic_db() - self.config_db = dvs.get_config_db() - self.counter_db = dvs.get_counters_db() + from conftest import ApplDbValidator, AsicDbValidator + from dvslib.dvs_database import DVSDatabase + + self.app_db: ApplDbValidator = dvs.get_app_db() + self.asic_db: AsicDbValidator = dvs.get_asic_db() + self.config_db: DVSDatabase = dvs.get_config_db() + self.counter_db: DVSDatabase = dvs.get_counters_db() # enable PG watermark self.set_pg_wm_status('enable') @@ -74,6 +78,10 @@ def get_pg_name_map(self): pg_name = "{}:{}".format(self.INTF, pg) pg_name_map[pg_name] = self.get_pg_oid(pg_name) return pg_name_map + + def check_syslog(dvs, marker, err_log, expected_cnt=1): + (exitcode, num) = dvs.runcmd(['sh', '-c', "awk \'/%s/,ENDFILE {print;}\' /var/log/syslog | grep \"%s\" | wc -l" % (marker, err_log)]) + assert num.strip() >= str(expected_cnt) @pytest.fixture def setup_teardown_test(self, dvs): @@ -246,3 +254,99 @@ def test_buffer_pg_update(self, dvs, setup_teardown_test): dvs.port_field_set(extra_port, "speed", orig_speed) dvs.port_admin_set(self.INTF, "down") dvs.port_admin_set(extra_port, "down") + + def test_no_pg_profile_for_speed_and_length(self, dvs: DockerVirtualSwitch, setup_teardown_test): + """ + Test to verify that buffermgrd correctly handles a scenario where no PG profile + is configured for a given speed (10000) and cable length (80m) for Ethernet0 (self.INTF). + """ + orig_cable_len = None + orig_port_speed = None + orig_port_status = None + orig_port_qos_map = None + + test_cable_len = "80m" # cable length must not exist for test_speed in + test_speed = "10000" + test_port_status ="down" # can be up or down, but it must exist in port configuration + test_port_pfc_enable = "3,4" # does not matter, but must exist + + try: + ################################## + ## Save original configurations ## + ################################## + + # Save original cable length + fvs_cable_len = self.config_db.get_entry("CABLE_LENGTH", "AZURE") + orig_cable_len = fvs_cable_len[self.INTF] if fvs_cable_len else None + + # Save original port speed and admin status + fvs_port = self.config_db.get_entry("PORT", self.INTF) + orig_port_speed, orig_port_status = (fvs_port["speed"], fvs_port["admin_status"]) if fvs_port else (None, None) + + # Save original port qos map + fvs_qos_map = self.config_db.get_entry("PORT_QOS_MAP", self.INTF) + orig_cable_len = fvs_qos_map["pfc_enable"] if fvs_qos_map else None + + ###################################### + ## Send configurations to CONFIG_DB ## + ###################################### + + # Configure cable length + self.change_cable_len(test_cable_len) + + # Configure port speed + dvs.port_field_set(self.INTF, "speed", test_speed) + + # Configure PFC enable + self.set_port_qos_table(self.INTF, test_port_pfc_enable) + + # Configure admin status + dvs.port_admin_set(self.INTF, test_port_status) + + # Wait for buffermgrd to process the changes + time.sleep(2) + + ################## + ## Verification ## + ################## + + # Add marker to log to make syslog verification easier + marker = dvs.add_log_marker() + + # Check syslog if this error is present. This is expected. + self.check_syslog(marker, "Failed to process invalid entry, drop it") + + finally: + ############################### + ## Revert to original values ## + ############################### + + # Revert values to original values + # If there are none, then assume entry/field never existed and should be deleted + + # Revert cable length + if orig_cable_len: + self.change_cable_len(orig_cable_len) + else: + self.config_db.delete_entry("CABLE_LENGTH", "AZURE") + + # Revert port speed + if orig_port_speed: + dvs.port_field_set(self.INTF, "speed", orig_port_speed) + else: + self.config_db.delete_field("PORT", self.INTF, "speed") + + # Revert admin status + if orig_port_status: + dvs.port_admin_set(self.INTF, orig_port_status) + else: + self.config_db.delete_field("PORT", self.INTF, "admin_status") + + # Revert port qos map + if orig_port_qos_map: + self.config_db.update_entry("PORT_QOS_MAP", self.INTF, orig_port_qos_map) + else: + self.config_db.delete_entry("PORT_QOS_MAP", self.INTF) + + + From 1306f0eb39bbfb61030183936c28ffe26a2dafcb Mon Sep 17 00:00:00 2001 From: mazora Date: Sun, 10 Mar 2024 18:27:04 +0200 Subject: [PATCH 3/4] Changed get value to use get() function in test Getting value by indexing could cause KeyError if value does not exist, instead call get() function which will return None if value does not exist. Also moved add_log_marker() function earlier to not accidently miss the syslog. --- tests/test_buffer_traditional.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/test_buffer_traditional.py b/tests/test_buffer_traditional.py index 5ae823fd5b..b5e4bdca09 100644 --- a/tests/test_buffer_traditional.py +++ b/tests/test_buffer_traditional.py @@ -277,15 +277,16 @@ def test_no_pg_profile_for_speed_and_length(self, dvs: DockerVirtualSwitch, setu # Save original cable length fvs_cable_len = self.config_db.get_entry("CABLE_LENGTH", "AZURE") - orig_cable_len = fvs_cable_len[self.INTF] if fvs_cable_len else None + orig_cable_len = fvs_cable_len.get(self.INTF) if fvs_cable_len else None # Save original port speed and admin status fvs_port = self.config_db.get_entry("PORT", self.INTF) - orig_port_speed, orig_port_status = (fvs_port["speed"], fvs_port["admin_status"]) if fvs_port else (None, None) + orig_port_speed = fvs_port.get("speed") if fvs_port else None + orig_port_status = fvs_port.get("admin_status") if fvs_port else None # Save original port qos map fvs_qos_map = self.config_db.get_entry("PORT_QOS_MAP", self.INTF) - orig_cable_len = fvs_qos_map["pfc_enable"] if fvs_qos_map else None + orig_cable_len = fvs_qos_map.get("pfc_enable") if fvs_qos_map else None ###################################### ## Send configurations to CONFIG_DB ## @@ -300,6 +301,10 @@ def test_no_pg_profile_for_speed_and_length(self, dvs: DockerVirtualSwitch, setu # Configure PFC enable self.set_port_qos_table(self.INTF, test_port_pfc_enable) + # Add marker to log to make syslog verification easier + # Set before setting admin status to not miss syslog + marker = dvs.add_log_marker() + # Configure admin status dvs.port_admin_set(self.INTF, test_port_status) @@ -310,8 +315,6 @@ def test_no_pg_profile_for_speed_and_length(self, dvs: DockerVirtualSwitch, setu ## Verification ## ################## - # Add marker to log to make syslog verification easier - marker = dvs.add_log_marker() # Check syslog if this error is present. This is expected. self.check_syslog(marker, "Failed to process invalid entry, drop it") From 98b2304aaf1251072652435876785c1499a2a205 Mon Sep 17 00:00:00 2001 From: mazora Date: Sun, 10 Mar 2024 21:00:05 +0200 Subject: [PATCH 4/4] Fix call to check_syslog() check_syslog() did not have "self" param call to check_syslog did not send dvs arg --- tests/test_buffer_traditional.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_buffer_traditional.py b/tests/test_buffer_traditional.py index b5e4bdca09..5e1f26bd50 100644 --- a/tests/test_buffer_traditional.py +++ b/tests/test_buffer_traditional.py @@ -79,7 +79,7 @@ def get_pg_name_map(self): pg_name_map[pg_name] = self.get_pg_oid(pg_name) return pg_name_map - def check_syslog(dvs, marker, err_log, expected_cnt=1): + def check_syslog(self, dvs, marker, err_log, expected_cnt=1): (exitcode, num) = dvs.runcmd(['sh', '-c', "awk \'/%s/,ENDFILE {print;}\' /var/log/syslog | grep \"%s\" | wc -l" % (marker, err_log)]) assert num.strip() >= str(expected_cnt) @@ -317,7 +317,7 @@ def test_no_pg_profile_for_speed_and_length(self, dvs: DockerVirtualSwitch, setu # Check syslog if this error is present. This is expected. - self.check_syslog(marker, "Failed to process invalid entry, drop it") + self.check_syslog(dvs, marker, "Failed to process invalid entry, drop it") finally: ###############################