Skip to content

machine_os: Preemptively load vfio kernel module.#678

Merged
fnordahl merged 2 commits intoopenstack-charmers:masterfrom
mkalcok:tmp-vfio-load
Apr 9, 2025
Merged

machine_os: Preemptively load vfio kernel module.#678
fnordahl merged 2 commits intoopenstack-charmers:masterfrom
mkalcok:tmp-vfio-load

Conversation

@mkalcok
Copy link
Copy Markdown
Contributor

@mkalcok mkalcok commented Apr 9, 2025

VFIO kernel module is not always loaded, which causes tests to fail when they try to set No-IOMMU mode. This change calls modprobe to ensure that VFIO is loaded before we try to use it.

@mkalcok mkalcok requested a review from fnordahl April 9, 2025 08:23
:raises: AssertionError, zaza.model.CommandRunFailed
"""
zaza.utilities.juju.remote_run(
unit.name, 'modprobe vfio', model_name=model_name, fatal=True)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would an alternative be to use the already existing load_kernel_module() function for this?:

def load_kernel_module(unit_name, module_name, module_arguments=None,
model_name=None):
"""Load kernel module on unit.
:param unit_name: Name of unit to operate on
:type unit_name: str
:param module_name: Name of kernel module to load
:type module_name: str
:param module_arguments: Extra arguments to pass to module on load
:type module_arguments: Optional[str]
:param model_name: Name of model to query
:type model_name: Optional[str]
:returns: The function is called for its side effects and does not return.
:raises: zaza.model.CommandRunFailed
"""
cmd = 'modprobe {} {}'.format(module_name, module_arguments or '')
zaza.utilities.juju.remote_run(unit_name, cmd, model_name=model_name)

And does the call belong here or on the test code (have no strong opinions on this one, just raising the question).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point about the load_kernel_module, I missed that.
Regarding the placement, I think that this is the right place. Up until now the tests called enable_vfio_unsafe_noiommu_mode with the assumption that the VFIO module is loaded. Loading the kernel here ensures that the assumption is correct (+ there are no side effects to trying to load already loaded module).

@mkalcok
Copy link
Copy Markdown
Contributor Author

mkalcok commented Apr 9, 2025

I wonder what changed since the last time the CI passed. I don't see any changes, yet linting fails on parts of the code that wasn't changed in this PR.

mkalcok added 2 commits April 9, 2025 13:00
VFIO kernel module is not always loaded, which causes tests to fail
when they try to set No-IOMMU mode. This change calls modprobe to
ensure that VFIO is loaded before we try to use it.

Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com>
Flake8 started to complain about unused 'global' and 'nonlocal'
annotations (F842). Their usage fell into three categories:

* variable was not assigned to after declaring it with 'global'.
* function declared variable as 'global' before returning it.
  However, this does not make returned value "globally writable"
* declaring 'global' for sets and dicts before modifying them.
  This is not necessary. These structures are globally mutable
  even without the 'global' statement.

Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.50%. Comparing base (4f05c70) to head (b20ec47).
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #678   +/-   ##
=======================================
  Coverage   89.50%   89.50%           
=======================================
  Files          42       42           
  Lines        4717     4718    +1     
=======================================
+ Hits         4222     4223    +1     
  Misses        495      495           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mkalcok mkalcok requested a review from fnordahl April 9, 2025 11:46
Copy link
Copy Markdown
Collaborator

@fnordahl fnordahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thank you for the code gardening required by new flake8 unblocking the zaza gate!

@fnordahl fnordahl merged commit 1655161 into openstack-charmers:master Apr 9, 2025
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants