SRIOV tests and unit tests (New) #1761

mreed8855 · 2025-02-28T20:35:41Z

sriov checkbox files

Description

Creates and SRIOV device in either a virtual machine or container
Verify's that the NIC is working.

Resolved issues

This is a replacment for PR #1293

Documentation

Tests

sriov checkbox files

pedro-avalos · 2025-03-01T13:59:28Z

@mreed8855 you seem to have forgotten to commit and push your bin/sriov.py file

codecov · 2025-03-03T06:20:51Z

Codecov Report

Attention: Patch coverage is 93.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 49.68%. Comparing base (e22361c) to head (dde0667).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
providers/base/bin/sriov.py	93.33%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1761      +/-   ##
==========================================
+ Coverage   49.55%   49.68%   +0.12%     
==========================================
  Files         377      378       +1     
  Lines       40645    40765     +120     
  Branches     6832     6843      +11     
==========================================
+ Hits        20142    20254     +112     
- Misses      19779    19785       +6     
- Partials      724      726       +2

Flag	Coverage Δ
provider-base	`25.73% <93.33%> (+0.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pedro-avalos

I have a few issues with some of the functions, but overall the test looks good.

After making these changes and updating the tests, you should run and submit a test run with a local clone of checkbox; then link the submission in the tests section of this PR, so we can review if the test runs successfully.

providers/base/bin/sriov.py

pedro-avalos · 2025-03-25T21:55:49Z

providers/base/bin/sriov.py

+    try:
+        if os.path.exists(vendor_id_path):
+            with open(vendor_id_path, "r") as file:
+                vendor_id = file.read().strip()
+
+        if vendor_id in VENDOR_INFO:
+            vendor_name = VENDOR_INFO[vendor_id]
+            if vendor_name != "Broadcom":
+                logging.info(
+                    "The interface {} is a(n) {} NIC".format(
+                        interface, vendor_name
+                    )
+                )
+            else:
+                logging.info(
+                    "Broadcom SRIOV testing is not supported at this time"
+                )
+                sys.exit(1)
+        else:
+            logging.info(
+                "{} has an unknown vendor  ID {}".format(interface, vendor_id)
+            )
+            sys.exit(1)
+
+    except Exception as e:
+        logging.info("An error occurred: {}".format(e))
+        sys.exit(1)


There are a couple issues I have with this function.

The massive try/except block

The very broad except Exception block

You should specify the encoding expected when calling open

I think we can raise exceptions that make sense instead of logging and exiting

Based on the above, take a look at my suggested edits for this function, and see if they makes sense:

Suggested change

try:

if os.path.exists(vendor_id_path):

with open(vendor_id_path, "r") as file:

vendor_id = file.read().strip()

if vendor_id in VENDOR_INFO:

vendor_name = VENDOR_INFO[vendor_id]

if vendor_name != "Broadcom":

logging.info(

"The interface {} is a(n) {} NIC".format(

interface, vendor_name

)

)

else:

logging.info(

"Broadcom SRIOV testing is not supported at this time"

)

sys.exit(1)

else:

logging.info(

"{} has an unknown vendor ID {}".format(interface, vendor_id)

)

sys.exit(1)

except Exception as e:

logging.info("An error occurred: {}".format(e))

sys.exit(1)

if not os.path.exists(vendor_id_path):

raise FileNotFoundError(

"Vendor ID path {} not found".format(vendor_id_path)

)

with open(vendor_id_path, "r", encoding="utf-8") as file:

vendor_id = file.read().strip()

if vendor_id not in VENDOR_INFO:

raise ValueError(

"{} has an unknown vendor ID {}".format(interface, vendor_id)

)

vendor_name = VENDOR_INFO[vendor_id]

if vendor_name == "Broadcom":

raise NotImplementedError(

"Broadcom SRIOV testing is not supported at this time"

)

logging.info("The interface %s is a(n) %s NIC", interface, vendor_name)

pedro-avalos · 2025-03-25T21:59:39Z

providers/base/bin/sriov.py

+def check_ubuntu_version():
+    logging.info("Check for 24.04 or greater")
+    try:
+        # Get distro information
+        version = distro.version()
+
+        # Check if the OS is Ubuntu and the version is greater than 24.04
+        if float(version) < 24.04:
+            logging.info(
+                "24.04 or greater is required, this is {}".format(version)
+            )
+            sys.exit(1)
+        else:
+            logging.info("The system is 24.04 or greater, proceed")
+    except Exception as e:
+        logging.info("An error occurred: {}".format(e))
+        sys.exit(1)


There are a couple issues with this test.

distro won't always be available; the import distro line above will fail with older Python versions.

The distro.version() call can return 24 when the distro.id() is ubuntu-core

I am not a fan of the logging.info for errors.

I think we can raise exceptions that make sense for some errors

Please look at the following function in virtualization.py, as it does a similar job as this function:

checkbox/providers/base/bin/virtualization.py

Line 136 in 04293a1

def get_release_to_test():

Note: The eventual goal is to extract this functionality to checkbox-support, but we're not there yet.

pedro-avalos · 2025-03-25T22:05:01Z

providers/base/bin/sriov.py

+def is_sriov_capable(interface):
+    """
+    Check if the specified network interface is SR-IOV capable and
+    configured to support at least one Virtual Function.
+    """
+    sriov_path = "/sys/class/net/{}/device/sriov_numvfs".format(interface)
+    num_vfs = NUM_OF_VIRTUAL_IFACES
+
+    try:
+        # Check if the interface supports SR-IOV
+        logging.info("checking if sriov_numvfs exits")
+        if not os.path.exists(sriov_path):
+            logging.error(
+                "SR-IOV not supported or interface {} \
+                          does not exist.".format(
+                    interface
+                )
+            )
+        else:
+            logging.info(
+                "SR-IOV before change {} VFs on interface {}.".format(
+                    num_vfs, interface
+                )
+            )
+            # First, disable VFs before changing the number to avoid issues
+            logging.info("setting numvfs to zero")
+            with open(sriov_path, "w") as f:
+                f.write("0")
+
+            # Set the desired number of VFs
+            with open(sriov_path, "w") as f:
+                f.write(str(num_vfs))
+
+            logging.info(
+                "SR-IOV enabled with {} VFs on interface {}.".format(
+                    num_vfs, interface
+                )
+            )
+
+    except (IOError, FileNotFoundError) as e:
+        logging.info("Failed to enable SR-IOV on {}: {}".format(interface, e))
+        sys.exit(1)
+
+    except Exception as e:
+        logging.info("An error occurred: {}".format(e))
+        sys.exit(1)


Same concerns as with the check_interface_vendor function.

Suggested change

def is_sriov_capable(interface):

"""

Check if the specified network interface is SR-IOV capable and

configured to support at least one Virtual Function.

"""

sriov_path = "/sys/class/net/{}/device/sriov_numvfs".format(interface)

num_vfs = NUM_OF_VIRTUAL_IFACES

try:

# Check if the interface supports SR-IOV

logging.info("checking if sriov_numvfs exits")

if not os.path.exists(sriov_path):

logging.error(

"SR-IOV not supported or interface {} \

does not exist.".format(

interface

)

)

else:

logging.info(

"SR-IOV before change {} VFs on interface {}.".format(

num_vfs, interface

)

)

# First, disable VFs before changing the number to avoid issues

logging.info("setting numvfs to zero")

with open(sriov_path, "w") as f:

f.write("0")

# Set the desired number of VFs

with open(sriov_path, "w") as f:

f.write(str(num_vfs))

logging.info(

"SR-IOV enabled with {} VFs on interface {}.".format(

num_vfs, interface

)

)

except (IOError, FileNotFoundError) as e:

logging.info("Failed to enable SR-IOV on {}: {}".format(interface, e))

sys.exit(1)

except Exception as e:

logging.info("An error occurred: {}".format(e))

sys.exit(1)

def is_sriov_capable(interface):

"""

Check if the specified network interface is SR-IOV capable and

configured to support at least one Virtual Function.

"""

sriov_path = "/sys/class/net/{}/device/sriov_numvfs".format(interface)

num_vfs = NUM_OF_VIRTUAL_IFACES

logging.info("Checking if sriov_numvfs exists")

if not os.path.exists(sriov_path):

raise FileNotFoundError(

"SR-IOV not supported or interface {} does not exist".format(

interface

)

)

logging.info(

"SR-IOV before change %d VFs on interface %s", num_vfs, interface

)

logging.info("Setting numvfs to zero")

with open(sriov_path, "w", encoding="utf-8") as f:

f.write("0")

logging.info("Setting numvfs to %d", num_vfs)

with open(sriov_path, "w", encoding="utf-8") as f:

f.write(str(num_vfs))

logging.info(

"SR-IOV enabled with %d VFs on interface %s", num_vfs, interface

)

pedro-avalos · 2025-03-26T12:58:37Z

providers/base/bin/sriov.py

+    instance = LXD(args.template, args.rootfs)
+    with LXD(args.template, args.rootfs) as instance:


The with statement should be enough to create the instance. If that's not the case, then there is a bug

Suggested change

instance = LXD(args.template, args.rootfs)

with LXD(args.template, args.rootfs) as instance:

with LXD(args.template, args.rootfs) as instance:

pedro-avalos · 2025-03-26T12:59:54Z

providers/base/bin/sriov.py

+    try:
+        logging.basicConfig(level=args.log_level)
+    except AttributeError:
+        pass


This shouldn't raise an AttributeError

Suggested change

try:

logging.basicConfig(level=args.log_level)

except AttributeError:

pass

logging.basicConfig(level=args.log_level)

pedro-avalos · 2025-03-26T13:01:44Z

providers/base/bin/sriov.py

+    # Verify args
+    try:
+        args.func(args)
+    except AttributeError:
+        parser.print_help()
+        return 1


Same as above, this shouldn't raise an AttributeError if the arguments are parsed correctly.

Suggested change

# Verify args

try:

args.func(args)

except AttributeError:

parser.print_help()

return 1

args.func(args)

pedro-avalos · 2025-03-26T13:07:26Z

providers/base/units/sriov/jobs.pxu

+plugin: shell
+category_id: sriov
+id: sriov/verify_lxd_vm_sriov
+environ: LXD_TEMPLATE KVM_IMAGE
+estimated_duration: 60.0
+requires:
+ executable.name == 'lxc'
+ package.name == 'lxd-installer' or snap.name == 'lxd'
+command: sriov.py --debug --interface lxdvm
+_purpose:
+ Verifies that sriov network device can created with an LXD Virtual Machine
+_summary:
+ Verify SRIOV NIC with LXD Virtual Machine


This should probably also be a template job like the one below. The command is missing the {interface}, for example

pedro-avalos · 2025-03-26T13:09:03Z

providers/base/units/sriov/jobs.pxu

+requires:
+ executable.name == 'lxc'
+ package.name == 'lxd' or package.name == 'lxd-installer' or snap.name == 'lxd'
+command: sudo sriov.py --debug --interface {interface} lxd


I'm pretty sure there's a user option you can give your unit. So you should be able to use user: root instead of calling sudo

After talking with Max, you should also be able to use resources to ensure that the release is above a certain version. Please take a look at the lsb (lsb.release) resource. Please add that to the requires section and the bootstrap_include in the test plan.

Correction, the os resource is cross-platform and works everywhere, that one should be used instead of the deprecated lsb resource.

pedro-avalos · 2025-03-26T13:10:13Z

providers/base/units/sriov/test-plan.pxu

+    sriov/verify_lxd_sriov-.*               certification-status=blocker
+    sriov/verify_lxd_vm_sriov-.*             certification-status=blocker
+bootstrap_include:
+    device


These also require the snap, package, and executable to be in bootstrap_include

SRIOV tests and unit tests

1f298f8

sriov checkbox files

mreed8855 mentioned this pull request Feb 28, 2025

Create a test for SRIOV Intel NIC's (New) #1293

Closed

mreed8855 added 2 commits March 2, 2025 23:36

Updates made to sriov.py and unit tests to increase the coverage

7880cf8

Updated for black format c heck

dde0667

pedro-avalos changed the title ~~SRIOV tests and unit tests~~ SRIOV tests and unit tests (New) Mar 3, 2025

Merge branch 'main' into sriov_update4

90b30a8

pedro-avalos self-requested a review March 17, 2025 18:40

pedro-avalos self-assigned this Mar 17, 2025

pedro-avalos requested changes Mar 26, 2025

View reviewed changes

Updates made to address the PR comments

9e51d1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SRIOV tests and unit tests (New) #1761

SRIOV tests and unit tests (New) #1761

mreed8855 commented Feb 28, 2025

pedro-avalos commented Mar 1, 2025

codecov bot commented Mar 3, 2025 •

edited

Loading

pedro-avalos left a comment

pedro-avalos Mar 25, 2025 •

edited

Loading

pedro-avalos Mar 25, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 25, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

pedro-avalos Mar 26, 2025

		instance = LXD(args.template, args.rootfs)
		with LXD(args.template, args.rootfs) as instance:

SRIOV tests and unit tests (New) #1761

Are you sure you want to change the base?

SRIOV tests and unit tests (New) #1761

Conversation

mreed8855 commented Feb 28, 2025

Description

Resolved issues

Documentation

Tests

pedro-avalos commented Mar 1, 2025

codecov bot commented Mar 3, 2025 • edited Loading

Codecov Report

pedro-avalos left a comment

Choose a reason for hiding this comment

pedro-avalos Mar 25, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2025 •

edited

Loading

pedro-avalos Mar 25, 2025 •

edited

Loading