Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve #58307 when modules.builtin is missing. #58839

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@dglinder
Copy link
Contributor

commented Jul 8, 2019

SUMMARY

Fixes #58307 when modules.builtin is missing.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

modprobe.py

ADDITIONAL INFORMATION

This should resolve the cases where the module.builtin file is missing and handle the modprobe correctly.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@jpmahowald
Copy link
Contributor

left a comment

I would rather not fail the module if missing at all, only a warning. On systems without modules.builtin, it reverts to the previous cannot detect builtin behavior, and lets modprobe make an attempt.

if line.endswith(module_file):
present = True
break
try:

This comment has been minimized.

Copy link
@jpmahowald

jpmahowald Jul 9, 2019

Contributor

Why insert a nested try catching the same exception? Simpler to have one exception covering both. Unlikely that /proc/modules would be ENOENT or EPERM, but might as well handle it the same way.

This comment has been minimized.

Copy link
@dglinder

dglinder Jul 12, 2019

Author Contributor

Possibly, I'll look into a single try/except block and see what that looks like. (I was trying to be surgical and only update the block of code that I could adequately validate, hence the try/except block around the "modules.builtin" file access.

# The open() failed
if e.errno != errno.ENOENT:
# We are ok if this file doesnt exist, but nothing else
module.fail_json(msg=to_native(e), exception=traceback.format_exc(), **result)
except IOError as e:
module.fail_json(msg=to_native(e), exception=traceback.format_exc(), **result)

This comment has been minimized.

Copy link
@jpmahowald

jpmahowald Jul 9, 2019

Contributor

What is the value of failing if /proc/modules or modules.builtin is unreadable? Maybe replace with warn if you want to tell the user about builtin_path. Later, modprobe will return an error if it is builtin.

This comment has been minimized.

Copy link
@dglinder

dglinder Jul 12, 2019

Author Contributor

I was trying to be specific and only fail when errno.ENOENT occurs (file doesn't exist to read from), but if it is not readable by the user that is a different issue that I don't have a good test case to validate with.

It appears that the Red Hat EL 6 "2.6.32-754.15.3.el6.x86_64" kernel package deletes this when the kernel is upgraded (I need to do more testing), but missing either of these is still apparently a valid (albeit non-optimal) configuration.

This comment has been minimized.

Copy link
@jpmahowald

jpmahowald Jul 12, 2019

Contributor

I don't have a good reason why the file would exist but not readable, they are readable in packaged kernels. What I am suggesting is to move this exception in reading the builtins file away from failing the module, as it has proven a little bit tricky. I consider modprobe return code the authority on whether it failed or not.

@dglinder

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

I would rather not fail the module if missing at all, only a warning. On systems without modules.builtin, it reverts to the previous cannot detect builtin behavior, and lets modprobe make an attempt.

I admit I am a Python noob, but what you mention is what I was trying to do. If there's a cleaner place to let the code handle this and still continue the playbook I'm open to that input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.