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

sap_hana_preconfigure: update kernel parameters for SLES #638

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

Wabri
Copy link
Member

@Wabri Wabri commented Feb 6, 2024

Closes #337

@sean-freeman sean-freeman changed the title fix(sap_hana_preconfigure/tasks/SLES): update kernel paramenters sap_hana_preconfigure: update kernel parameters for SLES Feb 8, 2024
@sean-freeman
Copy link
Member

@Wabri I'm good with this, but before I approve - did you want to break it down so GRUB options can be altered independently? These are Ansible Tasks that apply to SUSE family already:

https://github.com/sap-linuxlab/community.sap_install/blob/main/roles/sap_hana_preconfigure/tasks/sapnote/2684254/configuration.yml

@Wabri
Copy link
Member Author

Wabri commented Feb 8, 2024

@sean-freeman done, needed to change the condition on the GRUB mkconfig task to check if some of the items have changed the grub configuration

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@berndfinger berndfinger self-requested a review February 9, 2024 09:53
@berndfinger
Copy link
Member

@Wabri Please add me as a reviewer for all preconfigure roles, for sap_hana_install, and sap_swpm.
Regarding this PR, there are two ansible-lint errors which need to be solved, see https://github.com/sap-linuxlab/community.sap_install/actions/runs/7832505611/job/21371352104?pr=638.

Also, add a comment before the line in which you implement #noqa (example: https://github.com/Wabri/community.sap_install/blob/fb32d0d80c01a462793b17285856e1f9e7553470/roles/sap_hana_preconfigure/tasks/SLES/configuration.yml#L54C1-L54C69), similar to

# Reason for noqa: Avoid calling one more command for getting the currently enabled repos
. See also https://ansible.readthedocs.io/projects/lint/usage/#muting-warnings-to-avoid-false-positives (search for best practice in this chapter).

- remove a trailing space
- add a `noqa no-changed-when` in order to prevent a false-positive of
  the linting
@Wabri
Copy link
Member Author

Wabri commented Feb 9, 2024

@berndfinger good catch, I've update with noqa and some explanation

@Wabri Wabri marked this pull request as draft February 9, 2024 11:39
@Wabri Wabri force-pushed the feature/337/sles-kernel branch from c4ab092 to 6fcc1a1 Compare February 9, 2024 13:54
@Wabri Wabri force-pushed the feature/337/sles-kernel branch from a63e0ab to 1179289 Compare February 9, 2024 14:23
@Wabri Wabri marked this pull request as ready for review February 9, 2024 14:27
@Wabri
Copy link
Member Author

Wabri commented Feb 9, 2024

@sean-freeman @berndfinger I've try a different approach to solve the problem using the meta module (https://docs.ansible.com/ansible/latest/collections/ansible/builtin/meta_module.html) and trigger the grub2 update only if one item cause the change of the grub configuration. Let me know if it's clean enough for both of you.

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍 novel way of achieving the outcome, cool

@Wabri Wabri self-assigned this Feb 12, 2024
@marcelmamula
Copy link
Contributor

@sean-freeman @berndfinger I've try a different approach to solve the problem using the meta module (https://docs.ansible.com/ansible/latest/collections/ansible/builtin/meta_module.html) and trigger the grub2 update only if one item cause the change of the grub configuration. Let me know if it's clean enough for both of you.

Interesting use of meta module, but can you retest it few times and inside of wrapper?
My use of meta with end_play yielded inconsistent behavior across multiple nodes in wrapped play so I am hesitant to use it.

I had to switch to use pause module for debugging instead of meta: end_play because of that.

@Wabri Wabri force-pushed the feature/337/sles-kernel branch from 9b5bcd9 to 43167ee Compare February 13, 2024 15:18
@Wabri Wabri merged commit 4823078 into sap-linuxlab:dev Mar 5, 2024
3 checks passed
@Wabri Wabri deleted the feature/337/sles-kernel branch March 5, 2024 16:01
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