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_ha_pacemaker_cluster: Variable changes for different os and platforms #642

Merged
merged 4 commits into from
Mar 13, 2024

Conversation

marcelmamula
Copy link
Contributor

This PR changes:

  1. Platform vars are now using variable dictionaries sourced from os_family vars for easier control from each of them.
  2. New variables implemented to cover os_family differences:
  • aws fence agent
  • SRhook path
  • ocf SAP agents namespace
  1. nwas_common.yml was adjusted to be more dynamic and source from dictionary

I have tested this code on SLES for SAP SP5 and RHEL9.2 along with changes in ha_cluster.

@sean-freeman Thank you for call yesterday to double check contents with me.

@marcelmamula
Copy link
Contributor Author

Added bugfix for creation of extra_packages list after re-testing for ASCS cluster.

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.

Agree with the overall PR, suggesting amendments for future maintenance ease though.

@sean-freeman
Copy link
Member

👋 @marcelmamula Sorry for overdue deep review, been OOO + flu. There are some (thankfully benign) bugs hidden in plainsight discovered as we recompile the vars with this PR, but just out of scope for this PR (e.g. __sap_ha_pacemaker_cluster_repos var defined incorrectly in GCP and MS Azure platform vars files with -rhui- in the repo name)

@marcelmamula
Copy link
Contributor Author

👋 @marcelmamula Sorry for overdue deep review, been OOO + flu. There are some (thankfully benign) bugs hidden in plainsight discovered as we recompile the vars with this PR, but just out of scope for this PR (e.g. __sap_ha_pacemaker_cluster_repos var defined incorrectly in GCP and MS Azure platform vars files with -rhui- in the repo name)

Thank you @sean-freeman and I hope you feel better.
I have lot more changes queued waiting for this PR to move, because of dependencies.

  • repos are not used for SUSE as they come from registered product, so rhel changes can be done separately by @berndfinger
  • os_family conditional might be good one time use, but using it across multiple files increased clutter and decreases readability, that is reason why I proposed dictionaries in os_family var files.

Copy link
Contributor

@ja9fuchs ja9fuchs left a comment

Choose a reason for hiding this comment

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

As per our chat:
All dict lookups should have a | default(...) appended to cover undefined dictionary keys and avoid having to maintain placeholder keys in every single OS vars file.

As agreed, OS vars file changes and RHEL repo definitions for platforms will be adjusted separately in later PRs.

@marcelmamula
Copy link
Contributor Author

As per our chat: All dict lookups should have a | default(...) appended to cover undefined dictionary keys and avoid having to maintain placeholder keys in every single OS vars file.

As agreed, OS vars file changes and RHEL repo definitions for platforms will be adjusted separately in later PRs.

default was added to package dictionaries, keeping stonith without it as there is no common default value for both os for AWS - as discussed.

Copy link
Contributor

@ja9fuchs ja9fuchs left a comment

Choose a reason for hiding this comment

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

I'm fine with the PR and some improvements planned in separate PRs. 👍

@ja9fuchs ja9fuchs requested a review from sean-freeman March 11, 2024 16:26
@sean-freeman
Copy link
Member

2x Approvals, so this can merge when ready @marcelmamula

However, I would like to note we need to have consistency in the dictionaries before we release the next version. For example....

vars/<<os_family>>.yml OS Major/Minor

  • Dictionary for OS Package Repos on Target Platform (to ensure activated)
  • Dictionary for OS Packages on Target Platform
  • Dictionary for OS Packages for a SAP HA scenario (hana_scaleout etc.)

vars/<<platform>>.yml Platform

  • Dictionary for STONITH Method
    • STONITH_Method_1
      • OS (RHEL/SLES)
        • Fence Agent (and options)
  • Dictionary for VIP Method
    • VIP_Method_1
      • OS (RHEL/SLES)
        • Resource Agent/s (and options)

@marcelmamula marcelmamula merged commit b7b1063 into sap-linuxlab:dev Mar 13, 2024
4 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.

3 participants