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

Make modprobe module check for builtins as well #37150

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@cognifloyd
Contributor

cognifloyd commented Mar 7, 2018

Without this modprobe always reports changed when modprobe-ing a builtin module.

SUMMARY

This makes the modprobe module check /usr/lib/$kernel_release/modules.builtin to avoid reporting changed when state is present and the module is builtin.

When the module is builtin, it is already loaded and running modprobe can't change anything (neither adding or removing the kernel module). So, if it's builtin, this should never report changed.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

modprobe module

ANSIBLE VERSION
devel
@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 7, 2018

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/system/modprobe.py:96:26: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/system/modprobe.py:97:22: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

@cognifloyd

This comment has been minimized.

Contributor

cognifloyd commented Mar 7, 2018

from irc:

(16:16:13) alikins: cognifloyd: sounds bugfixy to me. What should happen if you 'state=absent' a builtin module?
(16:18:23) cognifloyd: It will fail.
(16:18:34) cognifloyd: rmmod will report a helpful message saying it is builtin.

⚡ root@pahoran $ modprobe -r nfs
modprobe: ERROR: Module nfs is builtin.

(16:19:45) cognifloyd: rc=1

elif state == 'absent':
if present:
if not module.check_mode:
rc, out, err = module.run_command([module.get_bin_path('modprobe', True), '-r', name])
if rc != 0:
module.fail_json(msg=err, rc=rc, stdout=out, stderr=err, **result)

Then it's up to the user how to handle a builtin module error appropriately if they are trying to use state=absent.

@ansibot ansibot removed the ci_verified label Mar 7, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 7, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/system/modprobe.py:97:38: E225 missing whitespace around operator

click here for bot help

@abadger

This comment has been minimized.

Member

abadger commented Jul 24, 2018

Looks like this needs a rebase but the idea here seems sound to me. There's a whiff of kludge about having to massage the data so much but I can't think of a better way to do it.

Since this has the potential to break existing playbooks we better have a changelog ( https://github.com/ansible/ansible/tree/devel/changelogs/fragments ) and porting guide entry: https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guides/porting_guide_2.7.rst#noteworthy-module-changes In the porting guide, be sure to mention that builtins can now make the module fail and how to work around that.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 26, 2018

Make modprobe module check for builtins as well
Without this modprobe always reports changed when modprobe-ing a builtin module.

With this, if a kernel module is a builtin, the modprobe module will:
- succeed (without incorrectly reporting changed) if ``state`` is ``present``;
- fail if ``state`` is ``absent``

The failure will have whatever error message modprobe returns when
attempting to remove a builtin module. For example:
``modprobe: ERROR: Module nfs is builtin.``

@cognifloyd cognifloyd force-pushed the cognifloyd:patch-2 branch from 80ce070 to fe9b132 Nov 26, 2018

@cognifloyd

This comment has been minimized.

Contributor

cognifloyd commented Nov 26, 2018

Rebased. Added changelog entry. And added an entry in the 2.8 porting guide. Will this get backported to 2.7 as well? (ie should it be in the 2.7 porting guide?)

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment