From d19d904f6a47ea244e93c57f644e95061c3ab308 Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Sat, 26 Aug 2023 08:01:37 +0800 Subject: [PATCH 1/5] [Mellanox] Fix issue: watchdogutil command does not work (#16091) (#16260) - Why I did it watchdogutil uses platform API watchdog instance to control/query watchdog status. In Nvidia watchdog status, it caches "armed" status in a object member "WatchdogImplBase.armed". This is not working for CLI infrastructure because each CLI will create a new watchdog instance, the status cached in previous instance will totally lose. Consider following commands: admin@sonic:~$ sudo watchdogutil arm -s 100 =====> watchdog instance1, armed=True Watchdog armed for 100 seconds admin@sonic:~$ sudo watchdogutil status ======> watchdog instance2, armed=False Status: Unarmed admin@sonic:~$ sudo watchdogutil disarm =======> watchdog instance3, armed=False Failed to disarm Watchdog - How I did it Use sysfs to query watchdog status - How to verify it Manual test Unit test Conflicts: platform/mellanox/mlnx-platform-api/sonic_platform/watchdog.py platform/mellanox/mlnx-platform-api/tests/test_watchdog.py --- .../sonic_platform/watchdog.py | 49 +++--- .../mlnx-platform-api/tests/test_watchdog.py | 141 ++++++++++++++++++ 2 files changed, 165 insertions(+), 25 deletions(-) create mode 100644 platform/mellanox/mlnx-platform-api/tests/test_watchdog.py diff --git a/platform/mellanox/mlnx-platform-api/sonic_platform/watchdog.py b/platform/mellanox/mlnx-platform-api/sonic_platform/watchdog.py index 879aabfd3530..5635b1869224 100644 --- a/platform/mellanox/mlnx-platform-api/sonic_platform/watchdog.py +++ b/platform/mellanox/mlnx-platform-api/sonic_platform/watchdog.py @@ -27,6 +27,7 @@ import time from sonic_platform_base.watchdog_base import WatchdogBase +from . import utils """ ioctl constants """ IO_WRITE = 0x40000000 @@ -80,15 +81,17 @@ def __init__(self, wd_device_path): super(WatchdogImplBase, self).__init__() self.watchdog_path = wd_device_path - self.watchdog = os.open(self.watchdog_path, os.O_WRONLY) + self._watchdog = None + self.timeout = self._gettimeout() - # Opening a watchdog descriptor starts - # watchdog timer; - # by default it should be stopped - self._disablecard() - self.armed = False + @property + def watchdog(self): + if self._watchdog is None: + self._watchdog = self.open_handle() + return self._watchdog - self.timeout = self._gettimeout() + def open_handle(self): + return os.open(self.watchdog_path, os.O_WRONLY) def _enablecard(self): """ @@ -131,10 +134,7 @@ def _gettimeout(self): @return watchdog timeout """ - req = array.array('I', [0]) - fcntl.ioctl(self.watchdog, WDIOC_GETTIMEOUT, req, True) - - return int(req[0]) + return utils.read_int_from_file('/run/hw-management/watchdog/main/timeout') def _gettimeleft(self): """ @@ -142,10 +142,7 @@ def _gettimeleft(self): @return time left in seconds """ - req = array.array('I', [0]) - fcntl.ioctl(self.watchdog, WDIOC_GETTIMELEFT, req, True) - - return int(req[0]) + return utils.read_int_from_file('/run/hw-management/watchdog/main/timeleft') def arm(self, seconds): """ @@ -159,11 +156,10 @@ def arm(self, seconds): try: if self.timeout != seconds: self.timeout = self._settimeout(seconds) - if self.armed: + if self.is_armed(): self._keepalive() else: self._enablecard() - self.armed = True ret = self.timeout except IOError: pass @@ -176,10 +172,9 @@ def disarm(self): """ disarmed = False - if self.armed: + if self.is_armed(): try: self._disablecard() - self.armed = False disarmed = True except IOError: pass @@ -191,7 +186,7 @@ def is_armed(self): Implements is_armed WatchdogBase API """ - return self.armed + return utils.read_str_from_file('/run/hw-management/watchdog/main/state') == 'active' def get_remaining_time(self): """ @@ -200,7 +195,7 @@ def get_remaining_time(self): timeleft = WD_COMMON_ERROR - if self.armed: + if self.is_armed(): try: timeleft = self._gettimeleft() except IOError: @@ -213,13 +208,15 @@ def __del__(self): Close watchdog """ - os.close(self.watchdog) + if self._watchdog is not None: + os.close(self._watchdog) class WatchdogType1(WatchdogImplBase): """ Watchdog type 1 """ + TIMESTAMP_FILE = '/tmp/nvidia/watchdog_timestamp' def arm(self, seconds): """ @@ -230,7 +227,8 @@ def arm(self, seconds): ret = WatchdogImplBase.arm(self, seconds) # Save the watchdog arm timestamp # requiered for get_remaining_time() - self.arm_timestamp = time.time() + os.makedirs('/tmp/nvidia', exist_ok=True) + utils.write_file(self.TIMESTAMP_FILE, str(time.time())) return ret @@ -243,8 +241,9 @@ def get_remaining_time(self): timeleft = WD_COMMON_ERROR - if self.armed: - timeleft = int(self.timeout - (time.time() - self.arm_timestamp)) + if self.is_armed(): + arm_timestamp = utils.read_float_from_file(self.TIMESTAMP_FILE) + timeleft = int(self.timeout - (time.time() - arm_timestamp)) return timeleft diff --git a/platform/mellanox/mlnx-platform-api/tests/test_watchdog.py b/platform/mellanox/mlnx-platform-api/tests/test_watchdog.py new file mode 100644 index 000000000000..68d29d38a654 --- /dev/null +++ b/platform/mellanox/mlnx-platform-api/tests/test_watchdog.py @@ -0,0 +1,141 @@ +# +# Copyright (c) 2023 NVIDIA CORPORATION & AFFILIATES. +# Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +import os +import pytest +import sys +if sys.version_info.major == 3: + from unittest import mock +else: + import mock + +test_path = os.path.dirname(os.path.abspath(__file__)) +modules_path = os.path.dirname(test_path) +sys.path.insert(0, modules_path) + +from sonic_platform.chassis import Chassis +from sonic_platform.watchdog import get_watchdog, \ + WatchdogType2, \ + WatchdogType1, \ + is_mlnx_wd_main, \ + is_wd_type2 + + +class TestWatchdog: + @mock.patch('sonic_platform.watchdog.is_mlnx_wd_main') + @mock.patch('sonic_platform.watchdog.os.listdir') + def test_get_watchdog_no_device(self, mock_listdir, mock_is_main): + mock_listdir.return_value = [] + assert get_watchdog() is None + + mock_listdir.return_value = ['invalid'] + mock_is_main.return_value = True + assert get_watchdog() is None + + mock_listdir.return_value = ['watchdog1'] + mock_is_main.return_value = False + assert get_watchdog() is None + + @mock.patch('sonic_platform.watchdog.is_mlnx_wd_main') + @mock.patch('sonic_platform.watchdog.is_wd_type2') + @mock.patch('sonic_platform.watchdog.os.listdir', mock.MagicMock(return_value=['watchdog1', 'watchdog2'])) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.open_handle', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.fcntl.ioctl', mock.MagicMock()) + @pytest.mark.parametrize('test_para', + [(True, WatchdogType2), (False, WatchdogType1)]) + def test_get_watchdog(self, mock_is_type2, mock_is_main, test_para): + mock_is_main.side_effect = lambda dev: dev == 'watchdog2' + mock_is_type2.return_value = test_para[0] + chassis = Chassis() + watchdog = chassis.get_watchdog() + assert isinstance(watchdog, test_para[1]) + assert watchdog.watchdog_path == '/dev/watchdog2' + + def test_is_mlnx_wd_main(self): + mock_os_open = mock.mock_open(read_data='mlx-wdt-main') + with mock.patch('sonic_platform.watchdog.open', mock_os_open): + assert is_mlnx_wd_main('') + + mock_os_open = mock.mock_open(read_data='invalid') + with mock.patch('sonic_platform.watchdog.open', mock_os_open): + assert not is_mlnx_wd_main('') + mock_os_open.side_effect = IOError + with mock.patch('sonic_platform.watchdog.open', mock_os_open): + assert not is_mlnx_wd_main('') + + @mock.patch('sonic_platform.watchdog.os.path.exists') + @pytest.mark.parametrize('test_para', + [True, False]) + def test_is_wd_type2(self, mock_exists, test_para): + mock_exists.return_value = test_para + assert is_wd_type2('') is test_para + + @mock.patch('sonic_platform.utils.read_str_from_file') + def test_is_armed(self, mock_read): + watchdog = WatchdogType2('watchdog2') + mock_read.return_value = 'inactive' + assert not watchdog.is_armed() + mock_read.return_value = 'active' + assert watchdog.is_armed() + + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.open_handle', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.fcntl.ioctl', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.is_armed') + def test_arm_disarm_watchdog2(self, mock_is_armed): + watchdog = WatchdogType2('watchdog2') + assert watchdog.arm(-1) == -1 + mock_is_armed.return_value = False + watchdog.arm(10) + mock_is_armed.return_value = True + watchdog.arm(5) + watchdog.disarm() + + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.open_handle', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.fcntl.ioctl', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.is_armed') + def test_arm_disarm_watchdog1(self, mock_is_armed): + watchdog = WatchdogType1('watchdog1') + assert watchdog.arm(-1) == -1 + mock_is_armed.return_value = False + watchdog.arm(10) + mock_is_armed.return_value = True + watchdog.arm(5) + watchdog.disarm() + + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.open_handle', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.fcntl.ioctl', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase._gettimeleft', mock.MagicMock(return_value=10)) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.is_armed') + def test_get_remaining_time_watchdog2(self, mock_is_armed): + watchdog = WatchdogType2('watchdog2') + mock_is_armed.return_value = False + assert watchdog.get_remaining_time() == -1 + watchdog.arm(10) + mock_is_armed.return_value = True + assert watchdog.get_remaining_time() == 10 + + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.open_handle', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.fcntl.ioctl', mock.MagicMock()) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase._gettimeleft', mock.MagicMock(return_value=10)) + @mock.patch('sonic_platform.watchdog.WatchdogImplBase.is_armed') + def test_get_remaining_time_watchdog1(self, mock_is_armed): + watchdog = WatchdogType1('watchdog2') + mock_is_armed.return_value = False + assert watchdog.get_remaining_time() == -1 + watchdog.arm(10) + mock_is_armed.return_value = True + assert watchdog.get_remaining_time() > 0 From d91565ba5ee844fcedb47cd1f689dbfdd58890ba Mon Sep 17 00:00:00 2001 From: judyjoseph <53951155+judyjoseph@users.noreply.github.com> Date: Fri, 25 Aug 2023 17:02:26 -0700 Subject: [PATCH 2/5] sudo not required explicitly as /bin/ip netns identify is part of READ_ONLY_CMDS in sudoers file (#16258) Cherry-pick PR :#16115 --- src/sonic-py-common/sonic_py_common/multi_asic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-py-common/sonic_py_common/multi_asic.py b/src/sonic-py-common/sonic_py_common/multi_asic.py index 0dc2ef97682b..a0523d1ce61e 100644 --- a/src/sonic-py-common/sonic_py_common/multi_asic.py +++ b/src/sonic-py-common/sonic_py_common/multi_asic.py @@ -157,7 +157,7 @@ def get_current_namespace(pid=None): """ net_namespace = None - command = ["sudo /bin/ip netns identify {}".format(os.getpid() if not pid else pid)] + command = ["/bin/ip netns identify {}".format(os.getpid() if not pid else pid)] proc = subprocess.Popen(command, stdout=subprocess.PIPE, shell=True, From 8757e6b8d9af18b6c5e7af38a64edf1bd58501ea Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Sat, 26 Aug 2023 08:03:46 +0800 Subject: [PATCH 3/5] [YANG SONIC-ACL] Fix Yang definition of IN_PORTS and OUT_PORTS (#16220) (#16235) How I did it Update Yang definition of IN_PORTS and OUT_PORTS to string. Since we cannot split the string with comma (,) and validate each substring is a valid SONiC port name. The only restriction for them is must be a string. How to verify it Verified by building sonic_yang_models-1.0-py3-none-any.whl. While building the target package, unit tests were run and passed. Build a SONiC image based on 202205 branch and installed on physical DUT. Re try the steps in [Yang] Incorrect definition of IN_PORTS and OUT_PORTS in sonic-acl.yang #16190 and can see below success response: Co-authored-by: Zhijian Li --- .../tests/yang_model_tests/tests/acl.json | 6 + .../yang_model_tests/tests_config/acl.json | 108 ++++++++++++++++++ .../yang-templates/sonic-acl.yang.j2 | 12 +- 3 files changed, 120 insertions(+), 6 deletions(-) diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests/acl.json b/src/sonic-yang-models/tests/yang_model_tests/tests/acl.json index a3d21104794e..0f0d11a529ac 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests/acl.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests/acl.json @@ -14,6 +14,12 @@ "eStrKey" : "Mandatory", "eStr": ["ACL_RULE", "PRIORITY"] }, + "ACL_RULE_WITH_VALID_IN_PORTS": { + "desc": "Configure ACL_RULE with valid IN_PORTS." + }, + "ACL_RULE_WITH_VALID_OUT_PORTS": { + "desc": "Configure ACL_RULE with valid OUT_PORTS." + }, "ACL_TABLE_EMPTY_PORTS": { "desc": "Configure ACL_TABLE with empty ports." }, diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/acl.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/acl.json index ce5a9839d0d9..11633164a166 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/acl.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/acl.json @@ -482,6 +482,114 @@ } } }, + "ACL_RULE_WITH_VALID_IN_PORTS": { + "sonic-acl:sonic-acl": { + "sonic-acl:ACL_RULE": { + "ACL_RULE_LIST": [ + { + "ACL_TABLE_NAME": "NO-NSW-PACL-V4", + "IN_PORTS": "Ethernet0,Ethernet1", + "PACKET_ACTION": "FORWARD", + "PRIORITY": 9999, + "RULE_NAME": "Rule_20", + "SRC_IPV6": "2001::1/64" + } + ] + }, + "sonic-acl:ACL_TABLE": { + "ACL_TABLE_LIST": [ + { + "ACL_TABLE_NAME": "NO-NSW-PACL-V4", + "policy_desc": "Filter IPv4", + "ports": [ + "Ethernet0", + "Ethernet1" + ], + "stage": "INGRESS", + "type": "L3" + } + ] + } + }, + "sonic-port:sonic-port": { + "sonic-port:PORT": { + "PORT_LIST": [ + { + "admin_status": "up", + "alias": "eth0", + "description": "Ethernet0", + "lanes": "0,1,2,3", + "mtu": 9000, + "name": "Ethernet0", + "speed": 25000 + }, + { + "admin_status": "up", + "alias": "eth1", + "description": "Ethernet1", + "lanes": "4,5,6,7", + "mtu": 9000, + "name": "Ethernet1", + "speed": 25000 + } + ] + } + } + }, + "ACL_RULE_WITH_VALID_OUT_PORTS": { + "sonic-acl:sonic-acl": { + "sonic-acl:ACL_RULE": { + "ACL_RULE_LIST": [ + { + "ACL_TABLE_NAME": "NO-NSW-PACL-V4", + "OUT_PORTS": "Ethernet0,Ethernet1", + "PACKET_ACTION": "FORWARD", + "PRIORITY": 9999, + "RULE_NAME": "Rule_20", + "SRC_IPV6": "2001::1/64" + } + ] + }, + "sonic-acl:ACL_TABLE": { + "ACL_TABLE_LIST": [ + { + "ACL_TABLE_NAME": "NO-NSW-PACL-V4", + "policy_desc": "Filter IPv4", + "ports": [ + "Ethernet0", + "Ethernet1" + ], + "stage": "EGRESS", + "type": "L3" + } + ] + } + }, + "sonic-port:sonic-port": { + "sonic-port:PORT": { + "PORT_LIST": [ + { + "admin_status": "up", + "alias": "eth0", + "description": "Ethernet0", + "lanes": "0,1,2,3", + "mtu": 9000, + "name": "Ethernet0", + "speed": 25000 + }, + { + "admin_status": "up", + "alias": "eth1", + "description": "Ethernet1", + "lanes": "4,5,6,7", + "mtu": 9000, + "name": "Ethernet1", + "speed": 25000 + } + ] + } + } + }, "ACL_TABLE_DEFAULT_VALUE_STAGE": { "sonic-acl:sonic-acl": { "sonic-acl:ACL_TABLE": { diff --git a/src/sonic-yang-models/yang-templates/sonic-acl.yang.j2 b/src/sonic-yang-models/yang-templates/sonic-acl.yang.j2 index ccbb1639303e..f48d551e1bb8 100644 --- a/src/sonic-yang-models/yang-templates/sonic-acl.yang.j2 +++ b/src/sonic-yang-models/yang-templates/sonic-acl.yang.j2 @@ -128,14 +128,14 @@ module sonic-acl { } } - leaf-list IN_PORTS { - /* Values in leaf list are UNIQUE */ - type uint16; + leaf IN_PORTS { + /* Values is a list of SONiC port name (/port:sonic-port/port:PORT/port:PORT_LIST/port:name) joined by comma */ + type string; } - leaf-list OUT_PORTS { - /* Values in leaf list are UNIQUE */ - type uint16; + leaf OUT_PORTS { + /* Values is a list of SONiC port name (/port:sonic-port/port:PORT/port:PORT_LIST/port:name) joined by comma */ + type string; } choice src_port { From f04206922a06932ddf80c6f81c0e432110696a18 Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Sat, 26 Aug 2023 08:04:44 +0800 Subject: [PATCH 4/5] [submodule] Update submodule sonic-platform-common to the latest HEAD automatically (#16264) src/sonic-platform-common * b41db16 - (HEAD -> 202205, origin/202205) Move tx_disable/tx_disabled_channel/rx_los/tx_fault to get_transceiver_status API (#359) (#395) (32 hours ago) [longhuan-cisco] --- src/sonic-platform-common | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sonic-platform-common b/src/sonic-platform-common index a6dd67e622c3..b41db16c3cca 160000 --- a/src/sonic-platform-common +++ b/src/sonic-platform-common @@ -1 +1 @@ -Subproject commit a6dd67e622c360c13dcccfdc77bbc3743dd5b783 +Subproject commit b41db16c3cca49adc92b0e6a9787983ae2f0dd88 From d264df398456fd3fd6143574f2e0bb78d31e7f64 Mon Sep 17 00:00:00 2001 From: mssonicbld <79238446+mssonicbld@users.noreply.github.com> Date: Sat, 26 Aug 2023 08:05:37 +0800 Subject: [PATCH 5/5] Dell S6100 Platform API 2.0 fixes (#16208) (#16252) Why I did it Dell S6100 Platform components needs to be updated. How I did it Modified platform.json to fix the issue. How to verify it Run sonic-mgmt component test and check whether it passes. Co-authored-by: Aravind Mani <53524901+aravindmani-1@users.noreply.github.com> --- device/dell/x86_64-dell_s6100_c2538-r0/platform.json | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/device/dell/x86_64-dell_s6100_c2538-r0/platform.json b/device/dell/x86_64-dell_s6100_c2538-r0/platform.json index 819f104aa53a..ba9fb90ad6ab 100644 --- a/device/dell/x86_64-dell_s6100_c2538-r0/platform.json +++ b/device/dell/x86_64-dell_s6100_c2538-r0/platform.json @@ -469,7 +469,13 @@ }, { "name": "QSFP+ or later" - } + }, + { + "name": "SFP/SFP+/SFP28" + }, + { + "name": "SFP/SFP+/SFP28" + } ] }, "interfaces": {}