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

add persistent option for modprobe #5424

Conversation

haddystuff
Copy link
Contributor

SUMMARY

Add persistent option as was requested in #4028

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/system/modprobe.py

ADDITIONAL INFORMATION

If kernel module doesn't load after reboot(by PCI IDs, USB IDs, DMI IDs or similar triggers) you can use static files in /etc/modules-load.d to load this module. This is what I implemented. If you add option permanent and set it as true ansible will add module file which should enable module on startup. If you set permanent option as false then ansible will try to find line where you enabled this module previously and comment it out. Same works with kernel module options in /etc/modprobe.d directory.
Example:

 - name: Add the dummy module
    community.general.modprobe:
      name: dummy
      state: present
      params: 'numdummies=4'
      persistent: true

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module plugins plugin (any type) system tests tests unit tests/unit needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 25, 2022
@github-actions
Copy link

github-actions bot commented Oct 25, 2022

Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/community.general/actions/runs/3615033372

File changes:

  • M collections/community/general/modprobe_module.html
Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/community.general/community.general/docsbuild/base/collections/community/general/modprobe_module.html b/home/runner/work/community.general/community.general/docsbuild/head/collections/community/general/modprobe_module.html
index 9c6fcf8..3b348b9 100644
--- a/home/runner/work/community.general/community.general/docsbuild/base/collections/community/general/modprobe_module.html
+++ b/home/runner/work/community.general/community.general/docsbuild/head/collections/community/general/modprobe_module.html
@@ -177,6 +177,22 @@
 </div></td>
 </tr>
 <tr class="row-even"><td><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-persistent"></div><p class="ansible-option-title" id="ansible-collections-community-general-modprobe-module-parameter-persistent"><strong>persistent</strong></p>
+<a class="ansibleOptionLink" href="#parameter-persistent" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
+</div></td>
+<td><div class="ansible-option-cell"><p>Persistency between reboots for configured module.</p>
+<p>This option creates files in <code class="docutils literal notranslate"><span class="pre">/etc/modules-load.d/</span></code> and <code class="docutils literal notranslate"><span class="pre">/etc/modprobe.d/</span></code> that make your module configuration persistent during reboots.</p>
+<p>Note that it is usually a better idea to rely on the automatic module loading by PCI IDs, USB IDs, DMI IDs or similar triggers encoded in the kernel modules themselves instead of configuration like this.</p>
+<p>In fact, most modern kernel modules are prepared for automatic loading already.</p>
+<p>Also this options work only with distributives that uses systemd.</p>
+<p class="ansible-option-line"><span class="ansible-option-choices">Choices:</span></p>
+<ul class="simple">
+<li><p><code class="ansible-option-default-bold docutils literal notranslate"><span class="pre">false</span></code> <span class="ansible-option-choices-default-mark">← (default)</span></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">true</span></code></p></li>
+</ul>
+</div></td>
+</tr>
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-state"></div><p class="ansible-option-title" id="ansible-collections-community-general-modprobe-module-parameter-state"><strong>state</strong></p>
 <a class="ansibleOptionLink" href="#parameter-state" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>

@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 25, 2022
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added some first comments.

changelogs/fragments/4028-modprobe-persistent-option.yml Outdated Show resolved Hide resolved
plugins/modules/system/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/system/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/system/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/system/modprobe.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/system/test_modprobe.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 26, 2022
@haddystuff
Copy link
Contributor Author

Made all the suggested changes

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

I've been looking at this PR for some time now, and I'm not sure whether this option (as a boolean toggle with a default value) is a good idea. How can you for example say "the module should be still loaded now, but not persistently anymore" with this toggle?

I first thought it might be better to update state to have two more values, presistent and not_persistent, so that present means "I don't care whether it is persistent", and absent implies "not persistent", but then I know too little about this to be sure this is an accurate description of what can be done / makes sense. I guess you could declare a module as persistent without having it loaded now? (Which would not be possible with these states.) Also since persistent has stronger requirements than the remainder of the module, coupling it to state=absent is a big problem.

- Note that it is usually a better idea to rely on the automatic module loading by PCI IDs, USB IDs, DMI IDs or similar triggers encoded in the
kernel modules themselves instead of configuration like this.
- In fact, most modern kernel modules are prepared for automatic loading already.
- Also this options work only with distributives that uses systemd.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Also this options work only with distributives that uses systemd.
- Also this options work only with distributions that uses systemd.

@haddystuff
Copy link
Contributor Author

haddystuff commented Oct 30, 2022

Answering your first question - yes. That's the exact idea behind this feature. You need to set status to present and persistent to false. Basically if persistent set to false then this module behavior would be the same as it was before. One problem with this approach - if you previously added module persistently and want to disable persistency but module should still be activated - that's not gonna work in one task. I can fix this and check if module activated persistently with persistent option set to false and then disable it if needed. Also I had an idea of using persistent as multiple choice parameter with options like: loaded, unloaded, ignore(or some sort of value that you can use if you don't care about persistency). This could also be handy in future if we want to add more states like blacklist.

Second question - No, you cannot declare module as persistent without having it loaded now. As i already mentioned persistency is connected to state. You can just take a look at main function to understand how it works.

@felixfontein
Copy link
Collaborator

Please note that in #5461 the collection repository was restructured to remove the directory tree in plugins/modules/, and the corresponding tree in tests/unit/plugins/modules/. Your PR modifies files in this directory structure, and unfortunately now has some conflicts, potentially because of this. Please rebase with the current main branch to remove the conflicts.

@ansibullbot ansibullbot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 3, 2022
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 8, 2022
@haddystuff haddystuff force-pushed the add_permanent_option_to_modprobe branch from 9b5e4d4 to 44f8647 Compare December 4, 2022 19:51
@ansibullbot ansibullbot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci CI is older than 7 days, rerun before merging labels Dec 4, 2022
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Dec 4, 2022
@haddystuff
Copy link
Contributor Author

haddystuff commented Dec 4, 2022

Sorry that it took me so long, but I've made this rebase.
BTW what about my previous message? Should I change something?

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 18, 2023
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 18, 2023
@haddystuff
Copy link
Contributor Author

haddystuff commented Feb 21, 2023

Hi @felixfontein, can you please help me with understanding tests result: I don't quite get why this one test is failed. What should I change?

@felixfontein
Copy link
Collaborator

@haddystuff please simply ignore it, it will fail until you rebase with current main (please don't do that just for this test).

@haddystuff haddystuff requested review from russoz and removed request for felixfontein February 22, 2023 19:27
@haddystuff
Copy link
Contributor Author

Thank you @felixfontein

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Just one small improvement when compiling the regexps, other than that it LGTM

plugins/modules/modprobe.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/test_modprobe.py Outdated Show resolved Hide resolved
plugins/modules/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/modprobe.py Outdated Show resolved Hide resolved
plugins/modules/modprobe.py Outdated Show resolved Hide resolved
- move regexps compiling to __init__
- move AnsibleModule to build_module function and use this function in tests instead of AnsibleModule
- fix terminlogy issue in documentation
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2023
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2023
@haddystuff
Copy link
Contributor Author

@felixfontein, all issues you and @russoz have mentioned are fixed

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll merge tomorrow evening if nobody objects.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Feb 26, 2023
@felixfontein felixfontein merged commit 29f5033 into ansible-collections:main Feb 26, 2023
@patchback
Copy link

patchback bot commented Feb 26, 2023

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/29f5033737a7fd86349ff3daab7d7ee7db66ad00/pr-5424

Backported as #6102

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 26, 2023
* add persistent option for modprobe

* add suggested changes + fix broken test

* change modprobe module path in tests due to rebase

* change persistent option type from bool to str with choices

* fix unused import

* add example with persistent option

* fix some minor issues after review

- move regexps compiling to __init__
- move AnsibleModule to build_module function and use this function in tests instead of AnsibleModule
- fix terminlogy issue in documentation

* fix unused-import

(cherry picked from commit 29f5033)
@felixfontein
Copy link
Collaborator

@haddystuff thanks for your contribution, and sorry it took so long to get this done!
@russoz thanks for reviewing this!

felixfontein pushed a commit that referenced this pull request Feb 26, 2023
…robe (#6102)

add persistent option for modprobe (#5424)

* add persistent option for modprobe

* add suggested changes + fix broken test

* change modprobe module path in tests due to rebase

* change persistent option type from bool to str with choices

* fix unused import

* add example with persistent option

* fix some minor issues after review

- move regexps compiling to __init__
- move AnsibleModule to build_module function and use this function in tests instead of AnsibleModule
- fix terminlogy issue in documentation

* fix unused-import

(cherry picked from commit 29f5033)

Co-authored-by: Alex Groshev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module plugins plugin (any type) system tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants