NRPE: add check: check_sriov_numvfs.py #670

SPFZ · 2022-01-13T08:36:41Z

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.

This check is intended to be used in various networking charms like ovn-chassis, therefore it is added here to avoid replicating the code.

The goal of the check is to identify network interface configuration problems regarding virtual functions.

Documentation:

Check to verify the number of virtual functions (VFs) active on network
interfaces. Based on /sys/class/net/<interface>/device/sriov_numvfs and
/sys/class/net/<interface>/device/sriov_totalvfs.

    usage: check_sriov_numvfs.py [-h] sriov_numvfs [sriov_numvfs ...]

    Check sriov numvfs configuration

    positional arguments:
    sriov_numvfs  format: <interface>:<numvfs>

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

Check to verify the number of virtual functions (VFs) active on network interfaces. Based on /sys/class/net/<interface>/device/sriov_numvfs and /sys/class/net/<interface>/device/sriov_totalvfs.

jtroup

Great start, thanks.

charmhelpers/contrib/network/files/__init__.py

charmhelpers/contrib/network/files/check_sriov_numvfs.py

charmhelpers/contrib/charmsupport/nrpe.py

tests/contrib/network/files/__init__.py

tests/contrib/network/files/test_check_sriov_numvfs.py

… code.

auria

Overall, changes look good. I've shared a couple of suggestions. One changes what is considered "unknown" and what is "critical". The other is a readability suggestion.

auria · 2022-01-17T11:55:01Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+        if error_msg:
+            print("CRITICAL: {} problems detected\n".format(len(error_msg)) + "\n".join(error_msg))
+            sys.exit(2)
+    except:  # noqa: E722


I'm not opposed to a catch-all scope for unexpected exceptions. However, we should consider adding a previous specific list of exceptions that we think could be raised (and maybe raise a critical error when they happen). Alerting is triggered in Nagios when unknown/critical/recover events happen, so this change would only help in sharing a more meaningful message (a sentence vs a python traceback).

e.g. PermissionError, FileNotFoundError, AssertionError.

Side note: instead of using "assert ...", an exception class inherited from Exception (e.g. ArgsFormatError) could be raised when parsing CLI arguments (and caught here).

Thank you for the suggestions, I changed it to match your suggestion.

charmhelpers/contrib/network/files/check_sriov_numvfs.py

…improvements

auria

lgtm, thank you.

wolsen

Looks good at the high level, but a few comments inline.

wolsen · 2022-01-19T15:12:49Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+
+
+def get_interface_setting(file):
+    """Return the value content of a setting file as int"""


The docstrings should also document the function arguments and return expectations (including exceptions raised).

I added parameter and return docstrings to all functions now, please verify.

wolsen · 2022-01-19T15:13:25Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+
+
+def check_interface_numvfs(iface, numvfs):
+    """Check SR-IOV numvfs config"""


same as docstring comment as above

wolsen · 2022-01-19T15:20:16Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+
+
+def parse_sriov_numvfs(device_numvfs):
+    """Parse parameters and check format"""


wolsen · 2022-01-19T16:10:28Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+    iface = str(device_numvfs.split(":")[0])
+    numvfs = int(device_numvfs.split(":")[1])


device_numvfs doesn't need to be split again as you already have it in parts. You may also want to consider just unpacking the split into iface and numvfs directly. Yes, if there are too many ':' characters, it will raise a ValueError but its essentially the len(parts) != 2 check. Also, FYI the parens on the return are superfluous

try: iface, numvfs = device_numvfs.split(':') numvfs = int(numvfs) if len(iface) < 3 or numvfs < 0: raise ArgsFormatError(msg) return iface, numvfs except ValueError: raise ArgsFormatError(msg)

Alternatively, you could use regexes as well - though that is arguably more or less complex, depending on your love of regex:

DEV_VF_PATTERN = r'(?P<iface>[0-9a-z]{3,}):(?P<numvfs>\d+)' ... match = re.match(DEV_VF_PATTERN, device_numvfs) if match is None: raise ArgsFormatError(msg) return match.group('iface'), int(match.group('numvfs'))

Great suggestion! I changed it to use regex.

wolsen · 2022-01-19T16:49:07Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+        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


any particular reason you are relying on a ValueError here rather than using an ArgsFormatError? Its not necessarily wrong, and conveys information - but arguably a non-int value here is an ArgsFormatError

No particular reason. Due to the change to regex this is an ArgsFormatError now.

wolsen · 2022-01-19T17:08:31Z

charmhelpers/contrib/network/files/check_sriov_numvfs.py

+
+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"


Can you elaborate a bit more on why this is a critical alert when the requested VFs cannot be provided by the card? It seems conceivable to me that 2 cards with different capabilities across different units have the same interface name but one can serve 128 VFs and another 64. Setting the value to 128 maxes out both available VFs but it is fewer than 128*2.

…strings

SPFZ · 2022-01-20T09:59:43Z

@wolsen Thank you for the review! I improved the MR based on your comments.

Regarding the sriov_totalvfs: My assumption is that the number of virtual functions that was requested should be available for use.

If the requested numvfs > sriov_totalvfs then the alert will have two messages:

Number of VFs on interface does not match expected (because numvfs != sriov_numvfs L95)
Maximum number of VFs available on interface is lower than the expected (because numvfs > sriov_totalvfs L103)

The second message is just a hint for an operator that the reason might be the hardware or hardware configuration that does not support the requested number of VFs. This might be helpful if the cluster hardware is heterogeneous or a NIC has been replace with a different model that supports less VFs (we had a ase like this in the past). I can convert it to a Warning or remove it entirely if this behaviour is not desired but it might lead to a situation when VMs can not be deployed because there are no more VFs available.

wolsen · 2022-01-21T12:24:39Z

@wolsen Thank you for the review! I improved the MR based on your comments.

Regarding the sriov_totalvfs: My assumption is that the number of virtual functions that was requested should be available for use.

If the requested numvfs > sriov_totalvfs then the alert will have two messages:

Number of VFs on interface does not match expected (because numvfs != sriov_numvfs L95)

Maximum number of VFs available on interface is lower than the expected (because numvfs > sriov_totalvfs L103)

The second message is just a hint for an operator that the reason might be the hardware or hardware configuration that does not support the requested number of VFs. This might be helpful if the cluster hardware is heterogeneous or a NIC has been replace with a different model that supports less VFs (we had a ase like this in the past). I can convert it to a Warning or remove it entirely if this behaviour is not desired but it might lead to a situation when VMs can not be deployed because there are no more VFs available.

To explore it further, for these given checks, what actions are available to the operator with to avoid getting spammed with critical alerts when the heterogeneous scenario occurs?

SPFZ · 2022-01-21T13:17:32Z

To explore it further, for these given checks, what actions are available to the operator with to avoid getting spammed with critical alerts when the heterogeneous scenario occurs?

If the hardware is heterogeneous by design then an operator would have to create separate applications with different settings. We already have cases like this where were we have different type of machines due to different NICs or customer needs for the machines (with/without SR-IOV). There we deploy separate applications due to differences in hardware related settings (e.g. nova-compute and ovn-chassis).

An alternative would be to make this behavior configurable - essentially disabling checks 1. and 2. from #670 (comment).

wolsen · 2022-01-24T23:38:49Z

To explore it further, for these given checks, what actions are available to the operator with to avoid getting spammed with critical alerts when the heterogeneous scenario occurs?

If the hardware is heterogeneous by design then an operator would have to create separate applications with different settings. We already have cases like this where were we have different type of machines due to different NICs or customer needs for the machines (with/without SR-IOV). There we deploy separate applications due to differences in hardware related settings (e.g. nova-compute and ovn-chassis).

An alternative would be to make this behavior configurable - essentially disabling checks 1. and 2. from #670 (comment).

I think if its known in advance, then the heterogeneous by design could be accounted for. However, in the case of an existing deployment having to consider this I think it places a burden on the operator in this scenario. One option is certainly to make this configurable and this is a worthwhile discussion as to whether it should be configurable and what the default value should be.

However I think there's value in the other checks that are part of this PR and I don't want to hold those up for the discussion on this particular check. I propose that you break out the case where numvfs expected > total_vfs_sriov into another pull request and we can have appropriate discourse separately.

Does that sound reasonable?

SPFZ · 2022-01-25T13:45:30Z

However I think there's value in the other checks that are part of this PR and I don't want to hold those up for the discussion on this particular check. I propose that you break out the case where numvfs expected > total_vfs_sriov into another pull request and we can have appropriate discourse separately.
Does that sound reasonable?

Only the "VFs are enabled" check would remain.

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

Because the second check would still be CRITICAL.
E.g. card supports 64 VFs, check is set to 128 VFs then numvfs=64 != sriov_numvfs=128
https://github.com/SPFZ/charm-helpers/blob/master/charmhelpers/contrib/network/files/check_sriov_numvfs.py#L95

If that is needed to get this PR to move forward then I can remove the checks 2 and 3.

add check_sriov_numvfs.py

35573e1

Check to verify the number of virtual functions (VFs) active on network interfaces. Based on /sys/class/net/<interface>/device/sriov_numvfs and /sys/class/net/<interface>/device/sriov_totalvfs.

jtroup reviewed Jan 13, 2022

View reviewed changes

SPFZ added 2 commits January 13, 2022 15:15

Implement suggested improvements to check_sriov_numvfs.py and related…

b31fe62

… code.

Add safeguard against exceptions to check_sriov_numvfs.py

a77cfa2

auria suggested changes Jan 17, 2022

View reviewed changes

check_sriov_numvfs.py: improved handling of known exceptions; format …

e7b79dc

…improvements

auria previously approved these changes Jan 18, 2022

View reviewed changes

wolsen requested changes Jan 19, 2022

View reviewed changes

check_sriov_numvfs.py: parse arguments via regex; extend function doc…

b8b75e8

…strings

SPFZ dismissed auria’s stale review via b8b75e8 January 20, 2022 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NRPE: add check: check_sriov_numvfs.py #670

NRPE: add check: check_sriov_numvfs.py #670

SPFZ commented Jan 13, 2022

jtroup left a comment

auria left a comment

auria Jan 17, 2022

SPFZ Jan 18, 2022

auria left a comment

wolsen left a comment

wolsen Jan 19, 2022

SPFZ Jan 20, 2022

wolsen Jan 19, 2022

wolsen Jan 19, 2022

wolsen Jan 19, 2022

SPFZ Jan 20, 2022

wolsen Jan 19, 2022

SPFZ Jan 20, 2022

wolsen Jan 19, 2022

SPFZ commented Jan 20, 2022 •

edited

Loading

wolsen commented Jan 21, 2022

SPFZ commented Jan 21, 2022

wolsen commented Jan 24, 2022

SPFZ commented Jan 25, 2022



		def get_interface_setting(file):
		"""Return the value content of a setting file as int"""



		def check_interface_numvfs(iface, numvfs):
		"""Check SR-IOV numvfs config"""



		def parse_sriov_numvfs(device_numvfs):
		"""Parse parameters and check format"""

		iface = str(device_numvfs.split(":")[0])
		numvfs = int(device_numvfs.split(":")[1])

NRPE: add check: check_sriov_numvfs.py #670

Are you sure you want to change the base?

NRPE: add check: check_sriov_numvfs.py #670

Conversation

SPFZ commented Jan 13, 2022

jtroup left a comment

Choose a reason for hiding this comment

auria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auria left a comment

Choose a reason for hiding this comment

wolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SPFZ commented Jan 20, 2022 • edited Loading

wolsen commented Jan 21, 2022

SPFZ commented Jan 21, 2022

wolsen commented Jan 24, 2022

SPFZ commented Jan 25, 2022

SPFZ commented Jan 20, 2022 •

edited

Loading