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 support for blacklisting modules #2

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add support for blacklisting modules #2

wants to merge 10 commits into from

Conversation

balbirsinghcan
Copy link
Contributor

*By default, this will create state in the form of /etc/modprobe.d/blacklist.conf, which should all work as expected as long as the service is running and stopped during startup and reboot. Corner cases such as apply/stop service and reboot, will leave the state file behind

Review suggestions -- areas that need closer look

  1. Are we blacklisting the correct modules?
  2. Should we document the state file created and expectations more clearly in a following commit
  3. Is the implementation for get/set blacklist modules correct and efficient?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

vallishgv and others added 10 commits February 13, 2019 18:54
Signed-off-by: Vallish Vaidyeshwara <[email protected]>
Reviewed-by: Balbir Singh <[email protected]>
Reviewed-by: Frederick Lefebvre <[email protected]>
Reviewed-by: Frank van der Linden <[email protected]>
Non-root users executing autotune fail trying to
grab the service lock. However this code flow is
not user friendly. Root check needs to be more
gracious and user friendly.

Signed-off-by: Vallish Vaidyeshwara <[email protected]>
Reviewed-by: Balbir Singh <[email protected]>
Signed-off-by: Vallish Vaidyeshwara <[email protected]>
Reviewed-by: Balbir Singh <[email protected]>
…easier to add new sections

Signed-off-by: Balbir Singh <[email protected]>
Add support for blacklisting un-needed modules. This
support today does not take instance type into account,
but in the future we would want to consider

Signed-off-by: Balbir Singh <[email protected]>
This patch adds support for generation of actual changes
to /etc/modprobe.d/blacklist.conf. The code checks to
see if the blacklist <module> already exists in any of
the files in /etc/modprobe.d and if so, skips it.

Support is also added for rollback of the config via
autotune rollback

Signed-off-by: Balbir Singh <[email protected]>
Signed-off-by: Balbir Singh <[email protected]>
Copy link
Contributor

@vallishgv vallishgv left a comment

Choose a reason for hiding this comment

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

This patch series can leave blacklisted modules configured in the case of "autotune apply" and the system crashes after autotune configures the system. Since these changes are always required for EC2 instances, it would be better if we move the logic of blacklisting modules to %post and have it run as part of autotune package installation.

@vallishgv
Copy link
Contributor

Once we move the logic into %post, we would not need to worry about reverting blacklisting of modules. Blacklisting of modules is one time effort.

@fllinden
Copy link

autotune's original scope was optimizing system performance - how does blacklisting modules fall in to that scope? It it does, then that's fine, but if it doesn't, the scope of autotune is being widened to system configuration - not just system optimization. I'm weary of feature creep.

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.

4 participants