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

feat: Initialize a bootloader role #71

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

spetrosi
Copy link
Contributor

@spetrosi spetrosi commented Oct 27, 2023

Enhancement: Add bootloader system role to manage bootloader configuration consistently.

Adding support for the following functionality:

  • Gathering bootloader facts
  • Modifying bootloader arguments
  • Modifying bootloader timeout
  • Modifying bootloader password

Issue Tracker Tickets (Jira or BZ if any): RHELPLAN-35009

@spetrosi spetrosi requested a review from richm as a code owner October 27, 2023 13:15
@spetrosi
Copy link
Contributor Author

[citest]

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
bootloader_timeout: 5
bootloader_password: null
bootloader_remove_password: false
bootloader_reboot_ok: true
roles:
- linux-system-roles.bootloader
```
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have some examples of that show the kernel command line before running the role, and what the command line looks like after running the role will various parameters e.g.

before
args="rhgb quiet"

with parameters as above will result in
args="rhgb quiet crashkernel=auto no_timer_check debug rd.lvm.lv resume"

with parameters as above with `previous: replaced` will result in
args="quiet crashkernel=auto no_timer_check debug rd.lvm.lv resume"

most people using this role will be familiar with grubby other tools that let you edit the kernel command line in a similar format, so having examples like that should help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIll do

handlers/main.yml Outdated Show resolved Hide resolved
handlers/reboot.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated
- name: Generate boot loader password
shell: >-
set -euo pipefail;
( echo {{ bootloader_password }} ; echo {{ bootloader_password }} )
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
( echo {{ bootloader_password }} ; echo {{ bootloader_password }} )
( echo {{ bootloader_password | quote }} ; echo {{ bootloader_password | quote }} )

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe there is a more secure way to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no. The usual way to do this is by running grub2-setpassword, but it does not allow inputtng via echo:

# ( echo "12345" ; echo "12345" ) | grub2-setpassword
stty: standard input: Inappropriate ioctl for device

There is no way to input other than echo, no env var for this or so.

tasks/modify_settings.yml Outdated Show resolved Hide resolved
tasks/modify_settings.yml Outdated Show resolved Hide resolved
@spetrosi
Copy link
Contributor Author

[citest]

2 similar comments
@spetrosi
Copy link
Contributor Author

[citest]

@spetrosi
Copy link
Contributor Author

[citest]

tasks/modify_settings.yml Outdated Show resolved Hide resolved
@spetrosi
Copy link
Contributor Author

spetrosi commented Nov 1, 2023

[citest]

@spetrosi
Copy link
Contributor Author

spetrosi commented Nov 1, 2023

[citest]

@spetrosi spetrosi marked this pull request as ready for review November 1, 2023 16:56
@spetrosi spetrosi marked this pull request as draft November 1, 2023 16:56
@spetrosi
Copy link
Contributor Author

spetrosi commented Nov 1, 2023

[citest]

tests/tests_settings.yml Outdated Show resolved Hide resolved
@spetrosi
Copy link
Contributor Author

spetrosi commented Nov 1, 2023

[citest]

@spetrosi
Copy link
Contributor Author

spetrosi commented Nov 1, 2023

[citest]

library/bootloader_facts.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
library/bootloader_facts.py Outdated Show resolved Hide resolved
library/bootloader_facts.py Outdated Show resolved Hide resolved
library/bootloader_facts.py Outdated Show resolved Hide resolved
library/bootloader_facts.py Outdated Show resolved Hide resolved
@spetrosi
Copy link
Contributor Author

[citest]

@spetrosi
Copy link
Contributor Author

@richm can you please take a look what I am missing in unit tests?

@richm
Copy link
Contributor

richm commented Nov 29, 2023

tests/unit/bootloader_settings.yml? should be a .py python file?

@spetrosi spetrosi marked this pull request as ready for review December 6, 2023 12:33
@spetrosi
Copy link
Contributor Author

spetrosi commented Dec 6, 2023

[citest]

Copy link

codecov bot commented Dec 6, 2023

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@spetrosi
Copy link
Contributor Author

spetrosi commented Dec 6, 2023

[citest]

1 similar comment
@spetrosi
Copy link
Contributor Author

spetrosi commented Dec 7, 2023

[citest]

@spetrosi
Copy link
Contributor Author

spetrosi commented Dec 7, 2023

TODO:

  1. Check mode for bottloader_settings.py
  2. Adding and removing kernels - add a state dict key to bootloader_settings. Maybe it would make sense to accept a single entry for bootloader_settings.kernel.path, bootloader_settings.kernel.index, bootloader_settings.kernel.title. In this case, users won't be able to apply the same bottloader settings to a set of specific kernels, but I do not think that they will use it at all. You still can apply settings to ALL, or just copy settings for several specifi kernels if you need to. This way, bootloader_settings will be more consistent in adding a new kernel and changing an existing kernel.
  3. Support Debian for SAP roles
  4. Change boot device - add a new variable bootloader_boot_device.

Add bootloader_facts.py
Add bootloader_settings module to configure settings instead of tasks
Add unit tests for bootloader_settings and bootloader_facts
@spetrosi
Copy link
Contributor Author

spetrosi commented Dec 8, 2023

[citest]

@spetrosi spetrosi merged commit c0d7bda into linux-system-roles:main Dec 8, 2023
21 of 23 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