-
Notifications
You must be signed in to change notification settings - Fork 59
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: Regressions in Dev version for vmware/bare-metal deployments #923
base: dev
Are you sure you want to change the base?
Conversation
…luster_fence_agent_packages_platform not defined (this is case for on-prem/bare metal)
…o prevent errors when setting corosync totem errors on bare metal RHEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both changes makes sense, thank you. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Before we proceed, @marcelmamula do you think it makes sense to add this to vars/main.yml
to make it consistent with e.g. __sap_ha_pacemaker_cluster_platform_extra_packages: []
in there already?
@rob0d I agree with @ja9fuchs that it would be better to remove current change with __sap_ha_pacemaker_cluster_platform_extra_packages: []
__sap_ha_pacemaker_cluster_fence_agent_packages_platform: [] |
…kages platform to prevent errors
Yes. That makes more sense. I didn't want to define a default empty value as I wasn't sure if you rely on "is defined" somewhere else in the code which I can't test. |
We rather not rely on non-existence of vars. :) Did you push the update already? It's not showing up... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ja9fuchs sorry, forgot to press the button. It's there now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fixes. :)
👍
@marcelmamula @ja9fuchs
I've found a few regressions in 1.5.0 (or current dev branch) which broke the role when executing on bare metal or vmware systems. I see most changes which caused this are related to cloud systems. This PR should fix both known errors (another error has been fixed in the current dev branch).