From 35573e1b6b73bea25c60c4a02f4718fdbb8b841c Mon Sep 17 00:00:00 2001 From: Stephan Pampel <7194553+SPFZ@users.noreply.github.com> Date: Thu, 13 Jan 2022 09:26:13 +0100 Subject: [PATCH 1/5] add check_sriov_numvfs.py Check to verify the number of virtual functions (VFs) active on network interfaces. Based on /sys/class/net//device/sriov_numvfs and /sys/class/net//device/sriov_totalvfs. --- charmhelpers/contrib/charmsupport/nrpe.py | 12 +++ .../contrib/network/files/__init__.py | 13 +++ .../network/files/check_sriov_numvfs.py | 101 ++++++++++++++++++ tests/contrib/charmsupport/test_nrpe.py | 9 ++ tests/contrib/network/files/__init__.py | 13 +++ .../network/files/test_check_sriov_numvfs.py | 82 ++++++++++++++ 6 files changed, 230 insertions(+) create mode 100644 charmhelpers/contrib/network/files/__init__.py create mode 100755 charmhelpers/contrib/network/files/check_sriov_numvfs.py create mode 100644 tests/contrib/network/files/__init__.py create mode 100644 tests/contrib/network/files/test_check_sriov_numvfs.py diff --git a/charmhelpers/contrib/charmsupport/nrpe.py b/charmhelpers/contrib/charmsupport/nrpe.py index 8d1753c37..e086957ba 100644 --- a/charmhelpers/contrib/charmsupport/nrpe.py +++ b/charmhelpers/contrib/charmsupport/nrpe.py @@ -520,3 +520,15 @@ def remove_deprecated_check(nrpe, deprecated_services): for dep_svc in deprecated_services: log('Deprecated service: {}'.format(dep_svc)) nrpe.remove_check(shortname=dep_svc) + + +def add_sriov_numvfs_checks(nrpe, sriov_numvfs): + """ + Add checks for openvswitch + :param NRPE nrpe: NRPE object to add check to + :param str sriov_numvfs: space separated list of : + """ + nrpe.add_check( + shortname='sriov_numvfs', + description='Check SRIOV numvfs', + check_cmd='check_sriov_numvfs.py {}'.format(sriov_numvfs)) diff --git a/charmhelpers/contrib/network/files/__init__.py b/charmhelpers/contrib/network/files/__init__.py new file mode 100644 index 000000000..d7567b863 --- /dev/null +++ b/charmhelpers/contrib/network/files/__init__.py @@ -0,0 +1,13 @@ +# Copyright 2014-2015 Canonical Limited. +# +# 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. diff --git a/charmhelpers/contrib/network/files/check_sriov_numvfs.py b/charmhelpers/contrib/network/files/check_sriov_numvfs.py new file mode 100755 index 000000000..0e815a659 --- /dev/null +++ b/charmhelpers/contrib/network/files/check_sriov_numvfs.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +""" +Check to verify the number of virtual functions (VFs) active on network +interfaces. Based on /sys/class/net//device/sriov_numvfs and +/sys/class/net//device/sriov_totalvfs. + + usage: check_sriov_numvfs.py [-h] sriov_numvfs [sriov_numvfs ...] + + Check sriov numvfs configuration + + positional arguments: + sriov_numvfs format: : + +For each interfaces:numvfs pair given it verifies that: + 1. VFs are enabled + 2. VFs are configured are the same as specified in the check parameter + 3. VFs configured do not exceed the maximum available on the interface + +Example: ./check_sriov_numvfs.py ens3f0:32 ens3f1:32 ens6f0:32 ens6f1:32 +""" + +import argparse +import os.path +import sys + + +DEVICE_TEMPLATE = "/sys/class/net/{0}" +SRIOV_NUMVFS_TEMPLATE = "/sys/class/net/{0}/device/sriov_numvfs" +SRIOV_TOTALVFS_TEMPLATE = "/sys/class/net/{0}/device/sriov_totalvfs" + + +def get_interface_setting(file): + """ Return the value content of a setting file as int """ + with open(file) as f: + value = f.read() + return int(value) + + +def check_interface_numvfs(iface, numvfs): + """ Check SRIOV numvfs config """ + sriov_numvfs_path = SRIOV_NUMVFS_TEMPLATE.format(iface) + sriov_totalvfs_path = SRIOV_TOTALVFS_TEMPLATE.format(iface) + msg = [] + if os.path.exists(DEVICE_TEMPLATE.format(iface)): + if (not os.path.exists(sriov_totalvfs_path) or + not os.path.exists(sriov_numvfs_path)): + msg.append("{}: VFs are disabled or not-available".format(iface)) + else: + sriov_numvfs = get_interface_setting(sriov_numvfs_path) + sriov_totalvfs = get_interface_setting(sriov_totalvfs_path) + if numvfs != sriov_numvfs: + msg.append( + "{}: Number of VFs on interface ({}) does not match check ({})" + .format(iface, sriov_numvfs, numvfs) + ) + if numvfs > sriov_totalvfs: + msg.append( + "{}: Maximum number of VFs available on interface ({}) is lower than the check ({})" + .format(iface, sriov_totalvfs, numvfs) + ) + return msg + + +def parse_sriov_numvfs(device_numvfs): + """ Parse parameters and check format """ + msg = "parameter format must be ':', e.g. ens3f0:32, given; {}".format(device_numvfs) + assert device_numvfs != '', msg + parts = device_numvfs.split(':') + assert len(parts) == 2, msg + assert len(parts[0]) > 0, msg + assert int(parts[1]) > 0, msg + iface = str(device_numvfs.split(':')[0]) + numvfs = int(device_numvfs.split(':')[1]) + return (iface, numvfs) + + +def parse_args(): + """Parse command-line options.""" + parser = argparse.ArgumentParser(description="Check sriov numvfs configuration") + parser.add_argument("sriov_numvfs", nargs='+', help="format: :") + args = parser.parse_args() + return args + + +def main(): + """Parse args and check the sriov numvfs.""" + args = parse_args() + error_msg = [] + + for device_numvfs in args.sriov_numvfs: + iface, numvfs = parse_sriov_numvfs(device_numvfs) + error_msg += check_interface_numvfs(iface, numvfs) + if error_msg: + print("CRITICAL: {} problems detected\n".format(len(error_msg)) + "\n".join(error_msg)) + sys.exit(2) + + print("OK: sriov_numvfs set to " + ", ".join(args.sriov_numvfs)) + + +if __name__ == "__main__": + main() diff --git a/tests/contrib/charmsupport/test_nrpe.py b/tests/contrib/charmsupport/test_nrpe.py index 334cf18a2..2464ee68b 100644 --- a/tests/contrib/charmsupport/test_nrpe.py +++ b/tests/contrib/charmsupport/test_nrpe.py @@ -494,3 +494,12 @@ def test_copy_nrpe_checks_nrpe_files_dir(self): self.patched['copy2'].assert_called_once_with( 'filea', '/usr/local/lib/nagios/plugins/filea') + + @patch.object(nrpe.NRPE, 'add_check') + def test_add_sriov_numvfs_checks(self, mock_nrpe_add_check): + foo = nrpe.NRPE() + nrpe.add_sriov_numvfs_checks(foo, 'ens3f0:32 ens3f1:32') + mock_nrpe_add_check.assert_called_once_with( + shortname='sriov_numvfs', + description='Check SRIOV numvfs', + check_cmd='check_sriov_numvfs.py ens3f0:32 ens3f1:32') diff --git a/tests/contrib/network/files/__init__.py b/tests/contrib/network/files/__init__.py new file mode 100644 index 000000000..d7567b863 --- /dev/null +++ b/tests/contrib/network/files/__init__.py @@ -0,0 +1,13 @@ +# Copyright 2014-2015 Canonical Limited. +# +# 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. diff --git a/tests/contrib/network/files/test_check_sriov_numvfs.py b/tests/contrib/network/files/test_check_sriov_numvfs.py new file mode 100644 index 000000000..2829ac591 --- /dev/null +++ b/tests/contrib/network/files/test_check_sriov_numvfs.py @@ -0,0 +1,82 @@ +import mock +import os +import tempfile +import unittest + +from charmhelpers.contrib.network.files import check_sriov_numvfs + +__author__ = 'Stephan Pampel ' + + +class TestCheckSriovNumfs(unittest.TestCase): + + interface_folder = os.path.join(tempfile.gettempdir(), 'ens3f0') + sriov_numvfs_file = os.path.join(interface_folder, 'sriov_numvfs') + sriov_totalvfs_file = os.path.join(interface_folder, 'sriov_totalvfs') + + def __init__(self, *args, **kwargs): + super(TestCheckSriovNumfs, self).__init__(*args, **kwargs) + if not os.path.exists(self.interface_folder): + os.mkdir(self.interface_folder) + with open(self.sriov_numvfs_file, "w") as f: + f.write("32") + with open(self.sriov_totalvfs_file, "w") as f: + f.write("64") + + def __del__(self): + if os.path.exists(self.interface_folder): + for file in os.listdir(self.interface_folder): + os.remove(os.path.join(self.interface_folder, file)) + os.removedirs(self.interface_folder) + + def test_parameter_parsing(self): + """ check if the sriov_numvfs parameter is parsed correctly """ + iface, numvfs = check_sriov_numvfs.parse_sriov_numvfs('ens3f0:32') + self.assertEqual(iface, 'ens3f0') + self.assertEqual(numvfs, 32) + + for param in ['', ':', ':test', ':32', 'ens3f2', 'a:1:b']: + self.assertRaises(AssertionError, check_sriov_numvfs.parse_sriov_numvfs, param) + + for param in ['ens3f2:', 'ens3f2:test']: + self.assertRaises(ValueError, check_sriov_numvfs.parse_sriov_numvfs, param) + + def test_check_interface_numvfs_no_interface(self): + """ check should report nothing if the interface does not exists """ + self.assertListEqual( + check_sriov_numvfs.check_interface_numvfs('no-interface', 0), [] + ) + + @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.DEVICE_TEMPLATE', interface_folder) + def test_check_interface_numvfs_vfs_disabled(self): + """ check if virtual functions are disabled """ + self.assertListEqual( + check_sriov_numvfs.check_interface_numvfs('ens3f0', 0), + ['ens3f0: VFs are disabled or not-available'] + ) + + @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.DEVICE_TEMPLATE', interface_folder) + @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.SRIOV_NUMVFS_TEMPLATE', sriov_numvfs_file) + @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.SRIOV_TOTALVFS_TEMPLATE', sriov_totalvfs_file) + def test_check_interface_numvfs_vfs_enabled(self): + """ check if virtual functions are enabled """ + + # check numvfs correct + self.assertListEqual( + check_sriov_numvfs.check_interface_numvfs('ens3f0', 32), + [] + ) + + # check numvfs != check + self.assertListEqual( + check_sriov_numvfs.check_interface_numvfs('ens3f0', 16), + ['ens3f0: Number of VFs on interface (32) does not match check (16)'] + ) + + # check numvfs > sriov_totalvfs + with open(self.sriov_numvfs_file, "w") as f: + f.write("128") + self.assertListEqual( + check_sriov_numvfs.check_interface_numvfs('ens3f0', 128), + ['ens3f0: Maximum number of VFs available on interface (64) is lower than the check (128)'] + ) From b31fe62645e934556704f850baaff83b8e73dd32 Mon Sep 17 00:00:00 2001 From: Stephan Pampel <7194553+SPFZ@users.noreply.github.com> Date: Thu, 13 Jan 2022 15:15:33 +0100 Subject: [PATCH 2/5] Implement suggested improvements to check_sriov_numvfs.py and related code. --- charmhelpers/contrib/__init__.py | 13 ---- charmhelpers/contrib/charmsupport/nrpe.py | 6 +- .../contrib/network/files/__init__.py | 13 ---- .../network/files/check_sriov_numvfs.py | 69 ++++++++++++------- tests/contrib/charmsupport/test_nrpe.py | 4 +- tests/contrib/network/files/__init__.py | 13 ---- .../network/files/test_check_sriov_numvfs.py | 18 ++--- 7 files changed, 59 insertions(+), 77 deletions(-) diff --git a/charmhelpers/contrib/__init__.py b/charmhelpers/contrib/__init__.py index d7567b863..e69de29bb 100644 --- a/charmhelpers/contrib/__init__.py +++ b/charmhelpers/contrib/__init__.py @@ -1,13 +0,0 @@ -# Copyright 2014-2015 Canonical Limited. -# -# 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. diff --git a/charmhelpers/contrib/charmsupport/nrpe.py b/charmhelpers/contrib/charmsupport/nrpe.py index e086957ba..f3215db06 100644 --- a/charmhelpers/contrib/charmsupport/nrpe.py +++ b/charmhelpers/contrib/charmsupport/nrpe.py @@ -1,4 +1,4 @@ -# Copyright 2012-2021 Canonical Limited. +# Copyright 2012-2022 Canonical Limited. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -522,9 +522,9 @@ def remove_deprecated_check(nrpe, deprecated_services): nrpe.remove_check(shortname=dep_svc) -def add_sriov_numvfs_checks(nrpe, sriov_numvfs): +def add_sriov_numvfs_check(nrpe, sriov_numvfs): """ - Add checks for openvswitch + Add a check for the number of VFs configured for SR-IOV interfaces :param NRPE nrpe: NRPE object to add check to :param str sriov_numvfs: space separated list of : """ diff --git a/charmhelpers/contrib/network/files/__init__.py b/charmhelpers/contrib/network/files/__init__.py index d7567b863..e69de29bb 100644 --- a/charmhelpers/contrib/network/files/__init__.py +++ b/charmhelpers/contrib/network/files/__init__.py @@ -1,13 +0,0 @@ -# Copyright 2014-2015 Canonical Limited. -# -# 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. diff --git a/charmhelpers/contrib/network/files/check_sriov_numvfs.py b/charmhelpers/contrib/network/files/check_sriov_numvfs.py index 0e815a659..79b39c522 100755 --- a/charmhelpers/contrib/network/files/check_sriov_numvfs.py +++ b/charmhelpers/contrib/network/files/check_sriov_numvfs.py @@ -1,12 +1,25 @@ #!/usr/bin/env python3 +# Copyright 2014-2022 Canonical Limited. +# +# 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. """ -Check to verify the number of virtual functions (VFs) active on network +Check to verify the number of virtual functions (VFs) active on SR-IOV network interfaces. Based on /sys/class/net//device/sriov_numvfs and /sys/class/net//device/sriov_totalvfs. usage: check_sriov_numvfs.py [-h] sriov_numvfs [sriov_numvfs ...] - Check sriov numvfs configuration + Check SR-IOV number of virtual functions configuration positional arguments: sriov_numvfs format: : @@ -16,6 +29,8 @@ 2. VFs are configured are the same as specified in the check parameter 3. VFs configured do not exceed the maximum available on the interface + Non-existent interfaces are not checked. + Example: ./check_sriov_numvfs.py ens3f0:32 ens3f1:32 ens6f0:32 ens6f1:32 """ @@ -30,40 +45,46 @@ def get_interface_setting(file): - """ Return the value content of a setting file as int """ + """Return the value content of a setting file as int""" with open(file) as f: value = f.read() return int(value) def check_interface_numvfs(iface, numvfs): - """ Check SRIOV numvfs config """ + """Check SR-IOV numvfs config""" sriov_numvfs_path = SRIOV_NUMVFS_TEMPLATE.format(iface) sriov_totalvfs_path = SRIOV_TOTALVFS_TEMPLATE.format(iface) msg = [] - if os.path.exists(DEVICE_TEMPLATE.format(iface)): - if (not os.path.exists(sriov_totalvfs_path) or - not os.path.exists(sriov_numvfs_path)): - msg.append("{}: VFs are disabled or not-available".format(iface)) - else: - sriov_numvfs = get_interface_setting(sriov_numvfs_path) - sriov_totalvfs = get_interface_setting(sriov_totalvfs_path) - if numvfs != sriov_numvfs: - msg.append( - "{}: Number of VFs on interface ({}) does not match check ({})" - .format(iface, sriov_numvfs, numvfs) - ) - if numvfs > sriov_totalvfs: - msg.append( - "{}: Maximum number of VFs available on interface ({}) is lower than the check ({})" - .format(iface, sriov_totalvfs, numvfs) - ) + + # Ignore non-existing interfaces + if not os.path.exists(DEVICE_TEMPLATE.format(iface)): + return [] + + # Ensure that SR-IOV/VT-d and IOMMU are enabled + if (not os.path.exists(sriov_totalvfs_path) or + not os.path.exists(sriov_numvfs_path)): + return ["{}: VFs are disabled or not-available".format(iface)] + + # Verify number of virtual functions + sriov_numvfs = get_interface_setting(sriov_numvfs_path) + sriov_totalvfs = get_interface_setting(sriov_totalvfs_path) + if numvfs != sriov_numvfs: + msg.append( + "{}: Number of VFs on interface ({}) does not match expected ({})" + .format(iface, sriov_numvfs, numvfs) + ) + if numvfs > sriov_totalvfs: + msg.append( + "{}: Maximum number of VFs available on interface ({}) is lower than the expected ({})" + .format(iface, sriov_totalvfs, numvfs) + ) return msg def parse_sriov_numvfs(device_numvfs): - """ Parse parameters and check format """ - msg = "parameter format must be ':', e.g. ens3f0:32, given; {}".format(device_numvfs) + """Parse parameters and check format""" + msg = "Parameter format must be ':', e.g. ens3f0:32, given: {}".format(device_numvfs) assert device_numvfs != '', msg parts = device_numvfs.split(':') assert len(parts) == 2, msg @@ -76,7 +97,7 @@ def parse_sriov_numvfs(device_numvfs): def parse_args(): """Parse command-line options.""" - parser = argparse.ArgumentParser(description="Check sriov numvfs configuration") + parser = argparse.ArgumentParser(description="Check SR-IOV number of virtual functions configuration") parser.add_argument("sriov_numvfs", nargs='+', help="format: :") args = parser.parse_args() return args diff --git a/tests/contrib/charmsupport/test_nrpe.py b/tests/contrib/charmsupport/test_nrpe.py index 2464ee68b..d01a59f70 100644 --- a/tests/contrib/charmsupport/test_nrpe.py +++ b/tests/contrib/charmsupport/test_nrpe.py @@ -496,9 +496,9 @@ def test_copy_nrpe_checks_nrpe_files_dir(self): '/usr/local/lib/nagios/plugins/filea') @patch.object(nrpe.NRPE, 'add_check') - def test_add_sriov_numvfs_checks(self, mock_nrpe_add_check): + def test_add_sriov_numvfs_check(self, mock_nrpe_add_check): foo = nrpe.NRPE() - nrpe.add_sriov_numvfs_checks(foo, 'ens3f0:32 ens3f1:32') + nrpe.add_sriov_numvfs_check(foo, 'ens3f0:32 ens3f1:32') mock_nrpe_add_check.assert_called_once_with( shortname='sriov_numvfs', description='Check SRIOV numvfs', diff --git a/tests/contrib/network/files/__init__.py b/tests/contrib/network/files/__init__.py index d7567b863..e69de29bb 100644 --- a/tests/contrib/network/files/__init__.py +++ b/tests/contrib/network/files/__init__.py @@ -1,13 +0,0 @@ -# Copyright 2014-2015 Canonical Limited. -# -# 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. diff --git a/tests/contrib/network/files/test_check_sriov_numvfs.py b/tests/contrib/network/files/test_check_sriov_numvfs.py index 2829ac591..d6d0ddc71 100644 --- a/tests/contrib/network/files/test_check_sriov_numvfs.py +++ b/tests/contrib/network/files/test_check_sriov_numvfs.py @@ -30,7 +30,7 @@ def __del__(self): os.removedirs(self.interface_folder) def test_parameter_parsing(self): - """ check if the sriov_numvfs parameter is parsed correctly """ + """Ensure that the format of the sriov_numvfs parameter is parsed correctly""" iface, numvfs = check_sriov_numvfs.parse_sriov_numvfs('ens3f0:32') self.assertEqual(iface, 'ens3f0') self.assertEqual(numvfs, 32) @@ -42,14 +42,14 @@ def test_parameter_parsing(self): self.assertRaises(ValueError, check_sriov_numvfs.parse_sriov_numvfs, param) def test_check_interface_numvfs_no_interface(self): - """ check should report nothing if the interface does not exists """ + """Check should ignore the interface if it does not exists""" self.assertListEqual( check_sriov_numvfs.check_interface_numvfs('no-interface', 0), [] ) @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.DEVICE_TEMPLATE', interface_folder) def test_check_interface_numvfs_vfs_disabled(self): - """ check if virtual functions are disabled """ + """Check should report if virtual functions are disabled """ self.assertListEqual( check_sriov_numvfs.check_interface_numvfs('ens3f0', 0), ['ens3f0: VFs are disabled or not-available'] @@ -59,24 +59,24 @@ def test_check_interface_numvfs_vfs_disabled(self): @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.SRIOV_NUMVFS_TEMPLATE', sriov_numvfs_file) @mock.patch('charmhelpers.contrib.network.files.check_sriov_numvfs.SRIOV_TOTALVFS_TEMPLATE', sriov_totalvfs_file) def test_check_interface_numvfs_vfs_enabled(self): - """ check if virtual functions are enabled """ + """Check if numvfs is evaluated correctly""" - # check numvfs correct + # check numvfs correct should pass self.assertListEqual( check_sriov_numvfs.check_interface_numvfs('ens3f0', 32), [] ) - # check numvfs != check + # check numvfs != expected should faild self.assertListEqual( check_sriov_numvfs.check_interface_numvfs('ens3f0', 16), - ['ens3f0: Number of VFs on interface (32) does not match check (16)'] + ['ens3f0: Number of VFs on interface (32) does not match expected (16)'] ) - # check numvfs > sriov_totalvfs + # check numvfs > sriov_totalvfs should fail with open(self.sriov_numvfs_file, "w") as f: f.write("128") self.assertListEqual( check_sriov_numvfs.check_interface_numvfs('ens3f0', 128), - ['ens3f0: Maximum number of VFs available on interface (64) is lower than the check (128)'] + ['ens3f0: Maximum number of VFs available on interface (64) is lower than the expected (128)'] ) From a77cfa2f211ef07a2a28e61f9cdc57c9e84aadbb Mon Sep 17 00:00:00 2001 From: Stephan Pampel <7194553+SPFZ@users.noreply.github.com> Date: Thu, 13 Jan 2022 15:32:36 +0100 Subject: [PATCH 3/5] Add safeguard against exceptions to check_sriov_numvfs.py --- .../network/files/check_sriov_numvfs.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/charmhelpers/contrib/network/files/check_sriov_numvfs.py b/charmhelpers/contrib/network/files/check_sriov_numvfs.py index 79b39c522..50682649c 100755 --- a/charmhelpers/contrib/network/files/check_sriov_numvfs.py +++ b/charmhelpers/contrib/network/files/check_sriov_numvfs.py @@ -36,6 +36,7 @@ import argparse import os.path +import traceback import sys @@ -108,12 +109,17 @@ def main(): args = parse_args() error_msg = [] - for device_numvfs in args.sriov_numvfs: - iface, numvfs = parse_sriov_numvfs(device_numvfs) - error_msg += check_interface_numvfs(iface, numvfs) - if error_msg: - print("CRITICAL: {} problems detected\n".format(len(error_msg)) + "\n".join(error_msg)) - sys.exit(2) + try: + for device_numvfs in args.sriov_numvfs: + iface, numvfs = parse_sriov_numvfs(device_numvfs) + error_msg += check_interface_numvfs(iface, numvfs) + if error_msg: + print("CRITICAL: {} problems detected\n".format(len(error_msg)) + "\n".join(error_msg)) + sys.exit(2) + except: # noqa: E722 + print("{} raised unknown exception '{}'".format(__file__, sys.exc_info()[0])) + traceback.print_exc(file=sys.stdout) + sys.exit(3) print("OK: sriov_numvfs set to " + ", ".join(args.sriov_numvfs)) From e7b79dcba824815e380081347375331d270a91e5 Mon Sep 17 00:00:00 2001 From: Stephan Pampel <7194553+SPFZ@users.noreply.github.com> Date: Tue, 18 Jan 2022 10:15:05 +0100 Subject: [PATCH 4/5] check_sriov_numvfs.py: improved handling of known exceptions; format improvements --- .../network/files/check_sriov_numvfs.py | 59 +++++++++++++------ .../network/files/test_check_sriov_numvfs.py | 6 +- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/charmhelpers/contrib/network/files/check_sriov_numvfs.py b/charmhelpers/contrib/network/files/check_sriov_numvfs.py index 50682649c..7b22c19ce 100755 --- a/charmhelpers/contrib/network/files/check_sriov_numvfs.py +++ b/charmhelpers/contrib/network/files/check_sriov_numvfs.py @@ -45,6 +45,12 @@ SRIOV_TOTALVFS_TEMPLATE = "/sys/class/net/{0}/device/sriov_totalvfs" +class ArgsFormatError(Exception): + """This indicates argument format that is not supported.""" + + pass + + def get_interface_setting(file): """Return the value content of a setting file as int""" with open(file) as f: @@ -63,8 +69,7 @@ def check_interface_numvfs(iface, numvfs): return [] # Ensure that SR-IOV/VT-d and IOMMU are enabled - if (not os.path.exists(sriov_totalvfs_path) or - not os.path.exists(sriov_numvfs_path)): + if not os.path.exists(sriov_totalvfs_path) or not os.path.exists(sriov_numvfs_path): return ["{}: VFs are disabled or not-available".format(iface)] # Verify number of virtual functions @@ -72,34 +77,43 @@ def check_interface_numvfs(iface, numvfs): sriov_totalvfs = get_interface_setting(sriov_totalvfs_path) if numvfs != sriov_numvfs: msg.append( - "{}: Number of VFs on interface ({}) does not match expected ({})" - .format(iface, sriov_numvfs, numvfs) + "{}: Number of VFs on interface ({}) does not match expected ({})".format( + iface, sriov_numvfs, numvfs + ) ) if numvfs > sriov_totalvfs: msg.append( - "{}: Maximum number of VFs available on interface ({}) is lower than the expected ({})" - .format(iface, sriov_totalvfs, numvfs) + "{}: Maximum number of VFs available on interface ({}) is lower than the expected ({})".format( + iface, sriov_totalvfs, numvfs + ) ) return msg def parse_sriov_numvfs(device_numvfs): """Parse parameters and check format""" - msg = "Parameter format must be ':', e.g. ens3f0:32, given: {}".format(device_numvfs) - assert device_numvfs != '', msg - parts = device_numvfs.split(':') - assert len(parts) == 2, msg - assert len(parts[0]) > 0, msg - assert int(parts[1]) > 0, msg - iface = str(device_numvfs.split(':')[0]) - numvfs = int(device_numvfs.split(':')[1]) + msg = "Parameter format must be ':', e.g. ens3f0:32, given: '{}'".format( + device_numvfs + ) + parts = device_numvfs.split(":") + if ( + len(parts) != 2 or # exactly 2 parts + len(parts[0]) < 3 or # interface name should be at least 3 chars + len(parts[1]) < 1 or # numvfs should be at least 1 char + int(parts[1]) < 0 # numvfs should be a valid int > 0 + ): + raise ArgsFormatError(msg) + iface = str(device_numvfs.split(":")[0]) + numvfs = int(device_numvfs.split(":")[1]) return (iface, numvfs) def parse_args(): """Parse command-line options.""" - parser = argparse.ArgumentParser(description="Check SR-IOV number of virtual functions configuration") - parser.add_argument("sriov_numvfs", nargs='+', help="format: :") + parser = argparse.ArgumentParser( + description="Check SR-IOV number of virtual functions configuration" + ) + parser.add_argument("sriov_numvfs", nargs="+", help="format: :") args = parser.parse_args() return args @@ -114,14 +128,23 @@ def main(): iface, numvfs = parse_sriov_numvfs(device_numvfs) error_msg += check_interface_numvfs(iface, numvfs) if error_msg: - print("CRITICAL: {} problems detected\n".format(len(error_msg)) + "\n".join(error_msg)) + error_list = "\n".join(error_msg) + print( + "CRITICAL: {} problems detected\n{}".format(len(error_msg), error_list) + ) sys.exit(2) + except (FileNotFoundError, PermissionError, ValueError, ArgsFormatError) as e: + msg = "CRITICAL: Exception {} occurred during check: '{}'" + print(msg.format(e.__class__.__name__, e)) + traceback.print_exc(file=sys.stdout) + sys.exit(2) except: # noqa: E722 print("{} raised unknown exception '{}'".format(__file__, sys.exc_info()[0])) traceback.print_exc(file=sys.stdout) sys.exit(3) - print("OK: sriov_numvfs set to " + ", ".join(args.sriov_numvfs)) + msg = "OK: sriov_numvfs set to '{}' (non existing interface are ignored)" + print(msg.format(", ".join(args.sriov_numvfs))) if __name__ == "__main__": diff --git a/tests/contrib/network/files/test_check_sriov_numvfs.py b/tests/contrib/network/files/test_check_sriov_numvfs.py index d6d0ddc71..3c8e328b2 100644 --- a/tests/contrib/network/files/test_check_sriov_numvfs.py +++ b/tests/contrib/network/files/test_check_sriov_numvfs.py @@ -35,10 +35,10 @@ def test_parameter_parsing(self): self.assertEqual(iface, 'ens3f0') self.assertEqual(numvfs, 32) - for param in ['', ':', ':test', ':32', 'ens3f2', 'a:1:b']: - self.assertRaises(AssertionError, check_sriov_numvfs.parse_sriov_numvfs, param) + for param in ['', ':', ':test', ':32', 'ens3f2', 'a:1:b', 'ens3f2:']: + self.assertRaises(check_sriov_numvfs.ArgsFormatError, check_sriov_numvfs.parse_sriov_numvfs, param) - for param in ['ens3f2:', 'ens3f2:test']: + for param in ['ens3f2:test']: self.assertRaises(ValueError, check_sriov_numvfs.parse_sriov_numvfs, param) def test_check_interface_numvfs_no_interface(self): From b8b75e860168fdab30a050b5badcd3b080d61261 Mon Sep 17 00:00:00 2001 From: Stephan Pampel <7194553+SPFZ@users.noreply.github.com> Date: Thu, 20 Jan 2022 10:42:06 +0100 Subject: [PATCH 5/5] check_sriov_numvfs.py: parse arguments via regex; extend function docstrings --- .../network/files/check_sriov_numvfs.py | 53 +++++++++++++------ .../network/files/test_check_sriov_numvfs.py | 5 +- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/charmhelpers/contrib/network/files/check_sriov_numvfs.py b/charmhelpers/contrib/network/files/check_sriov_numvfs.py index 7b22c19ce..62bd0c23e 100755 --- a/charmhelpers/contrib/network/files/check_sriov_numvfs.py +++ b/charmhelpers/contrib/network/files/check_sriov_numvfs.py @@ -36,11 +36,13 @@ import argparse import os.path +import re import traceback import sys DEVICE_TEMPLATE = "/sys/class/net/{0}" +DEVICE_VF_PATTERN = r'(?P[0-9a-z]{3,}):(?P\d+)' SRIOV_NUMVFS_TEMPLATE = "/sys/class/net/{0}/device/sriov_numvfs" SRIOV_TOTALVFS_TEMPLATE = "/sys/class/net/{0}/device/sriov_totalvfs" @@ -52,14 +54,28 @@ class ArgsFormatError(Exception): def get_interface_setting(file): - """Return the value content of a setting file as int""" + """Return the value content of a settings file as int. + + :param file: Full path of the settings file. + :type file: str + :returns: The interface setting as int. + :rtype: int + """ with open(file) as f: value = f.read() return int(value) def check_interface_numvfs(iface, numvfs): - """Check SR-IOV numvfs config""" + """Verify SR-IOV interface configuration for number of virtual functions. + + :param iface: Name of the interface. + :type iface: str + :param numvfs: Number of virtual functions that should to be configured. + :type numvfs: int + :returns: List of check violations found for the device. + :rtype: [str] + """ sriov_numvfs_path = SRIOV_NUMVFS_TEMPLATE.format(iface) sriov_totalvfs_path = SRIOV_TOTALVFS_TEMPLATE.format(iface) msg = [] @@ -72,15 +88,18 @@ def check_interface_numvfs(iface, numvfs): if not os.path.exists(sriov_totalvfs_path) or not os.path.exists(sriov_numvfs_path): return ["{}: VFs are disabled or not-available".format(iface)] - # Verify number of virtual functions sriov_numvfs = get_interface_setting(sriov_numvfs_path) sriov_totalvfs = get_interface_setting(sriov_totalvfs_path) + + # Verify that number of virtual functions is set to the expected number if numvfs != sriov_numvfs: msg.append( "{}: Number of VFs on interface ({}) does not match expected ({})".format( iface, sriov_numvfs, numvfs ) ) + + # Verify that the device supports the amount of VFs that we expect to be configured if numvfs > sriov_totalvfs: msg.append( "{}: Maximum number of VFs available on interface ({}) is lower than the expected ({})".format( @@ -91,25 +110,29 @@ def check_interface_numvfs(iface, numvfs): def parse_sriov_numvfs(device_numvfs): - """Parse parameters and check format""" + """Parse sriov_numvfs string, verify format and return parsed values. + + :param device_numvfs: The devicename and number of Virtual Functions, format ':'. + :type device_numvfs: str + :returns: The interface name and number of VFs. + :rtype: (str, int) + :raises: ArgsFormatError if the format of the parameter is not correct. + """ msg = "Parameter format must be ':', e.g. ens3f0:32, given: '{}'".format( device_numvfs ) - parts = device_numvfs.split(":") - if ( - len(parts) != 2 or # exactly 2 parts - len(parts[0]) < 3 or # interface name should be at least 3 chars - len(parts[1]) < 1 or # numvfs should be at least 1 char - int(parts[1]) < 0 # numvfs should be a valid int > 0 - ): + match = re.match(DEVICE_VF_PATTERN, device_numvfs) + if match is None: raise ArgsFormatError(msg) - iface = str(device_numvfs.split(":")[0]) - numvfs = int(device_numvfs.split(":")[1]) - return (iface, numvfs) + return match.group('iface'), int(match.group('numvfs')) def parse_args(): - """Parse command-line options.""" + """Parse command-line options. + + :returns: Argparsed cli parameters as argparse.Namespace + :rtype: argparse.Namespace + """ parser = argparse.ArgumentParser( description="Check SR-IOV number of virtual functions configuration" ) diff --git a/tests/contrib/network/files/test_check_sriov_numvfs.py b/tests/contrib/network/files/test_check_sriov_numvfs.py index 3c8e328b2..84dbff0f9 100644 --- a/tests/contrib/network/files/test_check_sriov_numvfs.py +++ b/tests/contrib/network/files/test_check_sriov_numvfs.py @@ -35,12 +35,9 @@ def test_parameter_parsing(self): self.assertEqual(iface, 'ens3f0') self.assertEqual(numvfs, 32) - for param in ['', ':', ':test', ':32', 'ens3f2', 'a:1:b', 'ens3f2:']: + for param in ['', ':', ':test', ':32', 'ens3f2', 'a:1:b', 'ens3f2:', 'ens3f2:test']: self.assertRaises(check_sriov_numvfs.ArgsFormatError, check_sriov_numvfs.parse_sriov_numvfs, param) - for param in ['ens3f2:test']: - self.assertRaises(ValueError, check_sriov_numvfs.parse_sriov_numvfs, param) - def test_check_interface_numvfs_no_interface(self): """Check should ignore the interface if it does not exists""" self.assertListEqual(